[Form] Add choice_translation_locale option for Intl choice types #26825

yceruto · 2018-04-05T21:19:59Z

Q	A
Branch?	master (4.1)
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#26737
License	MIT
Doc PR	symfony/symfony-docs#...

This PR adds possibility to show the list of elements for a custom locale different to the current one and proposes also deprecate the ChoiceLoaderInterface implementation from all of them, moving it to a separate class.

See related issue #26737 for full description and use-case.

After:

$formBuilder->add('country', CountryType::class, [
    'choice_translation_locale' => 'es',
]);

Alternative of #23629

TODO:

Update UPGRADE-*.md and src/**/CHANGELOG.md files.
Add some tests.

yceruto · 2018-04-05T22:30:43Z

src/Symfony/Component/Form/Extension/Core/Type/CountryType.php

+                // BC layer
+                $self = clone $this;
+                $self->choiceLoader = $choiceLoader;
+


Instead of deprecating each ChoiceLoaderInterface methods (trigerring a lot of messages) shouldn't be enough just one here? deprecating the usage of this form type as choice loader? wdyt?

Can't we always return the new loader here and just add deprecations to the ChoiceLoaderInterface's methods without modifying them? We indeed target to remove the ChoiceLoaderInterface from all of these types, right?

mmh.. I guess that was what we did in https://github.com/symfony/symfony/pull/23648/files#diff-c6198934d4f35bc102ff362d40b71285R61, but isn't it a BC break?

I mean, if we return the new loader now then, we're ignoring any custom behavior for someone overriding loadChoiceList() method without previous advice, no? ping @ro0NL ?

I guess it's a pragmatic choice; switching an implementation detail. Technically no API was broken 😅

Maybe to overcome preserve the legacy loader somehow, like return new CallbackLegacyChoiceLoader($callable, $this).

we're ignoring any custom behavior for someone overriding loadChoiceList()

Im really not sure how big an issue this is, in practice. In theory you're right.

That way you can check if a API method is overridden on the legacy loader and deprecate from there, while keep calling it.

I agree with @ro0NL. Not sure it is worth it.

Agree, in that case I prefer @ro0NL's version, I did the changes, thanks!

ogizanagi

Thanks for taking care of this @yceruto :)

ogizanagi · 2018-04-07T17:13:53Z

