[Serializer] XmlEncoder: fix negative int and large numbers handling #22478

dunglas · 2017-04-19T20:40:14Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#22329, #22333
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?

vlastv · 2017-04-19T20:55:48Z

@dunglas (sorry) in my and you test:

<document index="182077241760011681341821060401202210011000045913000000017100">Name</document>

index attribute is not a number, but very similar.

After converting to a float, you lost data...

ghost · 2017-04-20T07:51:18Z

happy to help

nilq · 2017-04-20T07:51:32Z

my name is dungles

dunglas · 2017-04-20T08:32:37Z

index attribute is not a number, but very similar.

Sorry @valisj I'm not sure to understand what you mean here

var_dump(is_numeric('182077241760011681341821060401202210011000045913000000017100'));
bool(true)

dunglas · 2017-04-20T08:57:30Z

After converting to a float, you lost data...

Indeed, but as with the JsonEncoder (and as usually with programming languages). But as you pointed out in the issue, it's problematic that things looking like numbers are automatically converted. It should be an opt-in feature (through a context flag). It's a BC break but it's safer and will avoid unexpected behaviors like this one. ping @symfony/deciders

nicolas-grekas

👍

xabbuh · 2017-04-20T14:49:17Z

src/Symfony/Component/Serializer/Encoder/XmlEncoder.php

-            if (ctype_digit($attr->nodeValue)) {
-                $data['@'.$attr->nodeName] = (int) $attr->nodeValue;
-            } else {
+            if (!is_numeric($attr->nodeValue)) {


Why don't we keep the current behaviour here to return everything that does not represent an integer as a string?

Because the current behavior is faulty. It doesn't handle negative and large integers.

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?

Large integers must be casted to float (as do json_encode) not to integers.

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 · 2017-04-23T22:32:53Z

Thank you @dunglas.

@vlastv

…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  | Bug fix? | yes | New feature? | no  | BC breaks? | no | Deprecations? | no  | Tests pass? | yes | Fixed tickets | #22329, #22333  | 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

vlastv · 2017-04-24T05:31:18Z

@fabpot Nooooo...
Now XmlEncoder never return data from XML without distortion.
Test in this PR incorrect!!!

@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.
But, in XML we can't control as encoding data. Now we will never get the original data...

thx...

sstok · 2017-04-24T07:18:49Z

Basically what this tests is 182077241760011681341821060401202210011000045913000000017100 === (int) '182077241760011681341821060401202210011000045913000000017100' which will work because the string is converted to an int, and so you bypass the overflow.

But it doesn't contain the exact same input data! 😱 not to mention that 02471 is also numeric but not an integer.

sstok · 2017-04-24T07:28:27Z

@dunglas In JSON it's only converted when the value is an actual integer.

https://3v4l.org/Zbag5

var_dump(json_decode('{"he": "02471"}')); gives object(stdClass)#1 (1) { ["he"]=> string(5) "02471" }

https://3v4l.org/34giH

var_dump(json_decode('{"he": 182077241760011681341821060401202210011000045913000000017100}'));: object(stdClass)#1 (1) { ["he"]=> float(1.8207724176001E+59) }

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 · 2017-06-09T20:37:59Z

@sstok I'm experiencing a similar issue.

I have an attribute with a value of 00000018 and it's being converted into a float as 18 instead of 00000018 like I need it to be. I don't see any easy way of getting the original value.

@dunglas any suggestions?

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

carsonbot added Status: Needs Review Serializer Bug labels Apr 19, 2017

dunglas force-pushed the fix_22333 branch from c3ecea9 to 5a74492 Compare April 19, 2017 20:41

[Serializer] XmlEncoder: fix negative int and large numbers handling

1eeadb0

dunglas force-pushed the fix_22333 branch from 5a74492 to 1eeadb0 Compare April 19, 2017 20:42

nicolas-grekas added this to the 2.7 milestone Apr 20, 2017

nicolas-grekas approved these changes Apr 20, 2017

View reviewed changes

xabbuh reviewed Apr 20, 2017

View reviewed changes

fabpot merged commit 1eeadb0 into symfony:2.7 Apr 23, 2017

This was referenced May 1, 2017

Release v2.7.27 #22598

Merged

Release v2.8.20 #22599

Merged

Release v3.2.8 #22601

Merged

ragboyjr mentioned this pull request Jun 9, 2017

Xml encoder optional type cast #23122

Closed

dunglas deleted the fix_22333 branch June 14, 2017 18:12

marcin-kruk mentioned this pull request Oct 21, 2020

XmlEncoder: unexpected type of decoded float attributes which start with zero #38666

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serializer] XmlEncoder: fix negative int and large numbers handling #22478

[Serializer] XmlEncoder: fix negative int and large numbers handling #22478

dunglas commented Apr 19, 2017

vlastv commented Apr 19, 2017 •

edited

Loading

ghost commented Apr 20, 2017

nilq commented Apr 20, 2017

dunglas commented Apr 20, 2017

dunglas commented Apr 20, 2017

nicolas-grekas left a comment

xabbuh Apr 20, 2017

dunglas Apr 20, 2017 •

edited

Loading

xabbuh Apr 20, 2017

dunglas Apr 20, 2017

dunglas Apr 20, 2017 •

edited

Loading

fabpot commented Apr 23, 2017

vlastv commented Apr 24, 2017

sstok commented Apr 24, 2017

sstok commented Apr 24, 2017

ragboyjr commented Jun 9, 2017

[Serializer] XmlEncoder: fix negative int and large numbers handling #22478

[Serializer] XmlEncoder: fix negative int and large numbers handling #22478

Conversation

dunglas commented Apr 19, 2017

vlastv commented Apr 19, 2017 • edited Loading

ghost commented Apr 20, 2017

nilq commented Apr 20, 2017

dunglas commented Apr 20, 2017

dunglas commented Apr 20, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

xabbuh Apr 20, 2017

Choose a reason for hiding this comment

dunglas Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

xabbuh Apr 20, 2017

Choose a reason for hiding this comment

dunglas Apr 20, 2017

Choose a reason for hiding this comment

dunglas Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

fabpot commented Apr 23, 2017

vlastv commented Apr 24, 2017

sstok commented Apr 24, 2017

sstok commented Apr 24, 2017

ragboyjr commented Jun 9, 2017

vlastv commented Apr 19, 2017 •

edited

Loading

dunglas Apr 20, 2017 •

edited

Loading

dunglas Apr 20, 2017 •

edited

Loading