[Form] Improved ChoiceList implementation and made form naming more restrictive #3156

webmozart · 2012-01-19T16:43:35Z

Bug fix: yes
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #2869, #3021, #1919, #3153
Todo: adapt documentation

The changes in this PR are primarily motivated by the fact that invalid form/field names lead to various problems.

When a name contains any characters that are not permitted in HTML "id" attributes, these are invalid
When a name contains periods ("."), form validation is broken, because they confuse the property path resolution
Since choices in expanded choice fields are directly translated to field names, choices applying to either 1. or 2. lead to problems. But choices should be unrestricted.
Unless a choice field is not expanded and does not allow multiple selection, it is not possible to use empty strings as choices, which might be desirable in some occasions.

The solution to these problems is to

Restrict form names to disallow unpermitted characters (solves 1. and 2.)
Generate integer indices to be stored in the HTML "id" and "name" attributes and map them to the choices (solves 3.). Can be reverted to the old behaviour by setting the option "index_generation" to ChoiceList::COPY_CHOICE
Generate integer values to be stored in the HTML "value" attribute and map them to the choices (solves 4.). Can be reverted to the old behaviour by setting the option "value_generation" to ChoiceList::COPY_CHOICE

Apart from these fixes, it is now possible to write more flexible choice lists. One of these is ObjectChoiceList, which allows to use objects as choices and is bundled in the core. EntityChoiceList has been made an extension of this class.

$form = $this->createFormBuilder()
    ->add('object', 'choice', array(
        'choice_list' => new ObjectChoiceList(
            array($obj1, $obj2, $obj3, $obj4),
            // property path determining the choice label (optional)
            'name',
            // preferred choices (optional)
            array($obj2, $obj3),
            // property path for object grouping (optional)
            'category',
            // property path for value generation (optional)
            'id',
            // property path for index generation (optional)
            'id'
        )
    ))
    ->getForm()
;

stof · 2012-01-19T16:51:24Z

UPGRADE-2.1.md

+  contain ASCII letters, digits, letters, colons and underscores, you can
+  restore the old behaviour by setting the option "index_strategy" of the
+  choice field to `ChoiceList::COPY_CHOICE`.


duplicated text. lines 119 to 124 are the same than 104 to 109

kriswallsmith · 2012-01-19T18:09:09Z

Rather than passing choices and a choice_labels arrays to the view would it make sense to introduce a ChoiceView class and pass one array of objects?

stof · 2012-01-22T15:32:36Z

@bschussek can you update your PR according to the feedback (and rebase it as it conflicts according to github) ?

RapotOR · 2012-01-22T16:04:56Z

src/Symfony/Bridge/Doctrine/Form/ChoiceList/ORMQueryBuilderLoader.php

+    {
+        $qb = clone ($this->queryBuilder);
+        $alias = current($qb->getRootAliases());
+        $where = $qb->expr()->in($alias.'.'.$identifier, "?1");


There is an issue if the parameter 1 is already defined in the query_builder option, no?

there is indeed an issue

Any idea how to fix this? @beberlei ?

It is possible to retrieve the list of parameters from the query_builder with getParameters().
In #3095, I used something like:

$parameter = "ORMQueryBuilderLoader_getEntitiesByIds_".$identifier; $where = $qb->expr()->in($alias.'.'.$identifier, ":".$parameter); return $qb->andWhere($where) ->getQuery() ->setParameter($parameter, $values, Connection::PARAM_STR_ARRAY) ->getResult();

But it is not totally unique... :)

Looks good enough to me.

…tions Fixes symfony#2869, fixes symfony#3021, fixes symfony#1919, fixes symfony#3153.

