Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
webmozart commentedJun 25, 2015
ping @symfony/deciders |
soullivaneuh commentedJun 25, 2015
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 commentedJun 25, 2015
@soullivaneuh Please consider that the BC breaks are only fornew functionality introduced in 2.7. So people who implemented the new interfaces are affected. Any code that worked < 2.7 is not affected. |
fabpot commentedJun 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 commentedJun 30, 2015
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 commentedJun 30, 2015
Thank you@webmozart. |
…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 commentedJul 1, 2015
@webmozart Could we have a doc note with more explanation about introduced BC breaks and concrete case of how to handle it? |
webmozart commentedJul 1, 2015
@soullivaneuh Yes, that's still missing. @fabpot Where should we add the relevant notes? UPGRADE-2.7? |
weaverryan commentedJul 1, 2015
@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 commentedJul 2, 2015
See#15174 for the upgrade notes. |
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
* 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 in8982c32) [Serializer] Fix Groups PHPDoc Enhance hhvm test skip message fix for legacy asset() with EmptyVersionStrategy [Form] Added upgrade notes for#15061
* 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 in8982c32) [Serializer] Fix Groups PHPDoc Enhance hhvm test skip message fix for legacy asset() with EmptyVersionStrategy [Form] Added upgrade notes for#15061
MasterB commentedJul 23, 2015
since update to Symfony 2.7.2 Is there a new bug? |
jakzal commentedJul 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. |
MasterB commentedJul 24, 2015
@jakzal yes, same on latest 2.7 dev version. I think its going the right way now with referenz from@soullivaneuh |
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#15061Commits-------747c1a5 [Form] fix reworked choice list phpdoc
I introduced a bug in the 2.7 ChoiceList implementation when choices are passed as groups:
In this example, since the choices
$yesObjand$maybeObjhave the same array index0, 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:
ChoiceListInterfacewas reverted to how it was in 2.6 and doesnot extend the newChoiceListInterfaceanymore.LegacyChoiceListAdapterwhen they are passed to any place in the framework where a new choice list is expected.ChoiceListInterfacehas two additional methodsgetStructuredValues()andgetOriginalKeys()now.ArrayKeyChoiceList::toArrayKey()was marked as internal.ChoiceListFactoryInterface::createView()does not accept arrays and Traversables anymore for the$groupByparameter (for simplicity).@fabpot Where should we document the upgrade path for 2.7.1 => 2.7.2?