-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Falling back to first translation if no choice available #6619
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
Conversation
This makes the fallback better, but we should probably log a warning if possible as the translation is still invalid (and so should be fixed to have the proper pluralization in your UI) |
@stof as we don’t have a logger in the MessageSelector, whose job would it be to log a message? |
@@ -74,7 +74,7 @@ public function choose($message, $number, $locale) | |||
|
|||
$position = PluralizationRules::get($number, $locale); | |||
if (!isset($standardRules[$position])) { | |||
throw new \InvalidArgumentException(sprintf('Unable to choose a translation for "%s" with locale "%s". Double check that this translation has the correct plural options (e.g. "There is one apple|There are %%count%% apples").', $message, $locale)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to remove the phpdoc exception too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks!
There is a PR for adding logging in the Translator component (see #5537 -- this should probably be updated to use the new PSR-3 logger interface) |
@fabpot I don’t think this PR should wait for the logging support as it possibly breaks existing code bases. |
ping |
Why break BC on this, if you can make use of inheritance? I see value in both approaches |
We're also encountering this issue. If a user doesn't pass the MaxLength validation constraint, we are receiving the error:
I've provided the exact message in the validator, so I wouldn't think that the translation would be necessary. |
@jeremylivingston the MaxLength constraint is providing a pluralization, so the translation is done with |
@stof What if the message doesn't have separate plural and singular versions? It seems odd to force this even if it may not apply. I think that there should be a way to specify a single message in cases where you don't require a plural version. It should then know that if there are two messages that the latter is the plural version. Would this be a better implementation for a future version? I can add the same message for now, but it seems that this solution would be more flexible. |
@jeremylivingston The alternatives don't need to be in the key. they have to be in the translated string. So the renderer cannot use the key to decide whether the message is translated or no. |
related to #6988 |
I'm leaning toward 👎 on this. Removing the exception could lead to some confusion when users think their code is doing one thing but is actually doing another. My fix to the validator makes sense because the user is not explicitly calling the translator so we need flexibility there. Otherwise an exception is appropriate. |
I totally agree with @kriswallsmith. Closing. |
Bug fix: yes
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #4228
License of the code: MIT