Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Translation] TransChoice invalid plural translation#31110

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@Simperfit
Copy link
Contributor

@SimperfitSimperfit commentedApr 15, 2019•
edited
Loading

QA
Branch?4.4
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#27401
LicenseMIT
Doc PRsymfony/symfony-docs#11425

@SimperfitSimperfitforce-pushed thefeature/allow-to-not-throw-when-using-trans-and-transchoice branch fromf979dea to9c13ce2CompareApril 15, 2019 05:36
@stof
Copy link
Member

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

@SimperfitSimperfitforce-pushed thefeature/allow-to-not-throw-when-using-trans-and-transchoice branch from9c13ce2 to8a87162CompareApril 15, 2019 17:59
@Simperfit
Copy link
ContributorAuthor

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.

@SimperfitSimperfitforce-pushed thefeature/allow-to-not-throw-when-using-trans-and-transchoice branch from8a87162 to0aa48c2CompareApril 15, 2019 18:04
Copy link
Contributor

@ro0NLro0NL left a comment

Choose a reason for hiding this comment

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

what we should return

the default behavior?

<defaultspublic="false" />

<serviceid="translator"class="Symfony\Component\Translation\IdentityTranslator"public="true" />
<serviceid="Symfony\Component\Translation\TranslatorInterface"alias="translator" />
Copy link
Contributor

@ro0NLro0NLApr 16, 2019•
edited
Loading

Choose a reason for hiding this comment

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

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

@ro0NL
Copy link
Contributor

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

@Simperfit
Copy link
ContributorAuthor

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

@SimperfitSimperfitforce-pushed thefeature/allow-to-not-throw-when-using-trans-and-transchoice branch from0aa48c2 toa7fef52CompareApril 22, 2019 06:15
Copy link
Contributor

@ro0NLro0NL left a comment

Choose a reason for hiding this comment

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

@Simperfit isnt it weirdtransChoice is "debug-able" whereastrans() is not? Esp. sincetransChoice is deprecated.. so$debug is useless in 5.0?

Moreover, what to do with

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

@stof
Copy link
Member

trans can throw in 2 cases:

@Simperfit
Copy link
ContributorAuthor

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

@SimperfitSimperfitforce-pushed thefeature/allow-to-not-throw-when-using-trans-and-transchoice branch 2 times, most recently from8a3fdb5 to1ee08d4CompareApril 23, 2019 18:07
@SimperfitSimperfitforce-pushed thefeature/allow-to-not-throw-when-using-trans-and-transchoice branch from1ee08d4 toc7f5cf7CompareMay 18, 2019 05:53
@Simperfit
Copy link
ContributorAuthor

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

@Simperfit
Copy link
ContributorAuthor

Status: Needs Review

Copy link
Member

@xabbuhxabbuh left a comment

Choose a reason for hiding this comment

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

Do we need to update other usage of theTranslatorTrait (inSymfony\Bridge\Twig\Extension\TranslationExtension andSymfony\Component\Validator\ValidatorBuilder) too?

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

@SimperfitSimperfitforce-pushed thefeature/allow-to-not-throw-when-using-trans-and-transchoice branch from0732cc5 toc3d6800CompareJune 21, 2019 21:08
@Simperfit
Copy link
ContributorAuthor

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 ?

@javiereguiluzjaviereguiluz changed the title[Translation] TransChoice invalide plural translation[Translation] TransChoice invalid plural translationJul 12, 2019
@SimperfitSimperfitforce-pushed thefeature/allow-to-not-throw-when-using-trans-and-transchoice branch fromc3d6800 to1669eabCompareJuly 17, 2019 19:17
@Simperfit
Copy link
ContributorAuthor

Status: Needs Review

@Simperfit
Copy link
ContributorAuthor

cc@xabbuh

@nicolas-grekas
Copy link
Member

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

@SimperfitSimperfitforce-pushed thefeature/allow-to-not-throw-when-using-trans-and-transchoice branch from1669eab to47d6d7eCompareAugust 14, 2019 05:38
@nicolas-grekasnicolas-grekas modified the milestones:next,4.4Oct 27, 2019
@noniagriconomie
Copy link
Contributor

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-grekasnicolas-grekasforce-pushed thefeature/allow-to-not-throw-when-using-trans-and-transchoice branch from47d6d7e to5c5420bCompareFebruary 3, 2020 11:18
@nicolas-grekas
Copy link
Member

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

@noniagriconomie
Copy link
Contributor

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

@nicolas-grekas
Copy link
Member

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
Copy link
Contributor

@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
Copy link
Member

I don't know :)

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas requested changes

@xabbuhxabbuhxabbuh approved these changes

+3 more reviewers

@ro0NLro0NLro0NL left review comments

@maxheliasmaxheliasmaxhelias approved these changes

@noniagriconomienoniagriconomienoniagriconomie approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

8 participants

@Simperfit@stof@ro0NL@noniagriconomie@nicolas-grekas@xabbuh@maxhelias@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp