-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix: [11.0] Default locale wasn't correctly loaded when the browser only sent a part of the language like pl #22388
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
base: 11.0/bugfixes
Are you sure you want to change the base?
Conversation
d316553 to
6e42c39
Compare
|
Finally, this one won't differ from 10.0 one; so maybe just wait for 10.0/bf to be merged is OK. |
Yes absolutly, the only diff was using the Symfony method (and different file location). That why I'll let it for now in draft, or maybe we can even close this one ? |
Location change is handled by the merge; not a problem. |
3c3f99e to
9f41f6d
Compare
… a part of the language like pl insteand of pl_PL
…ages mapping in CFG_GLPI
6e42c39 to
a09fece
Compare
|
Anything relating to locales is going to be difficult and annoying to maintain ourselves. Please use a list of fallback regions from an official or standardized source like: I checked if this kind of list was included with the other CLDR stuff in the GetText package but it seems not. Edit: Maybe there is a cleaner source of just the default regions, but I haven't found it yet. |
|
|
||
| // Mapping of short language codes to their main locale | ||
| $CFG_GLPI['main_languages'] = [ | ||
| // 'ar' => 'ar_SA', // not sure about the default region for arabic language |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // 'ar' => 'ar_SA', // not sure about the default region for arabic language | |
| 'ar' => 'ar_SA', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about that, if not defined it will fallback to glpi default en_GB, so maybe it's better ?
I'm not an expert on arabic language but I think they are very different and I'm afraid in that case the user won't understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Unicode CLDR data shows ar_EG (Egypt) as the default region based on Egypt having a larger population, but we don't have that locale.
Another sane default would be ar_001 (world Modern Standard Arabic) if we had it.
Of the Arabic locales we have (Saudi Arabia, Iraq and Syria), Iraq would be the next largest by population given it surpassed SA's population in 2011.
From what I can find, ar_SA aligns closely with Modern Standard Arabic. There are not enough Arabic users of GLPI to get data from telemetry. From looking at our translations, Syria is only 2.5% translated for GLPI 11 while the other 2 are 99%.
My choice of ar_SA is based on it being more conventionally used as a default, but ar_IQ makes sense too. In reality, the translations are almost identical between the two and this fallback only really affects a small part of GLPI like the login screen.
Additionally, we could simply have the browser cache the last selected language in GLPI in the local storage so if I choose fr_FR but my browser is English, the next time I visit the GLPI login screen it will remain in fr_FR. Not a priority, but would probably be done at some point in the future anyways if moving more stuff client-side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As stated in the v10 PR, if we're not sure, we do not set default for now, and we'll adapt if needed.
Anyway, the the usage in GLPI, or the percent of translation seems not relevant (and that could change).
| 'nn' => 'nn_NO', | ||
| 'fa' => 'fa_IR', | ||
| 'pl' => 'pl_PL', | ||
| 'pt' => 'pt_PT', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 'pt' => 'pt_PT', | |
| 'pt' => 'pt_BR', |
Brazil has approximately 20x the population of Portugal, which makes the Brazil variant more common globally. In this case where we are guessing a user's region, it is more likely to be correct when using the variant with the most population. Also, going by GLPI's own telemetry pt_BR makes up 22% of default languages while pt_PT is just 0.95%. It is also the default region listed in the Unicode CLDR data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As said in a previous comment, I'm not sure the usage, nor the percent of translation is relevant here.
If we take the reverse: pt_PT and pt_BR will both fallback to pt when region is not specified. I'm not sure pt is meant to be Braziilan :/
Trying to guess a region fro a global locale is just a hack, there always will have cases that won't work.
Ho, and if it's to be changed, it must be done on v10 as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are talking about guessing a complete locale in a case where the user isn't authenticated and the browser does not send complete info. As with the data from CLDR, it is about guessing what the most likely locale would be. They use population data as a factor for their default region selection as did I. The only difference is I also took into account GLPI user "population". Since Brazil has a much larger population and both countries speak a form of Portuguese, a user with "pt" as the language is most likely a Portuguese (Brazilian) speaker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK for this one. This still must be changed on v10 as well.
Checklist before requesting a review
Description
Dedicated optional fix for #22321
This PR is dedicated for GLPI 11, it's based on the PR #22387 but it's use the Symfony method instead of using our custom http language extract.
This PR can be closed if we prefer to stay with that logic instead of using this one.
Otherwise the fallback logic is a copy paste from the 10.x PR