Skip to content

[Form] Add choice_translation_parameters option for ChoiceType form #36851

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented May 18, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #36845
License MIT
Doc PR symfony/symfony-docs#13677

@VincentLanglet VincentLanglet changed the title Add choice_translation_parameters option [Form] Add choice_translation_parameters option for ChoiceType form May 18, 2020
@nicolas-grekas nicolas-grekas added this to the next milestone May 18, 2020
@VincentLanglet VincentLanglet force-pushed the choice_translation_parameters branch 2 times, most recently from 3f1c059 to e37d6d4 Compare May 18, 2020 23:29
@VincentLanglet VincentLanglet marked this pull request as ready for review May 18, 2020 23:45
@VincentLanglet VincentLanglet requested a review from xabbuh as a code owner May 18, 2020 23:45
@VincentLanglet VincentLanglet marked this pull request as draft July 8, 2020 09:42
@VincentLanglet
Copy link
Contributor Author

Just found a situation where a callback is needed if the translations parameters are depending on the choice.
I need to handle a callback then.

@xabbuh Do you know how I can support something like

'choice_translation_parameters' => function ($choices, $key, $value) {
    return ['%foo%' => $choices->getFoo()];
}

@VincentLanglet VincentLanglet force-pushed the choice_translation_parameters branch from 1ea5346 to ecefd31 Compare July 12, 2020 22:19
@VincentLanglet VincentLanglet force-pushed the choice_translation_parameters branch from ecefd31 to d876022 Compare August 12, 2020 18:44
@VincentLanglet VincentLanglet marked this pull request as ready for review August 12, 2020 19:51
@VincentLanglet
Copy link
Contributor Author

Hi @xabbuh, I totally rework the PR and updated the symfony/symfony-docs#13677

I change the way I implemented choice_translation_parameters to work the same way it works for choice_attr.
This allow to not pass the same translation_parameters to the choices.

WDYT about it ? :)

@VincentLanglet VincentLanglet force-pushed the choice_translation_parameters branch 5 times, most recently from f31e0c4 to 453bd62 Compare August 23, 2020 14:59
@VincentLanglet
Copy link
Contributor Author

@xabbuh I don't know why the low build failing. Can you help me ? :)

@fabpot
Copy link
Member

fabpot commented Aug 23, 2020

@VincentLanglet You should probably bump de dep of symfony/form to ^5.2 in Twig Bridge.

@VincentLanglet VincentLanglet force-pushed the choice_translation_parameters branch from c9beb74 to 944648c Compare August 23, 2020 18:39
@VincentLanglet
Copy link
Contributor Author

@VincentLanglet You should probably bump de dep of symfony/form to ^5.2 in Twig Bridge.

Indeed ! Thanks.

I think this PR is finally ready to review then. :)

Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

LGTM, @xabbuh Can you review this one?

*/
public function createView(ChoiceListInterface $list, $preferredChoices = null, $label = null, $index = null, $groupBy = null, $attr = null)
public function createView(ChoiceListInterface $list, $preferredChoices = null, $label = null, $index = null, $groupBy = null, $attr = null, $labelTranslationParameters = [])
Copy link
Member

Choose a reason for hiding this comment

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

Adding the argument here is a BC break.

@jvasseur
Copy link
Contributor

jvasseur commented Oct 2, 2020

@VincentLanglet you don't need to pass a translatable as key in the choices array, you need to use the https://symfony.com/doc/current/reference/forms/types/choice.html#choice-label option to transform what you put in the choice array into a translatable object.

So yes, the feature does exists currently.

@VincentLanglet
Copy link
Contributor Author

@VincentLanglet you don't need to pass a translatable as key in the choices array, you need to use the https://symfony.com/doc/current/reference/forms/types/choice.html#choice-label option to transform what you put in the choice array into a translatable object.

So yes, the feature does exists currently.

Oh I didn't think it that way.

But, I propose the feature

'choice' => [
    'form.order.yes' => true,
    'form.order.no' => false,
],
'choice_translation_parameters' => [
    'form.order.yes' => ['%company%' => 'ACME Inc.'],
    'form.order.no' => [],
],

And you recommend

'choice' => [
    'form.order.yes' => true,
    'form.order.no' => false,
],
'choice_label' => function($choices, $key, $value) {
    $parameters = [
        'form.order.yes' => ['%company%' => 'ACME Inc.'],
    ];

    return new Translatable($key, $parameters[$key] ?? []);
}

I have trouble to see this solution as simpler.
So It could take some time before the introduction of a deprecation for the first solution, if we had one some day.

I'm not sure that the "Translatable should be the only option" is the best strategy for the developer.

@xabbuh Do you have some time to review this PR ? :)

@jvasseur
Copy link
Contributor

jvasseur commented Oct 2, 2020

Personally I tend to never use the keys in choices for labels and always define choice_label because I find it cleaner so I would do something like this :

'choices' => [
    true,
    false,
],
'choice_label' => function ($choices, $key, $value) {
    if ($value) {
        return t('form.order.yes', ['%company%' => 'ACME Inc.']);
    } else {
        return t('form.order.no'),
    };
}

