8000 [Form][2.7] Incorrect/invalid select tag is generated if choices have duplicate content · Issue #15606 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form][2.7] Incorrect/invalid select tag is generated if choices have duplicate content #15606

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
NeoFusion opened this issue Aug 24, 2015 · 14 comments
Closed

[Form][2.7] Incorrect/invalid select tag is generated if choices have duplicate content #15606

NeoFusion opened this issue Aug 24, 2015 · 14 comments
Labels
Bug Form Status: Needs Review

Comments

@NeoFusion
Copy link

Incorrect/invalid select tag is generated if choices have duplicate content ('Category 4').

$f = $this->formFactory->createNamedBuilder('category', 'form')
    ->add('parent_category', 'choice', array(
        'label'   => 'Name',
        'choices' => array(
            '1' => 'Category 1',
            '2' => 'Category 4',
            '3' => 'Category 3',
            '4' => 'Category 4',
            '5' => 'Category 5'
        )
    ))
    ->add('save', 'submit', array(
        'label' => 'Save'
    ))
    ->getForm();
<select>
    <option value="1" >Category 1</option>
    <option value="4" >Category 4</option>
    <option value="3" >Category 3</option>
    <option value="5" >Category 5</option>
</select>

Where is option with value="2"?
@malarzm
Copy link
Contributor
malarzm commented Aug 25, 2015

For the record this is also happening when using expanded choice list

@georaldc
Copy link
Contributor

Might have something to do with the Symfony\Component\Form\ChoiceList\ArrayKeyChoiceList::flatten() method, where the choice labels themselves are used as keys for the $structuredValues array (thus preventing duplicates).

Happens in Symfony 2.8 as well

@jakzal jakzal added the Form label Sep 16, 2015
@YetiCGN
Copy link
YetiCGN commented Sep 25, 2015

+1

This is preventing upgrading to 2.7 on an app of ours. I have suspected duplicate content being the culprit, this is BC breaking in any case!

I have created a minimal test case fwiw: https://gist.github.com/YetiCGN/99085117fb0f6829ccb1

@YetiCGN
Copy link
YetiCGN commented Sep 26, 2015

Might have something to do with the Symfony\Component\Form\ChoiceList\ArrayKeyChoiceList::flatten() method, where the choice labels themselves are used as keys for the $structuredValues array (thus preventing duplicates).

I have covered that part with a test, it succeeds. This happens somehow in createView, I suspect a ViewTransformer. Creating a test case for that right now.

Edit: Nope, not the ViewTransformer either ... still searching...

Edit 2: Found it. It's the fact that without a group_by option set, it defaults to a flipped version of the choices array. That is then taken to render the choices. See https://github.com/symfony/form/blob/master/Extension/Core/Type/ChoiceType.php#L448

@Tobion
Copy link
Contributor
Tobion commented Sep 27, 2015

Related to #15061

@YetiCGN
Copy link
YetiCGN commented Sep 27, 2015

Here is what I've learned so far:

  1. The group_by default, calling self::flipRecursive($options['choices']) uses values as keys, so duplicates are removed. Setting group_by to array_keys($choices) solves this part.
  2. The DefaultChoiceListFactory uses values as keys, so duplicates are removed.
  3. Currently the only solution for me is to workaround by detecting duplicates and adding spaces programmatically. We need categories with the same name as the ChoiceType used displays hierarchical categories and although they have the same name, the database ID is different and their differentiation is done by context, i.e. after which parent category it is listed. So there is a use case for NOT filtering out duplicate choice values.
  4. The SimpleChoiceList is still available, which was used in 2.3 and does not show this bug, but I don't know how to force it from the FormBuilder. In any case, going forward to 3.0 we would like to use duplicate values for ChoiceType choices, regardless. ;)
  5. It would be BC-breaking from 2.7.5 to 2.7.x (once it's solved) for people relying on duplicates being removed, but I think the BC break from 2.3 or 2.6 is bigger. They should filter duplicates before passing to a ChoiceType anyway, so if they suddenly appear then the user code should be fixed. ;)
  6. Even if I use the option choices_values and define a callback for choice_name to use the name from the original array, the choices show the same behavior.

@YetiCGN
Copy link
YetiCGN commented Sep 27, 2015

Actually, the real issue seems to be contained in the ChoiceType:
An ArrayKeyChoiceList is used by the factory, because the ChoiceType says so. If an ArrayChoiceList is used, everything is fine. The corresponding code is in ChoiceType.php line 281.

Only if the option choices_as_values is set will it use the ArrayChoiceList by calling createListFromChoices instead of createListFromFlippedChoices. But as said in the comment above, that also breaks once you set the names or labels to duplicate values with a callback.

This is hard to fix, because we would need a new option that is immediately deprecated but also active by default to solve this. Or the ChoiceType should fall back to the deprecated SimpleChoiceList when called with a choices option and for 3.0 we would need a differently named array key.

@webmozart What are your thoughts on this?

@trsteel88 trsteel88 mentioned this issue Oct 12, 2015
Closed
@jaugustin
Copy link
Contributor

We have the same BC break in our application migrated from 2.3.x => 2.7.2

There is a lot of use case where you can't delete duplicate based on label

@malarzm
Copy link
Contributor
malarzm commented Oct 21, 2015

Since there's still no fix here's how we tackled the problem (ugly, yes, but works): in affected forms we append label with object's id like 'UNIQUE:Id' and later strip / UNIQUE:.+$/ from label before rendering it (prefix is at your discretion, we use MongoId so it's quite easy to not strip "real" part of label and we're even omitting prefix). So in general append something unique to label and strip it in Twig later 👶

@ewgRa
Copy link
Contributor
ewgRa commented Oct 30, 2015

Same for me.

We use choices like this

    30   => 'Bundle incl. %s products for <strong>%s</strong>',
    20   => 'Bundle incl. %s products for <strong>%s</strong>',

and now it is broken.

I think it can be fixed in DefaultChoiceListFactory::addChoiceViewsGroupedBy. Instead of call it with $list->getStructuredValues() it must be called with $list->getOriginalValues()
and also in addChoiceViewsGroupedBy value can't be array also as I don't see reason why this private method is static.

I will prepare PR, will see if it will be work.

@ewgRa ewgRa mentioned this issue Nov 3, 2015
Closed
@ewgRa
Copy link
Contributor
ewgRa commented Nov 3, 2015

Pull request done.

@ewgRa ewgRa mentioned this issue Nov 18, 2015
Closed
@Jean85
Copy link
Contributor
Jean85 commented Nov 25, 2015

+1
I'm having the same bug, in 2.7.5.

@peelandsee
Copy link

same problem here,
BC break upgrading from 2.3.
thanks for reporting.

@webmozart webmozart mentioned this issue Nov 26, 2015
Merged
@webmozart
Copy link
Contributor

This is fixed in the referenced PR. Does the fix work for you?

nicolas-grekas added a commit that referenced this issue Nov 27, 2015
@nicolas-grekas
bug #16685 [Form] Fixed: Duplicate choice labels are remembered when …
d628d92 
…using "choices_as_values" = false (webmozart)

This PR was merged into the 2.7 branch.

Discussion
----------

[Form] Fixed: Duplicate choice labels are remembered when using "choices_as_values" = false

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #15606
| License       | MIT
| Doc PR        | -

Commits
-------

1179f07 [Form] Fixed: Duplicate choice labels are remembered when using "choices_as_values" = false
@Tobion Tobion closed this as completed Dec 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Form Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
0