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

[5.4] Adding missing locales to Translation MessageSelector #19562

Merged
merged 2 commits into from
Jun 11, 2017

Conversation

jn-jairo
Copy link
Contributor

Translations in the format below were returning the wrong plural with locales like pt_BR, because it does not find the locale in the getPluralIndex and always returns the default 0.

'apples' => 'There is one apple|There are many apples',

echo trans_choice('messages.apples', 1);
// There is one apple

echo trans_choice('messages.apples', 0);
// There is one apple

@lucasmichot
Copy link
Contributor

Should this not target master branch ?

case 'zh_CN':
case 'zh_HK':
case 'zh_SG':
case 'zh_TW':
return 0;
break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useless break

case 'nso':
case 'nso_ZA':
case 'xbr':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wbr should be after wa_BE

@jn-jairo
Copy link
Contributor Author

@lucasmichot Thanks for your review.

I don't know if it should target the master branch.

I'm using the version 5.4 and the trans_choice is not working for my locale pt_BR, so i fixed it, and target the 5.4 branch because I need this fix in the version 5.4.

If I need to target the master branch, I can do it, I just need to be sure that the master branch is the correct to make this change applied to 5.4 too.

@taylorotwell taylorotwell merged commit 6f3b325 into laravel:5.4 Jun 11, 2017
@taylorotwell
Copy link
Member

5.4 is fine. It's basically a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants