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

Make sure Piwik tracks and archives correct data with any locale #6620

Open
tsteur opened this issue Nov 9, 2014 · 6 comments
Open

Make sure Piwik tracks and archives correct data with any locale #6620

tsteur opened this issue Nov 9, 2014 · 6 comments
Labels
Bug For errors / faults / flaws / inconsistencies etc.

Comments

@tsteur
Copy link
Member

tsteur commented Nov 9, 2014

See #6435

I think most of the issues related to German locale (or any other locale that uses a comma instead of a dot to separate decimal values) are fixed but it is hard to tell as most parts of the code are not really covered by tests.

After working on #6435 there can be only one conclusion the more I think about it: We always have to set an en_* locale and format numbers in Twig templates and API (if we want so) etc using Twig filter or using a library or so.

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 en_* locale is used and those bugs are in places where nobody would expect them see the changes and my comments. Even the PiwikTracker was sending wrong values with non en_* locale. Another thing is that we cannot make sure all the libraries work correctly. For instance the pdf library uses floats quite a lot but also Zend etc. Last but not least we cannot expect plugin developers to be aware of this problem. The worst part: As we do not use a German locale in our tests we would not even notice once we track or display wrong metrics.

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.

We should also add a system check to make sure an English locale is installed.

@mattab
Copy link
Member

mattab commented Nov 10, 2014

Be aware that our code sets the German locale as soon as someone uses German language.

I wonder the reason we set the locale, maybe there was a reason? If there was no reason, maybe we could set the en_US default locale instead?

@mattab mattab added the Bug For errors / faults / flaws / inconsistencies etc. label Nov 10, 2014
@mattab mattab added this to the Short term milestone Nov 10, 2014
@tsteur
Copy link
Member Author

tsteur commented Nov 10, 2014

As written in the other issue we need to set it to make sure it uses for example a comma instead of a dot in the UI to separate decimal

@mattab
Copy link
Member

mattab commented Apr 8, 2015

@tsteur what would you reckon is the next step for this issue (what could we do to prevent data error bugs due to locale)? eg. adding more CI tests for locale != en or something else maybe...

@tsteur
Copy link
Member Author

tsteur commented Apr 20, 2015

The next step to do is what I wrote in the issue description I reckon. I'd probably try to fix the underlying problem. More tests won't help us at all. Those issues occur where you don't expect them. And even currently on Travis the tests that exist are skipped or so.

It would be a lot of work but I reckon to actually fix it we'd have to use always en_* locale or so. Then handle localization etc in views or so. Maybe there's a better solution but that's like the only one I can think of currently.

@mattab
Copy link
Member

mattab commented Apr 21, 2015

After working on #6435 there can be only one conclusion the more I think about it: We always have to set an en_* locale and format numbers in Twig templates and API (if we want so) etc using Twig filter or using a library or so.

Now i'm wondering why we even set the locale in the first place. I didn't remember why I added this and reading the setlocale doc I can't see any feature that we actually need. it was likely added only for number formatting - so we can replace the setlocale call by a static setlocale('en... to fix this.

then to make Piwik look nice to all users in any of their language, we'd need to apply formatting to numbers before they are output in the UI and email reports (discussed in #4114)

@mattab
Copy link
Member

mattab commented Apr 21, 2015

. I didn't remember why I added this and reading the setlocale doc I can't see any feature that we actually need.

well I wrote this too quickly... it is important to setlocale to the current language's locale because for example string operations the core piwik or plugins rely on, may use the locale info. For example in php when eg. uppercase / lowercase a given letter ( eg lcfirst), (and more string functions) use locale information.

Suggested steps:

  • leave the current setlocale using current selected language
  • add call to setlocale(LC_NUMERIC, 'en...'); to set the locale to english for numbers/floats data manipulation
  • add a system check to make sure an English locale is installed
  • (out of scope/next project: format numbers in UI and email reports Number format thousands, decimal, abbreviate large numbers l10n #4114)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc.
Projects
None yet
Development

No branches or pull requests

2 participants