[Form] choices ids are malformed #1919

brikou · 2011-08-07T11:19:01Z

Choices ids are not malformed, they should be normalized (slugged)

When using this...

<?php

$builder
    ->add('color', 'choice', array(
        'choices' => array(
            ($k = 'The color #1') => $k,
            ($k = 'The color #2') => $k,
            ($k = 'The color #3') => $k,
        ),
        'expanded' => true,
    ))
;

I get this...

<div id="form_color">
    <input type="radio" id="form_color_The color #1" name="form[color]" required="required" value="The color #1">
    <label for="form_color_The color #1" class="required">The color #1</label>

    <input type="radio" id="form_color_The color #2" name="form[color]" required="required" value="The color #2">
    <label for="form_color_The color #2" class="required">The color #2</label>

    <input type="radio" id="form_color_The color #3" name="form[color]" required="required" value="The color #3">
    <label for="form_color_The color #3" class="required">The color #3</label>
</div>

Instead of this...

<div id="form_color">
    <input type="radio" id="form_color_the-color-1" name="form[color]" required="required" value="The color #1">
    <label for="form_color_the-color-1" class="required">The color #1</label>

    <input type="radio" id="form_color_the-color-2" name="form[color]" required="required" value="The color #2">
    <label for="form_color_the-color-2" class="required">The color #2</label>

    <input type="radio" id="form_color_the-color-3" name="form[color]" required="required" value="The color #3">
    <label for="form_color_the-color-3" class="required">The color #3</label>
</div>

NB: Things would be easier to implement/test with a new standalone Inflector Component (with dasheize, pluralize, humanize ... functions)

phidah · 2011-08-09T06:33:55Z

Good idea, however it should be made sure that the same ID is not generated twice if the "robotization" of the names generate the same string.

brikou · 2011-08-09T06:40:33Z

thanks @phidah, I think that duplication of IDs should be prevented by coder himself when creating the form

phidah · 2011-08-09T06:50:35Z

I disagree, since the coder has no control of the "robotification" of the name.
Instead something should probably be appended in order to make them unique (such as "-1", "-2", etc.).

brikou · 2011-08-09T07:14:33Z

IMO implementing this kind of prevention is quite hard to do, and anyway if there is any duplication of id, it doesn't break the behaviour of the form.
For now I'd like to have feedback for the main pruporse of this PR ;)

stof · 2011-08-09T07:18:57Z

@brikou duplicated ids break the behavior of the form. Clicking on the label checks the radio input, but if the id is duplicated, it will either check both or a random one among them.

henrikbjorn · 2011-08-09T07:19:19Z

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

@@ -71,6 +71,7 @@ class FieldType extends AbstractType
    public function buildView(FormView $view, FormInterface $form)
    {
        $name = $form->getName();
+        $name = $this->robotize($name);


why not $name = $this->robotize($form->getName());

@henrikbjorn @phidah fixed ;)

brikou · 2011-08-09T07:36:43Z

@stof I meant it doesn't break the form submission behaviour (on the server side)... but having the same ids would mean that some labels would be the same (omitting special char like # ! ?)
This PR is not perfect but better than the actual behaviour of id generation.

henrikbjorn · 2011-08-09T07:37:23Z

I know lithium have a inflector class, maybe we can borrow it

stof · 2011-08-09T07:38:28Z

@brikou The form submission would not be broken but the usability of the form would be. So it is an issue.

brikou · 2011-08-09T07:45:19Z

@stof I finally agree (i finally thought about a problematic use case) so I guess a first loop over choices is required, isn't it... I also think we should write a test , but can you help me writing this test?

brikou · 2011-08-09T08:32:34Z

So this patch works with:

<?php

$builder
    ->add('color', 'choice', array(
        'choices' => array(
            ($k = 'The color #1') => $k,
            ($k = 'The color #2') => $k,
            ($k = 'The color #3') => $k,
        ),
        'expanded' => true,
    ))
;

but not with this (because dasheized names leads to not unique id):

<?php

$builder
    ->add('color', 'choice', array(
        'choices' => array(
            ($k = 'The color ???') => $k,
            ($k = 'The color !!!') => $k,
            ($k = 'The color $$$') => $k,
        ),
        'expanded' => true,
    ))
;

if anyone has a clue (@stof or even @fabpot ;)) on how to loop over sibling radio in order to make sure generated id is unique it would be great ;)

