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

[Serializer] XmlEncoder doesn't ignore PI nodes while encoding #27926

Merged
merged 1 commit into from Aug 25, 2018

Conversation

maidmaid
Copy link
Contributor

@maidmaid maidmaid commented Jul 11, 2018

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

It's sometimes important to get only the XML body without the processing instructions (like the <?xml version="1.0" ?> on the top of your XML doc). At the moment, it's possible to ignore them while decoding, but not while encoding.

$encoder = new XmlEncoder('response', null, $ignoredNodeTypes = [XML_PI_NODE]);
echo $encoder->encode([], 'xml');
Expected:
<response/>

Actual:
<?xml version="1.0"?>
<response/>

So, a new $encoderIgnoredNodeTypes arg is added to the constructor which will be:

public function __construct(string $rootNodeName = 'response', int $loadOptions = null, array $decoderIgnoredNodeTypes = array(XML_PI_NODE, XML_COMMENT_NODE), array $encoderIgnoredNodeTypes = array())

@dunglas
Copy link
Member

dunglas commented Jul 11, 2018

We follow the robustness principle "Be conservative in what you send, be liberal in what you accept", so while it's ok to ignore XML_PI_NODE by default when decoding, it must be generated by default when encoding.

I suggest to add a new constructor parameter:

public function __construct(string $rootNodeName = 'response', int $loadOptions = null, array $decoderIgnoredNodeTypes = array(XML_PI_NODE, XML_COMMENT_NODE), array $encoderIgnoredNodeTypes = array())

@nicolas-grekas nicolas-grekas added this to the next milestone Jul 23, 2018
@maidmaid maidmaid force-pushed the serializer-pi branch 2 times, most recently from d3d136d to 9c182a2 Compare August 6, 2018 22:53
@maidmaid
Copy link
Contributor Author

maidmaid commented Aug 6, 2018

I followed your suggestion @dunglas, could you review ?

@nicolas-grekas
Copy link
Member

Is the "BC break" label still valid? If not can you please update the PR description? Rebase needed btw.

@maidmaid
Copy link
Contributor Author

@nicolas-grekas Done :)

@dunglas
Copy link
Member

dunglas commented Aug 25, 2018

Thank you @maidmaid.

@dunglas dunglas merged commit 2223fcc into symfony:master Aug 25, 2018
dunglas added a commit that referenced this pull request Aug 25, 2018
…encoding (maidmaid)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[Serializer] XmlEncoder doesn't ignore PI nodes while encoding

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

It's sometimes important to get only the XML body without the processing instructions (like the `<?xml version="1.0" ?>` on the top of your XML doc). At the moment, it's possible to ignore them while decoding, but not while encoding.

```php
$encoder = new XmlEncoder('response', null, $ignoredNodeTypes = [XML_PI_NODE]);
echo $encoder->encode([], 'xml');
```
```
Expected:
<response/>

Actual:
<?xml version="1.0"?>
<response/>
```

So, a new `$encoderIgnoredNodeTypes` arg is added to the constructor which will be:

```php
public function __construct(string $rootNodeName = 'response', int $loadOptions = null, array $decoderIgnoredNodeTypes = array(XML_PI_NODE, XML_COMMENT_NODE), array $encoderIgnoredNodeTypes = array())
```

Commits
-------

2223fcc Allow to ignore PI while encoding
fabpot added a commit that referenced this pull request Aug 27, 2018
…redNodeTypes arg in XmlEncoder contrustor (maidmaid)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[Serializer] Update changelog about the new $encoderIgnoredNodeTypes arg in XmlEncoder contrustor

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

Commits
-------

49f3bfc Update changelog
@maidmaid maidmaid deleted the serializer-pi branch August 27, 2018 22:05
fabpot added a commit that referenced this pull request Aug 30, 2018
…ML encoding (maidmaid)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[Serializer] Add support for ignoring comments while XML encoding

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

In addition to #27926 which allowed to ignore XML processing instructions, this PR allows to ignore the XML comments while encoding.

Commits
-------

8f8230a Add support for ignoring comments while XML encoding
@javiereguiluz
Copy link
Member

I've created symfony/symfony-docs#10286 to document this new feature. Please, don't forget to create a doc issue for every new feature.

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

Successfully merging this pull request may close these issues.

None yet

6 participants