Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Serializer] Fix the XML comments encoding#28156

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

Merged
fabpot merged 1 commit intosymfony:masterfrommaidmaid:serializer-comment
Aug 19, 2018

Conversation

@maidmaid
Copy link
Contributor

@maidmaidmaidmaid commentedAug 8, 2018
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets/
LicenseMIT
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.

$encoder->encode(['#comment' =>' foo'],'xml');
Expected:<response>    <!-- foo --></response>Actual:<response>  <item key="#comment"> foo </item></response>

@nicolas-grekas
Copy link
Member

Which is the lowest branch where this applies? This should be the target branch.

@maidmaid
Copy link
ContributorAuthor

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 ?

@nicolas-grekas
Copy link
Member

I cannot. Why do you think it's supposed to work this way?
From the code, this would work:$data = array('#' => ' foo ');. Isn't it enough?

@nicolas-grekasnicolas-grekas added this to the2.8 milestoneAug 8, 2018
@nicolas-grekas
Copy link
Member

From the code, this would work: $data = array('#' => ' foo ');.

Actually not, which means this is a new feature, now a bug fix, isn't it?
It should be done in a BC-safe way, not sure that's the case right now, is it?

@maidmaid
Copy link
ContributorAuthor

From the code, this would work: $data = array('#' => ' foo ');.

Encoding['#' => ' foo '] works as expected (generates the value of the current node), but encoding['#comment' => ' foo '] doesn't work (is supposed to generate a comment node).

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.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Oh, ok for a bug fix then on my side. Here are some more random comments :)

* @throws NotEncodableValueException
*/
privatefunctionselectNodeType(\DOMNode$node,$val):bool
privatefunctionselectNodeType(\DOMNode$node,$val,$key =null):bool
Copy link
Member

@nicolas-grekasnicolas-grekasAug 15, 2018
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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) { ...?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Right, dealing directly inbuildXml() makes more sense because it's here that we manage thespecial cases like@,# and now like#comment.

@maidmaidmaidmaid changed the base branch frommaster to2.8August 15, 2018 20:59
@maidmaid
Copy link
ContributorAuthor

As we said it's a bug fix, I rebased on 2.8.

returnfalse;
}

privatefunctionappendComment(\DOMNode$node,string$data):bool

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Final protected like the others?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Done ! (Could you explain to me the goal of a final protected method please ?)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

goal of a final protected method

allowing child classes to call the method but not override it.

maidmaid reacted with thumbs up emoji
@fabpot
Copy link
Member

I don't see a big fix here but a new feature.

@maidmaid
Copy link
ContributorAuthor

maidmaid commentedAug 16, 2018
edited
Loading

@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>'

@maidmaidmaidmaidforce-pushed theserializer-comment branch 3 times, most recently from09efffe to477df1dCompareAugust 16, 2018 11:28
@fabpot
Copy link
Member

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

@maidmaid
Copy link
ContributorAuthor

Ok@fabpot, let's see it as a (missing) new feature. I'll soon rebase on master.

@maidmaidmaidmaid changed the base branch from2.8 tomasterAugust 18, 2018 15:26
@maidmaid
Copy link
ContributorAuthor

Rebase done.

@fabpot
Copy link
Member

Can you add a note in the CHANGELOG of the component?

@maidmaid
Copy link
ContributorAuthor

Note added.

@fabpot
Copy link
Member

Thank you@maidmaid.

@fabpotfabpot merged commitd94a37f intosymfony:masterAug 19, 2018
fabpot added a commit that referenced this pull requestAug 19, 2018
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
@maidmaidmaidmaid deleted the serializer-comment branchAugust 27, 2018 22:05
This was referencedNov 3, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@fabpotfabpotfabpot approved these changes

@dunglasdunglasAwaiting requested review from dunglas

@lyrixxlyrixxAwaiting requested review from lyrixx

@srozesrozeAwaiting requested review from sroze

Assignees

No one assigned

Projects

None yet

Milestone

2.8

Development

Successfully merging this pull request may close these issues.

4 participants

@maidmaid@nicolas-grekas@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp