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] 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

Merged
xabbuh merged 1 commit intosymfony:3.4fromaitboudad:translation-formatters
Sep 12, 2017

Conversation

aitboudad
Copy link
Contributor

@aitboudadaitboudad commentedMar 25, 2016
edited
Loading

QA
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
LicenseMIT
Doc PR~

Koc reacted with hooray emoji
{
@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;
Copy link
ContributorAuthor

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

What will be the replacement for$translator->transChoice('some_message2', 10, array('%count%' => 10)); ?

$translator->trans('some_message2', ['__number__' => 10, '%count%' => 10], null, null, TransChoiceMessageFormatter::NAME); ?

It looks a bit weird to me. I think that iftrans andtransChoice are merged together we should have a way to automatically choose between the 2 (maybe automatically takes a'%count%' key is their isn't any'__number__' one or something like that?)

aitboudad and HeahDude reacted with thumbs up emoji

@aitboudad
Copy link
ContributorAuthor

@Nicofuma merging them together looks better!

@linaori
Copy link
Contributor

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__number__ doesn't feel ideal either but having to rely on another pre-defined key is kind of magic as well.

What I know from experiences:

  • For transchoice, you always have to define a number
  • specifying the domain and locale is kind of annoying for most cases for the solution you present (messages domain/default locale)
  • A lot of times I translate without parameters but with a different domain

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

Using multiple formatter is something rare but this allow us to migrate safely toIntlMessageFormatter.
Right now if we combine trans with transChoice in one formatter it'll reduce some boilerplate,

__number__ is provider for only BC break not for end-users, we can use%count% like we did intwig transChoice

{{'Hello world'|transchoice(count) }} //Before{{'Hello world'|trans({'%count%':count }) }} // After

@linaori
Copy link
Contributor

So how would that be determined? I can have a%count% in my arguments without it being a transchoice.

$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
Copy link
ContributorAuthor

@iltar

So how would that be determined? I can have a %count% in my arguments without it being a transchoice.

agreed we should find another pre-defined key :/

Where would it be less boilerplate? Maybe I missed something?

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

What abouttrans_count ?

@linaori
Copy link
Contributor

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{{ 'foo' | trans({trans.count: 10}) }}.

Something else that popped my mind, simply no key:$translator->trans('foo', [10]). Sadly it's typehinted as array for this case. In twig it could be{{ 'foo' | trans(10) }} though.

aitboudad reacted with thumbs up emoji

@javiereguiluz
Copy link
Member

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 currenttransChoice() method? What's the problem we're trying to solve? Why the new code is longer to type and harder to learn? Thanks!

@mvrhov
Copy link

@javiereguiluz: take a look at howmessage formatter class works.
This is what aitboudad would like to support.

@javiereguiluz
Copy link
Member

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

@javiereguiluz having 2 fonctions that achieve the same behavior looks wrong to me
and I thinktranchoice will be useless for custom formattter such asIntlMessageFormatter orExpressionsMessageFormatter. The main goal for me is to make IntlMessageFormatter as default formatter.

we can keeptranchoice as a helper method but IMO it shouldn't be part ofTranslatorInterface
if you have any suggestions I would be happy to implement it ;)

@linaori
Copy link
Contributor

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

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

@javiereguiluz
For php:

 // 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 variabletranschoice =TransMessageFormatter::TRANS_CHOICE):

 {# 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
Copy link
Member

@aitboudad thanks for the info. I've updated your comment to add the missinguse import.

Does the new system mean that%count% is a hardcoded variable name in translation messages? If not, how could we implement this example with the new proposal?

$this->get('translator')->trans('There is one apple|There are %num% apples',5, ['%num%' =>'five']);

@aitboudad
Copy link
ContributorAuthor

@javiereguiluz

$this->get('translator')->trans('There is one apple|There are %num% apples',array(TransMessageFormatter::TRANS_CHOICE =>5,'%num%' =>'five'));

we can still using%count%:

$this->get('translator')->trans('There is one apple|There are %count% apples',array(TransMessageFormatter::TRANS_CHOICE =>5,'%count%' =>'five'));

$name = $name ?: $this->defaultFormatter;

foreach ($this->formatters as $formatter) {
if ($name === $formatter->getName()) {
Copy link
Contributor

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!

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

fixed thanks!

@aitboudadaitboudadforce-pushed thetranslation-formatters branch fromcef7950 to1d4343cCompareJuly 5, 2016 15:45
@aitboudadaitboudad changed the title[WIP][Translation] allow using multiple formatters with decoupling the def…[Translation] allow using multiple formatters with decoupling the default one.Jul 5, 2016
@aitboudadaitboudadforce-pushed thetranslation-formatters branch from1d4343c to1660808CompareJuly 5, 2016 15:47
@aitboudad
Copy link
ContributorAuthor

@nicolas-grekas you can push now!

@nicolas-grekasnicolas-grekasforce-pushed thetranslation-formatters branch 4 times, most recently fromee13182 tof8b9760CompareAugust 5, 2017 19:12
@nicolas-grekas
Copy link
Member

@aitboudad thanks, I just push forced one last time, addressing a few minor things meanwhile.

@@ -28,6 +28,13 @@
<tag name="monolog.logger" channel="translation" />
</service>

<service id="translator.formatter" alias="translator.formatter.default" />
<service id="Symfony\Component\Translation\Formatter\MessageFormatterInterface" alias="formatter" />
Copy link
Contributor

Choose a reason for hiding this comment

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

alias="translator.formatter"?

Choose a reason for hiding this comment

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

fixed thanks

@nicolas-grekas
Copy link
Member

I think this PR is ready to merge, failures are false positives.
@aitboudad would you be able to work on a doc PR if worth it?

@aitboudad
Copy link
ContributorAuthor

why not :), I'll try to work on it next weekend.

@aitboudad
Copy link
ContributorAuthor

@nicolas-grekas I missed to mention the doc PR#8284, I guess the PR is ready to merge.

Copy link
Member

@nicolas-grekasnicolas-grekas left a 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

* Passing a `Symfony\Component\Translation\MessageSelector` to `Translator` has been
deprecated. You should pass a message formatter instead

Before:
Copy link
Member

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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

fixed

@@ -28,6 +28,12 @@
<tag name="monolog.logger" channel="translation" />
</service>

<service id="translator.formatter" alias="translator.formatter.default" />
Copy link
Member

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).

Copy link
Member

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.

Copy link
Member

@nicolas-grekasnicolas-grekasSep 11, 2017
edited
Loading

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 ;) )

Copy link
Member

Choose a reason for hiding this comment

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

hm okay 😆

Copy link
ContributorAuthor

@aitboudadaitboudadSep 12, 2017
edited
Loading

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.

Copy link
Member

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)?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

you're right, removed!

@@ -234,6 +234,27 @@ Translation
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
Copy link
Member

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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

done

@xabbuh
Copy link
Member

Thank you@aitboudad.

@xabbuhxabbuh merged commit42183b0 intosymfony:3.4Sep 12, 2017
xabbuh added a commit that referenced this pull requestSep 12, 2017
…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
@aitboudadaitboudad deleted the translation-formatters branchSeptember 12, 2017 12:39
This was referencedOct 18, 2017
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull requestSep 24, 2019
… (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.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@fabpotfabpotfabpot left review comments

@javiereguiluzjaviereguiluzjaviereguiluz left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@sstoksstoksstok left review comments

@xabbuhxabbuhxabbuh approved these changes

@NyholmNyholmNyholm approved these changes

@HeahDudeHeahDudeHeahDude approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.

14 participants
@aitboudad@Nicofuma@linaori@HeahDude@javiereguiluz@mvrhov@stof@xabbuh@Koc@nicolas-grekas@fabpot@sstok@Nyholm@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp