October: Dropdown custom options method arguments wrong order

[iotch]

According to docs https://octobercms.com/docs/backend/forms#field-dropdown

The fourth method uses a specific method declared in the model's class. In the next example the listStatuses method should be defined in the model class. This method receives all the same arguments as the getDropdownOptions method, and should return an array of options in the format key => label.

The custom options getter method of the dropdown formwidget should receive the same arguments as getDropdownOptions method, however the order of arguments does not match.

See https://github.com/octobercms/october/blob/master/modules/backend/widgets/Form.php#L1263
and https://github.com/octobercms/october/blob/master/modules/backend/widgets/Form.php#L1278

[mjauvin]

Indeed, the following patch would resolve this.

diff --git a/modules/backend/widgets/Form.php b/modules/backend/widgets/Form.php
index c42ba56f..a0bf3d3b 100644
--- a/modules/backend/widgets/Form.php
+++ b/modules/backend/widgets/Form.php
@@ -1275,7 +1275,7 @@ class Form extends WidgetBase
                 ]));
             }

-            $fieldOptions = $this->model->$fieldOptions($field->value, $field->fieldName, $this->data);
+            $fieldOptions = $this->model->$fieldOptions($field->fieldName, $field->value, $this->data);
         }

         return $fieldOptions;

[LukeTowers]

Unfortunately I think this is just going to have to be resolved with a change in the documentation. Changing the code would break existing assumptions. I'll ask @daftspunk for input on this though.

[daftspunk]

The code we have today has emerged and perhaps diverged from the original spec based on real world requirements, creating some confusion. This is fine and @LukeTowers is correct in that the documentation should be updated to make the usage clearer.

I will do my best to describe these differences and why this isn't a case of misfortune. Originally, the method signatures looked something like this:

// (1) A autogen method for a field defined in fields.yaml called "foobar"
$model->getFoobarOptions($field->value)

// (2) An explicit method defined in fields.yaml called "someOptions"
$model->someOptions($field->value)

// (3) A catch all method operator
$model->getDropdownOptions($attribute, $field->value)

It is important to note, that 1 + 2 are closely related, so they share their method signatures. This is what the documentation refers to. The developer likely knows what the field name is at this point, so its not useful to pass it as an argument.

Using getDropdownOptions is considered a "catch-all" version and specifies the captured wild card value as its first argument, as it should and is expected, hence the first argument.

At some stage, we determined it is necessary to know more about the data set (the underlying data may not be the local model).

So for 1 +2, this dataset was added as optional arguments carrying the field name and reference to the data set.

// (1) A autogen method for a field defined in fields.yaml called "foobar"
$model->getFoobarOptions($field->value, $field->fieldName, $this->data)

// (2) An explicit method defined in fields.yaml called "someOptions"
$model->someOptions($field->value, $field->fieldName, $this->data)

For consistency 3 should actually have a signature like this

// (3) A catch all method operator
$model->getDropdownOptions($attribute, $field->value, $field->fieldName, $this->data)

But, a cursory glance would show that the first and third argument contain identical values, so the third argument is removed due to redundancy. Resulting in the finished outcome:

// (3) A catch all method operator
$model->getDropdownOptions($attribute, $field->value, $this->data)

Conclusion and thoughts:

This is a case where utility trumps consistency to produce a nice outcome. All developers, good developers, generally have a desire to make things nice.

But what is nice really? Nice is empathy first, consistency second. Empathy requires us to form a deep understanding of what the end user is experiencing when consuming work at that time, whether it be a piece of code or an API.

So at a high level, we may think changing these methods to use the same method signature is good for consistency. But we may also lose focus of what makes these things actually useful to the developer, sometimes there are small distinctions like this that need to be protected and understood.

Documentation should support the user experience of the code, so in this case, we should focus on improving the documentation.

If anything, I think that the third and fourth arguments are in the wrong positions, data should appear first and then the field value.

$model->getFoobarOptions($field->value, $this->data, $field->fieldName)

This would allow us to keep consistency if getDropdownOptions ever did require the fieldName, in the case where those values were not ever identical--now have to be added as 4th arg, inconsistent with the others.

This is probably the unfortunate oversight here, but not a problem we have to deal with today.

[mjauvin]

@daftspunk Interesting history lesson on ocms!

Please note that method signatures in the code are as follow:
(1)
$model->getFoobarOptions($field->value, $this->data)

(2)
$model->someOptions($field->value, $field->fieldName, $this->data)

(3)
$model->getDropdownOptions($attribute, $field->value, $this->data)

So the second argument ($field->fieldName) for case (1) has been dropped.

[github-actions[bot]]

This issue will be closed and archived in 3 days, as there has been no activity in the last 30 days. If this issue is still relevant or you would like to see action on it, please respond and we will get the ball rolling.