[Translation] TransChoice invalid plural translation #31110

Simperfit · Apr 15, 2019

Q	A
Branch?	4.4
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

src/Symfony/Contracts/Translation/TranslatorTrait.php

src/Symfony/Component/Translation/IdentityTranslator.php

src/Symfony/Bundle/FrameworkBundle/Resources/config/identity_translator.xml

src/Symfony/Component/Translation/IdentityTranslator.php

src/Symfony/Contracts/Translation/TranslatorTrait.php

stof · Apr 15, 2019

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

Simperfit · 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.

ro0NL

what we should return

the default behavior?

symfony/src/Symfony/Contracts/Translation/TranslatorTrait.php

Line 49 in 0aa48c2

return strtr($id, $parameters);

ro0NL · Apr 16, 2019

src/Symfony/Bundle/FrameworkBundle/Resources/config/identity_translator.xml

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


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

src/Symfony/Component/Translation/IdentityTranslator.php

ro0NL · 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 · Apr 22, 2019

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

ro0NL

@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

symfony/src/Symfony/Component/Validator/ValidatorBuilder.php

Lines 336 to 338 in a7fef52

    
           $translator = new class() implements TranslatorInterface, LocaleAwareInterface { 
        
               use TranslatorTrait; 
        
           };

should it be "debug aware" also?

src/Symfony/Component/Translation/IdentityTranslator.php

stof · 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 MessageFormatter does not perform the full parsing at configuration time polyfill#158)
using the Symfony legacy format with some pluralization (the alternative to the deprecated transChoice)

Simperfit · 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 · May 18, 2019

I like how it's working right now @stof @nicolas-grekas tell me if I need to change something.

Simperfit · May 25, 2019

Status: Needs Review

xabbuh

Do we need to update other usage of the TranslatorTrait (in Symfony\Bridge\Twig\Extension\TranslationExtension and Symfony\Component\Validator\ValidatorBuilder) too?

And we need to mark any version of the Translation component as conflicting with the FrameworkBundle.

src/Symfony/Bundle/FrameworkBundle/Resources/config/identity_translator.xml

Simperfit · Jun 21, 2019

Do we need to update other usage of the TranslatorTrait (in Symfony\Bridge\Twig\Extension \TranslationExtension and Symfony\Component\Validator\ValidatorBuilder) too?

I don't know, maybe @ro0NL has some thoughts on this ?

Simperfit · Jul 17, 2019

Status: Needs Review

Simperfit · Aug 6, 2019

cc @xabbuh

nicolas-grekas · Aug 8, 2019

A test case would be nice please (+rebase needed)

src/Symfony/Component/Translation/Tests/IdentityTranslatorTest.php

src/Symfony/Component/Translation/IdentityTranslator.php

noniagriconomie · Dec 6, 2019

It will be very cool to have it in 4.4 as a bug fix, but i fear it will not be considered as it
so sadly 5.X

thank you working on this @Simperfit :)

nicolas-grekas · Feb 3, 2020

Closing as this staled.
If you want to take over @noniagriconomie, this should be for master.

noniagriconomie · Feb 3, 2020

@nicolas-grekas hello
what do you mean by take over? finishing the review you left?
Yes i will do it for sure to continue @Simperfit works 👍

nicolas-grekas · Feb 3, 2020

I fixed all comments before closing so yes, this should be good to go on master, but with enough description in the PR to be able to understand the use case and that could act as the start of a documentation.

noniagriconomie · Feb 12, 2020

@nicolas-grekas related to #28375
can you confirm that this code is now useless for sf5+ ?

and as this is not considered as a bugfix, IMO sf 3 and 4 can not benefit this

thank you

nicolas-grekas · Feb 12, 2020

I don't know :)

carsonbot added Status: Needs Review Translation Feature labels Apr 15, 2019

Simperfit mentioned this pull request Apr 15, 2019

[Translation] TransChoice invalide plural translation symfony/symfony-docs#11425

Closed

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

noniagriconomie reviewed Apr 15, 2019

View reviewed changes

src/Symfony/Contracts/Translation/TranslatorTrait.php Outdated Show resolved Hide resolved

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

