8000 [Form] Fixed handling of choices passed in choice groups by webmozart · Pull Request #15061 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] Fixed handling of choices passed in choice groups #15061

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
Merged
merged 1 commit into from
Jun 30, 2015
Merged

[Form] Fixed handling of choices passed in choice groups #15061

merged 1 commit into from
Jun 30, 2015

Conversation

webmozart
Copy link
Contributor
Q A
Bug fix? yes
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets #14915
License MIT
Doc PR -

I introduced a bug in the 2.7 ChoiceList implementation when choices are passed as groups:

$form->add('response', 'choice', array(
    'choices' => array(
        'Decided' => array($yesObj, $noObj),
        'Undecided' => array($maybeObj),
    ),
    // use getName() for the labels
    'choice_label' => 'name',
    'choices_as_values' => true,
));

In this example, since the choices $yesObj and $maybeObj have the same array index 0, the same label is displayed for the two options. The problem is that we rely on the keys passed in the "choices" option to identify choices in a choice list (which are, as you see, not guaranteed to be free of duplicates).

This PR changes the new choice list implementation to identify choices by values instead. We already have the guarantee that choices can be identified uniquely by their string values.

This PR should be included in 2.7.2 to fix the regression.

Unfortunately, a few BC breaks in the new implementation are necessary to make this fix:

  • The legacy ChoiceListInterface was reverted to how it was in 2.6 and does not extend the new ChoiceListInterface anymore.
  • As a consequence, legacy choice lists need to be wrapped into a LegacyChoiceListAdapter when they are passed to any place in the framework where a new choice list is expected.
  • The new ChoiceListInterface has two additional methods getStructuredValues() and getOriginalKeys() now.
  • ArrayKeyChoiceList::toArrayKey() was marked as internal.
  • ChoiceListFactoryInterface::createView() does not accept arrays and Traversables anymore for the $groupBy parameter (for simplicity).

@fabpot Where should we document the upgrade path for 2.7.1 => 2.7.2?

@webmozart webmozart mentioned this pull request Jun 22, 2015
Closed
@webmozart
Copy link
Contributor Author

ping @symfony/deciders

@soullivaneuh
Copy link
Contributor

Well, seems to makes a lot of BC break...

Maybe just add a note about the issue and how to solve it would be "less" painful.

@webmozart
Copy link
Contributor Author

@soullivaneuh Please consider that the BC breaks are only for new functionality introduced in 2.7. So people who implemented the new interfaces are affected. Any code that worked < 2.7 is not affected.

@fabpot
Copy link
Member
fabpot commented Jun 28, 2015

Not sure if we can break BC in 2.7.2 as 2.7 was released almost a month ago now.

What others @symfony/deciders think?

@webmozart
Copy link
Contributor Author

Can this be merged? To emphasize: This change fixes a bug in a new 2.7 feature that is a regression compared to the old implementation. I think that more people will be affected by this bug than by the BC break, as I don't think that many (if any) people implemented the new 2.7 interfaces so far.

ping @symfony/deciders

@fabpot
Copy link
Member
fabpot commented Jun 30, 2015

Thank you @webmozart.

@fabpot fabpot merged commit 7623dc8 into symfony:2.7 Jun 30, 2015
fabpot added a commit that referenced this pull request Jun 30, 2015
@fabpot
bug #15061 [Form] Fixed handling of choices passed in choice groups (…
b16fc6c 
…webmozart)

This PR was merged into the 2.7 branch.

Discussion
----------

[Form] Fixed handling of choices passed in choice groups

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

I introduced a bug in the 2.7 ChoiceList implementation when choices are passed as groups:

```
$form->add('response', 'choice', array(
    'choices' => array(
        'Decided' => array($yesObj, $noObj),
        'Undecided' => array($maybeObj),
    ),
    // use getName() for the labels
    'choice_label' => 'name',
    'choices_as_values' => true,
));
```

In this example, since the choices `$yesObj` and `$maybeObj` have the same array index `0`, the same label is displayed for the two options. The problem is that we rely on the keys passed in the "choices" option to identify choices in a choice list (which are, as you see, not guaranteed to be free of duplicates).

This PR changes the new choice list implementation to identify choices by values instead. We already have the guarantee that choices can be identified uniquely by their string values.

This PR should be included in 2.7.2 to fix the regression.

Unfortunately, a few BC breaks in the new implementation are necessary to make this fix:

* The legacy `ChoiceListInterface` was reverted to how it was in 2.6 and does *not* extend the new `ChoiceListInterface` anymore.
* As a consequence, legacy choice lists need to be wrapped into a `LegacyChoiceListAdapter` when they are passed to any place in the framework where a new choice list is expected.
* The new `ChoiceListInterface` has two additional methods `getStructuredValues()` and `getOriginalKeys()` now.
* `ArrayKeyChoiceList::toArrayKey()` was marked as internal.
* `ChoiceListFactoryInterface::createView()` does not accept arrays and Traversables anymore for the `$groupBy` parameter (for simplicity).

@fabpot Where should we document the upgrade path for 2.7.1 => 2.7.2?

Commits
-------

7623dc8 [Form] Fixed handling of choices passed in choice groups
@soullivaneuh
Copy link
Contributor

@webmozart Could we have a doc note with more explanation about introduced BC breaks and concrete case of how to handle it?

