Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
marcin-kruk opened this issue Oct 21, 2020 · 0 comments
Closed

Comments

@marcin-kruk
Copy link

marcin-kruk commented Oct 21, 2020

Symfony version(s) affected: 3.4

Description
If an XML attribute contains a positive float value which starts with 0, e.g. 0.123, XmlEncoder will decode it as a string.
If a float value starts with some other number, or is negative, then it is decoded as a float.

How to reproduce

<?php
require_once __DIR__.'/vendor/autoload.php';
use Symfony\Component\Serializer\Encoder\XmlEncoder;

$encoder = new XmlEncoder();

$xml = '<document value="0.123">...</document>';
$result = $encoder->decode($xml, 'xml');
var_dump($result);
// array(2) {
//   ["@value"]=>
//   string(5) "0.123"        <-- a float was expected, but it's a string
//   ["#"]=>
//   string(3) "..."
// }

Note that:

$xml = '<document value="1.123">...</document>';
$result = $encoder->decode($xml, 'xml');
var_dump($result);
// array(2) {
//   ["@value"]=>
//   float(1.123)        <-- float, as expected
//   ["#"]=>
//   string(3) "..."
// }

$xml = '<document value="-1.123">...</document>';
$result = $encoder->decode($xml, 'xml');
var_dump($result);
// array(2) {
//   ["@value"]=>
//   float(-1.123)        <-- negative values are also fine
//   ["#"]=>
//   string(3) "..."
// }

$xml = '<document value="-0.123">...</document>';
$result = $encoder->decode($xml, 'xml');
var_dump($result);
// array(2) {
//   ["@value"]=>
//   float(-0.123)        <-- ...even if negative value starts with 0
//   ["#"]=>
//   string(3) "..."
// }

Possible Solution
It looks like this behaviour was introduced in #32438 (as suggested here).
The naive approach would be to just extend the added check with the "but zero is not followed by a dot" condition, like in #38669.
Seems to work, but it feels like a nasty workaround, not a proper solution. And probably someone will find another case where it fails to produce a reasonable result.
Also, this section of code, i.e. trying to decode floats, has already been changed and fixed several times before (#22478, 8f6e67d and #32438), so it's not that easy to get it right and consider all edge cases. Maybe it would be better to try a different approach, something like #36482 or similar?
Btw, I assume we don't want to support comma as decimal separator.

Additional context
Normally I would say it's a limitation of the XmlEncoder, but returning a string only for floats which start with zero - it might be super confusing and difficult to debug.
For example (actually, that's how I found it), if you have a method which accepts floats (type hinting it) and receives as arguments values decoded by XmlEncoder, and the XML data looks like this:

...
<foo bar="1.9558"/>
<foo bar="25.653"/>
<foo bar="7.4615"/>
<foo bar="0.86828"/>
<foo bar="317.62"/>
...

The process will be sometimes failing, but it will look very randomly. Looking at the data, everything seems to be valid.

@fabpot fabpot closed this as completed Oct 23, 2020
fabpot added a commit that referenced this issue Oct 23, 2020
…th 0 (Marcin Kruk)

This PR was squashed before being merged into the 3.4 branch.

Discussion
----------

[Serializer] fix decoding float XML attributes starting with 0

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #38666
| License       | MIT
| Doc PR        | n/a

This is a naive approach to fix #38666, assuming it is something worth fixing.

I checked different cases and it seems to be fixing all of them, but I bet there will be some other edge cases which still won't be covered properly.

Commits
-------

97b4306 [Serializer] fix decoding float XML attributes starting with 0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants