[Form] Fix `ChoiceType` options #6393

HeahDude · 2016-03-23T12:58:22Z

Q	A
Doc fix?	yes
New docs?	no
Applies to	all
Fixed tickets	#6144

HeahDude · 2016-03-23T13:01:36Z

reference/forms/types/choice.rst

+            'choices_as_values' => true,
+            'data' => true, // pre selected choice
+            'empty_data' => '1', // default submitted value
+        ));


Should we detail that when multiple it should be:

'data' => array(true), 'empty_data' => array('1'),

??

Ref symfony/symfony#18817.

This clearly needs to explained.

Man, this note basically doesn't make any sense to me :). I don't think it's your fault - I just find this choice stuff really hard. What exactly is the problem that we're trying to warn the user about? The only issue I can understand is that empty_data (if you want to set that) would need to equal 1 to simulate true (I think, because apparently these values will render as 0, 1, 2 because they can't be cast into strings?).

I'm normally under the impression that the user will rarely care about the HTML value attribute that is rendered/submitted - as long as if I select "Yes", I end up with true in my code.

This PR was merged into the 2.7 branch. Discussion ---------- [Form] remove useless copy in ChoiceType | Q | A | ------------- | --- | Branch? | 2.7+ | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | symfony/symfony-docs#6393 `ChoiceListFactoryInterface` expected `$groupBy` to be a callable or null, not an array ([ref](https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Form/ChoiceList/Factory/ChoiceListFactoryInterface.php#L111)). The factory defaults `groupBy` to `ChoiceListInterface::getStructuredValues()` ([ref](https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php#L122)). Hence, the copy of the choices array and the recursive flip are useless and may be slowing down performances imho (especially with `EntityType`). Commits ------- 562f5e4 [Form] remove useless code in ChoiceType

This PR was merged into the 2.7 branch. Discussion ---------- [Form] remove useless copy in ChoiceType | Q | A | ------------- | --- | Branch? | 2.7+ | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | symfony/symfony-docs#6393 `ChoiceListFactoryInterface` expected `$groupBy` to be a callable or null, not an array ([ref](https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Form/ChoiceList/Factory/ChoiceListFactoryInterface.php#L111)). The factory defaults `groupBy` to `ChoiceListInterface::getStructuredValues()` ([ref](https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php#L122)). Hence, the copy of the choices array and the recursive flip are useless and may be slowing down performances imho (especially with `EntityType`). Commits ------- 562f5e4 [Form] remove useless code in ChoiceType

HeahDude · 2016-04-25T14:24:06Z

This one needs reviews :)

alexislefebvre · 2016-05-10T16:47:19Z

reference/forms/types/choice.rst

@@ -83,40 +94,68 @@ code, while the **key** is what will be shown to the user.
    and will be removed in 3.0. To read about the old API, read an older version of
    the docs.

+.. note::
+
+    Pre selected choices will depend on the **data** passed to field and the values of


@HeahDude Should it be Preselected?

Je ne sais pas :) I would natively say "pré-selectionné" with a dash.

In case of doubt, our official reference is the "American English Oxford Dictionary": preselected is correct.

Thank you very much @javiereguiluz! Reference added in my favorites :)

HeahDude · 2016-06-25T17:21:36Z

Comments addressed.

Status: Needs Review

xabbuh · 2016-07-04T11:52:17Z

reference/forms/types/options/preferred_choices.rst.inc

@@ -46,6 +46,8 @@ This will "prefer" the "now" and "tomorrow" choices only:
 Finally, if your values are objects, you can also specify a property path string
 on the object that will return true or false.

+See the `choice_label`_ for details about using a property path or a callable.


This reference doesn't work.

weaverryan · 2016-07-10T17:41:27Z

reference/forms/types/choice.rst

+
+Also, the :class:``Symfony\\Component\\Form\\ChoiceList\\Factory\\ChoiceListFactoryInterface`` will cache the choice list
+so the same :class:``Symfony\\Component\\Form\\ChoiceList\\Loader\\ChoiceLoaderInterface`` can be used in different fields with more performance
+(reducing N queries to 1).


minor: a few of these lines got real long :)

HeahDude · 2016-07-10T19:46:59Z

@weaverryan thanks for the review!

Status: Needs Work

yceruto · 2017-01-04T15:03:36Z

reference/forms/types/choice.rst

+    Pre selected choices will depend on the **data** passed to the field and
+    the values of the ``choices`` option. However submitted choices will depend
+    on the **string** matching the **choice**. In the example above, the default
+    values are incrementing integers because ``null`` cannot be casted to string.


because null is not a scalar value.

https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Form/ChoiceList/ArrayChoiceList.php#L237

In fact, I think it's a bug ;) we can show "", "1", "0" values for array(null, true, false) choices and be more consistent.

Trying to fix that in symfony/symfony#21160

As said in your PR you should close it, this is why this needs a better doc :)

yceruto · 2017-01-04T16:29:37Z

reference/forms/types/choice.rst

+                'No' => false,
+            ),
+            'choices_as_values' => true,
+            'data' => true, // pre selected choice


This should be a bad practice. The reader could interpret that is a way to set default value and it isn't. As you know, if an object is bound to the form, the data option overrides this default value.

I suggest you to use $this->createFormBuilder(array('isAttending' => true)) from a controller for this sample. WDYT?

@yceruto I understand your comment but this is not about a bad practice here, it's about making a clear difference as I'm not sure #6265 is enough though.