ro0NL reviewed Apr 15, 2019

View reviewed changes

src/Symfony/Contracts/Translation/TranslatorTrait.php Outdated Show resolved Hide resolved

ro0NL reviewed Apr 15, 2019

View reviewed changes

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

ro0NL reviewed Apr 15, 2019

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Resources/config/identity_translator.xml Show resolved Hide resolved

nicolas-grekas requested changes Apr 15, 2019

View reviewed changes

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

src/Symfony/Contracts/Translation/TranslatorTrait.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Apr 15, 2019

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

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

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

ro0NL reviewed Apr 16, 2019

View reviewed changes

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

ro0NL reviewed Apr 22, 2019

View reviewed changes

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

Simperfit force-pushed the feature/allow-to-not-throw-when-using-trans-and-transchoice branch 2 times, most recently from 8a3fdb5 to 1ee08d4 Compare April 23, 2019 18:07

Simperfit force-pushed the feature/allow-to-not-throw-when-using-trans-and-transchoice branch from 1ee08d4 to c7f5cf7 Compare May 18, 2019 05:53

xabbuh requested changes Jun 21, 2019

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Resources/config/identity_translator.xml Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jun 21, 2019

Simperfit force-pushed the feature/allow-to-not-throw-when-using-trans-and-transchoice branch from 0732cc5 to c3d6800 Compare June 21, 2019 21:08

javiereguiluz changed the title ~~[Translation] TransChoice invalide plural translation~~ [Translation] TransChoice invalid plural translation Jul 12, 2019

Simperfit force-pushed the feature/allow-to-not-throw-when-using-trans-and-transchoice branch from c3d6800 to 1669eab Compare July 17, 2019 19:17

noniagriconomie approved these changes Aug 3, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Aug 3, 2019

xabbuh approved these changes Aug 8, 2019

View reviewed changes

maxhelias approved these changes Aug 8, 2019

View reviewed changes

Simperfit force-pushed the feature/allow-to-not-throw-when-using-trans-and-transchoice branch from 1669eab to 47d6d7e Compare August 14, 2019 05:38

nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019

nicolas-grekas requested changes Nov 5, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Nov 5, 2019

[Translation] TransChoice invalide plural translation

5c5420b

nicolas-grekas force-pushed the feature/allow-to-not-throw-when-using-trans-and-transchoice branch from 47d6d7e to 5c5420b Compare February 3, 2020 11:18

nicolas-grekas closed this Feb 3, 2020

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

	$translator = new class() implements TranslatorInterface, LocaleAwareInterface {
	use TranslatorTrait;
	};

Search code, repositories, users, issues, pull requests...

Uh oh!

[Translation] TransChoice invalid plural translation #31110

[Translation] TransChoice invalid plural translation #31110

Uh oh!

Conversation

Simperfit commented Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stof commented Apr 15, 2019

Uh oh!

Simperfit commented Apr 15, 2019

Uh oh!

ro0NL left a comment

Choose a reason for hiding this comment

Uh oh!

ro0NL Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ro0NL commented Apr 16, 2019

Uh oh!

Simperfit commented Apr 22, 2019

Uh oh!

ro0NL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stof commented Apr 23, 2019

Uh oh!

Simperfit commented Apr 23, 2019

Uh oh!

Simperfit commented May 18, 2019

Uh oh!

Simperfit commented May 25, 2019

Uh oh!

xabbuh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Simperfit commented Jun 21, 2019

Uh oh!

Simperfit commented Jul 17, 2019

Uh oh!

Simperfit commented Aug 6, 2019

Uh oh!

nicolas-grekas commented Aug 8, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

noniagriconomie commented Dec 6, 2019

Uh oh!

nicolas-grekas commented Feb 3, 2020

Uh oh!

noniagriconomie commented Feb 3, 2020

Uh oh!

nicolas-grekas commented Feb 3, 2020

Uh oh!

noniagriconomie commented Feb 12, 2020

Uh oh!

nicolas-grekas commented Feb 12, 2020

Uh oh!

Uh oh!

Simperfit commented Apr 15, 2019 •

edited

Loading

ro0NL Apr 16, 2019 •

edited

Loading