-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
[Form] Add choice_translation_parameters option for ChoiceType form #36851
Conversation
VincentLanglet
commented
May 18, 2020
•
edited
Loading
edited
Q | A |
---|---|
Branch? | master |
Bug fix? | no |
New feature? | yes |
Deprecations? | no |
Tickets | Fix #36845 |
License | MIT |
Doc PR | symfony/symfony-docs#13677 |
3f1c059
to
e37d6d4
Compare
src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_4_layout.html.twig
Outdated
Show resolved
Hide resolved
Just found a situation where a callback is needed if the translations parameters are depending on the choice. @xabbuh Do you know how I can support something like
|
1ea5346
to
ecefd31
Compare
ecefd31
to
d876022
Compare
Hi @xabbuh, I totally rework the PR and updated the symfony/symfony-docs#13677 I change the way I implemented WDYT about it ? :) |
f31e0c4
to
453bd62
Compare
@xabbuh I don't know why the |
@VincentLanglet You should probably bump de dep of |
c9beb74
to
944648c
Compare
Indeed ! Thanks. I think this PR is finally ready to review then. :) |
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.
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 = []) |
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.
Adding the argument here is a BC break.
src/Symfony/Component/Form/ChoiceList/Factory/ChoiceListFactoryInterface.php
Show resolved
Hide resolved
src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Form/ChoiceList/Factory/PropertyAccessDecorator.php
Outdated
Show resolved
Hide resolved
@VincentLanglet you don't need to pass a translatable as key in the So yes, the feature does exists currently. |
Oh I didn't think it that way. But, I propose the feature
And you recommend
I have trouble to see this solution as simpler. 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 ? :) |
Personally I tend to never use the keys in
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
It starts to be verbose. And if it was the recommended way, Symfony wouldn't use the I find your solution over-complicated, you don't like mine. For instance
And
And none of these solution are deprecated. |
@VincentLanglet Can you have a look at the failing tests? |
5fde5f8
to
a962f60
Compare
I fixed the error I got with missing space in
line in the I also remove the deprecation I added since there is already one.
But I don't know how to avoid getting this in tests. I'm getting this because the |
a962f60
to
026ed90
Compare
We've just moved away from |
Sure #38469 |
…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
…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
…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