Of course this example doesn't looks that good but in most cases your choices will contains the necessary data to construct the full translation.

And even if it is a bit more verbose I think it's a better solution since it keeps the translation key and the translation parameters close to each others.

@VincentLanglet
Copy link
Contributor Author

Personally I tend to never use the keys in choices for labels and always define choice_label because I find it cleaner so I would do something like this :

'choices' => [
    true,
    false,
],
'choice_label' => function ($choices, $key, $value) {
    if ($value) {
        return t('form.order.yes', ['%company%' => 'ACME Inc.']);
    } else {
        return t('form.order.no'),
    };
}

Of course this example doesn't looks that good but in most cases your choices will contains the necessary data to construct the full translation.

And even if it is a bit more verbose I think it's a better solution since it keeps the translation key and the translation parameters close to each others.

It's easier when it's a true/false case.

But if choice are let's say [1, 2, 3, 4], you ends with

'choice_label' => function ($choices, $key, $value) {
    if ($value === 1) {
        return t('form.order.1', ['%company%' => 'ACME Inc.']);
    } elseif ($value === 2) {
        return t('form.order.2', ['%company%' => 'ACME Inc.']);
    } elseif ($value === 3) {
        return t('form.order.3', ['%company%' => 'ACME Inc.']);
    } else {
        return t('form.order.4'),
    };
}

It starts to be verbose.

And if it was the recommended way, Symfony wouldn't use the key of the choices as label.

I find your solution over-complicated, you don't like mine.
I don't see any issue to do the same thing different ways. It will satisfied more developer.

For instance choice_label can accept

function ($choice) {
   return $choice->getFoo();
}

And

'foo`

And none of these solution are deprecated.

@fabpot
Copy link
Member

fabpot commented Oct 3, 2020

@VincentLanglet Can you have a look at the failing tests?

@VincentLanglet VincentLanglet force-pushed the choice_translation_parameters branch 2 times, most recently from 5fde5f8 to a962f60 Compare October 4, 2020 16:31
@VincentLanglet
Copy link
Contributor Author

@VincentLanglet Can you have a look at the failing tests?

I fixed the error I got with missing space in resolved_form_type_1.txt,
But maybe there should be something about the

trim_trailing_whitespace = true

line in the .editorconfig file. My IDE is just automatically removing the trailing whitespaces every time.

I also remove the deprecation I added since there is already one.

The "Symfony\Component\Form\ChoiceList\Factory\DefaultChoiceListFactory::createView()" method will require a new "array|callable $labelTranslationParameters" argument in the next major version of its interface "Symfony\Component\Form\ChoiceList\Factory\ChoiceListFactoryInterface", not defining it is deprecated.

But I don't know how to avoid getting this in tests. I'm getting this because the DefaultChoiceListFactory::createView doesn't have the $labelTranslationParameters in his definition, because it would be a BC-break.

@fabpot @xabbuh

@VincentLanglet VincentLanglet force-pushed the choice_translation_parameters branch from a962f60 to 026ed90 Compare October 4, 2020 17:03
@fabpot fabpot closed this Oct 7, 2020
@fabpot
Copy link
Member

fabpot commented Oct 7, 2020

We've just moved away from master as the main branch to use 5.x instead. Unfortunately, I cannot reopen the PR and change the target branch to 5.x. Can you open a new PR referencing this one to not loose the discussion? Thank you for your understanding and for your help.

@VincentLanglet
Copy link
Contributor Author

Sure #38469

@nicolas-grekas nicolas-grekas modified the milestones: 5.x, 5.2 Oct 14, 2020
fabpot added a commit that referenced this pull request Dec 10, 2020
…centLanglet)

This PR was squashed before being merged into the 5.3-dev branch.

Discussion
----------

[Form] Add "choice_translation_parameters" option

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #36845 <!-- prefix each issue number with "Fix #", if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#13677

Original PR: #36851

Commits
-------

1ce5b03 [Form] Add "choice_translation_parameters" option
symfony-splitter pushed a commit to symfony/form that referenced this pull request Dec 10, 2020
…centLanglet)

This PR was squashed before being merged into the 5.3-dev branch.

Discussion
----------

[Form] Add "choice_translation_parameters" option

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #36845 <!-- prefix each issue number with "Fix #", if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#13677

Original PR: symfony/symfony#36851

Commits
-------

1ce5b03c2a [Form] Add "choice_translation_parameters" option
symfony-splitter pushed a commit to symfony/twig-bridge that referenced this pull request Dec 10, 2020
…centLanglet)

This PR was squashed before being merged into the 5.3-dev branch.

Discussion
----------

[Form] Add "choice_translation_parameters" option

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #36845 <!-- prefix each issue number with "Fix #", if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#13677

Original PR: symfony/symfony#36851

Commits
-------

1ce5b03c2a [Form] Add "choice_translation_parameters" option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ChoiceType Field should have a choice_translation_parameters option.
6 participants