Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decode entities if the "decodeEntities" flag is not set #486

Merged
merged 4 commits into from May 15, 2019

Conversation

Projects
None yet
3 participants
@leofeyer
Copy link
Member

commented May 13, 2019

This PR fixes #360 by decoding entities in the diff view if the decodeEntities flag is not set.

@leofeyer leofeyer added the defect label May 13, 2019

@leofeyer leofeyer added this to the 4.4.40 milestone May 13, 2019

@leofeyer leofeyer requested a review from Toflar May 13, 2019

@leofeyer leofeyer self-assigned this May 13, 2019

@Toflar

This comment has been minimized.

Copy link
Member

commented May 14, 2019

I don't think this is correct. This would still compare encoded HTML entities with each other, wouldn't it? Comparison should be done on the real content. Only the diff output should be encoded. /cc @ausi

@leofeyer

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

Only the diff output should be encoded

I have updated the PR accordingly.

However, I am generally unsure if this is the right thing to do. The diff view compares the raw data as you can also see here:

Now if we decide to handle entities, we should also handle serialized data, shouldn't we?

@leofeyer leofeyer force-pushed the hotfix/diff-view branch from fa09914 to 211026b May 14, 2019

@Toflar

This comment has been minimized.

Copy link
Member

commented May 14, 2019

I think comparing the raw data is not very helpful which is why we consider decodeEntities in the first place. The diff view is for the users so I actually think using var_export() for deserialized arrays would be great :).

@leofeyer

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

Ah, never mind. We already handle serialized data – it just does not work for the meta wizard yet. 🤦‍♂

@Toflar Then please review the new implementation.

@leofeyer

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

No, the new implementation is wrong. We do all modifications prior to generating the diff view:

  • Decrypt the content
  • Convert serialized arrays into strings
  • Convert binary UUIDs to their hex equivalents
  • Convert date fields

So if we want to decode entities, we should also do it there.

@Toflar

This comment has been minimized.

Copy link
Member

commented May 14, 2019

I don't know why things are the way they are. I only know that decoding html entities must be done when you output them in HTML context.

@leofeyer

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

I have updated the PR accordingly. Now we are decoding the entities before comparing the values and we are encoding special characters when generating the diff view.

@leofeyer leofeyer force-pushed the hotfix/diff-view branch from 30f1026 to fabac13 May 14, 2019

@Toflar

This comment has been minimized.

Copy link
Member

commented May 14, 2019

This looks better now 😄

@ausi
Copy link
Member

left a comment

I think the changes in DiffRenderer.php can be reverted. Currently they don’t do anything and HTML encoding is already handled by https://github.com/phpspec/php-diff/blob/v1.1.0/lib/Diff/Renderer/Html/Array.php#L186

The changes in Versions.php are correct IMO.

@leofeyer

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

Reverted in 1dfa66c.

@ausi

ausi approved these changes May 15, 2019

@Toflar

Toflar approved these changes May 15, 2019

@leofeyer leofeyer merged commit 8caef69 into 4.4 May 15, 2019

3 of 4 checks passed

Travis CI - Pull Request Build Errored
Details
Travis CI - Branch Build Passed
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@leofeyer leofeyer deleted the hotfix/diff-view branch May 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.