[Form] Deprecated usage of "choices" option in sub types #21919

HeahDude · 2017-03-08T06:20:47Z

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#...
License	MIT
Doc PR	~

Follows #21880 and the discussion in #20771.

xabbuh · 2017-03-08T10:49:50Z

UPGRADE-4.0.md

@@ -184,6 +184,31 @@ Form
   }
   ```

+ * Using the "choices" option in ``CountryType``, ``CurrencyType``, ``LanguageType``,
+  ``LocaleType``, and ``TimezoneType`` is now ignored. Use the "choice_loader" option
+   instead or explicitly set it to ``null``.


Should we really support both ways to solve it?

Yes I think so, if one wants to override choices, he should not be forced to override the loader.

It makes no sense IMO to enforce lazy load something that is already loaded, if you have your choices, just pass them, do not encapsulate them in an objet that will eventually call a function to return the initial thing.

@xabbuh supporting a single way to achieve it would actually involve more work to forbid one of them. Both ways are just features of the ChoiceType: if choice_loader is not null, it is used. Otherwise choices is used.

The CountryType & co currently have to do extra work to ensure that this choices wins over the default choice_loader, due to BC (there was no default choice loader previously).

xabbuh · 2017-03-08T10:50:38Z

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

@@ -38,7 +38,13 @@ public function configureOptions(OptionsResolver $resolver)
    {
        $resolver->setDefaults(array(
            'choice_loader' => function (Options $options) {
-                return $options['choices'] ? null : $this;
+                if ($options['choices']) {
+                    @trigger_error(sprintf('Using "choices" option in %s has been deprecated since version 3.3 and will be ignored in 4.0. Override the "choice_loader" option instead.', __CLASS__), E_USER_DEPRECATED);


Either we should only support the choice_loader solution in the future or talk about the possibility to explicitly set it to null here too.

I hesitated, considering your comments I'd rather add it explicitly here too, thanks.

Done, I've just updated the notices.

xabbuh · 2017-03-09T09:14:13Z

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

@@ -38,7 +38,13 @@ public function configureOptions(OptionsResolver $resolver)
    {
        $resolver->setDefaults(array(
            'choice_loader' => function (Options $options) {
-                return $options['choices'] ? null : $this;
+                if ($options['choices']) {
+                    @trigger_error(sprintf('Using "choices" option in %s has been deprecated since version 3.3 and will be ignored in 4.0. Override the "choice_loader" option instead or set it to null.', __CLASS__), E_USER_DEPRECATED);


Using the "choices" option [...]

Fixed everywhere.

xabbuh · 2017-03-09T09:16:28Z

src/Symfony/Component/Form/Tests/Extension/Core/Type/ExtendedChoiceTypeTest.php

+    public function testChoiceLoaderIsOverridden($type)
+    {
+        $factory = Forms::createFormFactoryBuilder()
+            ->addTypeExtension(new LazyChoiceTypeExtension($type))


Do we really need the type extension here?

I would keep it like it for consistency with the test above, and because it test both overriding the option directly and doing it with an extension, so looks safer as is.

HeahDude · 2017-03-26T20:53:47Z

Comments addressed and rebased. Ready for final review before merging, ping @symfony/deciders.

xabbuh · 2017-03-27T16:45:45Z

UPGRADE-3.3.md

+Form
+------
+
+ * Using the "choices" option in ``CountryType``, ``CurrencyType``, ``LanguageType``,


I think we need to reword this a bit to make clear that this is only deprecated as long as you do not set the choice_loader option to null (something like "using the choices option without overriding the choice_loader option is deprecated").

Good idea! Done.

xabbuh

👍

Tobion · 2017-03-28T08:51:07Z

UPGRADE-3.3.md

+   ```php
+   $builder->add('custom_locales', LocaleType::class, array(
+       'choices' => $availableLocales,
+       'choice_loader' => null,


That doesn't look intuitive to me.

Well, this is because choice_loader has priority over choices in ChoiceType (which is not documented IIRC btw)

And this should not be used ideally, the ChoiceType is more accurate in such cases, I'll update the doc to clarify this.

Wouldn't it be "better" to drop this example and just keep the second code snippet? And remove the recommendation to use choice_loader as null?

I understand this does not look pretty, but this is the actual quick fix for some breaking app.

What we can't do is to stop supporting "choices" option as it's inherited from the ChoiceType, and in such cases null is needed because of its precedence.

What we can do is to better document these types options, and explicit that "choices" or "choice_loader" options should be overridden on a ChoiceType or an EntityType only, not on a child already doing so, unless the previous value is reused.

I would like to address your comment, but as I said in a previous outdated diff:

It makes no sense IMO to enforce lazy load something that is already loaded, if you have your choices, just pass them, do not encapsulate them in an objet that will eventually call a function to return the initial thing.

Then the real fix would be to use the ChoiceType (if the previous "choices" values is not used), I could add this instead, but objectively I would let it as is now.

I'm waiting for your final call on this symfony deciders.

I am fine with how it is now. But we should indeed clarify the documentation about the differences of the choices and choice_loader options and when to use what.

fabpot · 2017-04-05T18:15:06Z

Thank you @HeahDude.

…es (HeahDude) This PR was merged into the 3.3-dev branch. Discussion ---------- [Form] Deprecated usage of "choices" option in sub types | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #...  | License | MIT | Doc PR | ~ Follows #21880 and the discussion in #20771. Commits ------- b5b56fc [Form] Deprecated usage of "choices" option in sub types

This PR was merged into the 3.3 branch. Discussion ---------- Explain how to properly override choices Update the documentation for symfony/symfony#21919. This fixes #7779. Commits ------- a9f934e explain how to properly override choices

carsonbot added Status: Needs Review Form Deprecation labels Mar 8, 2017

xabbuh reviewed Mar 8, 2017

View reviewed changes

HeahDude force-pushed the bc/choice-types branch from bbdba78 to 7ba98df Compare March 8, 2017 18:40

xabbuh reviewed Mar 9, 2017

View reviewed changes

nicolas-grekas added this to the 3.3 milestone Mar 10, 2017

HeahDude force-pushed the bc/choice-types branch 2 times, most recently from 1477302 to 64ddfa1 Compare March 26, 2017 20:52

xabbuh reviewed Mar 27, 2017

View reviewed changes

[Form] Deprecated usage of "choices" option in sub types

b5b56fc

HeahDude force-pushed the bc/choice-types branch from 64ddfa1 to b5b56fc Compare March 27, 2017 16:53

xabbuh approved these changes Mar 28, 2017

View reviewed changes

Tobion reviewed Mar 28, 2017

View reviewed changes

fabpot merged commit b5b56fc into symfony:master Apr 5, 2017

HeahDude deleted the bc/choice-types branch April 5, 2017 21:00

javiereguiluz mentioned this pull request Apr 9, 2017

[Form] Deprecated usage of "choices" option in sub types symfony/symfony-docs#7779

Closed

HeahDude restored the bc/choice-types branch April 23, 2017 08:39

HeahDude deleted the bc/choice-types branch April 23, 2017 08:39

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

xabbuh mentioned this pull request Jun 3, 2017

Explain how to properly override choices symfony/symfony-docs#7990

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] Deprecated usage of "choices" option in sub types #21919

[Form] Deprecated usage of "choices" option in sub types #21919

HeahDude commented Mar 8, 2017

xabbuh Mar 8, 2017

HeahDude Mar 8, 2017 •

edited

Loading

stof Mar 8, 2017 •

edited

Loading

xabbuh Mar 8, 2017

HeahDude Mar 8, 2017

HeahDude Mar 8, 2017

xabbuh Mar 9, 2017

HeahDude Mar 26, 2017

xabbuh Mar 9, 2017

HeahDude Mar 26, 2017

HeahDude commented Mar 26, 2017

xabbuh Mar 27, 2017

HeahDude Mar 27, 2017

xabbuh left a comment

Tobion Mar 28, 2017

stof Mar 28, 2017

HeahDude Mar 28, 2017

fabpot Mar 29, 2017

HeahDude Mar 29, 2017

xabbuh Mar 29, 2017

fabpot commented Apr 5, 2017

[Form] Deprecated usage of "choices" option in sub types #21919

[Form] Deprecated usage of "choices" option in sub types #21919

Conversation

HeahDude commented Mar 8, 2017

Choose a reason for hiding this comment

HeahDude Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

stof Mar 8, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeahDude commented Mar 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh 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

fabpot commented Apr 5, 2017

HeahDude Mar 8, 2017 •

edited

Loading

stof Mar 8, 2017 •

edited

Loading