While I was updating the 2.x branch to use
Drupal.org's GitLab CI →
for automated linting and testing, I noticed that tests were failing in the 2.x branch. Manual testing suggested that tests were failing due to TypeError: array_search(): Argument #2 ($haystack) must be of type array, string given in array_search() (line 93 of web/modules/contrib/multiselect/src/Element/Multiselect.php). I ended up fixing the broken tests in commit 9e76427.
However, while trying to figure out how to fix the tests, I noticed that the logic in \Drupal\multiselect\Element\Multiselect::getOptions() was really difficult to follow, especially on the following lines:
/* L81 */ if (!($value_valid && ((!$value_is_array && (string) $element['#value'] === $key || ($value_is_array && in_array($key, $element['#value']))) || $empty_choice))) {
/* L91 */ if ($value_valid && ((!$value_is_array && (string) $element['#value'] === $key || ($value_is_array && in_array($key, $element['#value']))) || $empty_choice)) {
From personal experience, logic that is hard to follow makes debugging and maintenance harder. As a co-maintainer of this project, I want this module to be as easy to maintain as possible, so that I don't have to spend a lot of (unpaid) time maintaining it.
(Optional) Add unit/kernel tests for \Drupal\multiselect\Element\Multiselect::getOptions() so that it's easier to see if refactoring breaks functionality.
Refactor the code in \Drupal\multiselect\Element\Multiselect::getOptions(). I daresay temporary variables and/or helper functions with descriptive names would make it easier to see the intent of the authors.
None.
None.
None.
Needs review
2.0
Code
Hi!
I've separate checking logic for 81 and 91 lines on several methods.
please check it