[Serializer] Fix the XML comments encoding #28156
Conversation
fb683af to
26f8e31
Compare
|
Which is the lowest branch where this applies? This should be the target branch. |
|
2.8 is supposed to be the lowest branch, but before that, can you ensure me it's actually a bug and not a feature ? |
|
I cannot. Why do you think it's supposed to work this way? |
Actually not, which means this is a new feature, now a bug fix, isn't it? |
Encoding I meant : encoding of XML comment wasn't handled before this PR. It's why we can see that as a new feature. On the other hand, the encoding and the decoding of a XML comment behave differently, it's why we also can see that as a bug. |
| @@ -445,7 +453,7 @@ private function needsCdataWrapping(string $val): bool | |||
| * | |||
| * @throws NotEncodableValueException | |||
| */ | |||
| private function selectNodeType(\DOMNode $node, $val): bool | |||
| private function selectNodeType(\DOMNode $node, $val, $key = null): bool | |||
There was a problem hiding this comment.
why make it an optional argument? i.e.: shouldn't the call inside appendNode also pass the key?
otherside of the question: if the anwer is no, what about dealing with the #comment check in buildXml directly, adding } elseif ('#comment' === $key) { ...?
There was a problem hiding this comment.
Right, dealing directly in buildXml() makes more sense because it's here that we manage the special cases like @, # and now like #comment.
26f8e31 to
caadb94
Compare
|
As we said it's a bug fix, I rebased on 2.8. |
| @@ -235,6 +236,13 @@ public function getRootNodeName() | |||
| return false; | |||
| } | |||
|
|
|||
| private function appendComment(\DOMNode $node, string $data): bool | |||
There was a problem hiding this comment.
Final protected like the others?
There was a problem hiding this comment.
Done ! (Could you explain to me the goal of a final protected method please ?)
There was a problem hiding this comment.
goal of a final protected method
allowing child classes to call the method but not override it.
|
I don't see a big fix here but a new feature. |
|
@fabpot IMO, we should get an output equal to the input after decoding and then encoding it. Said differently, if this the condition below isn't respected, I think it's a bug : $encoded === $encoder->encode($encoder->decode($encoded, $format), $format))In this case, without this PR, the result it's like : '<a><!-- b --></a>' === '<a><item key="#comment"> b </item></a>' |
09efffe to
477df1d
Compare
|
When the feature was added to the decoder, nobody thought about adding it to the decoder as well. That's sad we didn't think about this before. I still see it as a new feature (a missing one and a very welcome one for sure). |
|
Ok @fabpot, let's see it as a (missing) new feature. I'll soon rebase on master. |
477df1d to
0a1e599
Compare
0a1e599 to
14c66fe
Compare
|
Rebase done. |
|
Can you add a note in the CHANGELOG of the component? |
14c66fe to
d94a37f
Compare
|
Note added. |
|
Thank you @maidmaid. |
This PR was merged into the 4.2-dev branch.
Discussion
----------
[Serializer] Fix the XML comments 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 | /
When we decode a XML comment, we get `['#comment' => ' foo ']`. But when we encode this same content, the result is not the expected one.
```php
$encoder->encode(['#comment' => ' foo '], 'xml');
```
```
Expected:
<response>
<!-- foo -->
</response>
Actual:
<response>
<item key="#comment"> foo </item>
</response>
```
Commits
-------
d94a37f Allow to encode xml comments
When we decode a XML comment, we get
['#comment' => ' foo ']. But when we encode this same content, the result is not the expected one.