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] XmlEncoder: fix negative int and large numbers handling#22478
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
vlastv 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.
@dunglas (sorry) in my and you test:
After converting to a float, you lost data... |
ghost commentedApr 20, 2017
happy to help |
nilq commentedApr 20, 2017
my name is dungles |
dunglas commentedApr 20, 2017
Sorry@valisj I'm not sure to understand what you mean here |
dunglas commentedApr 20, 2017
Indeed, but as with the |
nicolas-grekas left a comment
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.
👍
| if (ctype_digit($attr->nodeValue)) { | ||
| $data['@'.$attr->nodeName] = (int)$attr->nodeValue; | ||
| }else { | ||
| if (!is_numeric($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.
Why don't we keep the current behaviour here to return everything that does not represent an integer as a string?
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.
Because the current behavior is faulty. It doesn't handle negative and large integers.
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.
ctype_digit() should be able to detect large integers, shouldn't it? So we only need to be able to handle negative integers. But dealing with everything that looks numeric could be too broad and cause BC breaks, couldn't it?
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.
Large integers must be casted to float (as dojson_encode) not to integers.
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.
The current behavior is already unreliable and can breaks things. It's why I suggest to make this feature opt-in and disabled by default.
fabpot commentedApr 23, 2017
Thank you@dunglas. |
…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
vlastv commentedApr 24, 2017
@fabpot Nooooo... @dunglas do not compare with JSON. In JSON has number and string tokens, xml has only string. $doc ='{ "one": "182077241760011681341821060401202210011000045913000000017100", "two": 182077241760011681341821060401202210011000045913000000017100}';var_dump($jsonEncoder->decode($doc,'json')); array(2) { ["one"]=> string(60)"182077241760011681341821060401202210011000045913000000017100" ["two"]=> float(1.8207724176001E+59)}It all depends on how the data is encoded. thx... |
sstok commentedApr 24, 2017
Basically what this tests is But it doesn't contain the exact same input data! 😱 not to mention that |
sstok commentedApr 24, 2017
@dunglas In JSON it's only converted when the value is an actual integer.
We cannot prevent something from being a string, but we shouldn't blindly convert convert any number to an integer as this will break for octal and international phone numbers 😃 |
ragboyjr commentedJun 9, 2017
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/aThis 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
Alternative to#22333.
JsonEncoderand native PHP functions likeceildo)@vlastv, I've adapted your test. Can you check if it fixes your problem?