pkp/pkp-lib#9910 Use UI Language in Web Feeds #3
Conversation
There was a problem hiding this comment.
@bozana I just think it's better to keep the country information of the locale (if available).
|
Hi @jonasraoni, I considered the comments, could you please take another look? |
WebFeedGatewayPlugin.php
Outdated
| @@ -126,7 +128,8 @@ public function fetch($args, $request): bool | |||
| 'latestDate' => $latestDate, | |||
| 'feedUrl' => $request->getRequestUrl(), | |||
| 'userGroups' => $userGroups, | |||
| 'includeIdentifiers' => $includeIdentifiers | |||
| 'includeIdentifiers' => $includeIdentifiers, | |||
| 'language' => str_replace('_', '-', str_replace(['@cyrillic', '@latin', '_Latn', '_Cyrl'], '', Locale::getLocale())), | |||
There was a problem hiding this comment.
Looks like the BCP 47 format is roughly <language>-<script>-<region> (there are other optional sub-components, but not important for us), so this will work for our current locales, but other variants might break this in the future, because our locale has a different order <language>_<region>@<script>.
Also, the str_replace() seems to be too limited (I guess you've extracted this from the Locale class, because I saw someone added it there), as there are extra scripts that we're not covering.
I think it's enough just to fix the "order".
About the limited scripts on the replace, that's something for another issue.
There was a problem hiding this comment.
Hmmm... I was only looking at our currently existing UI locales and the fact that we will move to Weblate locales soon (s. pkp/pkp-lib#9707).
But, maybe I should consider all Weblate locales:
I see now that Weblate has different scripts (than Latn and Cyrl) but also something with @ (e.g. hi@hinglish, pt_BR@formal, sr@ijekavian_Latn).
Regarding the order: it seems Weblate locales have the order language_script_region (e.g. en_Shaw_US, zh_Hant_HK).
Thus, currently I am not sure what pattern would work to get it correctly. Maybe I can use PHP intl functions locale_get_primary_language and locale_get_region to get it right? 🤔
| @@ -126,7 +128,8 @@ public function fetch($args, $request): bool | |||
| 'latestDate' => $latestDate, | |||
| 'feedUrl' => $request->getRequestUrl(), | |||
| 'userGroups' => $userGroups, | |||
| 'includeIdentifiers' => $includeIdentifiers | |||
| 'includeIdentifiers' => $includeIdentifiers, | |||
| 'language' => locale_get_region(Locale::getLocale()) ? locale_get_primary_language(Locale::getLocale()) . '-' . locale_get_region(Locale::getLocale()) : locale_get_primary_language(Locale::getLocale()), | |||
There was a problem hiding this comment.
@jonasraoni, what about this solution?
There was a problem hiding this comment.
The PKP Locale class doesn't provide such things, so I agree with the fix. Another option is to make the Locale::_parse() method public.
About the code, I'd just store the locale_get_region(Locale::getLocale()) and locale_get_primary_language(Locale::getLocale()) in variables, to make the code shorter and perhaps use the class-based version of \Locale::getRegion() and \Locale::getPrimaryLanguage().
There was a problem hiding this comment.
Another possible problem, that makes me think that it's better to make that method public is that perhaps some PHP locales are not very compatible with our ones.
I mean, we have be@cyrillic, but a call to Locale::getScript('be@cyrillic') won't bring what we would expect, while be-Cyrl will.
|
@jonasraoni, what about this solution? |
s. pkp/pkp-lib#9910