Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Serializer] Fix decoding attribute that contains many digits#22333
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| }else { | ||
| $data['@'.$attr->nodeName] =$attr->nodeValue; | ||
| } | ||
| $data['@'.$attr->nodeName] =$attr->nodeValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 toint loses information and keep a string only in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@jvasseur I changed in PR description BC break flag.
If checking possibility cast, thenonly positive integer will return as int and all others as string.
I think this behavior is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
'-321312' is properly converted to -321312.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@sstok this is a BC break, earlier would return as a stringctype_digit('-321312') === false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My comment about positive integer actual only if need save BC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
-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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@sstok method must return only one data type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This method always returns an array 😛 the values of the array vary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I propose to open a second PR with your implementation.
You will also have a break BC
dunglas commentedApr 19, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I suggest to return a |
dunglas commentedApr 19, 2017
By the way the current handling of negative integers is faulty and should be considered as a bug. |
vlastv commentedApr 19, 2017
@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/aAlternative 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
Uh oh!
There was an error while loading.Please reload this page.