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

[Form] Fix precision of MoneyToLocalizedStringTransformer's divisions and multiplications #24036

Merged
merged 1 commit into from Sep 29, 2017

Conversation

Projects
None yet
10 participants
@Rubinum
Contributor

Rubinum commented Aug 30, 2017

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? no
License MIT

There is a PHP Bug with the accuracy of divisions and multiplications when /= and *= are used.
Here is the proof: https://3v4l.org/u1DkX
It would be better to use bcmul() and bcdiv() in the MoneyToLocalizedStringTransformer.php to prevent this bug.

@xabbuh xabbuh added this to the 2.7 milestone Aug 30, 2017

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Aug 30, 2017

Member

IMO this is a bugfix and should then be based on the 2.7 branch.

Member

xabbuh commented Aug 30, 2017

IMO this is a bugfix and should then be based on the 2.7 branch.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Aug 30, 2017

Member

and even if we decide to consider it as a feature, it should go in 3.4, not in master (which is currently 4.0)

Member

stof commented Aug 30, 2017

and even if we decide to consider it as a feature, it should go in 3.4, not in master (which is currently 4.0)

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 30, 2017

Member

We should try harder to fix the issue for everyone without resorting to bcmath: the function_exists() check means some ppl will have the correct behavior, and some others will have the bug. This wouldn't qualify as a correct bugfix to me.

Member

nicolas-grekas commented Aug 30, 2017

We should try harder to fix the issue for everyone without resorting to bcmath: the function_exists() check means some ppl will have the correct behavior, and some others will have the bug. This wouldn't qualify as a correct bugfix to me.

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Aug 30, 2017

Member

@nicolas-grekas polyfill for bcmath? ;) Not that trivial I assume.

Member

Tobion commented Aug 30, 2017

@nicolas-grekas polyfill for bcmath? ;) Not that trivial I assume.

@dbrumann

This comment has been minimized.

Show comment
Hide comment
@dbrumann

dbrumann Aug 31, 2017

Contributor

@Tobion I think bcmath-polyfill wouldn't be out of the question. I have seen stuff like adding/multiplying large numbers as code kata before. I think it's more of a question of whether the trade off precision vs. performance is worth it? After all, if I know this issue I can avoid it in other ways in my application which better suit my needs. For example I could be doing (int)(string) $value which is fine in my code where I know that using the divisor will only return floats that should represent a full int value (and where I can probably add comments as to why this weird construct is there), but probably not in Symfony?

edit: But I would certainly be interested in seeing a more generic solution to this. I'm interested to see what @nicolas-grekas or someone else can come up with. 🙂

Contributor

dbrumann commented Aug 31, 2017

@Tobion I think bcmath-polyfill wouldn't be out of the question. I have seen stuff like adding/multiplying large numbers as code kata before. I think it's more of a question of whether the trade off precision vs. performance is worth it? After all, if I know this issue I can avoid it in other ways in my application which better suit my needs. For example I could be doing (int)(string) $value which is fine in my code where I know that using the divisor will only return floats that should represent a full int value (and where I can probably add comments as to why this weird construct is there), but probably not in Symfony?

edit: But I would certainly be interested in seeing a more generic solution to this. I'm interested to see what @nicolas-grekas or someone else can come up with. 🙂

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 31, 2017

Member

