Don’t rely on the current locale
ausi opened this issue · 10 comments
See contao/core#8592
A solution could be to use number_format() instead of number-to-string conversion.
While this issue could be fixed easily, I’m not sure if a locale other than C for LC_NUMERIC should be a supported environment.
If we want Contao to run independent of the locale settings, we would not only have to change all float-to-string conversions but also every call to strtoupper or strtolower that relies on a mapping between ABCDEFGHIJKLMNOPQRSTUVWXYZ and abcdefghijklmnopqrstuvwxyz. E.g. with a tr_TR locale the output of strtoupper('i') can result in either i, I or İ.
I think changing the locale for LC_NUMERIC or LC_CTYPE is a misconfiguration and should not be supported by Contao. Setting LC_MONETARY, LC_TIME or LC_MESSAGES should be safe IMO. I’m not sure about LC_COLLATE.
@contao/developers what do you think? Should we support such environments, or should we add a requirement that LC_NUMERIC and LC_CTYPE have to be set to C?
@Xendiadyon can you explain why you are using such a locale configuration?
It certainly is NOT a misconfiguration as the round() function is intended for DISPLAYING numeric formats for humans and NOT to generate machine readable output.
If we follow your argumentation, setting any locale (date etc) is "misconfiguration" because humans should simply read as the machine does.
We must however ensure our output is the same as the next system in the chain (in this case the browser) expects them to be.
The occurences where we use round() must only be targeted at screen output (like showing in HTML and the like) and not for script usage, we need to hunt these down.
Therefore, it is a misusage and not misconfiguration IMO.
Using number_format should be just fine.
Sidenote: many providers preconfigure the locale and do not allow customers to change them on webserver level, others don't even let users configure the php.ini setting and on even other systems the locale is system wide defined. We can't let those fall down simply because we are too lazy to do our homework properly.
Edit: corrected the function name.
round() is used to round a number and it returns a float, it doesn’t rely on the locale, it just handles numbers and isn’t related to this issue I think.
This issue is about the behavior of things like casting a float into a string. E.g.: (string)1.2 could result in "1.2" or "1,2" depending on the locale. Also strtoupper('i') returns different results depending on the locale.
I'm with @discordier on this one. We should not rely on a function that relies on a user configurable setting here. Especially not as we know that we always need a specific formatting!
round() is great when you want to display a rounded number, as it will automatically format the output depending on the locale. The same is true for casting a float into a string, because if the locale is de_DE, we want (string) 12.5 to become 12,5.
But in an evaluation, we always want a float to be a float, therefore we have to use number_format().
round() will not automatically format the number. It's only the conversion from a float to a string.
round()is great when you want to display a rounded number, as it will automatically format the output depending on the locale.
As I already mentioned, this is not the case. round() returns a float and does not rely on the locale.
I just found out that Symfony supports different locales for LC_NUMERIC, see symfony/symfony#2557. So I think we should support it in contao/image too and I will fix this issue.
But setting LC_CTYPE does not work in Symfony (and Contao) currently, I opend symfony/symfony#20979 for that.
As I already mentioned, this is not the case.
round()returns afloatand does not rely on the locale.
Yes, I was wrong about that. Sorry.
Me also, however, my case stands, when converting to string, we must ensure it is still machine readable.
I wrote above in a hurry.
OK, I think we agree that setting the locale for LC_NUMERIC should be a supported environment, as it is in Symfony.
If you have an opinion on LC_CTYPE too, please comment on symfony/symfony#20979.