@webmozart
Copy link
Contributor Author

@soullivaneuh Yes, that's still missing.

@fabpot Where should we add the relevant notes? UPGRADE-2.7?

@weaverryan
Copy link
Member

@webmozart I think UPGRADE-2.7 - we can mention something in there about 2.7.2 containing the new change. That'll give us something to help for updating the docs too - so thx :)

webmozart added a commit to webmozart/symfony that referenced this pull request Jul 2, 2015
@webmozart
[Form] Added upgrade notes for symfony#15061
6325b4c
@webmozart webmozart mentioned this pull request Jul 2, 2015
Merged
@webmozart
Copy link
Contributor Author

See #15174 for the upgrade notes.

@Tobion Tobion mentioned this pull request Jul 2, 2015
Closed
fabpot added a commit that referenced this pull request Jul 2, 2015
@fabpot
minor #15174 [Form] Added upgrade notes for #15061 (webmozart)
2217c1f 
This PR was merged into the 2.7 branch.

Discussion
----------

[Form] Added upgrade notes for #15061

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

This PR contains the upgrade notes for #15061.

Commits
-------

6325b4c [Form] Added upgrade notes for #15061
@Tobion Tobion mentioned this pull request Jul 3, 2015
Closed
fabpot added a commit that referenced this pull request Jul 9, 2015
@fabpot
Merge branch '2.7' into 2.8
f5fefef 
* 2.7:
  Added 'default' color
  [HttpFoundation] Reload the session after regenerating its id
  [HttpFoundation] Add a test case to confirm a bug in session migration
  [Serializer] Fix ClassMetadata::sleep()
  [2.6] Static Code Analysis for Components and Bundles
  [Finder] Command::addAtIndex() fails with Command instance argument
  [DependencyInjection] Freeze also FrozenParameterBag::remove
  [Twig][Bridge] replaced `extends` with `use` in bootstrap_3_horizontal_layout.html.twig
  fix CS
  fixed CS
  Add a way to reset the singleton
  [Security] allow to use `method` in XML configs
  [Serializer] Fix Groups tests.
  Remove duplicate example
  Remove var not used due to returning early (introduced in 8982c32)
  [Serializer] Fix Groups PHPDoc
  Enhance hhvm test skip message
  fix for legacy asset() with EmptyVersionStrategy
  [Form] Added upgrade notes for #15061
fabpot added a commit that referenced this pull request Jul 9, 2015
@fabpot
Merge branch '2.8'
924ac40 
* 2.8:
  Added 'default' color
  [HttpFoundation] Reload the session after regenerating its id
  [HttpFoundation] Add a test case to confirm a bug in session migration
  [Serializer] Fix ClassMetadata::sleep()
  [2.6] Static Code Analysis for Components and Bundles
  [Finder] Command::addAtIndex() fails with Command instance argument
  [DependencyInjection] Freeze also FrozenParameterBag::remove
  [Twig][Bridge] replaced `extends` with `use` in bootstrap_3_horizontal_layout.html.twig
  fix CS
  fixed CS
  Add a way to reset the singleton
  [Security] allow to use `method` in XML configs
  [Serializer] Fix Groups tests.
  Remove duplicate example
  Remove var not used due to returning early (introduced in 8982c32)
  [Serializer] Fix Groups PHPDoc
  Enhance hhvm test skip message
  fix for legacy asset() with EmptyVersionStrategy
  [Form] Added upgrade notes for #15061
@MasterB
Copy link
Contributor
MasterB commented Jul 23, 2015

since update to Symfony 2.7.2
at grouped choices the choices not marked selected anymore.

->add('organizationUnits', 'entity', array('multiple'=>true, 'expanded'=>false,
                                                       'class' => 'XYZBundle\Entity\Institute',
                                                        'choices' => $this->em->getRepository('XYZBundle:Departement')->getGroupedInstitutes()))

Is there a new bug?
any working example on documentation?

@jakzal
Copy link
Contributor
jakzal commented Jul 23, 2015

@MasterB please try to reproduce your problem on the latest develop version of 2.7. If it still exists in there, report a new bug.

@soullivaneuh soullivaneuh mentioned this pull request Jul 24, 2015
Merged
@MasterB
Copy link
Contributor
MasterB commented Jul 24, 2015

@jakzal yes, same on latest 2.7 dev version. I think its going the right way now with referenz from @soullivaneuh

@bobvandevijver bobvandevijver mentioned this pull request Jul 29, 2015
Closed
@bicpi bicpi mentioned this pull request Aug 4, 2015
Closed
@Tobion Tobion mentioned this pull request Aug 15, 2015
Merged
Tobion added a commit that referenced this pull request Aug 21, 2015
@Tobion
minor #15558 [Form] fix reworked choice list phpdoc (Tobion)
6983416 
This PR was merged into the 2.7 branch.

Discussion
----------

[Form] fix reworked choice list phpdoc

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

Fix leftover phpdoc change from #15061

Commits
-------

747c1a5 [Form] fix reworked choice list phpdoc
@Tobion Tobion mentioned this pull request Sep 27, 2015
Closed
@HeahDude HeahDude mentioned this pull request Mar 26, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug Form
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@webmozart @soullivaneuh @fabpot @weaverryan @MasterB @jakzal
0