[Serializer] Fix decoding attribute that contains many digits #22333
Conversation
| Q | A |
|---|---|
| Branch? | 2.7 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | yes |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #22329 |
| License | MIT |
| } else { | ||
| $data['@'.$attr->nodeName] = $attr->nodeValue; | ||
| } | ||
| $data['@'.$attr->nodeName] = $attr->nodeValue; |
There was a problem hiding this comment.
This is a BC break in case someone relies on the type of the decoded data (this could happen easily with PHP7 et strict types).
Maybe you could check if the cast to int loses information and keep a string only in this case.
There was a problem hiding this comment.
@jvasseur I changed in PR description BC break flag.
If checking possibility cast, then only positive integer will return as int and all others as string.
I think this behavior is wrong.
There was a problem hiding this comment.
'-321312' is properly converted to -321312.
There was a problem hiding this comment.
@sstok this is a BC break, earlier would return as a string ctype_digit('-321312') === false
There was a problem hiding this comment.
My comment about positive integer actual only if need save BC.
There was a problem hiding this comment.
on different data in attribute you get different type, in PHP 7 with strict_types=1 this can create exception;
There was a problem hiding this comment.
-321312 would have been converted to a string. Which technically is a bug, the double cast checking ensures it's converted to an integer ONLY when the actual result would not change.
(string) ((int)'-321312') === '-321312'.
https://3v4l.org/mvlQN
There was a problem hiding this comment.
@sstok method must return only one data type
There was a problem hiding this comment.
This method always returns an array 😛 the values of the array vary.
There was a problem hiding this comment.
I propose to open a second PR with your implementation.
You will also have a break BC
|
I suggest to return a |
|
By the way the current handling of negative integers is faulty and should be considered as a bug. |
|
@dunglas we can not know whether a string is a number in fact, or it is a random circumstance. |
…s handling (dunglas) This PR was merged into the 2.7 branch. Discussion ---------- [Serializer] XmlEncoder: fix negative int and large numbers handling | Q | A | ------------- | --- | Branch? | 2.7 <!-- see comment below --> | Bug fix? | yes | New feature? | no <!-- don't forget updating src/**/CHANGELOG.md files --> | BC breaks? | no | Deprecations? | no <!-- don't forget updating UPGRADE-*.md files --> | Tests pass? | yes | Fixed tickets | #22329, #22333 <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | n/a Alternative to #22333. * Negative integers are now handled * Float are now handled * Large numbers are converted to float (as the `JsonEncoder` and native PHP functions like `ceil` do) @vlastv, I've adapted your test. Can you check if it fixes your problem? Commits ------- 1eeadb0 [Serializer] XmlEncoder: fix negative int and large numbers handling