I think we can work around the issue by enforcing a precision of 14, which is way enough for a money transformer, and is the minimal precision to represent most float accurately (the max is 17, but we don't care for the potential precision loss at this scale in this specific case).

Member

nicolas-grekas commented Aug 31, 2017

I think we can work around the issue by enforcing a precision of 14, which is way enough for a money transformer, and is the minimal precision to represent most float accurately (the max is 17, but we don't care for the potential precision loss at this scale in this specific case).

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Aug 31, 2017

Contributor

When working with money/currency and precision is required, the usage of strings and proper libraries should be enforced. The usage of floats/doubles and simply using * and / can lead to extremely annoying bugs as shown in the example.

Contributor

iltar commented Aug 31, 2017

When working with money/currency and precision is required, the usage of strings and proper libraries should be enforced. The usage of floats/doubles and simply using * and / can lead to extremely annoying bugs as shown in the example.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 31, 2017

Member

See #21769 for a similar issue that we solved using cast-to-string.
We should do the same here IMHO.

Member

nicolas-grekas commented Aug 31, 2017

See #21769 for a similar issue that we solved using cast-to-string.
We should do the same here IMHO.

@Rubinum

This comment has been minimized.

Show comment
Hide comment
@Rubinum

Rubinum Sep 2, 2017

Contributor

@nicolas-grekas thank you for digging down through the older PRs.
As I understand the older PR that you mentioned, a solution could be to cast the whole operation to string? The return type of reverseTransform and transform would change from int|float to string.


/**
* Transforms a localized money string into a normalized format.
*
* @uses extension bcmath
*
* @param string $value Localized money string
*
* @return string Normalized number
*
* @throws TransformationFailedException If the given value is not a string
*                                       or if the value can not be transformed.
*/
public function reverseTransform($value)
    {
        $value = parent::reverseTransform($value);
        if (null !== $value) {
            $value = (string) ($value * $this->divisor);
        }
        return $value;
    }

/**
* Transforms a localized money string into a normalized format.
*
* @requires extension bcmath
*
* @param string $value Localized money string
*
* @return string Normalized number
*
* @throws TransformationFailedException If the given value is not a string
*                                       or if the value can not be transformed.
*/
public function transform($value)
    {
        if (null !== $value) {
            if (!is_numeric($value)) {
                throw new TransformationFailedException('Expected a numeric.');
            }
            $value = (string) ($value / $this->divisor);
        }
        return parent::transform($value);
    }

I mean we could do it the other way around and cast the string from my solution to float to stick to the return type:

$value = (float) function_exists('bcmul') ? bcmul((string) $value, (string) $this->divisor, $this->scale) : $value * $this->divisor;
/* ... */
$value = (float) function_exists('bcdiv') ? bcdiv((string) $value, (string) $this->divisor, $this->scale) : $value / $this->divisor;

But this looks a bit weird in my opinion ^^. What do you prefer?

Contributor

Rubinum commented Sep 2, 2017

@nicolas-grekas thank you for digging down through the older PRs.
As I understand the older PR that you mentioned, a solution could be to cast the whole operation to string? The return type of reverseTransform and transform would change from int|float to string.


/**
* Transforms a localized money string into a normalized format.
*
* @uses extension bcmath
*
* @param string $value Localized money string
*
* @return string Normalized number
*
* @throws TransformationFailedException If the given value is not a string
*                                       or if the value can not be transformed.
*/
public function reverseTransform($value)
    {
        $value = parent::reverseTransform($value);
        if (null !== $value) {
            $value = (string) ($value * $this->divisor);
        }
        return $value;
    }

/**
* Transforms a localized money string into a normalized format.
*
* @requires extension bcmath
*
* @param string $value Localized money string
*
* @return string Normalized number
*
* @throws TransformationFailedException If the given value is not a string
*                                       or if the value can not be transformed.
*/
public function transform($value)
    {
        if (null !== $value) {
            if (!is_numeric($value)) {
                throw new TransformationFailedException('Expected a numeric.');
            }
            $value = (string) ($value / $this->divisor);
        }
        return parent::transform($value);
    }

I mean we could do it the other way around and cast the string from my solution to float to stick to the return type:

$value = (float) function_exists('bcmul') ? bcmul((string) $value, (string) $this->divisor, $this->scale) : $value * $this->divisor;
/* ... */
$value = (float) function_exists('bcdiv') ? bcdiv((string) $value, (string) $this->divisor, $this->scale) : $value / $this->divisor;

But this looks a bit weird in my opinion ^^. What do you prefer?

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Sep 3, 2017

Member
public function reverseTransform($value)
{
    $value = parent::reverseTransform($value);

    if (null !== $value) {
        $value = (string) ($value * $this->divisor);

        if (ctype_digit($value)) {
            $value = (int) $value;
        } else {
            $value = (float) $value;
        }
    }

    return $value;
}

What about something like this?

Member

xabbuh commented Sep 3, 2017

public function reverseTransform($value)
{
    $value = parent::reverseTransform($value);

    if (null !== $value) {
        $value = (string) ($value * $this->divisor);

        if (ctype_digit($value)) {
            $value = (int) $value;
        } else {
            $value = (float) $value;
        }
    }

    return $value;
}

What about something like this?

@nicolas-grekas nicolas-grekas changed the title from Replaced division and multiplication operator by bcmul() and bcdiv(). to [Form] Fix precision of MoneyToLocalizedStringTransformer's divisions and multiplications Sep 11, 2017

@Rubinum

This comment has been minimized.

Show comment
Hide comment
@Rubinum

Rubinum Sep 12, 2017

Contributor

@smoench. Thank you for that hint. I removed it.

Contributor

Rubinum commented Sep 12, 2017

@smoench. Thank you for that hint. I removed it.

@smoench

This comment has been minimized.

Show comment
Hide comment
@smoench

smoench Sep 21, 2017

Contributor

Could this be merged?

Contributor

smoench commented Sep 21, 2017

Could this be merged?

@Rubinum Rubinum changed the base branch from master to 2.7 Sep 26, 2017

if (!is_numeric($value)) {
throw new TransformationFailedException('Expected a numeric.');
}
$value /= $this->divisor;
$value = (string) ($value / $this->divisor);

This comment has been minimized.

@xabbuh

xabbuh Sep 26, 2017

Member

This looks wrong to me. Previously, $value was a float here, but now it is a string.

@xabbuh

xabbuh Sep 26, 2017

Member

This looks wrong to me. Previously, $value was a float here, but now it is a string.

This comment has been minimized.

@Rubinum

Rubinum Sep 27, 2017

Contributor

I changed it from (float) (string) ($value / $this->divisor); to (string) ($value / $this->divisor); because this functions purpose is to Transforms a normalized format into a localized money string. according to the PHPDoc-Block. The return type is also string, thats why I changed it. Should I put a float cast in front of it again?

@Rubinum

Rubinum Sep 27, 2017

Contributor

I changed it from (float) (string) ($value / $this->divisor); to (string) ($value / $this->divisor); because this functions purpose is to Transforms a normalized format into a localized money string. according to the PHPDoc-Block. The return type is also string, thats why I changed it. Should I put a float cast in front of it again?

This comment has been minimized.

@Tobion

Tobion Sep 29, 2017

Member

that is fine. the parent transform should be able handle both string or float.

@Tobion

Tobion Sep 29, 2017

Member

that is fine. the parent transform should be able handle both string or float.

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Sep 29, 2017

Member

Some background: We know floats cannot be represented perfectly so there is some lost precision.
So a float like 36.55 * 100 can actually be 3654.99999999.
The problem when you cast that float to int, PHP cuts the scale and that leaves you with an 3654 instead of 6355. By string casting, PHP will use the number precision and realize 3654.99999999 is just meant to be 3655 and then casting that to float again the float will be represented as something like 3655.00000001 instead which will also cast to the expected integer again.

I wondered how many common montary values are affected by this difference and if it's reliable. Or if there are numbers where the string cast approach actually doesn't work as expected as well. I wrote a little script: https://3v4l.org/JdfjZ

For numbers between 1 and 99999.
Unexpected count traditional approach: 4586
Unexpected count new approach: 0

Member

Tobion commented Sep 29, 2017

Some background: We know floats cannot be represented perfectly so there is some lost precision.
So a float like 36.55 * 100 can actually be 3654.99999999.
The problem when you cast that float to int, PHP cuts the scale and that leaves you with an 3654 instead of 6355. By string casting, PHP will use the number precision and realize 3654.99999999 is just meant to be 3655 and then casting that to float again the float will be represented as something like 3655.00000001 instead which will also cast to the expected integer again.

I wondered how many common montary values are affected by this difference and if it's reliable. Or if there are numbers where the string cast approach actually doesn't work as expected as well. I wrote a little script: https://3v4l.org/JdfjZ

For numbers between 1 and 99999.
Unexpected count traditional approach: 4586
Unexpected count new approach: 0

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Sep 29, 2017

Member

That makes me wonder by PHP by default does not do this automatically, i.e. (int) $float === (int) (string) $float. I guess that would solve some unexpected floating precision with type casting either explicitly or implicitly by scalar type declarations.

Member

Tobion commented Sep 29, 2017

That makes me wonder by PHP by default does not do this automatically, i.e. (int) $float === (int) (string) $float. I guess that would solve some unexpected floating precision with type casting either explicitly or implicitly by scalar type declarations.

@Tobion

Tobion approved these changes Sep 29, 2017

@fabpot

fabpot approved these changes Sep 29, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 29, 2017

Member

Thank you @Rubinum.

Member

fabpot commented Sep 29, 2017

Thank you @Rubinum.

@fabpot fabpot merged commit ab47c78 into symfony:2.7 Sep 29, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Sep 29, 2017

bug #24036 [Form] Fix precision of MoneyToLocalizedStringTransformer'…
…s divisions and multiplications (Rubinum)

This PR was merged into the 2.7 branch.

Discussion
----------

[Form] Fix precision of MoneyToLocalizedStringTransformer's divisions and multiplications

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | no
| License       | MIT

There is a [PHP Bug](https://bugs.php.net/bug.php?id=75004) with the accuracy of divisions and multiplications when `/=` and `*=` are used.
Here is the proof: https://3v4l.org/u1DkX
It would be better to use `bcmul()` and `bcdiv()` in the `MoneyToLocalizedStringTransformer.php` to prevent this bug.

Commits
-------

ab47c78 Added improvement for accuracy in MoneyToLocalizedStringTransformer.
@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Sep 29, 2017

Member

I've found out the behavior also depends on the precision ini setting.

When setting ini_set('precision', 17); or when using the new ini_set('precision', -1); from https://wiki.php.net/rfc/precise_float_value, there is no unexpected behavior.
So for master with php 7.1., we could remove these workarounds and add recommendation to our requirements to use ini_set('precision', -1) which seems much better in most cases. That is the new PHP default anyway, but some users might not make use of it and use old ini values.
For example https://3v4l.org does not use the new ini even for PHP >= 7.1 by default.

Member

Tobion commented Sep 29, 2017

I've found out the behavior also depends on the precision ini setting.

When setting ini_set('precision', 17); or when using the new ini_set('precision', -1); from https://wiki.php.net/rfc/precise_float_value, there is no unexpected behavior.
So for master with php 7.1., we could remove these workarounds and add recommendation to our requirements to use ini_set('precision', -1) which seems much better in most cases. That is the new PHP default anyway, but some users might not make use of it and use old ini values.
For example https://3v4l.org does not use the new ini even for PHP >= 7.1 by default.

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Sep 29, 2017

Member

Actually, this change is not reliable at all what I was fearing the whole time. It only works because the precision is only 14. But if you increase the precision or use PHP 7.1 with more precision by default, (float) (string) $float does not change anything at all. See https://3v4l.org/La5iR

Member

Tobion commented Sep 29, 2017

Actually, this change is not reliable at all what I was fearing the whole time. It only works because the precision is only 14. But if you increase the precision or use PHP 7.1 with more precision by default, (float) (string) $float does not change anything at all. See https://3v4l.org/La5iR

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 29, 2017

Member

We could for precision to 14 if we want to provide robustness for this (here and in #21769)
(or we an assume nobody changes this "precision" setting and ensure this in check.php)
@Tobion would you do one of the 2 PRs?

Member

nicolas-grekas commented Sep 29, 2017

We could for precision to 14 if we want to provide robustness for this (here and in #21769)
(or we an assume nobody changes this "precision" setting and ensure this in check.php)
@Tobion would you do one of the 2 PRs?

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Sep 29, 2017

Member

Neither of that is a proper solution. The only one is using fixed precision calculation with bcmath.

Member

Tobion commented Sep 29, 2017

Neither of that is a proper solution. The only one is using fixed precision calculation with bcmath.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 29, 2017

Member

Then bcmath for ppl having it and precision 14 for others? I don't think we can add a strong requirement for bcmath, do you?

Member

nicolas-grekas commented Sep 29, 2017

Then bcmath for ppl having it and precision 14 for others? I don't think we can add a strong requirement for bcmath, do you?

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Sep 29, 2017

Member

Using bcmath and precision as fallback seems fine to not break BC. But IMO we should add requirement for bcmath in Symfony 4 so we can remove that ugly workaround. Bcmath is included by default in php so that shouldn't cause much trouble.

Member

Tobion commented Sep 29, 2017

Using bcmath and precision as fallback seems fine to not break BC. But IMO we should add requirement for bcmath in Symfony 4 so we can remove that ugly workaround. Bcmath is included by default in php so that shouldn't cause much trouble.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 29, 2017

Member

It's included really? Cool, so we can trigger a deprecation on 3.4 and throw on 4.0.

Member

nicolas-grekas commented Sep 29, 2017

It's included really? Cool, so we can trigger a deprecation on 3.4 and throw on 4.0.

This was referenced Oct 5, 2017

fabpot added a commit that referenced this pull request May 17, 2018

bug #26781 [Form] Fix precision of MoneyToLocalizedStringTransformer'…
…s divisions on transform() (syastrebov)

This PR was merged into the 2.7 branch.

Discussion
----------

[Form] Fix precision of MoneyToLocalizedStringTransformer's divisions on transform()

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | no
| License       | MIT
| Doc PR        |

Related issue #21026.
Previous PR #24036.
Similar fix for `transform()` method.

Commits
-------

f94b7aa fix rounding from string

symfony-splitter pushed a commit to symfony/form that referenced this pull request May 17, 2018

bug #26781 [Form] Fix precision of MoneyToLocalizedStringTransformer'…
…s divisions on transform() (syastrebov)

This PR was merged into the 2.7 branch.

Discussion
----------

[Form] Fix precision of MoneyToLocalizedStringTransformer's divisions on transform()

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | no
| License       | MIT
| Doc PR        |

Related issue symfony/symfony#21026.
Previous PR symfony/symfony#24036.
Similar fix for `transform()` method.

Commits
-------

f94b7aadd3 fix rounding from string
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment