Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Translator] Deprecated transChoice and moved it away from contracts#28375
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Nyholm left a comment
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.
I added some comments that I think will help the review.
UPGRADE-4.2.md Outdated
| ----------- | ||
| * The`TranslatorInterface` has been deprecated in favor of`Symfony\Contracts\Translation\TranslatorInterface` | ||
| * The`Translator::transChoice()` has been deprecated in favor of using`Translator::trans()` with intl message format |
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.
I choose to use the implementation (Translator) here. We already say that the full interface is deprecated.
| * @param int $number The number of items represented for the message | ||
| * @param string $locale The locale to use for choosing | ||
| * @param string$message The message being translated | ||
| * @param int|float $number The number of items represented for the message |
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.
We actually support floats.
| privatefunctiongetPluralizationRule(int$number,string$locale):int | ||
| { | ||
| return PluralizationRules::get($number,$locale,false); | ||
| returnstrtr($this->selector->choose((string)$id,$number,$locale ?:$this->getLocale()),$parameters); |
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.
FYI: In 4.1 we use(int) $number.
| { | ||
| returnnewIdentityTranslator(); | ||
| } | ||
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.
These tests are moved from Contracts.
| */ | ||
| interface TranslatorInterfaceextends BaseTranslatorInterface | ||
| { | ||
| /** |
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.
Moved this back from Contracts
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| publicfunctiontransChoice($id,$number,array$parameters =array(),$domain =null,$locale =null) |
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.
This function was removed. It was a replacement for the deprecatedMessageSelector. I do not think it is needed anymore, right?
nicolas-grekasSep 6, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
right,same for getPluralizationRule below
nicolas-grekas left a comment
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.
Open questions:
- what should happen when the intl extension is not installed?
- should be ship a command converting from one format to the other in this PR?
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| publicfunctiontransChoice($id,$number,array$parameters =array(),$domain =null,$locale =null) |
nicolas-grekasSep 6, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
right,same for getPluralizationRule below
Nyholm commentedSep 6, 2018
I think we should ship with "
I think that is a good idea. But that is not a requirement for this PR (nor for 4.2). But the sooner the better. |
nicolas-grekas commentedSep 6, 2018
I think it would be friendly to make migrating to the new format as seamless as possible, running a command to update existing catalogs would be awesome. It should be shipped at the same time as the deprecation to me. If doable of course. We should also deprecate the twig helpers btw! (as highlighted by the CI ;) ) |
Nyholm commentedSep 6, 2018
Okey. I'll will not do it in this PR. It is a lot of code and hard to review already. I will work on a migration script now and add that in a other PR.
Thank you. I saw those now. 👍 Label: Needs work |
stof commentedSep 6, 2018
Well, you cannot deprecate the And we also need to figure out the migration path between format. |
| { | ||
| if ($this->selector) { | ||
| returnstrtr($this->selector->choose((string)$id, (int)$number,$locale ?:$this->getLocale()),$parameters); | ||
| @trigger_error(sprintf('The "%s()" method is deprecated since Symfony 4.1, use the "trans()" method with intl formatted messages instead.',__METHOD__),E_USER_DEPRECATED); |
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.
4.1 ? That's too late for that
Nyholm commentedSep 17, 2018
1a5de0a to868e9bdComparenicolas-grekas commentedSep 21, 2018
I just rebased+squashed the PR by pushing on your fork. |
868e9bd to39fcccfCompare| if ($this->translatorinstanceof LegacyTranslatorInterface) { | ||
| $trans =$this->translator->transChoice($id,$number,$parameters,$domain,$locale); | ||
| }else { | ||
| $trans =$this->doTransChoice($id,$number,$parameters,$domain,$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.
Is this correct to use the trait here and in the DataCollector?
| publicfunctionaddViolation() | ||
| { | ||
| if (null ===$this->plural) { | ||
| if (null ===$this->plural || !$this->translatorinstanceof LegacyTranslatorInterface) { |
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.
This will cause some deprecations. How can I know if I should use transChoice or not?
Nyholm commentedSep 22, 2018
@stof Thank you for the review.
The migration path would be to run the About supporting both formats: That is a fair point, do you have any suggestions? Status: needs review |
562bdff to7cdef58Comparenicolas-grekas commentedOct 1, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I pushed a new approach here, see 2nd commit: I think we should not deprecate the current plural format as this would create a too steep migration path for devs. Instead, I think we should make "trans()" able to resolve plurals. That's already the case when using the INTL message format, so let's make it work with the Symfony format. What is proposed now is to make the For Twig, There is a potential for conflicts with existing messages when these two conditions are met together:
We could choose another special parameter name if we think the risk is unreasonable (to me it's OK). Ready for review. |
7cdef58 to943f23bCompare943f23b to54421edCompareNyholm left a comment
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.
I've reviewed this carefully. I think it looks alright.
About%count%. I was considering to use# because that is that INTL is using. However, the potential conflicts are larger and we do not need to take this step towards INTL format right now.
👍
| $trans =$this->translator->transChoice($id,$number,$parameters,$domain,$locale); | ||
| $this->collectMessage($locale,$domain,$id,$trans,$parameters,$number); | ||
| $this->collectMessage($locale,$domain,$id,$trans,array('%count%' =>$number) +$parameters); |
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.
I've always got problems when adding arrays. Shouldn't we use the more easy-to-read array_merge?
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.
array_merge is a function call, personally I prefer using the language directly.
| publicfunction__construct(string$fieldName,CsrfTokenManagerInterface$tokenManager,string$tokenId,string$errorMessage,$translator =null,string$translationDomain =null,ServerParams$serverParams =null) | ||
| { | ||
| if (null !==$translator && !$translatorinstanceof LegacyTranslatorInterface && !$translatorinstanceof TranslatorInterface) { | ||
| thrownew \TypeError(sprintf('Argument 5 passed to %s() must be an instance of %s, %s given',__METHOD__, TranslatorInterface::class,\is_object($translator) ?\get_class($translator) :\gettype($translator))); |
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.
missing . at the end of the exception message.
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
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.
There are many other occurrences.
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.
oups, now all of them are fixed
54421ed to211e8afCompare211e8af todc5f3bfComparefabpot commentedOct 6, 2018
Thank you@Nyholm. |
…from contracts (Nyholm, nicolas-grekas)This PR was merged into the 4.2-dev branch.Discussion----------[Translator] Deprecated transChoice and moved it away from contracts| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | Issue:symfony/symfony-docs#10264This will address comment made here:#27399 (review)This PR moves the `transChoice()` method away from Contracts and back to the component. I also reverted changes in the `IdentityTranslator`. I will still use the deprecated `MessageSelector` but this is not fine since `transChoice()` is also deprecated.Commits-------dc5f3bf Make trans + %count% parameter resolve pluralsd870a85 [Translator] Deprecated transChoice and moved it away from contracts
This will address comment made here:#27399 (review)
This PR moves the
transChoice()method away from Contracts and back to the component. I also reverted changes in theIdentityTranslator. I will still use the deprecatedMessageSelectorbut this is not fine sincetransChoice()is also deprecated.