Fixed the params in ChoiceType methods #9835

javiereguiluz · 2018-05-28T06:50:17Z

This finishes #9173.

vudaltsov · 2018-05-28T06:56:30Z

reference/forms/types/options/choice_attr.rst.inc

@@ -23,7 +23,7 @@ If an array, the keys of the ``choices`` array must be used as keys::
            'Maybe' => null,
        ),
        'choices_as_values' => true,
-        'choice_attr' => function($val, $key, $index) {
+        'choice_attr' => function($category, $key, $value) {


In this case the first argument will be one of choices values true | false | null. So I think, it should be called $choiceValue.

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?

vudaltsov · 2018-05-28T07:02:58Z

reference/forms/types/options/choice_label.rst.inc

@@ -20,7 +20,7 @@ more control::
            'maybe' => null,
        ),
        'choices_as_values' => true,
-        'choice_label' => function ($value, $key, $index) {
+        'choice_label' => function ($category, $key, $value) {


Same here ($choiceValue, $key, $value).

vudaltsov · 2018-05-28T07:03:02Z

reference/forms/types/options/group_by.rst.inc

@@ -26,7 +26,7 @@ Take the following example::
            '1 month' => new \DateTime('+1 month'),
        ),
        'choices_as_values' => true,
-        'group_by' => function($value, $key, $index) {
+        'group_by' => function($vategory, $key, $value) {


Same here ($choiceValue, $key, $value).

vudaltsov · 2018-05-28T07:06:46Z

reference/forms/types/options/group_by.rst.inc

@@ -26,7 +26,7 @@ Take the following example::
            '1 month' => new \DateTime('+1 month'),
        ),
        'choices_as_values' => true,
-        'group_by' => function($value, $key, $index) {
+        'group_by' => function($vategory, $key, $value) {
            if ($value <= new \DateTime('+3 days')) {


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.

vudaltsov · 2018-05-28T08:49:10Z

reference/forms/types/choice.rst

            /** @var Category $category */
            return strtoupper($category->getName());
        },
-        'choice_attr' => function($category, $key, $index) {
+        'choice_attr' => function($category, $key, $value) {
            return ['class' => 'category_'.strtolower($category->getName())];
        },



vudaltsov

Sorry, found some other things that could be fixed in this PR. And some other improvements I will do in a new one.

vudaltsov · 2018-05-28T08:50:57Z

reference/forms/types/options/choice_label.rst.inc

@@ -20,7 +20,7 @@ more control::
            'maybe' => null,
        ),
        'choices_as_values' => true,
-        'choice_label' => function ($value, $key, $index) {
+        'choice_label' => function ($choiceValue, $key, $value) {
            if ($value == true) {


I think that is makes more sense to check true === $choiceValue here.

vudaltsov · 2018-05-29T13:30:46Z

@javiereguiluz , let's finish this? :)
Any objections to my last comment?

@xabbuh , could you review this PR, please?

javiereguiluz · 2018-05-29T14:00:20Z

@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 PR was squashed before being merged into the 2.8 branch (closes #9835). Discussion ---------- Fixed the params in ChoiceType methods This finishes #9173. Commits ------- b14cf70 Fixed the params in ChoiceType methods

javiereguiluz · 2018-05-29T14:06:51Z

This is now merged! @vudaltsov when you create your PR, please make any change needed to create variables easy to understand. E.g. $choiceValue -> is the value chosen by the user or the value shown in the choice list? Better use $selectedValue or $selectedIndex, etc. Thanks!

vudaltsov · 2018-05-29T14:25:36Z

I think that $choiceValue is understandable enough because that's the value from the choices option. We can rename it only when we know what it is in terms of model data (like $category).

yceruto · 2018-05-29T14:40:33Z

@vudaltsov what about just $choice? that's how it is called internally for all cases:
https://github.com/symfony/symfony/blob/3a2eb0d9514f0b322ab31a128aa48b9f89e28d50/src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php#L121

https://github.com/symfony/symfony/blob/3a2eb0d9514f0b322ab31a128aa48b9f89e28d50/src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php#L130

https://github.com/symfony/symfony/blob/3a2eb0d9514f0b322ab31a128aa48b9f89e28d50/src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php#L140

https://github.com/symfony/symfony/blob/3a2eb0d9514f0b322ab31a128aa48b9f89e28d50/src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php#L144

https://github.com/symfony/symfony/blob/3a2eb0d9514f0b322ab31a128aa48b9f89e28d50/src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php#L203

vudaltsov · 2018-05-29T14:41:53Z

@yceruto , okay! I will do that in my PR.

javiereguiluz · 2018-05-29T15:57:28Z

I still think it's not understandable. I can't understand anything here --> function ($choice, $index, $value)

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.

vudaltsov · 2018-05-29T16:02:39Z

~~If we want to be verbose: function ($choiceValue, $choiceKey, $requestValue).~~
~~The last argument is the value from $_REQUEST (or Request $request), most of the time nobody cares about it.~~
key is better than index because it obviously might be non-numeric.

javiereguiluz · 2018-05-29T16:06:03Z

So $requestValue is the value selected by the user? (if true: $selectedValue maybe?)

vudaltsov · 2018-05-29T16:08:40Z

@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 <option value="X">.

vudaltsov · 2018-05-29T16:11:10Z

What about this: function ($choiceData, $choiceKey, $viewValue)

yceruto · 2018-05-29T16:27:45Z

or... function ($choiceValue, $choiceKey, $viewValue) ?

vudaltsov · 2018-05-29T16:31:03Z

The problem with $choiceValue is that we have an option choice_value which is about $viewValue and that might be misleading. While data indicates that this is more of a model thing, the actual choice data.

yceruto · 2018-05-29T16:41:43Z

Well, I think it's better explain each argument below the first example so.

Fixed the params in ChoiceType methods

0c4fa63

javiereguiluz added bug Form labels May 28, 2018

javiereguiluz added this to the 2.8 milestone May 28, 2018

carsonbot added the Status: Needs Review label May 28, 2018

javiereguiluz mentioned this pull request May 28, 2018

Swap $key and $index parameters in ChoiceType #9173

Closed

vudaltsov suggested changes May 28, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels May 28, 2018

Fixes

4587360

carsonbot added Status: Needs Review and removed Status: Needs Work labels May 28, 2018

vudaltsov reviewed May 28, 2018

View reviewed changes

carsonbot removed the Status: Needs Review label May 28, 2018

vudaltsov suggested changes May 28, 2018

View reviewed changes

carsonbot added the Status: Needs Work label May 28, 2018

Finished the feature

1b5a5d6

javiereguiluz added Status: Reviewed and removed Status: Needs Work labels May 29, 2018

javiereguiluz closed this May 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed the params in ChoiceType methods #9835

Fixed the params in ChoiceType methods #9835

javiereguiluz commented May 28, 2018

vudaltsov May 28, 2018

javiereguiluz May 28, 2018

vudaltsov May 28, 2018

vudaltsov May 28, 2018

vudaltsov May 28, 2018

vudaltsov May 28, 2018

vudaltsov left a comment

vudaltsov May 28, 2018

vudaltsov commented May 29, 2018

javiereguiluz commented May 29, 2018

javiereguiluz commented May 29, 2018

vudaltsov commented May 29, 2018

yceruto commented May 29, 2018

vudaltsov commented May 29, 2018

javiereguiluz commented May 29, 2018

vudaltsov commented May 29, 2018 •

edited

Loading

javiereguiluz commented May 29, 2018

vudaltsov commented May 29, 2018

vudaltsov commented May 29, 2018

yceruto commented May 29, 2018

vudaltsov commented May 29, 2018

yceruto commented May 29, 2018

Fixed the params in ChoiceType methods #9835

Fixed the params in ChoiceType methods #9835

Conversation

javiereguiluz commented May 28, 2018

vudaltsov May 28, 2018

Choose a reason for hiding this comment

javiereguiluz May 28, 2018

Choose a reason for hiding this comment

vudaltsov May 28, 2018

Choose a reason for hiding this comment

vudaltsov May 28, 2018

Choose a reason for hiding this comment

vudaltsov May 28, 2018

Choose a reason for hiding this comment

vudaltsov May 28, 2018

Choose a reason for hiding this comment

vudaltsov left a comment

Choose a reason for hiding this comment

vudaltsov May 28, 2018

Choose a reason for hiding this comment

vudaltsov commented May 29, 2018

javiereguiluz commented May 29, 2018

javiereguiluz commented May 29, 2018

vudaltsov commented May 29, 2018

yceruto commented May 29, 2018

vudaltsov commented May 29, 2018

javiereguiluz commented May 29, 2018

vudaltsov commented May 29, 2018 • edited Loading

javiereguiluz commented May 29, 2018

vudaltsov commented May 29, 2018

vudaltsov commented May 29, 2018

yceruto commented May 29, 2018

vudaltsov commented May 29, 2018

yceruto commented May 29, 2018

vudaltsov commented May 29, 2018 •

edited

Loading