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

[Translation] TransChoice invalide plural translation #31110

Conversation

Projects
None yet
6 participants
@Simperfit
Copy link
Contributor

commented Apr 15, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #27401
License MIT
Doc PR symfony/symfony-docs#11425

@Simperfit Simperfit force-pushed the Simperfit:feature/allow-to-not-throw-when-using-trans-and-transchoice branch from d3ded2f to f979dea Apr 15, 2019

@Simperfit Simperfit force-pushed the Simperfit:feature/allow-to-not-throw-when-using-trans-and-transchoice branch from f979dea to 9c13ce2 Apr 15, 2019

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 15, 2019

@stof

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

and this does not solve the similar issue when using the ICU format (MessageFormatter also throws for invalid pluralization)

@Simperfit Simperfit force-pushed the Simperfit:feature/allow-to-not-throw-when-using-trans-and-transchoice branch from 9c13ce2 to 8a87162 Apr 15, 2019

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

First of all thanks @ro0NL and @nicolas-grekas for the review, I'm working on this right now and wanted to know what we should return, an empty string ?

Do we update all the translation components (such as TranslationExtension, MessageFormatter) or do we only do that on the transChoice of the IdentityTranslator so we are strictly following the issue. (that's what I've done right now, tell me if we want to go further.

@Simperfit Simperfit force-pushed the Simperfit:feature/allow-to-not-throw-when-using-trans-and-transchoice branch from 8a87162 to 0aa48c2 Apr 15, 2019

@ro0NL
Copy link
Contributor

left a comment

what we should return

the default behavior?

return strtr($id, $parameters);

@@ -10,7 +10,9 @@
<service id="Symfony\Component\Translation\TranslatorInterface" alias="translator" />

This comment has been minimized.

Copy link
@ro0NL

ro0NL Apr 16, 2019

Contributor

the default identity translator above this one should be updated as well.. might even be a public alias to identity_translator 🤔

Show resolved Hide resolved src/Symfony/Component/Translation/IdentityTranslator.php Outdated
Show resolved Hide resolved src/Symfony/Component/Translation/IdentityTranslator.php Outdated
Show resolved Hide resolved src/Symfony/Component/Translation/IdentityTranslator.php Outdated
Show resolved Hide resolved src/Symfony/Component/Translation/IdentityTranslator.php Outdated
@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

last but not least; trans() (derived from the trait) might still throw in case of a plural message ... should we use it as doTrans, and try/catch it as well in IdentityTranslator::trans()? Not sure if that's what @stof meant...

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

@ro0NL I don't know, I think it could be on the user at this point.

@Simperfit Simperfit force-pushed the Simperfit:feature/allow-to-not-throw-when-using-trans-and-transchoice branch from 0aa48c2 to a7fef52 Apr 22, 2019

@ro0NL
Copy link
Contributor

left a comment

@Simperfit isnt it weird transChoice is "debug-able" whereas trans() is not? Esp. since transChoice is deprecated.. so $debug is useless in 5.0?

Moreover, what to do with

$translator = new class() implements TranslatorInterface, LocaleAwareInterface {
use TranslatorTrait;
};
should it be "debug aware" also?

@stof

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

trans can throw in 2 cases:

  • using an ICU formatter, with an invalid pattern (MessageFormatter will throw in its constructor, our polyfill will throw later due to symfony/polyfill#158)
  • using the Symfony legacy format with some pluralization (the alternative to the deprecated transChoice)
@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

So what to do here ? Do we need to not throw and pass the debug directly in the trans method ?
Or in all caller ? cc @nicolas-grekas

@Simperfit Simperfit force-pushed the Simperfit:feature/allow-to-not-throw-when-using-trans-and-transchoice branch from a7fef52 to 8a3fdb5 Apr 23, 2019

@Simperfit Simperfit force-pushed the Simperfit:feature/allow-to-not-throw-when-using-trans-and-transchoice branch from 8a3fdb5 to 1ee08d4 Apr 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.