Skip to content

[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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

[Serializer] Fix decoding attribute that contains many digits #22333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d23da47
Add test
vlastv Apr 7, 2017
54cab19
Fix CS
vlastv Apr 7, 2017
b56fca4
Fix
vlastv Apr 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions src/Symfony/Component/Serializer/Encoder/XmlEncoder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,11 +301,7 @@ private function parseXmlAttributes(\DOMNode $node)
$data = array();

foreach ($node->attributes as $attr) {
if (ctype_digit($attr->nodeValue)) {
$data['@'.$attr->nodeName] = (int) $attr->nodeValue;
} else {
$data['@'.$attr->nodeName] = $attr->nodeValue;
}
$data['@'.$attr->nodeName] = $attr->nodeValue;
Copy link
Contributor

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 to int loses information and keep a string only in this case.

Copy link
Contributor Author

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, then only positive integer will return as int and all others as string.
I think this behavior is wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://3v4l.org/fRfCH

'-321312' is properly converted to -321312.

Copy link
Contributor Author

@vlastv vlastv Apr 7, 2017 •
edited
Loading

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 string ctype_digit('-321312') === false

Copy link
Contributor Author

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.

Copy link
Contributor Author

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;

Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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

}

return $data;
Expand Down
11 changes: 11 additions & 0 deletions src/Symfony/Component/Serializer/Tests/Encoder/XmlEncoderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,17 @@ public function testDecodeWithoutItemHash()
$this->assertEquals($expected, $this->encoder->decode($xml, 'xml'));
}

public function testDecodeBigDigitAttributes()
{
$source = '<?xml version="1.0"?>'.PHP_EOL.
'<document index="182077241760011681341821060401202210011000045913000000017100">Name</document>'.PHP_EOL;
$expected = array(
'#' => 'Name',
'@index' => '182077241760011681341821060401202210011000045913000000017100',
);
$this->assertEquals($expected, $this->encoder->decode($source, 'xml'));
}

/**
* @expectedException \Symfony\Component\Serializer\Exception\UnexpectedValueException
*/
Expand Down