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

[Serializer] Ignore comments when decoding XML #26445

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@q0rban
Contributor

q0rban commented Mar 7, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets N/A
License MIT
Doc PR N/A

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".

q0rban added some commits Mar 7, 2018

[Serializer] Ignore comments when decoding XML
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.
@ostrolucky

i like this

*/
public function __construct(string $rootNodeName = 'response', int $loadOptions = null)
public function __construct(string $rootNodeName = 'response', int $loadOptions = null, array $ignoredNodeTypes = null)

This comment has been minimized.

@ostrolucky

ostrolucky Mar 7, 2018

Contributor
, array $ignoredNodeTypes = array(XML_PI_NODE, XML_COMMENT_NODE)

then you can get rid of ternary

This comment has been minimized.

@q0rban

q0rban Mar 7, 2018

Contributor

I wondered about that. I was following the pattern I saw for $loadOptions, but I'm happy to switch to that.

@20uf

20uf approved these changes Mar 7, 2018

@q0rban

This comment has been minimized.

Contributor

q0rban commented Mar 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.

*/
public function __construct(string $rootNodeName = 'response', int $loadOptions = null)
public function __construct(string $rootNodeName = 'response', int $loadOptions = null, array $ignoredNodeTypes = array(XML_PI_NODE, XML_COMMENT_NODE))

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 14, 2018

Member

the default should be BC -safe, thus XML_COMMENT_NODE should not be listed, isn't it?

This comment has been minimized.

@ostrolucky

ostrolucky Mar 14, 2018

Contributor

Listing XML_COMMENT_NODE is for fixing a bug tho. I don't think anybody relies on old behaviour.

This comment has been minimized.

@q0rban

q0rban Mar 14, 2018

Contributor

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.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 19, 2018

Member

ok, can you add an entry in the UPGRADE file when, please?

This comment has been minimized.

@q0rban

q0rban Mar 19, 2018

Contributor

@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

fabpot approved these changes Mar 21, 2018

minor comment

@@ -11,6 +11,8 @@ CHANGELOG
* 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

This comment has been minimized.

@fabpot

fabpot Mar 21, 2018

Member

XML

@fabpot

This comment has been minimized.

Member

fabpot commented Mar 22, 2018

Thank you @q0rban.

@fabpot fabpot closed this in bbeca51 Mar 22, 2018

@q0rban q0rban deleted the q0rban:q0rban/XmlEncoder-ignore-comments branch Mar 22, 2018

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment