Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
fabpot merged 1 commit intosymfony:2.7fromdunglas:fix_22333
Apr 23, 2017

Conversation

@dunglas
Copy link
Member

QA
Branch?2.7
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#22329,#22333
LicenseMIT
Doc PRn/a

Alternative to#22333.

  • Negative integers are now handled
  • Float are now handled
  • Large numbers are converted to float (as theJsonEncoder and native PHP functions likeceil do)

@vlastv, I've adapted your test. Can you check if it fixes your problem?

maidmaid reacted with thumbs up emoji
@vlastv
Copy link
Contributor

vlastv commentedApr 19, 2017
edited
Loading

@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
Copy link

happy to help

sstok, dunglas, jvasseur, Nek-, and ogizanagi reacted with laugh emojinilq reacted with heart emoji

@nilq
Copy link

my name is dungles

@dunglas
Copy link
MemberAuthor

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
Copy link
MemberAuthor

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

Indeed, but as with theJsonEncoder (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-grekasnicolas-grekas added this to the2.7 milestoneApr 20, 2017
Copy link
Member

@nicolas-grekasnicolas-grekas left a 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)) {
Copy link
Member

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?

Copy link
MemberAuthor

@dunglasdunglasApr 20, 2017
edited
Loading

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.

Copy link
Member

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?

Copy link
MemberAuthor

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.

Copy link
MemberAuthor

@dunglasdunglasApr 20, 2017
edited
Loading

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
Copy link
Member

Thank you@dunglas.

sstok reacted with confused emoji

@fabpotfabpot merged commit1eeadb0 intosymfony:2.7Apr 23, 2017
fabpot added a commit that referenced this pull requestApr 23, 2017
…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
Copy link
Contributor

@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
Copy link
Contributor

Basically what this tests is182077241760011681341821060401202210011000045913000000017100 === (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 that02471 is also numeric but not an integer.

@sstok
Copy link
Contributor

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

https://3v4l.org/Zbag5

  • var_dump(json_decode('{"he": "02471"}')); givesobject(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 reacted with thumbs up emoji

This was referencedMay 1, 2017
@ragboyjr
Copy link
Contributor

@sstok I'm experiencing a similar issue.

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

@dunglas any suggestions?

@dunglasdunglas deleted the fix_22333 branchJune 14, 2017 18:12
fabpot added a commit that referenced this pull requestJun 16, 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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@xabbuhxabbuhxabbuh left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

2.7

Development

Successfully merging this pull request may close these issues.

9 participants

@dunglas@vlastv@nilq@fabpot@sstok@ragboyjr@nicolas-grekas@xabbuh@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp