search

Laravel-Backpack / CRUD

Build custom admin panels. Fast!
https://backpackforlaravel.com
MIT License
3.07k stars 884 forks source link

[Bug] Brazilian Portuguese lang folder name should be pt_BR (ISO 15897) #3537

Open matheusb-comp opened 3 years ago

matheusb-comp commented 3 years ago

Bug report

What I did

Configured the Laravel application locale using ISO 15897 codes in config/app.conf to:

'locale' => 'pt_BR',

Since the Laravel Documentation states:

For languages that differ by territory, you should name the language directories according to the ISO 15897. For example, "en_GB" should be used for British English rather than "en-gb".

What I expected to happen

Backpack strings such as "entries per page" to be translated to Brazilian Portuguese.

What happened

Text is shown in English, the configured fallback_locale.

What I've already tried to fix it

Checked that the folder names for Brazilian Portuguese are pt-BR and pt_br.

Screenshot from 2021-02-17 19-04-37

Of course, setting locale to pt-BR fixes the problem, but then it goes against Laravel recomendation to use the ISO 15897 language codes, and other languages like da_DK have lang folders with the proper name.

Backpack, Laravel, PHP, DB version

When I run php artisan backpack:version the output is:

### PHP VERSION:
PHP 7.4.3 (cli) (built: Oct  6 2020 15:47:56) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
    with Zend OPcache v7.4.3, Copyright (c), by Zend Technologies

### LARAVEL VERSION:
v8.21.0@a61cab167c35f465a923737ee6e6fb99cd5fde88

### BACKPACK VERSION:
4.1.34@d9ec59ab1f9fe03c7106911b5667d912c1cfe1f7
welcome[bot] commented 3 years ago

Hello there! Thanks for opening your first issue on this repo!

Just a heads-up: Here at Backpack we use Github Issues only for tracking bugs. Talk about new features is also acceptable. This helps a lot in keeping our focus on improving Backpack. If you issue is not a bug/feature, please help us out by closing the issue yourself and posting in the appropriate medium (see below). If you're not sure where it fits, it's ok, a community member will probably reply to help you with that.

Backpack communication channels:

Please keep in mind Backpack offers no official / paid support. Whatever help you receive here, on Gitter, Slack or Stackoverflow is thanks to our awesome awesome community members, who give up some of their time to help their peers. If you want to join our community, just start pitching in. We take pride in being a welcoming bunch.

Thank you!

-- Justin Case The Backpack Robot

pxpm commented 3 years ago

Hello @matheusb-comp thanks for raising this issue.

I guess you are right for what seems to be the most recent Laravel recommendation. I am not sure they recommended the same, or any other in past Laravel versions, atleast I could not find it: https://laravel.com/docs/7.x/localization#using-short-keys (for L7 for eg).

We can simply copy the contents of pt-BR into a new pt_BR folder and probably stick with Laravel convention (I think they only have one since now too), and deprecate pr-BR and any other malformed versions in 4.2

Edit: I just think we should address this now because we support L8 in 4.1, if that's the laravel convention I see this problem arising with other languages.

Let me know what you guys think @promatik @tabacitu

matheusb-comp commented 3 years ago

Apparently the text was added to the documentation recently, but it seems that Laravel already opted to use the POSIX locales format internally for some time.

An example of this can be seen in the Authentication UI view stub from version 5.7, where the underline from the locale is changed to a dash to conform with a BCP-47 language tag format.

<html lang="{{ str_replace('_', '-', app()->getLocale()) }}">
promatik commented 3 years ago

@matheusb-comp so the problem is the case? Because now we have pt-BR and pt_br folders, should it be pt_BR, with BR in uppercase?

This may be a problem because we can't have both, pt_br and pt_BR, so we would need to rename and "extinguish" the lowercase version, that may be in use by many.

@pxpm what do you think?

matheusb-comp commented 3 years ago

@promatik From what I understood, yes. The FileLoader applies the configured locale in the string and checks if the file exists.

The example given in the PR that added the note to the Laravel Documentation is also interesting, where the function used by trans_choice() to decide the plural index is a big switch-case that compares the locale using the {lang}_{REGION} format.

This may be a problem because we can't have both, pt_br and pt_BR

After re-reading the docs on localization, I realized that I can just override the lang files of the package. So I just copied the files from the package's pt_br folder to lang/vendor/backpack/pt_BR and it works now. The only problem with this is not getting automatic updates on these files.

pxpm commented 3 years ago

Indeed @promatik , I was thinking having both now, but yeah, it's impossible to have both folders. We can only fix this in 4.2 with a BC.

@matheusb-comp the lang strings don't change that much. Probably will not change until next version where we could introduce the breaking change, in the meanwhile you would not get "automatic" updates since those files are in user land, and can only be overriden if you specify --force when publishing assets.

Thanks guys for the clarification.

Best, Pedro

matheusb-comp commented 3 years ago

Changing only the Laravel lang folder name is also not enough. The locale is used to load dependencies i18n files, like the ones for Select2.

So the request for /packages/select2/dist/js/i18n/pt_BR.js results in a 404, and the widget shows texts in english.

Screenshot from 2021-02-19 16-49-58

promatik commented 3 years ago

@matheusb-comp good catch, in that case we need to; 1) standardize the locale string to ISO 15897 2) check every place where we use the locale strings and convert it in case it's needed.

tabacitu commented 3 years ago

Seems like exactly like what we're doing in #3598 , so let's move the conversation there please 🙏

PS. OMG what a shitshow with these language standards. Can't wait to be able to say "_just use pt_BR_" 😅

promatik commented 3 years ago

I'm reopening this issue since #3598 doesn't fix everything ✌