Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Translation] added support for adding custom message formatter#18314
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
| { | ||
| @trigger_error('The'.__METHOD__.' method is deprecated since version 3.1 and will be removed in 4.0. Use trans instead.',E_USER_DEPRECATED); | ||
| $parameters['__number__'] =$number; |
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.
One possible BC break: we need to preservenumber argument for the transChoice
Nicofuma commentedMar 25, 2016
What will be the replacement for
It looks a bit weird to me. I think that if |
aitboudad commentedMar 25, 2016
@Nicofuma merging them together looks better! |
linaori commentedMar 25, 2016
How would you envision this in the templates? Adding class names/constants is pretty annoying in twig. It also means that I have to passa lot of default values if I want to specify that. It feels like you have a lot more boilerplate to achieve the same. Having to write down What I know from experiences:
So I think we can agree that something is bothersome in the current implementation but i don't think the solution you present will solve it and will possibly even decrease the DX. |
aitboudad commentedMar 25, 2016
Using multiple formatter is something rare but this allow us to migrate safely toIntlMessageFormatter.
{{'Hello world'|transchoice(count) }} //Before{{'Hello world'|trans({'%count%':count }) }} // After |
linaori commentedMar 25, 2016
So how would that be determined? I can have a $translator->trans($v, ['%count%' =>10],null,null, TransChoiceMessageFormatter::NAME);// vs$translator->transChoice($v,10); Where would it be less boilerplate? Maybe I missed something? |
aitboudad commentedMar 25, 2016
agreed we should find another pre-defined key :/
As I said before TransChoiceMessageFormatter will be meged into TransMessageFormatter so we don't need to pass the formatter argument by default but we need to rely on a pre-defined key |
HeahDude commentedMar 25, 2016
What about |
linaori commentedMar 25, 2016
Ah okay, so it's the part where the key defines what type is used. In php you could use a constant but that would be cumbersome in twig. You could expose the value like Something else that popped my mind, simply no key: |
javiereguiluz commentedMar 29, 2016
At first glance this looks to me like a serious step back in DX: {# before#}{{'Hello world'|transchoice(count) }}{# after#}{{'Hello world'|trans({'%count%':count }) }} // before$translator->transChoice('some_message2',10,array('%count%' =>10));// after$translator->trans('some_message2', [TransMessageFormatter::TRANS_CHOICE =>10]); My questions: What's wrong with the current |
mvrhov commentedMar 29, 2016
@javiereguiluz: take a look at howmessage formatter class works. |
javiereguiluz commentedMar 29, 2016
@mvrhov yes, but I'd prefer if we first focus on the before/after changes for end users and then we care about the internal implementation. For now I feel that this proposal is worse than the current situation. |
aitboudad commentedMar 30, 2016
@javiereguiluz having 2 fonctions that achieve the same behavior looks wrong to me we can keep |
linaori commentedMar 30, 2016
@aitboudad Actually the only times I've used the transchoice, I've done so via twig and never in the code. I find it a rare occasion having to use the translator directly in the code in the first place (but does happen). |
javiereguiluz commentedMar 30, 2016
@aitboudad here it is how Translator works today from the controller and the templates: // simple$this->get('translator')->trans('Hello');// placeholders$this->get('translator')->trans('Hello %name%', ['%name%' =>'Fabien']);// pluralization$this->get('translator')->transChoice('There is one apple|There are %count% apples',10,array('%count%' =>10));// selecting the catalogue$this->get('translator')->trans('Hello', [],'admin');// selecting the locale$this->get('translator')->trans('Hello', [],'messages','fr_FR'); {# simple#}{%trans %}Hello{%endtrans %}{{'Hello'|trans }}{# placeholders#}{%transwith {'%name%':'Fabien' } %}Hello %name%{%endtrans %}{{'Hello'|trans({'%name%':'Fabien'}) }}{# pluralization#}{%transchoicecount %} {0} There are no apples|{1} There is one apple|]1,Inf[ There are %count% apples{%endtranschoice %}{{'{0} There ...|{1} There ...'|transchoice(count) }}{# selecting the catalogue#}{%transfrom'admin' %}Hello{%endtrans %}{{'Hello'|trans({},'app') }}{# selecting the locale#}{%transinto'fr_FR' %}Hello{%endtrans %}{{'Hello'|trans({},'messages','fr_FR') }} Please, provide the equivalent examples for the new way of using the Translator that you are proposing, so we can compare. Thanks! |
aitboudad commentedMar 30, 2016
@javiereguiluz // pluralization+ use Symfony\Component\Translation\Formatter\TransMessageFormatter;+-$this->get('translator')->transChoice(+$this->get('translator')->trans( 'There is one apple|There are %count% apples',- 10,- array('%count%' => 10)+ array(TransMessageFormatter::TRANS_CHOICE => 10) ); in Twig (I defined global variable {# pluralization #}-{% transchoice count %}+{% trans with { transchoice: count } %} {0} There are no apples|{1} There is one apple|]1,Inf[ There are %count% apples-{% endtranschoice %}+{% endtrans %}-{{ '{0} There ...|{1} There ...'|transchoice(count) }}+{{ '{0} There ...|{1} There ...'|trans({transchoice: count}) }} |
javiereguiluz commentedMar 30, 2016
@aitboudad thanks for the info. I've updated your comment to add the missing Does the new system mean that $this->get('translator')->trans('There is one apple|There are %num% apples',5, ['%num%' =>'five']); |
aitboudad commentedMar 30, 2016
$this->get('translator')->trans('There is one apple|There are %num% apples',array(TransMessageFormatter::TRANS_CHOICE =>5,'%num%' =>'five')); we can still using $this->get('translator')->trans('There is one apple|There are %count% apples',array(TransMessageFormatter::TRANS_CHOICE =>5,'%count%' =>'five')); |
d8b7369 to99eb699Compare| $name =$name ?:$this->defaultFormatter; | ||
| foreach ($this->formattersas$formatter) { | ||
| if ($name ===$formatter->getName()) { |
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.
Can't you store the name as array key when registering?
This loop will be executed forevery trans() call!
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!
cef7950 to1d4343cCompare1d4343c to1660808Compareaitboudad commentedAug 5, 2017
@nicolas-grekas you can push now! |
ee13182 tof8b9760Comparef8b9760 toa78ab4eComparenicolas-grekas commentedAug 5, 2017
@aitboudad thanks, I just push forced one last time, addressing a few minor things meanwhile. |
a78ab4e to504bcb8Compare| </service> | ||
| <serviceid="translator.formatter"alias="translator.formatter.default" /> | ||
| <serviceid="Symfony\Component\Translation\Formatter\MessageFormatterInterface"alias="formatter" /> |
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.
alias="translator.formatter"?
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
504bcb8 to34ff232Comparenicolas-grekas commentedAug 5, 2017
I think this PR is ready to merge, failures are false positives. |
aitboudad commentedAug 6, 2017
why not :), I'll try to work on it next weekend. |
34ff232 to0050eb1Compareaitboudad commentedSep 11, 2017
@nicolas-grekas I missed to mention the doc PR#8284, I guess the PR is ready to merge. |
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.
@xabbuh I'll let you merge if you don't mind
UPGRADE-3.4.md Outdated
| * Passing a `Symfony\Component\Translation\MessageSelector` to `Translator` has been | ||
| deprecated. You should pass a message formatter instead | ||
| Before: |
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.
Just noticed: This line and the following need to be indented by an additional space so that it will be rendered property as part of the list item.
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
| <tagname="monolog.logger"channel="translation" /> | ||
| </service> | ||
| <serviceid="translator.formatter"alias="translator.formatter.default" /> |
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.
Do we need this? I mean this is just redundant with what we do in the extension (in combination with the default option value).
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.
Oh, in fact there is one difference: The alias registered in the extension is private. So IMO that's a good argument to remove this one here. At least it will avoid confusion when reading the code.
nicolas-grekasSep 11, 2017 • 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.
(this one also is private, all our XML files start with public="false" as default ;) )
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.
hm okay 😆
aitboudadSep 12, 2017 • 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.
the default value can be overridden throughout the config, without using the alias we can't inject the default one if we need it.
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.
But we always create the alias inhttps://github.com/aitboudad/symfony/blob/fd9f484879a08e00a42a3888d8b1681923b8eb0b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L1078, don't we ($config['formatter'] will have the default value if the user didn't provide any)?
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're right, removed!
0050eb1 tofd9f484Compare| and will be removed in 4.0, use `Symfony\Component\Translation\Writer\TranslationWriter::write` | ||
| instead. | ||
| * Passing a `Symfony\Component\Translation\MessageSelector` to `Translator` has been |
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.
Should probably be added to the component's changelog file too. And we also need to update theUPGRADE-4.0.md file accordingly.
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.
done
fd9f484 to42183b0Comparexabbuh commentedSep 12, 2017
Thank you@aitboudad. |
…formatter (aitboudad)This PR was merged into the 3.4 branch.Discussion----------[Translation] added support for adding custom message formatter| Q | A || --- | --- || Branch? | master || Bug fix? | no || New feature? | yes || BC breaks? | no || Deprecations? | yes || Tests pass? | yes || Fixed tickets |#6009,#10152, one item in#11742,#11948 || License | MIT || Doc PR | ~ |Commits-------42183b0 [Translation] Support adding custom message formatter
… (aitboudad)This PR was merged into the 3.4 branch.Discussion----------[Translation] add how to create custom message formatter.| Q | A || --- | --- || Doc fix? | no || New docs? | yes || Applies to | 3.4 || Fixed tickets |symfony/symfony#18314 |Commits-------02f31ac [Translation] create custom message formatter.
Uh oh!
There was an error while loading.Please reload this page.