src/Symfony/Component/Form/Extension/Core/Type/CountryType.php

 use Symfony\Component\OptionsResolver\OptionsResolver;

 class CountryType extends AbstractType implements ChoiceLoaderInterface
 {
    /**
-     * Country loaded choice list.
+     * BC layer.


By marking it @deprecated instead, even if it's private, would help a bit when reading these classes from an IDE as the property usages in this class will be striked out.

ogizanagi · 2018-04-07T17:21:47Z

src/Symfony/Component/Form/Extension/Core/Type/CountryType.php

+                // BC layer
+                $self = clone $this;
+                $self->choiceLoader = $choiceLoader;
+


Can't we always return the new loader here and just add deprecations to the ChoiceLoaderInterface's methods without modifying them? We indeed target to remove the ChoiceLoaderInterface from all of these types, right?

yceruto · 2018-04-08T00:36:42Z

@ogizanagi thank you for this review.

Status: Waiting consensus on depreciation path :)
Status: Needs Work

yceruto · 2018-04-08T17:56:16Z

Status: Needs Review

yceruto · 2018-04-18T03:18:30Z

Just squashing commits, ready on my side.

fabpot

small CS change needed

fabpot · 2018-04-20T04:35:35Z

src/Symfony/Component/Form/Extension/Core/Type/CountryType.php

     */
    public function loadChoiceList($value = null)
    {
+        @trigger_error(sprintf('Method "%s" is deprecated since Symfony 4.2. Use "choice_loader" option instead.', __METHOD__), E_USER_DEPRECATED);


I prefer one sentence for deprecations: ... since Symfony 4.2, use ...

xabbuh · 2018-04-20T06:53:18Z

UPGRADE-4.2.md

+Form
+----
+
+ * Deprecated `ChoiceLoaderInterface` implementation in `CountryType`, `LanguageType`, `LocaleType` and `CurrencyType`. Use the "choice_loader" option instead.


"choice_loader" should be enclosed by backticks too

Deprecated the ChoiceLoaderInterface implementation [...]

Fixed, thank you.

xabbuh · 2018-04-20T06:57:10Z

src/Symfony/Component/Form/Extension/Core/Type/CountryType.php

 use Symfony\Component\OptionsResolver\OptionsResolver;

-class CountryType extends AbstractType implements ChoiceLoaderInterface
+class CountryType extends AbstractType


I would not drop the implements here. Technically, that's a BC break.

yceruto · 2018-04-20T15:47:07Z

Done.

fabpot · 2018-04-22T06:19:33Z

Thank you @yceruto.

…hoice types (yceruto, fabpot) This PR was merged into the 4.1-dev branch. Discussion ---------- [Form] Add choice_translation_locale option for Intl choice types | Q | A | ------------- | --- | Branch? | master (4.2) | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #26737 | License | MIT | Doc PR | symfony/symfony-docs#... This PR adds possibility to show the list of elements for a custom locale different to the current one and proposes also deprecate the `ChoiceLoaderInterface` implementation from all of them, moving it to a separate class. See related issue #26737 for full description and use-case. After: ```php $formBuilder->add('country', CountryType::class, [ 'choice_translation_locale' => 'es', ]); ``` Alternative of #23629 TODO: - [x] Update `UPGRADE-*.md` and `src/**/CHANGELOG.md` files. - [x] Add some tests. Commits ------- 9592fa6 moved feature to 4.1 e250dfa Add choice_translation_locale option for Intl choice types

…nterface in intl forms (yceruto) This PR was merged into the 5.0-dev branch. Discussion ---------- [Form] Remove deprecated implementation of ChoiceLoaderInterface in intl forms | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - See previous PR symfony/symfony#26825 (4.1) Commits ------- b22cbf41c0 Remove deprecated implementation of ChoiceLoaderInterface in intl forms

…nterface in intl forms (yceruto) This PR was merged into the 5.0-dev branch. Discussion ---------- [Form] Remove deprecated implementation of ChoiceLoaderInterface in intl forms | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - See previous PR #26825 (4.1) Commits ------- b22cbf4 Remove deprecated implementation of ChoiceLoaderInterface in intl forms

carsonbot added Status: Needs Review Feature Deprecation labels Apr 5, 2018

yceruto changed the title ~~Add choice_translation_locale option for Intl choice types~~ [Form] Add choice_translation_locale option for Intl choice types Apr 5, 2018

yceruto force-pushed the choice_translation_locale branch from c81bf25 to b26715f Compare April 5, 2018 21:37

ogizanagi added this to the 4.2 milestone Apr 5, 2018

ogizanagi added the Form label Apr 5, 2018

yceruto force-pushed the choice_translation_locale branch from b26715f to 1480bbd Compare April 5, 2018 22:18

yceruto commented Apr 5, 2018

View reviewed changes

ogizanagi reviewed Apr 7, 2018

View reviewed changes

yceruto force-pushed the choice_translation_locale branch 2 times, most recently from b4c0658 to 59d4d00 Compare April 8, 2018 00:29

carsonbot added Status: Needs Work and removed Status: Needs Review labels Apr 8, 2018

carsonbot added Status: Needs Review and removed Status: Needs Work labels Apr 8, 2018

yceruto force-pushed the choice_translation_locale branch from bbc7834 to a69b31f Compare April 9, 2018 11:02

yceruto force-pushed the choice_translation_locale branch from a69b31f to a4797ac Compare April 18, 2018 02:59

fabpot approved these changes Apr 20, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 20, 2018

fabpot mentioned this pull request Apr 20, 2018

[Form] Added locale option for intl form types. #23629

Closed

xabbuh reviewed Apr 20, 2018

View reviewed changes

yceruto force-pushed the choice_translation_locale branch 2 times, most recently from 82381e4 to e250dfa Compare April 20, 2018 15:41

Add choice_translation_locale option for Intl choice types

e250dfa

fabpot approved these changes Apr 22, 2018

View reviewed changes

moved feature to 4.1

9592fa6

fabpot force-pushed the choice_translation_locale branch from 3f197b6 to 9592fa6 Compare April 22, 2018 06:17

fabpot modified the milestones: next, 4.1 Apr 22, 2018

fabpot merged commit 9592fa6 into symfony:master Apr 22, 2018

javiereguiluz mentioned this pull request Apr 22, 2018

[Form] Add choice_translation_locale option for Intl choice types symfony/symfony-docs#9657

Closed

yceruto deleted the choice_translation_locale branch April 22, 2018 16:13

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

yceruto mentioned this pull request May 28, 2019

[Form] Remove deprecated implementation of ChoiceLoaderInterface in intl forms #31665

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] Add choice_translation_locale option for Intl choice types #26825

[Form] Add choice_translation_locale option for Intl choice types #26825

yceruto commented Apr 5, 2018 •

edited by fabpot

Loading

yceruto Apr 5, 2018

ogizanagi Apr 7, 2018

yceruto Apr 8, 2018 •

edited

Loading

ro0NL Apr 8, 2018

ro0NL Apr 8, 2018

ogizanagi Apr 8, 2018

yceruto Apr 8, 2018

ogizanagi left a comment

ogizanagi Apr 7, 2018

ogizanagi Apr 7, 2018

yceruto commented Apr 8, 2018

yceruto commented Apr 8, 2018

yceruto commented Apr 18, 2018

fabpot left a comment

fabpot Apr 20, 2018

yceruto Apr 20, 2018

xabbuh Apr 20, 2018

xabbuh Apr 20, 2018

yceruto Apr 20, 2018

xabbuh Apr 20, 2018

yceruto Apr 20, 2018

yceruto commented Apr 20, 2018

fabpot commented Apr 22, 2018

[Form] Add choice_translation_locale option for Intl choice types #26825

[Form] Add choice_translation_locale option for Intl choice types #26825

Conversation

yceruto commented Apr 5, 2018 • edited by fabpot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto Apr 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto commented Apr 8, 2018

yceruto commented Apr 8, 2018

yceruto commented Apr 18, 2018

fabpot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto commented Apr 20, 2018

fabpot commented Apr 22, 2018

yceruto commented Apr 5, 2018 •

edited by fabpot

Loading

yceruto Apr 8, 2018 •

edited

Loading