weaverryan · 2017-10-06T20:54:19Z

Ping @HeahDude! Could you go through my review and finish this? Thx!

This PR was merged into the 3.4 branch. Discussion ---------- [Form] Fixed some phpdocs | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | symfony/symfony-docs#6393 ref symfony/symfony-docs#6144, symfony/symfony-docs#6297, #14050 Commits ------- b9162e8 [Form] Fixed some phpdocs

javiereguiluz · 2019-09-23T14:57:36Z

It's sad but this pull request is no longer mergeable. Things have changed too much and there are too many conflicts. We can open a new pull request in the future and get some inspiration from this one. I'm sorry for having let this pull request stale and thanks anyway for the contribution.

This PR was squashed before being merged into the 3.4 branch. Discussion ---------- [Form] minor fixes in ChoiceType options Revival of #6393 Commits ------- 4ee4102 [Form] minor fixes in ChoiceType options

HeahDude reviewed Mar 23, 2016
View reviewed changes

HeahDude force-pushed the fix-choice_type-options-allowed_types branch 4 times, most recently from 6da211f to b49bb5a Compare March 26, 2016 01:23

This was referenced Mar 26, 2016

[Form] remove useless copy in ChoiceType symfony/symfony#18316

Merged

[Form] [DX] choice_attr and choice_label_attr options in ChoiceType symfony/symfony#18318

Open

This was referenced Mar 30, 2016

Update documentation to describe ArrayChoiceList usage #5405

Closed

fix #17993 - Deprecated callable strings symfony/symfony#18020

Merged

HeahDude mentioned this pull request Apr 7, 2016

[Form] Incorrect description for choice_attr option of ChoiceType field #6443

Closed

HeahDude mentioned this pull request May 10, 2016

[Form][2.7] Preselection of choice field with boolean value symfony/symfony#14712

Closed

alexislefebvre reviewed May 10, 2016
View reviewed changes

xabbuh added the Status: Needs Work label May 11, 2016

[Form] Fix ChoiceType options

5430cec

HeahDude force-pushed the fix-choice_type-options-allowed_types branch from b49bb5a to 5430cec Compare June 25, 2016 17:20

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jun 25, 2016

xabbuh reviewed Jul 4, 2016
View reviewed changes

weaverryan reviewed Jul 10, 2016
View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jul 10, 2016

HeahDude mentioned this pull request Jan 4, 2017

choice_value callback argument can be null #7311

Closed

yceruto reviewed Jan 4, 2017

View reviewed changes

yceruto mentioned this pull request Jan 4, 2017

[Form] ChoiceType: Fix cast to string from null choice symfony/symfony#21160

Closed

yceruto reviewed Jan 4, 2017

View reviewed changes

HeahDude mentioned this pull request Feb 13, 2017

choice_value and choice_label problem symfony/symfony#21587

Closed

HeahDude mentioned this pull request Mar 4, 2017

Document callable for objects #7565

Closed

HeahDude added the Form label Mar 29, 2017

HeahDude added this to the 2.7 milestone Mar 29, 2017

codedmonkey added a commit to codedmonkey/symfony-docs that referenced this pull request Feb 22, 2018

Incorporate feedback on symfony#6393

b44c2b3

codedmonkey mentioned this pull request Feb 22, 2018

[WIP] [Form] Choice Loaders #9313

Closed

javiereguiluz modified the milestones: 2.7, 2.8 May 28, 2018

xabbuh modified the milestones: 2.8, 3.4 Nov 28, 2018

HeahDude mentioned this pull request Mar 12, 2019

[Form] Fixed some phpdocs symfony/symfony#30534

Merged

javiereguiluz closed this Sep 23, 2019

HeahDude mentioned this pull request Feb 17, 2020

[Form] minor fixes in ChoiceType options #13156

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] Fix `ChoiceType` options #6393

[Form] Fix `ChoiceType` options #6393

HeahDude commented Mar 23, 2016

HeahDude Mar 23, 2016

HeahDude May 20, 2016

weaverryan Jul 10, 2016

HeahDude commented Apr 25, 2016

alexislefebvre May 10, 2016 •

edited

Loading

HeahDude May 10, 2016

javiereguiluz May 10, 2016

HeahDude May 10, 2016

HeahDude commented Jun 25, 2016

xabbuh Jul 4, 2016

weaverryan Jul 10, 2016

HeahDude commented Jul 10, 2016

yceruto Jan 4, 2017

yceruto Jan 4, 2017 •

edited

Loading

HeahDude Jan 4, 2017

yceruto Jan 4, 2017

HeahDude Jan 4, 2017 •

edited

Loading

weaverryan commented Oct 6, 2017

javiereguiluz commented Sep 23, 2019

[Form] Fix ChoiceType options #6393

[Form] Fix ChoiceType options #6393

Conversation

HeahDude commented Mar 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeahDude commented Apr 25, 2016

alexislefebvre May 10, 2016 • 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

HeahDude commented Jun 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeahDude commented Jul 10, 2016

Choose a reason for hiding this comment

yceruto Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeahDude Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

weaverryan commented Oct 6, 2017

javiereguiluz commented Sep 23, 2019

[Form] Fix `ChoiceType` options #6393

[Form] Fix `ChoiceType` options #6393

alexislefebvre May 10, 2016 •

edited

Loading

yceruto Jan 4, 2017 •

edited

Loading

HeahDude Jan 4, 2017 •

edited

Loading