Fixed the params in ChoiceType methods #9835
Conversation
| ), | ||
| 'choices_as_values' => true, | ||
| 'choice_attr' => function($val, $key, $index) { | ||
| 'choice_attr' => function($category, $key, $value) { |
There was a problem hiding this comment.
In this case the first argument will be one of choices values true | false | null. So I think, it should be called $choiceValue.
There was a problem hiding this comment.
I'm going to do that change ... but then 'choice_attr' => function($choiceValue, $key, $value) is going to look confusing. choiceValue and value? What's the difference?
| ), | ||
| 'choices_as_values' => true, | ||
| 'choice_label' => function ($value, $key, $index) { | ||
| 'choice_label' => function ($category, $key, $value) { |
There was a problem hiding this comment.
Same here ($choiceValue, $key, $value).
| ), | ||
| 'choices_as_values' => true, | ||
| 'group_by' => function($value, $key, $index) { | ||
| 'group_by' => function($vategory, $key, $value) { |
There was a problem hiding this comment.
Same here ($choiceValue, $key, $value).
| 'choices_as_values' => true, | ||
| 'group_by' => function($value, $key, $index) { | ||
| 'group_by' => function($vategory, $key, $value) { | ||
| if ($value <= new \DateTime('+3 days')) { |
There was a problem hiding this comment.
Here the comparison should be against the first argument $choiceValue. I will check that for 2.8. But it's definitely true for 4.0. The first argument of the callback is the model data. The third $value is just a string from request.
reference/forms/types/choice.rst Outdated
| 'choice_attr' => function($category, $key, $value) { | ||
| return ['class' => 'category_'.strtolower($category->getName())]; | ||
| }, | ||
| |
| 'choices_as_values' => true, | ||
| 'choice_label' => function ($value, $key, $index) { | ||
| 'choice_label' => function ($choiceValue, $key, $value) { | ||
| if ($value == true) { |
There was a problem hiding this comment.
I think that is makes more sense to check true === $choiceValue here.
| @javiereguiluz , let's finish this? :) @xabbuh , could you review this PR, please? |
| @vudaltsov I thought you were going to create a new PR instead of this one ... so I was waiting :) I'm going to finish and merge this so you can create a new PR. Thanks! |
| This is now merged! @vudaltsov when you create your PR, please make any change needed to create variables easy to understand. E.g. |
| I think that |
| @vudaltsov what about just |
| @yceruto , okay! I will do that in my PR. |
| I still think it's not understandable. I can't understand anything here --> Please, let's pick variable names that the reader cannot mistake. For example, which of the following names best describe the previous arguments? function ($selectedChoice, $choiceIndex, $choiceValue) function ($availableChoice, $selectedIndex, $selectedValue) function ($availableChoice, $choiceIndex, $choiceValue) etc. |
|
|
| So |
| @javiereguiluz , I gave a wrong explanation. The third argument is not the selected value. This callback will be called for all choices. So we are not talking about user here - the submit has not happened yet. The third argument is how the choice is represented in a view in |
| What about this: |
| or... |
| The problem with |
| Well, I think it's better explain each argument below the first example so. |
4 participants
This finishes #9173.