Add label sort ability to Select element

I have a few nit-picks and one more substantive suggestion at the end:

1. +++ b/core/includes/form.inc
   @@ -90,7 +97,17 @@ function form_select_options($element, $choices = NULL) {
          return [];
        }
        $choices = $element['#options'];
   +    $sort_options = isset($element['#sort_options']) && $element['#sort_options'];
   +    $sort_start = (isset($element['#sort_start']) ? $element['#sort_start'] : 0);
      }
   +  else {
   +    // We are within an option group.
   +    $sort_options = isset($choices['#sort_options']) && $choices['#sort_options'];
   +    $sort_start = (isset($choices['#sort_start']) ? $choices['#sort_start'] : 0);
   +    unset($choices['#sort_options']);
   +    unset($choices['#sort_start']);
   +  }
       

   Too bad: PHP 7 is required for new Drupal 8 sites, but not for existing ones, so we still cannot use the null coalescing operator. This makes me sad. I would leave off the optional parentheses when setting $sort_start, but I understand that there are arguments on both sides. Personally, I assume people know that = has lower precedence than ?:, but I put in the optional parentheses in ! ($object instanceof 'class').

2. @@ -125,6 +142,16 @@ function form_select_options($element, $choices = NULL) {
   +  if ($sort_options) {
   +    $unsorted = array_slice($options, 0, $sort_start);
   +    $sorted = array_slice($options, $sort_start);
   +    uasort($sorted, function ($a, $b) {
   +        $label1 = (string) $a['label'];
   +        $label2 = (string) $b['label'];
   +        return strcmp($label1, $label2);
   +    });
   +    $options = array_merge($unsorted, $sorted);
   +  }
       

   Again, it is a matter of preference. I would use return strcmp((string) $a['label'], (string) $b['label']);. Also, is there any reason for indenting four spaces inside the anonymous function?

3. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestSelectForm.php
   @@ -125,6 +126,61 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   ...
   +    $sortable_options = [];
   +    foreach ($unsorted_options as $key => $item) {
   ...
   +    }
   ...
   +    $sortable_options['sso_zzgroup']['#sort_options'] = TRUE;
       

   Generally, we try to avoid any sort of logic in automated tests. The foreach loop is only 6 lines shorter than the definition of the $unsorted_options array. I suggest making a similar explicit definition of $sortable_options. Of course, this code is in the supporting module rather than the test iteself, so perhaps we should leave it as is.

4. From the issue description:

   $form['example_select'] = [
     '#type' => 'select',
     '#title' => $this->t('Select element'),
            '#options' => [
             '_none' => $this->t('- None -'),
           ] + $options,
           '#ajax' => TRUE,
           '#sort_options' => TRUE,
           '#sort_start' => 1,
   ];
       

   I fixed the indentation, but did not change the example. Looking at the existing documentation in Select.php, I notice the #empty_option and #empty_value options. I think we should encourage people to use these options rather than handle the empty option as in this example. (I will add a similar suggestion on #2900409: [Meta] Improve UI of Reference field settings form.) So I suggest changing this to something like 'first' => $this->t('First option'),.

I feel a little foolish, since I was the first to suggest (on Slack) adding the #sort_start option, and it was explicitly to handle the "- None -" option. I now wonder if there are any uses for this option. Are we adding an option that no one will ever use?

Looking at form_select_options() after applying the patch from #3, I see that the code for handling the empty option comes after the code for the sort options. It should still work, but I think it would be a good idea to check this in the test, to make sure that future changes do not break it.

Looking more closely at form_select_options(), I see I was very wrong in my earlier comments. Thanks for getting it straight! On the plus side, I no longer feel foolish for suggesting the #sort_start option.

Thanks also for expanding the test coverage.

I have just two tiny complaints, both about consistency:

1. +++ b/core/includes/form.inc
   @@ -63,6 +64,12 @@ function template_preprocess_select(&$variables) {
   + *   - #sort_options: (optional) If set to TRUE (default is FALSE), sort the
   + *     options by their labels. Can be set within an option group to sort that
   + *     group.
   + *   - #sort_start: (optional) Option index to start sorting at, where 0 is the
   + *     first option (default is 0, and this only applies if #sort_options is
   + *     TRUE). Can be used within an option group.
   +++ b/core/lib/Drupal/Core/Render/Element/Select.php
   @@ -14,6 +14,14 @@
   + * - #sort_options: (optional) If set to TRUE (default is FALSE), sort the
   + *   options by their labels, after rendering and translation is complete.
   + *   Can be set within an option group to sort that group.
   + * - #sort_start: (optional) Option index to start sorting at, where 0 is the
   + *   first option (default is 0, and this only applies if #sort_options is
   + *   TRUE). Can be used within an option group. If an empty option is being
   + *   added automatically (see #empty_option and #empty_value properties), and
   + *   you want it to stay at the top of the list, be sure to start sorting at 1.
      

   I am not sure why we have essentially the same documentation in two different places, but the small differences between the two bother me. (Only the second mentions rendering and translation, and only the second mentions the empty option.) It is too easy to imagine myself staring at the documentation in form.inc, reading it over and over, and thinking, "I am sure there is something about the empty option here, but I cannot find it."

2. +++ b/core/includes/form.inc
   @@ -90,7 +97,17 @@ function form_select_options($element, $choices = NULL) {
   +    $sort_options = isset($element['#sort_options']) && $element['#sort_options'];
   +    $sort_start = (isset($element['#sort_start']) ? $element['#sort_start'] : 0);
      }
   +  else {
   +    // We are within an option group.
   +    $sort_options = isset($choices['#sort_options']) && $choices['#sort_options'];
   +    $sort_start = isset($choices['#sort_start']) ? $choices['#sort_start'] : 0;
   +    unset($choices['#sort_options']);
   +    unset($choices['#sort_start']);
   +  }
      

   I find it easier to read, in this case, without the optional parentheses, but my intention in #4.1 ("I would ...") was to mention that preference, not to insist. It is OK to keep the optional parentheses, but please be consistent in the two parts of the if block.

I tested by applying the patch from #2900409-63: [Meta] Improve UI of Reference field settings form and installing the Umami demo profile. I checked in English and Spanish, and I also checked using the "empty option" settings. To confirm that sorting took place after translation, I paid attention to "Language [langcode]" (en) and "Idioma [langcode]" (es). In all cases, the patch worked as expected. (I also hacked "- None -" to "None -" to make sure that it would not stay at the top if it were part of the sorting.)

This needs a change record, so I am adding the corresponding tag. The release notes snippet is a good start on that.

Speaking of the release notes snippet, I am updating it as I suggested in #4.

@jhodgdon:

Thanks for the updates. Having the documentation in a single place is even better than having it consistent in two places. I am not sure how these docblocks are processed, nor what the standards are, so I will defer to you on which one to keep.

I did not re-test since these changes (except for one line) "only" affect the documentation blocks.

I reviewed the CR, and it looks good.

1. +++ b/core/includes/form.inc
   @@ -90,7 +72,17 @@ function form_select_options($element, $choices = NULL) {
   +    // We are within an option group.
   +    $sort_options = isset($choices['#sort_options']) && $choices['#sort_options'];
   +    $sort_start = isset($choices['#sort_start']) ? $choices['#sort_start'] : 0;
   

   As far as I can see there is no test coverage of using #sort_start in an option group.

2. +++ b/core/modules/system/tests/src/Functional/Form/FormTest.php
   @@ -464,6 +464,73 @@ public function testEmptySelect() {
   +    // Verify the expected order of the options in the unsorted and
   +    // sorted elements.
   +    $expected_order = [
   +      'uso_first',
   +      'uso_second',
   +      'uso_zzgroup',
   +      'uso_gc',
   +      'uso_ga',
   +      'uso_gb',
   +      'uso_yygroup',
   +      'uso_ge',
   +      'uso_gd',
   +      'uso_gf',
   +      'uso_d',
   +      'uso_c',
   +      'uso_b',
   +      'uso_a',
   +      'sso_first',
   +      'sso_second',
   +      'sso_zzgroup',
   +      'sso_ga',
   +      'sso_gb',
   +      'sso_gc',
   +      'sso_a',
   +      'sso_d',
   +      'sso_b',
   +      'sso_c',
   +      'sso_yygroup',
   +      'sso_ge',
   +      'sso_gd',
   +      'sso_gf',
   +      'sno_empty',
   +      'sno_first',
   +      'sno_second',
   +      'sno_zzgroup',
   +      'sno_ga',
   +      'sno_gb',
   +      'sno_gc',
   +      'sno_a',
   +      'sno_d',
   +      'sno_b',
   +      'sno_c',
   +      'sno_yygroup',
   +      'sno_ge',
   +      'sno_gd',
   +      'sno_gf',
   +    ];
   +
   +    $prev_position = 0;
   +    foreach ($expected_order as $string) {
   +      $position = strpos($content, $string);
   +      $this->assertTrue($position > $prev_position, 'Item ' . $string . ' is in correct order');
   +      $prev_position = $position;
   +    }
   

   This bit of the test is a bit confusing because we're lumping all of the selects in one go. I think this might be better written as:

       $option_map_function = function (NodeElement $option) {
         if ($option->getTagName() === 'optgroup') {
           return $option->getAttribute('label');
         }
         return $option->getValue();
       };
   
       $field = $this->getSession()->getPage()->findField('sorted');
       $options = array_map($option_map_function, $field->findAll('css', 'option,optgroup'));
       $this->assertIdentical([
         'sso_first_element',
         'sso_second',
         'sso_zzgroup',
         'sso_ga',
         'sso_gb',
         'sso_gc',
         'sso_a',
         'sso_d',
         'sso_b',
         'sso_c',
         'sso_yygroup',
         'sso_ge',
         'sso_gd',
         'sso_gf',
       ], $options);
   

   That way we'll get better error messages and it's a bit easier to see what's wrong because each select element is tested individually.

3. +++ b/core/modules/system/tests/src/Functional/Form/FormTest.php
   @@ -464,6 +464,73 @@ public function testEmptySelect() {
   +    $this->assertFalse(strpos($content, '#sort_order'), 'Sort order property has been removed');
   +    $this->assertFalse(strpos($content, '#sort_start'), 'Sort start property has been removed');
   

   These should be using the correct assertion. $this->assertSession()->responseNotContains()

  public function testSelectSorting() {
    $this->drupalGet('form-test/select');
    $content = $this->getSession()->getPage()->getContent();

The $content variable is not used anywhere. At least eliminate the variable. Can we eliminate the entire line, or does it have side effects that we need?

The patch in #12 addresses the feedback in #11. I am happy to mark the issue RTBC if you do not like the following suggestions, but I think they will make the helper function a little easier to read.

  /**
   * Validates that the options are in the right order in a select.
   *
   * @param string $select
   *   Name of the select to verify.
   * @param string[] $order
   *   Expected order of its options.
   */
  protected function validateSelectSorting($select, $order) {
    $option_map_function = function (NodeElement $option) {
      if ($option->getTagName() === 'optgroup') {
        return $option->getAttribute('label');
      }
      return $option->getValue();
    };

    $field = $this->getSession()->getPage()->findField($select);
    $options = array_map($option_map_function,
      $field->findAll('css', 'option, optgroup'));
    $this->assertIdentical($order, $options);
  }

Kudos for using string[] in the @param comment instead of the less specific array. Let's add a type hint to the function declaration: validateSelectSorting($select, array $order).

The variable name $option does not suggest a NodeElement object: I suggest $node (as below) or $element. I know I am in the minority, but I think the ternary operator is clearer in these situations:

    $option_map_function = function (NodeElement $node) {
      return ($node->getTagName() === 'optgroup')
        ? $node->getAttribute('label')
        : $node->getValue();
    };

The variable name $field does not suggest a NodeElement of type select. I also think it is clearer to eliminate that intermediate variable:

    $option_nodes = $this->getSession()
      ->getPage()
      ->findField($select)
      ->findAll('css', 'option, optgroup');
    $options = array_map($option_map_function, $option_nodes);
    $this->assertIdentical($order, $options);

You can also consider combining the last two lines (eliminating the $options variable) and/or using an anonymous function inside array_map() (eliminating the $option_map_function variable).

@jhodgdon:

I am glad you liked my suggestions.

As I said in #13, I think that all the points in #11 have been addressed, so back to RTBC.

We have not updated the "Remaining tasks" in the issue description for a while. I just took care of that.

1. This works for alphabetical sorting only and list [1,2,11] will be sorted as [1,11,2] . Using strnatcmp() instead strcmp() will allow natural sorting (https://en.wikipedia.org/wiki/Natural_sort_order)
2. What about sort_direction - 'ASC' (default) and 'DESC' ?

Back to needs review for #16.

List [step1,step2,step11] will be sorted incorrectly, for example. I suggest to make two types of sorting: alphabetic (by default) and natural. And yet, for order, two kinds of sorting. If my amendments are adopted, it makes sense to think about the sorting options in the form of an array.

We discussed this issue at the Drupal usability meeting 2019-07-23.

We agreed that the additional options suggested in #16 should be postponed to a follow-up issue, for many of the reasons @jhodgdon has given:

1. We do not want to delay this issue and #2900409: [Meta] Improve UI of Reference field settings form with further discussion.
2. The additional options may be an edge case. We can continue to discuss whether to add them in the follow-up issue.
3. If strnatcmp() is useful here, then we should consider offering it as an option in other places in core.
4. As usual, we want to avoid adding complexity to core unless a feature will be widely useful.
5. The two options added by the current patch will actually be used in core, assuming that #2900409: [Meta] Improve UI of Reference field settings form is adopted.

Based on that, I am moving this issue back to RTBC. I will also add a follow-up issue to discuss the additional options.

I am also un-assigning this issue. (Better late than never.)

I added a follow-up issue: #3069844: Provide additional options for sorting the options in Select form elements.

I tend to agree that many cases where descending order and/or "natural" comparison would be useful are cases where sorting could be done when creating the Select element. The best way to argue against that opinion would be to find a use case in core where we would want to use one of the proposed new options. If there are a lot of such examples, then we might even decide to add the options for convenience even if the sorting could be done "early".

Do we have a follow-up issue to update https://api.drupal.org/api/drupal/developer%21topics%21forms_api_referen...?

@jibran:

Unless I am confused, that page is a reference for Drupal 7, despite the "8.x" in the URL. I am not aware of any similar reference for Drupal 8.

If I am confused, or if there is a similar page for D8, can you let me know how to update it? I do not think we have any follow-up issues to update the documentation after this issue.

1. +++ b/core/includes/form.inc
   @@ -90,7 +72,17 @@ function form_select_options($element, $choices = NULL) {
   +    $sort_start = isset($element['#sort_start']) ? $element['#sort_start'] : 0;
   ...
   +    $sort_start = isset($choices['#sort_start']) ? $choices['#sort_start'] : 0;
   

   we can use the null coalesce operator here now as we have access to php7 features on 8.8.x

2. +++ b/core/includes/form.inc
   @@ -125,6 +117,14 @@ function form_select_options($element, $choices = NULL) {
   +    uasort($sorted, function ($a, $b) {
   +      return strcmp((string) $a['label'], (string) $b['label']);
   +    });
   

   i was kind of hoping we could get the first use of the spaceship operator in core here, but alas, its not suitable :)

3. +++ b/core/lib/Drupal/Core/Render/Element/Select.php
   @@ -9,11 +9,32 @@
   + *   you want it to stay at the top of the list, be sure to start sorting at 1.
   
   @@ -68,6 +89,8 @@ public function getInfo() {
   +      '#sort_start' => 0,
   

   should we automatically do this if #empty_option/#empty_value are provided?

4. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestSelectForm.php
   @@ -125,11 +126,80 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      // Don't use $this->t() here, because we really don't want these
   +      // to be translated or added to localize.d.o. Using TranslatableMarkup
   +      // in places tests that casting to string is working, however.
   

   I can't parse this sentence, could we rewrite it - I think the grammar is slightly disjointed

5. +++ b/core/modules/system/tests/src/Functional/Form/FormTest.php
   @@ -464,6 +465,104 @@ public function testEmptySelect() {
   +  public function testSelectSorting() {
   

   I thought we were trying to avoid adding new functional tests for forms, we can use the kernel test is a form trick (https://www.previousnext.com.au/blog/drupal-8-ftw-it-test-or-it-form-act...) - should we be doing that here?

5 -- Hm.... All of the existing tests for Select are all in this same class, and all I've done is added a test method and a helper method to the existing test class. How about making that a follow-up issue

Agree, making it a follow up issue to convert to a kernel test makes sense

Thanks for all the other changes

I reviewed the updated patch. Three of my points below are just comments. Point (3) is a suggestion, and (4) is a strong suggestion. I will put off re-testing until next time.

1. In #4, I wrote,

   Too bad: PHP 7 is required for new Drupal 8 sites, but not for existing ones, so we still cannot use the null coalescing operator.

   I think I reviewed PHP requirements and the related change record on ending support for PHP 5: "PHP 5.5 only supported for existing Drupal 8 sites, new Drupal 8 sites require PHP 7.0.8". It could be clearer that the new site/old site distinction only matters for Drupal 8.7, and that Drupal 8.8 will require PHP 7.

2. When I first read Comment #29.3, I was afraid that it would be a BC problem since existing code uses the #empty_value property. Then I got a good night's sleep. It is safe to set #sort_start to 1 based on the #empty_value property since existing code will not set the #sort_options property.

3. +++ b/core/lib/Drupal/Core/Render/Element/Select.php
   @@ -129,6 +152,9 @@ public static function processSelect(&$element, FormStateInterface $form_state,
   +    // Provide the correct default value for #sort_start.
   +    $element['#sort_start'] = $element['#sort_start'] ??
   +      ((isset($element['#empty_value'])) ? 1 : 0);
       

   I would not put parentheses around isset($element['#empty_value']).

4. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestSelectForm.php
   @@ -4,6 +4,7 @@
   ...
   +    // Add a select to test sorting at the top level, and with some of the
   +    // option groups sorted, some left alone, and at least one with sort start
   +    // set to a non-default value.
   ...
   +    // Add a select to test sorting with a -NONE- option included,
   +    // and a sort start set.
   ...
   +    // Add a select to test sorting with a -NONE- option included,
   +    // and sort start not set.
       

   I guess I missed a chance to object in an earlier patch, since only the first snippet is new. I think "#sort_start" is clearer than "sort start" or "a sort start".

5. I reviewed the updates to both the release notes snippet in the issue summary and the change record. I think the changes are clear.

I reviewed an interdiff and verified the small changes since #30. Everything looks good.

The test coverage is much more thorough than my manual test, but I still think manual testing is worthwhile. As before, I installed the Umami demo profile and the latest patch from #2900409: [Meta] Improve UI of Reference field settings form. Then I added a reference field and looked at the options for the sort order. It worked in both English and Spanish. (The Spanish label for langcode is Idioma, which makes it easy to check that we are actually sorting the labels after translation and not the machine names.)

Both reviewed and tested, so I am happy to mark this issue RTBC.

I found a potential issue with this improvement. The Webform module provides a 'Select other' element where the 'Other...' option should always be last. Below is the HTML markup for a select menu with an 'Other...' option.

<select class="js-webform-choices webform-choices form-select" data-drupal-selector="edit-select-other-select" id="edit-select-other-select" name="select_other[select]">
<option value="" selected="selected">- None -</option>
<option value="Caucasian">Caucasian</option>
<option value="Latino/Hispanic">Latino/Hispanic</option>
<option value="Middle Eastern">Middle Eastern</option>
<option value="African">African</option>
<option value="Caribbean">Caribbean</option>
<option value="South Asian">South Asian</option>
<option value="East Asian">East Asian</option>
<option value="Mixed">Mixed</option>
<option value="_other_">Other…</option>
</select>

This is an edge case but...

Should there be an '#sort_end' property?

I am fine with RTBC.

Committed 7430b7a and pushed to 8.8.x. Thanks!

Published change record