Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

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

Open
matheusb-comp opened this issue Feb 17, 2021 · 10 comments
Open

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

matheusb-comp opened this issue Feb 17, 2021 · 10 comments
Labels
breaking change next-version Priority: SHOULD refactoring

Comments

@matheusb-comp
Copy link
Contributor

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
Copy link

welcome bot commented Feb 17, 2021

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:

  • Bug Reports, Feature Requests - Github Issues (here);
  • Quick help (How do I do X) - Gitter Chatroom;
  • Long questions (I have done X and Y and it won't do Z wtf) - Stackoverflow, using the backpack-for-laravel tag;
  • Showing off something you've made, asking for opinion on Backpack/Laravel matters - Reddit;

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
Copy link
Contributor

pxpm commented Feb 18, 2021 •
edited
Loading

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

@pxpm pxpm removed the triage label Feb 18, 2021
@matheusb-comp
Copy link
Contributor Author

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
Copy link
Contributor

@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
Copy link
Contributor Author

@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
Copy link
Contributor

pxpm commented Feb 19, 2021

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
Copy link
Contributor Author

matheusb-comp commented Feb 19, 2021 •
edited
Loading

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
Copy link
Contributor

@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.

@pxpm pxpm mentioned this issue Apr 16, 2021
Merged
@tabacitu
Copy link
Member

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
Copy link
Contributor

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

@promatik promatik reopened this Apr 24, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees
No one assigned
Labels
breaking change next-version Priority: SHOULD refactoring
Projects
Backpack v7.0
Status: No status
Backpack v6.0.0 (🚀 on 1st of July 2023)
Status: No status
Milestone
No milestone
Development

No branches or pull requests
4 participants
@tabacitu @matheusb-comp @promatik @pxpm