Use PHP 7.0 "spaceship" operator for user sort functions where possible

+++ b/core/lib/Drupal/Core/Language/Language.php
@@ -160,7 +160,7 @@ public static function sort(&$languages) {
+      return ($a_weight <=> $b_weight);

+++ b/core/modules/language/src/HttpKernel/PathProcessorLanguage.php
@@ -154,7 +154,7 @@ protected function initProcessors($scope) {
+      return ($a_weight <=> $b_weight);

no reason for () here

There's actually only one place to fix core/modules/layout_builder/src/Section.php

Probably second hunk in PathProcessorLanguage.php needs separate quickfix (optimization) to remove useless declaration of variables to compare - so I left it here

Other places are reverted because before 1/-1 there's check for equal value

None of these changes are necessary for PHP 8 compatibility as far as I can see. None of these are returning a bool. The issue summary needs to be updated. The changes are fine but things should be accurate.

Also the title needs fixing because neither of these methods return a bool - hence they do not trigger a warning.

I thing it's just hard to catch same weights in LB section
But ++ to update summary

+++ b/core/modules/layout_builder/src/Section.php
@@ -251,7 +251,7 @@ public function getComponentsByRegion($region) {
     uasort($components, function (SectionComponent $a, SectionComponent $b) {
-      return $a->getWeight() > $b->getWeight() ? 1 : -1;
+      return $a->getWeight() <=> $b->getWeight();

If weights are the same this fix is valid

Re-titled and update IS

None of these changes are necessary for PHP 8 compatibility as far as I can see. None of these are returning a bool.

There are a bunch more it looks like we can do, if we think this should be done everywhere:

core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
237: return ($a_weight < $b_weight) ? -1 : 1;

core/lib/Drupal/Core/Language/Language.php
163: return ($a_weight < $b_weight) ? -1 : 1;

core/lib/Drupal/Core/Routing/RouteProvider.php
395: return (int) $a['fit'] < (int) $b['fit'] ? 1 : -1;

core/lib/Drupal/Component/Utility/SortArray.php
129: return ($a_weight < $b_weight) ? -1 : 1;

core/modules/shortcut/src/Entity/Shortcut.php
188: return ($a_weight < $b_weight) ? -1 : 1;

core/modules/filter/src/FilterPluginCollection.php
97: return $a->weight < $b->weight ? -1 : 1;

core/modules/views_ui/src/Form/Ajax/ReorderDisplays.php
56: return $display1['position'] < $display2['position'] ? -1 : 1;

core/modules/help_topics/src/Plugin/HelpSection/HelpTopicSection.php
186: return $a->getPluginId() < $b->getPluginId() ? -1 : 1;

core/modules/image/src/ImageEffectPluginCollection.php
31: return ($a_weight < $b_weight) ? -1 : 1;

core/modules/tour/src/Entity/Tour.php
129: return ($a->getWeight() < $b->getWeight()) ? -1 : 1;

core/modules/views/src/ViewsData.php
307: return $a['weight'] < $b['weight'] ? -1 : 1;
310: return $a['title'] < $b['title'] ? -1 : 1;

core/modules/views/src/ViewsDataHelper.php
179: return $a_group < $b_group ? -1 : 1;
185: return $a_title < $b_title ? -1 : 1;

core/modules/views/tests/src/Kernel/ViewsKernelTestBase.php
115: return $order * (($a[$column] < $b[$column]) ? -1 : 1);

core/modules/views/tests/src/Functional/ViewTestBase.php
88: return $order * (($a[$column] < $b[$column]) ? -1 : 1);

core/modules/views/views.views.inc
282: return $label_counter[$a] > $label_counter[$b] ? -1 : 1;

core/modules/search/src/Entity/SearchPage.php
219: return ($a_status > $b_status) ? -1 : 1;

@longwave actually not, because other places using different comparison when values are equal

We still can use spaceship operator in most cases even if the == 0 comparison is done elsewhere?

Also surely e.g.

  protected function orderResultSet($result_set, $column, $reverse = FALSE) {
    $order = $reverse ? -1 : 1;
    usort($result_set, function ($a, $b) use ($column, $order) {
      if ($a[$column] == $b[$column]) {
        return 0;
      }
      return $order * (($a[$column] < $b[$column]) ? -1 : 1);
    });
    return $result_set;
  }

can just become

  protected function orderResultSet($result_set, $column, $reverse = FALSE) {
    $order = $reverse ? -1 : 1;
    usort($result_set, function ($a, $b) use ($column, $order) {
      return $order * ($a[$column] <=> $b[$column]);
    });
    return $result_set;
  }

Checked all places again and updated ones where it possible

@longwave Thank a lot for idea

We can change them all, even if the 0 value is not needed.

Most of this looks like a great cleanup/simplification. I didn't know about the "spaceship" until I saw this. Cool. ;)

1. +++ b/core/lib/Drupal/Component/Utility/SortArray.php
   @@ -122,11 +122,7 @@ public static function sortByKeyInt($a, $b, $key) {
   -    if ($a_weight == $b_weight) {
   -      return 0;
   -    }
   -
   -    return ($a_weight < $b_weight) ? -1 : 1;
   +    return $a_weight <=> $b_weight;
   

   Obvious win.

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -234,7 +234,7 @@ public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b)
   -    return ($a_weight < $b_weight) ? -1 : 1;
   +    return $a_weight <=> $b_weight;
   

   Less obvious. ;) The current code will return 1 if the weights are the same. New code will start returning 0. Is this really safe / desirable?

@longwave, would you be willing to clarify: "even if the 0 value is not needed."? Yes, all the tests are still passing, so that's good. But I'm not sure the full "change them all" approach is really safe.

Thoughts?

Thanks,
-Derek

Re #17.2, if you look at the wider context:

    if ($a_weight == $b_weight) {
      $a_label = $a->label();
      $b_label = $b->label();
      return strnatcasecmp($a_label, $b_label);
    }
    return $a_weight <=> $b_weight;

The spaceship operator can never return 0 as we already handled the equality case above; it can only ever return -1 or 1 now. This is the same for all the cases here, except where we explicitly returned 0 in the equality case and then we can use the spaceship operator alone, as in #17.1.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -234,7 +234,7 @@ public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b)
-    return ($a_weight < $b_weight) ? -1 : 1;
+    return $a_weight <=> $b_weight;

+++ b/core/lib/Drupal/Core/Language/Language.php
@@ -160,7 +160,7 @@ public static function sort(&$languages) {
-      return ($a_weight < $b_weight) ? -1 : 1;
+      return $a_weight <=> $b_weight;

+++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
@@ -392,7 +392,7 @@ protected function routeProviderRouteCompare(array $a, array $b) {
-    return (int) $a['fit'] < (int) $b['fit'] ? 1 : -1;
+    return (int) $b['fit'] <=> (int) $a['fit'];

+++ b/core/modules/filter/src/FilterPluginCollection.php
@@ -94,7 +94,7 @@ public function sortHelper($aID, $bID) {
-      return $a->weight < $b->weight ? -1 : 1;
+      return $a->weight <=> $b->weight;

+++ b/core/modules/help_topics/src/Plugin/HelpSection/HelpTopicSection.php
@@ -183,7 +183,7 @@ protected function getPlugins() {
-          return $a->getPluginId() < $b->getPluginId() ? -1 : 1;
+          return $a->getPluginId() <=> $b->getPluginId();

+++ b/core/modules/search/src/Entity/SearchPage.php
@@ -216,7 +216,7 @@ public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b)
-      return ($a_status > $b_status) ? -1 : 1;
+      return $b_status <=> $a_status;

+++ b/core/modules/shortcut/src/Entity/Shortcut.php
@@ -185,7 +185,7 @@ public static function sort(ShortcutInterface $a, ShortcutInterface $b) {
-    return ($a_weight < $b_weight) ? -1 : 1;
+    return $a_weight <=> $b_weight;

+++ b/core/modules/views/src/ViewsData.php
@@ -304,12 +304,9 @@ public function fetchBaseTables() {
-        return $a['weight'] < $b['weight'] ? -1 : 1;
+        return $a['weight'] <=> $b['weight'];
       }
-      if ($a['title'] != $b['title']) {
-        return $a['title'] < $b['title'] ? -1 : 1;
-      }
-      return 0;
+      return $a['title'] <=> $b['title'];

+++ b/core/modules/views/src/ViewsDataHelper.php
@@ -176,16 +176,12 @@ protected static function fetchedFieldSort($a, $b) {
-      return $a_group < $b_group ? -1 : 1;
+      return $a_group <=> $b_group;

+++ b/core/modules/views/views.views.inc
@@ -279,7 +279,7 @@ function views_entity_field_label($entity_type, $field_name) {
-    return $label_counter[$a] > $label_counter[$b] ? -1 : 1;
+    return $label_counter[$b] <=> $label_counter[$a];

I'm not in favour of these changes where the spaceship operator can never return 0 because of the surrounding context. The current code is more explicit about this. Whereas the new code obfuscates it for no reason. I think all of the above changes should be reverted.

#17.1 is great and we should be making changes like that.

FWIW for me the spaceship operator is more explicit, because I don't have to remember which way round is < and > vs -1 and 1, e.g:

+++ b/core/modules/views/src/ViewsDataHelper.php
@@ -176,16 +176,12 @@ protected static function fetchedFieldSort($a, $b) {
-      return $a_group < $b_group ? -1 : 1;
+      return $a_group <=> $b_group;

+++ b/core/modules/views/views.views.inc
@@ -279,7 +279,7 @@ function views_entity_field_label($entity_type, $field_name) {
-    return $label_counter[$a] > $label_counter[$b] ? -1 : 1;
+    return $label_counter[$b] <=> $label_counter[$a];

Here it is much more obvious to me that we are sorting $a_group before $b_group but $label_counter[$b] before $label_counter[$a] - ie the latter is a reverse sort. In other words: the magic constants of -1 and 1 are removed and the argument order always tells me which way we are sorting.

Whereas the new code obfuscates it for no reason.

@alexpott that's for readability reasons of code, moreover change record could propose usage of <=> on core (maybe it needs policy issue?)

@dww #17.2 that's how user sorts should work - return 0 when items are equal, also sorts becomes more predictable in php 8

#20 is a good point. Hmmm... I think both points (#20 and #19) are valid. But #20 is slightly more compelling. So let's ignore #19 for now unless a more compelling case can be made for it.

#18 re #17.2: Sorry I didn't apply and check wider context. Thanks!

#20: +1

#21: re: #17.2: Absolutely. User sorts should work like that. But I've been bitten so many times fixing bugs in core where the change "might be disruptive" that I'm now shy about behavior changes. ;) +1 to fixing these if allowed.

#22: I think #20 is more compelling, too. To address #19 perhaps we could adopt a comment convention in those (rare?) cases. If it really matters, would probably be clearer than having to parse surrounding context to check. E.g.:

  // 0 not possible.
  return $a_weight <=> $b_weight;

(or whatever).

Thanks everyone!
-Derek

Should we include strnatcasecmp() and similar ones in scope here? For instance:

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -227,14 +227,12 @@ public function createDuplicate() {
   public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b) {
-    $a_weight = isset($a->weight) ? $a->weight : 0;
-    $b_weight = isset($b->weight) ? $b->weight : 0;
-    if ($a_weight == $b_weight) {
-      $a_label = $a->label();
-      $b_label = $b->label();
-      return strnatcasecmp($a_label, $b_label);
+    $a_weight = $a->weight ?? 0;
+    $b_weight = $b->weight ?? 0;
+    if ($a_weight === $b_weight) {
+      return strtolower($a->label()) <=> strtolower($b->label());
     }
-    return ($a_weight < $b_weight) ? -1 : 1;
+    return $a_weight <=> $b_weight;
   }

No, we need to be careful because strnatcasecmp() and strtolower()/<=> do not sort identically:

>>> strnatcasecmp('A', '_');
=> -1

>>> strtolower('A') <=> strtolower('_');
=> 1

I think we should leave these alone as strnatcasecmp() already tells you how we are sorting, <=> is no clearer or more readable to me.

@longwave, thanks for your reply!

#22: I think #20 is more compelling, too. To address #19 perhaps we could adopt a comment convention in those (rare?) cases. If it really matters, would probably be clearer than having to parse surrounding context to check. E.g.:

// 0 not possible.
return $a_weight <=> $b_weight;

I am about to agree on this, but code should be self-explained as possible.

Here it is much more obvious to me that we are sorting $a_group before $b_group but $label_counter[$b] before $label_counter[$a] - ie the latter is a reverse sort. In other words: the magic constants of -1 and 1 are removed and the argument order always tells me which way we are sorting.

It's true. However, as the spaceship operator is new to me, the same as @dww (see #17), when I see a usage of it, it gives me a hint that the possible values/return values are -1, 0 and 1. If the possible values/return values are -1 or 1, in my mind ?: is better. Less confusing or less thinking while reading the code. I do not think most of people could get the info of "which way we are sorting" from the argument order if no extra explanation/comment.

So +1 to #19 to revert non-obvious <=>-applicable changes.

PHP 8 got rfc for sorts, so this clean-up can affect ordering as #26 said

Just re-roll for 9.3, I still see no much reasons to revert "non-obvious" changes as it much more readable comparing to 1/-1 magic numbers

Re #24 not sure comment will help as few lines above there's comparison for equality

LGTM.

+1 for RTBC.

Back to RTBC.

Applied using --3way and it worked great.

Committed b69a33e and pushed to 9.3.x. Thanks!