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: Duplicate choice labels are remembered when using "choices_as_values" = false#16685
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 commentedNov 26, 2015
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #15606 |
| License | MIT |
| Doc PR | - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
one empty line too much
ewgRa commentedNov 26, 2015
Seems it is cover my case, but not cover all cases. For example this one:https://gist.githubusercontent.com/ewgRa/2a71d44329be2be6446e/raw/733b5baf2b18e42149a670e92e0a27b87ff27567/gistfile1.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if ($choiceLabels) is enough. We do not actually need to count them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$choiceLabels is an array, so I think counting is more explicit and easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
in fact, there is a major performance issue in using count: $choiceLabels being a reference here, count is going to clone the array...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think you should remove the&, it creates an unnecessary ref mismatch
or is it really required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok, the answer is yes, there is a reason to keep the&, which means the count below has to be removed to prevent a useless memory duplication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I know, this is not really beautiful. But the closures need to access the same variable to be able to properly exchange information.
webmozart commentedNov 26, 2015
@ewgRa The keys you pass in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this may create a copy too (assuming we avoid the copy just above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
adding an& may be necessary I agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is without a& by intention. The outer$choiceLabels lives in the type's scope, i.e. every field uses the same variable to transfer information fromchoices tochoice_label during its construction. The inner closure, however, lives in the scope of the field and is used to generate ChoiceView instances during view construction. If we'd use the global variable instead, all choice fields in a form would have the labels of the last one, which is not what we want.
ewgRa commentedNov 26, 2015
@webmozart at least my case this PR solve. Maybe good idea to document it, or add checks in cases like I show at gist. |
webmozart commentedNov 26, 2015
Seesymfony/symfony-docs#5905 for documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$nextKey should defaut to0 instead of thisif
nicolas-grekas commentedNov 27, 2015
👍 (withone comment) |
…ces_as_values" = false
webmozart commentedNov 27, 2015
Updated |
nicolas-grekas commentedNov 27, 2015
Thank you@webmozart. |
…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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
shouldn't this use a reference too, to avoid copying the array ?
ghost commentedJan 5, 2016
I did not follow the full thread, but this last update seems to have changed what is returned by $config->getOption('choices'). I'm not sure if this is intentional or not, but it is a breaking change and should not have been added to a minor release. |
peter17 commentedJan 11, 2016
This change breaks compatibility with Symfony-2.7.7.
The form contains a choice list to Propel1 models. Setting |
xabbuh commentedJan 11, 2016
see#17163 which will be included in the next patch releases |