-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Seri 8000 alizer] Ignore comments when decoding XML #26445
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
Closed
Closed
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
928eb2d
[Serializer] Add test to ensure comments don't break XML decoding
q0rban 01ae25f
[Serializer] Test that decoding XML comments is possible
q0rban 744eea5
[Serializer] Ignore comments when decoding XML
q0rban 5fb630d
[Serializer] Update CHANGELOG.md re: new XmlEncoder argument.
q0rban 3856734
[Serializer] Fix style issues and @param type.
q0rban b55454e
[Serializer] Add XmlEncoder:: defaults directly to argument, per @ost…
q0rban 16f2683
[Serializer] Update documentation to match previous change to argument.
q0rban 91ec952
[Serializer] Update the 4.1 upgrade instructions to note XML decoding…
q0rban 070629b
[Serializer] Use XML instead of Xml in CHANGELOG.
q0rban File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the default should be BC -safe, thus
XML_COMMENT_NODE
should not be listed, isn't it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Listing XML_COMMENT_NODE is for fixing a bug tho. I don't think anybody relies on old behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had originally put BC breaks to yes, but it looks like that was changed at some point. Personally, I think comments should be stripped by default, as a comment at the top of the XML to decode ends up becoming the XML root node. See the tests for an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, can you add an entry in the UPGRADE file when, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicolas-grekas since this is in the 4.1 milestone, I assume you are referring to the UPGRADE-4.1.md file? If so, I pushed a commit to note the change in this PR.