Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Serializer] 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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Previously, if the first line of XML was a comment, that would be used asthe root node of the decoded XML. This work strips comments from decodedXML by default, but also allows for customizing which XML node types areignored during decoding.
ostrolucky left a comment
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 like this
| * @param int[] $ignoredNodeTypes an array of ignored XML node types, each one of the DOM Predefined XML_* Constants | ||
| */ | ||
| publicfunction__construct(string$rootNodeName ='response',int$loadOptions =null) | ||
| publicfunction__construct(string$rootNodeName ='response',int$loadOptions =null,array$ignoredNodeTypes =null) |
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.
, array$ignoredNodeTypes =array(XML_PI_NODE,XML_COMMENT_NODE)
then you can get rid of ternary
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 wondered about that. I was following the pattern I saw for$loadOptions, but I'm happy to switch to that.
dec179a toc6b294cComparec6b294c tob55454eCompareq0rban commentedMar 8, 2018
I realized I forgot to update the CHANGELOG.md with some changes I made to the function, so I've updated that now. |
| * @param int[] $ignoredNodeTypes an array of ignored XML node types, each one of the DOM Predefined XML_* Constants | ||
| */ | ||
| publicfunction__construct(string$rootNodeName ='response',int$loadOptions =null) | ||
| publicfunction__construct(string$rootNodeName ='response',int$loadOptions =null,array$ignoredNodeTypes =array(XML_PI_NODE,XML_COMMENT_NODE)) |
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, thusXML_COMMENT_NODE should not be listed, isn't it?
ostroluckyMar 14, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
fabpot left a comment
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.
minor comment
| * added optional`bool $escapeFormulas = false` argument to`CsvEncoder::__construct` | ||
| * added`AbstractObjectNormalizer::setMaxDepthHandler` to set a handler to call when the configured | ||
| maximum depth is reached | ||
| * added optional`int[] $ignoredNodeTypes` argument to`XmlEncoder::__construct`. Xml decoding now |
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.
XML
fabpot commentedMar 22, 2018
Thank you@q0rban. |
Uh oh!
There was an error while loading.Please reload this page.
Previously, if the first line of XML was a comment, that would be used as the root node of the decoded XML. This work strips comments from decoded XML by default, but also allows for customizing which XML node types are ignored during decoding. The first two commits in this PR contain tests only to prove the existence of this "bug".