Xml encoder optional type cast #23122
Conversation
c16ebe5 to
9c71e08
Compare
| $data['@a'] === '018' && | ||
| $data['@b'] === '-12.11' && | ||
| $data['@a'] === $data['node']['@a'] && | ||
| $data['@b'] === $data['node']['@b'] |
There was a problem hiding this comment.
I think these needs to be separate asserts (you can then probably use assertSame)
|
I think this might count as a new feature, since you are adding a new flag which must be explicitly opt-in to use, so it probably needs to go against the |
|
@pierredup I understand what you mean; however, the issue is that the bug/issue of typecasting everything was merged into 2.7. So, some type of fix will be needed for that. I couldn't figure out a way to fix that bug in a bc manner. The only other solution I could come up with would be to maybe use regex to parse the string and verify that it doesn't start with zero's, however, that could open its own can of worms which might cause other users to complain. If you have any ideas on how to fix the issue and merge into 2.7, then i'm all ears. |
7c59299 to
3279ab3
Compare
- This provides the ability to forgo the attribute type casting - Updated the CHANGELOG Signed-off-by: RJ Garcia <rj@bighead.net>
3279ab3 to
589a3cf
Compare
|
@pierredup updated the test case. |
|
I think that if the issue is only about numbers that start with a zero, then we could consider keeping them as strings |
|
|
||
| foreach ($node->attributes as $attr) { | ||
| if (!is_numeric($attr->nodeValue)) { | ||
| if (!is_numeric($attr->nodeValue) || !$typeCastAttributes) { |
There was a problem hiding this comment.
I propose excluding numbers that start with zero:
if (!is_numeric($attr->nodeValue) || (isset($attr->nodeValue[1]) && '0' === $attr->nodeValue[0])) {
There was a problem hiding this comment.
I agree for 2.7. Having this flag in 3.4 would be nice too.
There was a problem hiding this comment.
@nicolas-grekas That sounds good. That would fix my specific use case, not sure if it fixes all of the issues others might have.
I might just submit another MR then with the bug fix like @nicolas-grekas shows into 2.7. And rebase this off of 3.4 @dunglas
There was a problem hiding this comment.
Good idea, we can even do better:
- submit the fix proposed by Nicolas into 2.7
- rebase this PR against
masterand make this feature optin (document the BC break in the CHANGELOG)
WDYT?
There was a problem hiding this comment.
@dunglas @nicolas-grekas Sounds great. You guys are the boss! just let me know :).
There was a problem hiding this comment.
@ragboyjr I'm going to merge this one in 3.4 as a new feature. Can you create another PR on 2.7 for the bug fix? Thanks.
|
👍 to merge this in 3.4. It should be the default behavior IMO but it's too late now. |
|
Thank you @ragboyjr. |
This PR was submitted for the 2.7 branch but it was merged into the 3.4 branch instead (closes #23122). Discussion ---------- Xml encoder optional type cast | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22478 | License | MIT | Doc PR | n/a This fixes the issue where certain XML attributes are typecasted when you don't want them to by providing the ability opt out of any typecasting of xml attributes via an option in the context. If this is approved, then I'll add docs in the serializer component describing the new context option. Commits ------- 8f6e67d XML Encoder Optional Type Cast
|
Thanks @fabpot ! |
…agi) This PR was merged into the 3.4 branch. Discussion ---------- [Serializer] XmlEncoder: don't cast padded strings | Q | A | ------------- | --- | Branch? | 3.4 <!-- see below --> | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | #23122 (comment) <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | N/A This was a suggestion of @nicolas-grekas in #23122. Which seems to have been forgotten. But shouldn't we also avoid casting something like `.18`, `+18`, `-18`? Commits ------- c1bfaa1 [Serializer] XmlEncoder: don't cast padded strings
This fixes the issue where certain XML attributes are typecasted when you don't want them to by providing the ability opt out of any typecasting of xml attributes via an option in the context. If this is approved, then I'll add docs in the serializer component describing the new context option.