8000 [Security] Missing lt translations by rmikalkenas · Pull Request #41097 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Security] Missing lt translations #41097

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 13, 2021
Merged

[Security] Missing lt translations #41097

merged 1 commit into from
May 13, 2021

Conversation

rmikalkenas
Copy link
Contributor
@rmikalkenas rmikalkenas commented May 3, 2021 •
edited
Loading
Q A
Branch? 4.4
Bug fix? no
New feature? no
Deprecations? no
Tickets Fix #41054
License MIT

For minute/minutes translation I decided to use "min." abbreviation, because in lithuanian language the plural translation might not always match the word case.
For example:
1 minute -> 1 minutė
2 minutes -> 2 minutės
...
10 minutes -> 10 minučių
...
21 minutes -> 21 minutė
22 minutes -> 22 minutės
...
30 minutes -> 30 minučių

and so on...

@carsonbot carsonbot added this to the 4.4 milestone May 3, 2021
@rmikalkenas rmikalkenas force-pushed the 41054-lt-translation branch 2 times, most recently from 63abfa1 to 97e24f6 Compare May 3, 2021 17:37
@norkunas
Copy link
Contributor
norkunas commented May 4, 2021

I think pluralization could work in this form:

Per daug nepavykusių prisijungimo bandymų, pabandykite dar kartą po %minutes% minutės.|Per daug nepavykusių prisijungimo bandymų, pabandykite dar kartą po %minutes% minučių.|Per daug nepavykusių prisijungimo bandymų, pabandykite dar kartą po %minutes% minučių.

@rmikalkenas
Copy link
Contributor Author

@norkunas it would work if I modify translation file to support ICU message format (https://symfony.com/doc/current/translation/message_format.html), but I'm not sure if its a good way as other files are not using icu format.. Maybe @Nyholm could elaborate how to solve this issue..?

@norkunas
Copy link
Contributor
norkunas commented May 4, 2021

I looked in https://github.com/symfony/symfony/blob/5.x/src/Symfony/Component/Validator/Resources/translations/validators.lt.xlf#L27 so I thought it is supported :)

@rmikalkenas
Copy link
Contributor Author

Hm.. Somehow I thought that this approach is deprecated and no longer works, but looks like it still works, but you have to pass %count% => (int) parameter to translator. Anyway, if we want to use this approach, it would require to add %count% parameter to https://github.com/symfony/symfony/blob/5.x/src/Symfony/Component/Security/Core/Exception/TooManyLoginAttemptsAuthenticationException.php#L35 , but the pull request is aimed to 4.4 branch which currently does not contain this file. Maybe I should open another PR to add additional parameter?

@nicolas-grekas
Copy link
Member

Yes, please sent the PR on 5.2, we should probably keep %messages% next to %count%.

@rmikalkenas rmikalkenas mentioned this pull request May 8, 2021
Merged
@rmikalkenas rmikalkenas force-pushed the 41054-lt-translation branch from 97e24f6 to f2dedd8 Compare May 8, 2021 09:04
@rmikalkenas
Copy link
Contributor Author

Opened a separate PR to add %count% argument: #41134
And updated this PR to support plural translations

wouterj added a commit that referenced this pull request May 9, 2021
@wouterj
minor #41134 [Security] Translation count argument for TooManyLoginAt…
2a7b85d 
…temptsAuthenticationException (rmikalkenas)

This PR was merged into the 5.2 branch.

Discussion
----------

[Security] Translation count argument for TooManyLoginAttemptsAuthenticationException

| Q             | A
| ------------- | ---
| Branch?       | 5.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| License       | MIT

Added `%count%` argument to leverage translator's pluralization functionality as discussed in #41097

Commits
-------

2bf0b48 Provide count argument for TooManyLoginAttemptsAuthenticationException to be able to translate in plural way
symfony-splitter pushed a commit to symfony/security-core that referenced this pull request May 9, 2021
@wouterj
minor #41134 [Security] Translation count argument for TooManyLoginAt…
942b40f 
…temptsAuthenticationException (rmikalkenas)

This PR was merged into the 5.2 branch.

Discussion
----------

[Security] Translation count argument for TooManyLoginAttemptsAuthenticationException

| Q             | A
| ------------- | ---
| Branch?       | 5.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| License       | MIT

Added `%count%` argument to leverage translator's pluralization functionality as discussed in symfony/symfony#41097

Commits
-------

2bf0b485f9 Provide count argument for TooManyLoginAttemptsAuthenticationException to be able to translate in plural way
Nyholm
Nyholm approved these changes May 12, 2021
View reviewed changes
Copy link
Member
@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this and for making that other PR too.

Just one small question/suggestion.

src/Symfony/Component/Security/Core/Resources/translations/security.lt.xlf
@@ -70,6 +70,14 @@
<source>Invalid or expired login link.</source>
<target>Netinkama arba pasibaigusio galiojimo laiko prisijungimo nuoroda.</target>
</trans-unit>
<trans-unit id="19">
<source>Too many failed login attempts, please try again in %minutes% minute.</source>
<target>Per daug nepavykusių prisijungimo bandymų, pabandykite dar kartą po %minutes% minutės.</target>
Copy link
Member

Choose a reason for hiding this comment

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

If it make sense, You can write "one minute" here instead.

Ie (google translate) "vieną minutę".

Copy link
Contributor
@norkunas norkunas May 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If it make sense, You can write "one minute" here instead.

Ie (google translate) "vieną minutę".

actually "po 1 minutės" or "po vienos minutės" :)

Copy link
Member

Choose a reason for hiding this comment

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

If you prefer "1 minute" over "one minute" then I'm happy with this PR as it is.

=)

@Nyholm
Copy link
Member
Nyholm commented May 13, 2021

Awesome! Thank you

@Nyholm Nyholm merged commit 9066677 into symfony:4.4 May 13, 2021
@Nyholm Nyholm mentioned this pull request Jun 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nyholm Nyholm Nyholm approved these changes

@norkunas norkunas norkunas left review comments

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

@wouterj wouterj Awaiting requested review from wouterj wouterj is a code owner

Assignees
No one assigned
Labels
Missing translations Security Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
5 participants
@rmikalkenas @norkunas @nicolas-grekas @Nyholm @carsonbot
0