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
Wrong database values when float representation is not set to English standard #6435
Comments
Thanks for the bug report! until we properly support locale number representations in #4114 we should probably add could you confirm it works for you to add the code in https://github.com/piwik/piwik/blob/master/core/FrontController.php#L311-310 ? |
Doesn't seem to work there. I updated PIWIK to v2.8.3 and cleared the numeric values via
Not sure why that didn't work our. So I tested to put |
How did you start the archiving? Probably it was overwritten by https://github.com/piwik/piwik/blob/master/core/Translate.php#L222-L223 . Can you try to add it to FrontController and comment out the two lines in Translate.php temporarily? |
For archiving I just opened So It was indeed overwritten. Here's what my working code is looking like:
and
|
Maybe we should use something like |
👍 for number_format the value before writing in |
What about goals / conversion table etc? |
+1 forgot that are other |
Alright. Figured out we cannot use |
From what I have seen so far the best solution seems to be a |
The tests seem to still pass but the more I think about it the more it is a hack and it will most likely fail again at some point. Also it is very confusing when to use/convert it and when not. I am even sure I haven't covered all places and there could be also errors in some calculations. It also affects API requests. Requesting something in JSON + language DE will return a float as an actual float as supported by JSON using a "dot" see http://demo.piwik.org/index.php?module=API&method=API.getProcessedReport&idSite=7&period=day&date=2014-11-04&apiModule=UserCountry&apiAction=getCountry&showTimer=1&format=JSON&token_auth=anonymous&language=de whereas when requesting XML one will get a float with a comma as it was converted to a string in PHP before rendering it http://demo.piwik.org/index.php?module=API&method=API.getProcessedReport&idSite=7&period=day&date=2014-11-04&apiModule=UserCountry&apiAction=getCountry&showTimer=1&format=XML&token_auth=anonymous&language=de To actually fix this issue we should set the locale to Alternatively we could maybe always set @diosmosis @mnapoli @mattab thoughts? |
…not have to care about this. Makes it a tiny bit slower but less prone to errors
…sing locale de_DE... should try to submit a pull request forthis otherwise we will not be able to simply update this lib
…e is used (or any locle using comma instead of dot)
Here is a nice example of what I meant: When using German locale (or any other locale using comma instead of dot) it results in 'UTC-5,5' instead of 'UTC-5.5' which probably nobody would expect... resulting in a bug that it is not possible to add a site under circumstances as it says "Invalid Timezone" Such bugs are almost impossible to find: https://github.com/piwik/piwik/blob/19a2adbe7d8d7c9033cb99b4e57d2c5db515e836/plugins/SitesManager/API.php#L1247 Even Not sure how many more such issues there are. They are hard to identify and it is almost impossible to prevent more of those bugs unless we always work with |
If we do not always use a German locale I propose to run all tests using German locale in the future as most likely most of our users are using a locale that use a comma instead of a dot. We wouldn't notice such bugs otherwise |
It seems to me that we should try to set the locale to en_US or so - wouldnt' that help solve many problems and we wouldnt have to care as much about wrong locales? Though I know it's hard to set the setlocale since I see in codebase some other setlocale calls: https://github.com/piwik/piwik/search?utf8=%E2%9C%93&q=setlocale |
Yes, as mentioned. Problem is we then have to always format numbers manually when rendering anything to make sure we display a number using a comma instead of a dot in floats when a German or similar locale is used. This can be loads of work as well. Ideally Piwik wouldn't render everywhere in the code and then it would be easier to handle... so we probably have to weigh up the pros and cons |
There are code parts where it is hard to tell whether it causes bugs or not see https://github.com/piwik/piwik/blob/master/plugins/Actions/Reports/Base.php#L74 |
This caused me a lot of headache. I ran the system tests with a German locale and noticed so many values are completely different. I was looking for places where there were still any float to string conversions and I was wondering why the existing fix didn't work until I noticed the PiwikTracker did not send floats but floats with a comma as decimal point which in the end was casted to ints etc.
I just checked the Twig templates if they output any floats for usage in JavaScript but haven't found any yet. Also clicked through the UI with a German locale and couldn't find any JS error. If someone outputs something like |
I think most of the issues related to German locale (or other locale) are fixed but it is hard to tell as most parts of the code are not really covered by tests. Especially not by integration / unit tests which would be very helpful here. I think we can merge the current changes so it'll work at least for now (mostly). Then I think there can be only one conclusion the more I think about it: We always have to set an It is easy to explain: While we can live with the fact or bug when a number is not formatted correct in the UI (eg 5.5 instead of 5,5 => easy to fix) we cannot let the platform track or calculate wrong values. It is so easy to cause a bug / wrong values if not a Be aware that our code sets the German locale as soon as someone uses German language. Probably most of our users use a language where a comma is used instead of a dot. As soon as this locale is installed on the server we track wrong values. I am not sure how much work it is and whether we will be able to format the numbers everywhere but it is worth starting it even if it'll take a while. I think it is also worth it to run the UI tests with a locale/language that uses a comma instead of a dot in floats. Shall we handle all this in a separate issue? |
…not have to care about this. Makes it a tiny bit slower but less prone to errors
…sing locale de_DE... should try to submit a pull request forthis otherwise we will not be able to simply update this lib
…e is used (or any locle using comma instead of dot)
This caused me a lot of headache. I ran the system tests with a German locale and noticed so many values are completely different. I was looking for places where there were still any float to string conversions and I was wondering why the existing fix didn't work until I noticed the PiwikTracker did not send floats but floats with a comma as decimal point which in the end was casted to ints etc.
This one was not easy to find either. The segment value contained a comma instead of a dot causing the API to not find anything and to not return any data.
…le is used the wrong value will be saved (5,55 instead of 5.55)
@cauboy can you please try with 2.9.0-b7 just released? http://piwik.org/faq/how-to-update/faq_159/ This should be fixed, we'll close the issue once you confirm it's ok! |
ping @cauboy |
@mattab: Sorry for the late response… was a busy weekend. Approved: I updated to 2.9.0-b8 and it works fine for me. |
Problem:
The conversion rates in the Goal Plugin are floats but always integer valued (1,0%, 2,0%, …) on a server located in Germany.
Reason:
In PHP the automatic float-to-string conversion is based on the value of
setlocale(LC_NUMERIC)
. In English speaking countries a dot is used as the decimal separator while in some countries, e.g. Germany, a comma is used.Example, where this issue is critical:
This is harmful as the SQL query becomes invalid in the latter example.
Fix
Probably not a good solution, but for me the following change in
piwik/core/Db.php
solved the problem:Here are two posts about the setlocale issue:
http://mark-story.com/posts/view/php-floats-localization-and-landmines
http://blog.azure.km.ua/2012/12/php-typecasting-float-to-string-trap.html
The text was updated successfully, but these errors were encountered: