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

Fix round decimals with some non English locale #68

Closed
wants to merge 1 commit into from
Closed

Fix round decimals with some non English locale #68

wants to merge 1 commit into from

Conversation

sgomez
Copy link

@sgomez sgomez commented Dec 14, 2019

Decimals in SVG and EPS use the point notation. But in enviroments with intl module activate, the decimals can be represented with commas. For example when we use Spanish or Frech locale numeric.

I propose you to change round with number_format. This function does round too, uses point to decimal notation and returns a string.

@DASPRiD
Copy link
Member

DASPRiD commented Dec 14, 2019

This has nothing to do with the intl extension, but when you change the PHP locale to something else. This is simply not supported (changing the locale of the PHP process is a bad idea in general).

@DASPRiD DASPRiD closed this Dec 14, 2019
@sgomez sgomez deleted the fix-number-format branch December 14, 2019 15:39
@tscheepers
Copy link

@DASPRiD Currently the SVG generation code breaks, for all locales that use a comma instead of a dot for the decimal separator, e.g. mainland Europe locales. Dynamically setting it would be a bad idea, but supporting different locales from en_US would be nice right? This PR actually makes sense to me.

@wyattoday
Copy link
Contributor

I agree. This pull request should be accepted.

@DASPRiD
Copy link
Member

DASPRiD commented Apr 22, 2020

Well, I'm inclined to allow it, but you'd it needs to strip of redundant trailing zeroes, which this pull request right now wouldn't. It might make more sense to keep the rounding in place and use sprintf() around it instead with the %F formatting, which is non-locale aware.

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.

4 participants