…tion of HTML IDs and to (2) avoid problems with property paths. ad (1): HTML4 "id" attributes are limited to strings starting with a letter and containing only letters, digits, underscores, hyphens, periods and colons. ad (2): Property paths contain three special characters needed for correct parsing: left/right bracket and period. The rules for form naming are: * Names may start with a letter, a digit or an underscore. Leading digits or underscores will be stripped from the "id" attributes. * Names must only contain letters, digits, underscores, hyphens and colons. * Root forms may have an empty name. Solves symfony#1919 and symfony#3021 on a wider scope.

Tobion · 2012-01-23T19:30:17Z

src/Symfony/Component/Form/Extension/Core/ChoiceList/ChoiceList.php

+
+        foreach ($this->choices as $i => $choice) {
+            foreach ($choices as $j => $givenChoice) {
+                if ($choice === $this->fixChoice($givenChoice)) {


this seems to be "fixed" twice. first $choices = $this->fixChoices($choices); and then is each choice fixed again.

…view

…nique

webmozart · 2012-01-24T00:15:42Z

@kriswallsmith fixed

Fixed all outstanding issues. Would be glad if someone could review again, otherwise this PR is ready to merge.

Tobion · 2012-01-24T02:35:19Z

CHANGELOG-2.1.md

@@ -166,6 +166,37 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c
 * added support for empty form name at root level, this enables rendering forms
   without form name prefix in field names

+ * [BC BREAK] greatly improved `ChoiceListInterface` and all of its
+   implementations. `EntityChoiceList` was adapted, the methods `getEntities()`,
+   `getEntitiesByByKeys()`, `getIdentifier()` and `getIdentifierValues()` were


typo: getEntitiesByKeys

Tobion · 2012-01-24T03:20:59Z

src/Symfony/Component/Form/Extension/Core/ChoiceList/ChoiceListInterface.php

+     * Returns the choice views of the preferred choices as nested array with
+     * the choice groups as top-level keys.
+     *
+     * @return array A nested array containing the views with the corresponding


Maybe code example.

… in choice lists match their requirements

fabpot · 2012-01-25T15:17:59Z

Is it ready to be merged?

Tobion · 2012-01-25T15:35:50Z

Yes I think so. He said it's ready to be merged when reviewed.

webmozart · 2012-01-26T02:30:36Z

Yes.

stloyd · 2012-01-26T08:26:00Z

src/Symfony/Component/Form/Extension/Core/ChoiceList/ChoiceList.php

+     */
+    protected function fixValues(array $values)
+    {
+        return array_map(array($this, 'fixValue'), $values);


AFAIK foreach would be faster.

foreach ($values as $i => $value) { $values[$i] = $this->fixValue($value); }

Fixed. FYI: The difference is quite minimal, but still there.

webmozart · 2012-01-28T12:39:00Z

Fixed outstanding issues. Ready for merge.

@kriswallsmith

Commits ------- 8dc78bd [Form] Fixed YODA issues 600cec7 [Form] Added missing entries to CHANGELOG and UPGRADE b154f7c [Form] Fixed docblock and unneeded use statement 399af27 [Form] Implemented checks to assert that values and indices generated in choice lists match their requirements 5f6f75c [Form] Fixed outstanding issues mentioned in the PR 7c70976 [Form] Fixed text in UPGRADE file c26b47a [Form] Made query parameter name generated by ORMQueryBuilderLoader unique 18f92cd [Form] Fixed double choice fixing f533ef0 [Form] Added ChoiceView class for passing choice-related data to the view d72900e [Form] Incorporated changes suggested in PR comments 28d2f6d Removed duplicated lines from UPGRADE file e1fc5a5 [Form] Restricted form names to specific characters to (1) fix generation of HTML IDs and to (2) avoid problems with property paths. 87b16e7 [Form] Greatly improved ChoiceListInterface and all of its implementations Discussion ---------- [Form] Improved ChoiceList implementation and made form naming more restrictive Bug fix: yes Feature addition: yes Backwards compatibility break: **yes** Symfony2 tests pass: yes Fixes the following tickets: #2869, #3021, #1919, #3153 Todo: adapt documentation ![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue1919) The changes in this PR are primarily motivated by the fact that invalid form/field names lead to various problems. 1. When a name contains any characters that are not permitted in HTML "id" attributes, these are invalid 2. When a name contains periods ("."), form validation is broken, because they confuse the property path resolution 3. Since choices in expanded choice fields are directly translated to field names, choices applying to either 1. or 2. lead to problems. But choices should be unrestricted. 4. Unless a choice field is not expanded and does not allow multiple selection, it is not possible to use empty strings as choices, which might be desirable in some occasions. The solution to these problems is to * Restrict form names to disallow unpermitted characters (solves 1. and 2.) * Generate integer indices to be stored in the HTML "id" and "name" attributes and map them to the choices (solves 3.). Can be reverted to the old behaviour by setting the option "index_generation" to ChoiceList::COPY_CHOICE * Generate integer values to be stored in the HTML "value" attribute and map them to the choices (solves 4.). Can be reverted to the old behaviour by setting the option "value_generation" to ChoiceList::COPY_CHOICE Apart from these fixes, it is now possible to write more flexible choice lists. One of these is `ObjectChoiceList`, which allows to use objects as choices and is bundled in the core. `EntityChoiceList` has been made an extension of this class. $form = $this->createFormBuilder() ->add('object', 'choice', array( 'choice_list' => new ObjectChoiceList( array($obj1, $obj2, $obj3, $obj4), // property path determining the choice label (optional) 'name', // preferred choices (optional) array($obj2, $obj3), // property path for object grouping (optional) 'category', // property path for value generation (optional) 'id', // property path for index generation (optional) 'id' ) )) ->getForm() ; --------------------------------------------------------------------------- by kriswallsmith at 2012-01-19T18:09:09Z Rather than passing `choices` and a `choice_labels` arrays to the view would it make sense to introduce a `ChoiceView` class and pass one array of objects? --------------------------------------------------------------------------- by stof at 2012-01-22T15:32:36Z @bschussek can you update your PR according to the feedback (and rebase it as it conflicts according to github) ? --------------------------------------------------------------------------- by bschussek at 2012-01-24T00:15:42Z @kriswallsmith fixed Fixed all outstanding issues. Would be glad if someone could review again, otherwise this PR is ready to merge. --------------------------------------------------------------------------- by fabpot at 2012-01-25T15:17:59Z Is it ready to be merged? --------------------------------------------------------------------------- by Tobion at 2012-01-25T15:35:50Z Yes I think so. He said it's ready to be merged when reviewed. --------------------------------------------------------------------------- by bschussek at 2012-01-26T02:30:36Z Yes. --------------------------------------------------------------------------- by bschussek at 2012-01-28T12:39:00Z Fixed outstanding issues. Ready for merge.

craue · 2012-01-29T15:12:55Z

src/Symfony/Component/Form/Extension/Core/ChoiceList/SimpleChoiceList.php

+     *
+     * @see parent::addChoices
+     */
+    protected function addChoices(&$bucketForPreferred, &$bucketForRemaining, $choices, $labels, array $preferredChoices)


$labels is never used

it is in the parent

@inheritdoc

Commits ------- 57cc531 [Form] Improved PHPDocs of choice lists 9e7e2af [Form] Fixed PHPDoc: Used {@inheritdoc} where applicable 2c530cc Fixed typos in UPGRADE file 7899bea Added examples to UPGRADE d346ae6 Improved choice list sections of UPGRADE and CHANGELOG a676598 [Form] Added class LazyChoiceList Discussion ---------- [Form] Added LazyChoiceList Bug fix: no Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: - Todo: - ![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue3156) Adds a ChoiceList implementation that satisfies people who formerly extended ArrayChoiceList and loaded choices lazily in its `load` method. --------------------------------------------------------------------------- by craue at 2012-01-30T12:56:49Z :+1: --------------------------------------------------------------------------- by craue at 2012-01-30T14:55:38Z Not sure if it's a bug in this PR or in #3156, but the labels get replaced by their keys: ```php <?php use Symfony\Component\Form\Extension\Core\ChoiceList\ChoiceList; use Symfony\Component\Form\Extension\Core\ChoiceList\LazyChoiceList; use Symfony\Component\Form\Extension\Core\ChoiceList\SimpleChoiceList; class MyChoiceList extends LazyChoiceList { protected function loadChoiceList() { $choices = array( 'bla' => 'blaaaaaahhhh', ); return new SimpleChoiceList($choices, array(), ChoiceList::COPY_CHOICE, ChoiceList::COPY_CHOICE); } public function getChoices() { $choices = parent::getChoices(); // $choices is array('bla' => 'bla') return $choices; } } ``` If it's not this PR, I can of course open a new ticket for that. But I'm only working with `LazyChoiceList`s. --------------------------------------------------------------------------- by stof at 2012-01-30T16:07:41Z @craue the ``SimpleChoiceList`` is an implementation using the same string as label and value. If you need different ones, you need to use the ``ChoiceList`` implementation. --------------------------------------------------------------------------- by craue at 2012-01-30T16:22:06Z @stof: That would make `SimpleChoiceList` useless for almost any case. Are you sure? --------------------------------------------------------------------------- by craue at 2012-01-30T16:33:31Z The bug even occurs when using ``` return new ChoiceList(array_keys($choices), array_values($choices), array(), ChoiceList::COPY_CHOICE, ChoiceList::COPY_CHOICE); ``` --------------------------------------------------------------------------- by stof at 2012-01-30T16:40:19Z well, the SimpleChoiceList is for simple cases (thus its naming) where you want the same for the label and the values. And if you look at the class, you will see it extends the ChoiceList implementation which is the generic one. --------------------------------------------------------------------------- by craue at 2012-01-30T16:53:36Z For me, "simple" would mean that it just takes the array given, using keys as indices and values as labels. No fancy stuff messing around with anything. :D But, is there anything wrong in this code or in mine? @bschussek: Please enlighten me. --------------------------------------------------------------------------- by bschussek at 2012-01-30T17:16:58Z You are both wrong :) `getChoices` does not return the choices with their associated labels anymore. What you get now are the choice indices as array keys and the choice values as array values. How both are determined depends on the index and value generation strategy, which, in your case, are both COPY_CHOICE. The difference between simple and complex choice lists is, that simple choice lists can only contain scalar values as choices, while other choice lists (such as ObjectChoiceList, EntityChoiceList) may contain objects as choices. Choice labels are now stored in ChoiceView objects, which are returned by the various `get*Views` methods. --------------------------------------------------------------------------- by craue at 2012-01-30T18:07:43Z It's pretty annoying having provided an array with keys and values when initializing the `ChoiceList` but being unable to retrieve it again. Guess I just over-used or even abused those choice lists as kind of (not only form related) lookup tables. --------------------------------------------------------------------------- by bschussek at 2012-01-30T19:27:21Z @craue: What's your use case? --------------------------------------------------------------------------- by craue at 2012-01-30T20:10:16Z I just used choice lists extensively, even for not directly form-related stuff. In one case, I'm using two of them (which are also used individually) to build up a third one. That went well using the old `ArrayChoiceList`s and their `getChoices` method. Just thinking about creating another set of model classes which just contain my lists. So for only one select field in a form it'll take three classes then: (A) a list, (B) a choice list based on A, (C) a choice form type based on B. Oh well ... :D --------------------------------------------------------------------------- by craue at 2012-01-30T21:31:32Z Anyway, this PR for `LazyChoiceList` is great, so please merge it. ;) --------------------------------------------------------------------------- by craue at 2012-01-31T14:00:46Z @bschussek: Is it ready to be merged? --------------------------------------------------------------------------- by bschussek at 2012-01-31T16:59:17Z Yes

arnaud-lb · 2012-02-15T11:06:09Z

UPGRADE-2.1.md

+    or when the field is a single-choice field and is not required), you can
+    restore the old behaviour by setting the option "value_strategy" to
+    `ChoiceList::COPY_CHOICE`.


Why is this a problem to have a choice-list containing an empty string ? What will happen if a choice-list contain an empty string ?

I see multiple problems with the default strategy, and I can't use the old strategy because of the empty string problem:

If the order of choices change in the time between the display of a page, and the submition of a form, the wrong value will be set.

It works only with browsers: if one built an API, and that API uses forms to handle data posted by clients, the clients can't know the value they have to send.

How javascripts can know which generated value maps to which real value ?

Point 1 could be addressed by hashing the original value to generate the value (if values aren't objects).

Point 2 can only be addressed by not using this strategy (but what about empty strings ?)

Point 3 can be addressed by exposing the mapping to the client side, but this seems to become complicated.

stof reviewed Jan 19, 2012
View reviewed changes

stof mentioned this pull request Jan 22, 2012

[Form] DoctrineType, Add widget property to avoid retrieving the whole ChoiceList #3095

Closed

RapotOR reviewed Jan 22, 2012
View reviewed changes

webmozart added 4 commits January 23, 2012 18:28

[Form] Greatly improved ChoiceListInterface and all of its implementa…

87b16e7

…tions Fixes symfony#2869, fixes symfony#3021, fixes symfony#1919, fixes symfony#3153.

Removed duplicated lines from UPGRADE file

28d2f6d

[Form] Incorporated changes suggested in PR comments

d72900e

Tobion reviewed Jan 23, 2012
View reviewed changes

webmozart added 3 commits January 24, 2012 01:07

[Form] Added ChoiceView class for passing choice-related data to the …

f533ef0

…view

[Form] Fixed double choice fixing

18f92cd

[Form] Made query parameter name generated by ORMQueryBuilderLoader u…

c26b47a

…nique

[Form] Fixed text in UPGRADE file

7c70976

Tobion reviewed Jan 24, 2012
View reviewed changes

webmozart added 2 commits January 24, 2012 11:59

[Form] Fixed outstanding issues mentioned in the PR

5f6f75c

[Form] Implemented checks to assert that values and indices generated…

399af27

… in choice lists match their requirements

stloyd reviewed Jan 26, 2012
View reviewed changes

webmozart added 3 commits January 28, 2012 13:36

[Form] Fixed docblock and unneeded use statement

b154f7c

[Form] Added missing entries to CHANGELOG and UPGRADE

600cec7

[Form] Fixed YODA issues

8dc78bd

fabpot merged commit 8dc78bd into symfony:master Jan 28, 2012

craue reviewed Jan 29, 2012
View reviewed changes

craue mentioned this pull request Jan 30, 2012

[Form] Added LazyChoiceList #3218

Merged

jmikola mentioned this pull request Jan 31, 2012

[Form] Update MongoDBQueryBuilderLoader for bridge interface doctrine/DoctrineMongoDBBundle#74

Merged

arnaud-lb reviewed Feb 15, 2012
View reviewed changes

vicb mentioned this pull request Apr 27, 2012

[WIP][2.1][Form] Revert some BC breaks #4134

Closed

Uh oh!

[Form] Improved ChoiceList implementation and made form naming more restrictive #3156

[Form] Improved ChoiceList implementation and made form naming more restrictive #3156

Uh oh!

Conversation

webmozart commented Jan 19, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kriswallsmith commented Jan 19, 2012

Uh oh!

stof commented Jan 22, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webmozart commented Jan 24, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Jan 25, 2012

Uh oh!

Tobion commented Jan 25, 2012

Uh oh!

webmozart commented Jan 26, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webmozart commented Jan 28, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!