henrikbjorn · 2011-08-09T08:34:09Z

well we could just suffix them with uniqid() or something

phidah · 2011-08-09T09:19:37Z

Woundn't be very pretty but would work.

brikou · 2011-08-09T15:06:26Z

Ok now everything works without duplicated ids... (works event with multiple set to true) @stof can you give me your feedback

stof · 2011-08-09T16:17:24Z

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

+        $clean = iconv('UTF-8', 'ASCII//TRANSLIT', $str);
+        $clean = preg_replace("/[^a-zA-Z0-9\/_|+ -]/", '', $clean);
+        $clean = strtolower(trim($clean, '-'));
+        $clean = preg_replace("/[\/_|+ -]+/", $delimiter, $clean);


why removing the underscores ? they are valid AFAIK.

just to keep a single delimiter

but this is what force you to use a hack for the CSRF token field. And the form framework already uses underscores as delimiter in many places.

I know that underscore is used a lot but with the use of the dash as a delimiter, it emphasizes the hierarchy in the form's ids wich is usefull when writing js for example

$('#post_author-name'); // with dash, i can say that this selector is related to post's author_name field $('#post_author_name'); // with underscore, ... I don't know what is the relation

This is a silly example but I'm sure you understand what I'm trying to explain

fabpot · 2011-08-23T07:19:04Z

Apart form my comments, I don't really like this patch. I understand the problem but it makes things look really complicated.

brikou · 2011-08-23T07:24:00Z

I'm glad you understand this problem, I think a great thing would be to have a shared inflector class, which could be use by every component when needed (in form view to humanize label, dasheize id, by twig to generate class from "category" for instance, by doctrine to pluralize, camelize, tableize etc...).

@fabpot Should I close this PR or keep it open (to keep the emphase of this matter) and wait for a better implementation?

fabpot · 2011-08-23T07:33:42Z

Keep it open.

stof · 2011-08-23T10:18:44Z

@brikou having a shared one between the third-party libraries used by Symfony seems weird. Each of these library should not depend on Symfony2. Thus, Doctrine already has a tool to do this for their need, and for Twig, I don't see what you mean by generate class from "category"

…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.

@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.

dasheized field name

97f667b

henrikbjorn reviewed Aug 9, 2011
View reviewed changes

fixed

0eecc42

fixed duplicates ids

9af78fe

stof reviewed Aug 9, 2011
View reviewed changes

webmozart mentioned this pull request Jan 19, 2012

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

Merged

webmozart added a commit to webmozart/symfony that referenced this pull request Jan 23, 2012

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

87b16e7

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

fabpot closed this Jan 28, 2012

Uh oh!

[Form] choices ids are malformed #1919

[Form] choices ids are malformed #1919

Uh oh!

Conversation

brikou commented Aug 7, 2011

Uh oh!

phidah commented Aug 9, 2011

Uh oh!

brikou commented Aug 9, 2011

Uh oh!

phidah commented Aug 9, 2011

Uh oh!

brikou commented Aug 9, 2011

Uh oh!

stof commented Aug 9, 2011

Uh oh!

henrikbjorn Aug 9, 2011

Choose a reason for hiding this comment

Uh oh!

brikou Aug 9, 2011

Choose a reason for hiding this comment

Uh oh!

brikou commented Aug 9, 2011

Uh oh!

henrikbjorn commented Aug 9, 2011

Uh oh!

stof commented Aug 9, 2011

Uh oh!

brikou commented Aug 9, 2011

Uh oh!

brikou commented Aug 9, 2011

Uh oh!

henrikbjorn commented Aug 9, 2011

Uh oh!

phidah commented Aug 9, 2011

Uh oh!

brikou commented Aug 9, 2011

Uh oh!

stof Aug 9, 2011

Choose a reason for hiding this comment

Uh oh!

brikou Aug 9, 2011

Choose a reason for hiding this comment

Uh oh!

stof Aug 9, 2011

Choose a reason for hiding this comment

Uh oh!

brikou Aug 9, 2011

Choose a reason for hiding this comment

Uh oh!

fabpot commented Aug 23, 2011

Uh oh!

brikou commented Aug 23, 2011

Uh oh!

fabpot commented Aug 23, 2011

Uh oh!

stof commented Aug 23, 2011

Uh oh!

Uh oh!