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

Adding support for environments that have a system locales with different decimal separator #20

Merged
merged 3 commits into from
Feb 12, 2019
Merged

Adding support for environments that have a system locales with different decimal separator #20

merged 3 commits into from
Feb 12, 2019

Conversation

manowark
Copy link
Contributor

@manowark manowark commented Feb 11, 2019

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

STR:

  1. environment must have locale with decimal separator different from . (for example de_DE.UTF-8) or locale set by terminal variable like export LC_ALL=de_DE.UTF-8
  2. run any code which do calculations with float values and try to convert value to brick/math type

For example:

$decimal = \Brick\Math\BigDecimal::of(5/2);

It will not give an error if locale is equal to eu_US. But if locale is equal to de_DE then there will be an error Brick\Math\Exception\NumberFormatException : The given value "2,5" does not represent a valid number..
It happened because in this environment PHP will use , as decimal separator and ::of() method must be sensitive to it.

It can also happen when using Doctrine and convert float values from DB to brick/math type.

@BenMorel
Copy link
Member

Hi, thanks for your PR. TBH I had no idea that float to string conversion was affected by the current locale (I personally try to stay away from using setlocale()).

One thing that bothers me with your approach, is that you're changing the current behaviour to also parse strings using the current locale; while I agree that parsing a float, e.g. BigDecimal::of(5/2) must work whatever the current locale, I don't think that string parsing should be affected by the locale (we could add another locale-aware method to parse those, though, if that's useful).

I'd rather change this line, when $value is a float, to convert it to a string using a dot character prior to running the current regexp.

The problem is that apart from temporarily reverting setlocale() to 'C', I don't think there is a way to replicate PHP's variable-precision float to string conversion: sprintf's %F could have been a good candidate as it always outputs a dot, but it's fixed width.

@BenMorel
Copy link
Member

Actually, this approach breaks a lot of tests:
https://travis-ci.org/brick/math/jobs/491765751

This is because strings that were using dots are not parsed correctly after a global setlocale() has been applied, as mentioned above.

…rent decimal separator

 - added normalizing for string after conversion to it from float value
@manowark
Copy link
Contributor Author

@BenMorel, I added normalizing of string after conversion to it form float. Now I don't change regexp to valid format from locale configuration, just convert string to valid format.

src/BigNumber.php Outdated Show resolved Hide resolved
src/BigNumber.php Outdated Show resolved Hide resolved
tests/BigDecimalTest.php Outdated Show resolved Hide resolved
tests/BigDecimalTest.php Outdated Show resolved Hide resolved
@BenMorel
Copy link
Member

@manowark With the suggested changes it should be good to go! 👍

…rent decimal separator

 - fixed after code review
@manowark
Copy link
Contributor Author

@BenMorel, done.

@BenMorel BenMorel merged commit 510f506 into brick:master Feb 12, 2019
@BenMorel
Copy link
Member

Nice and clean. Thanks!

@manowark manowark deleted the big_decimal_in_different_locales branch February 12, 2019 10:43
@manowark
Copy link
Contributor Author

manowark commented Feb 12, 2019

@BenMorel, Thanks for the quick response and constructive comments.
When can I download the package with this fix? I mean tag 0.8.5.

@BenMorel
Copy link
Member

I'm going to release 0.8.5 as soon as I get the confirmation from Travis CI that master is OK! It should not take more than a few minutes.

@BenMorel
Copy link
Member

0.8.5 is released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants