Add a method to SortArray to sort by weight and title

Awesome test coverage. I wonder whether we can use it somewhere already in the patch.

4: 2151223-8.patch queued for re-testing.

2151223-8.patch seems to be good.

4: 2151223-8.patch queued for re-testing.

Committed/pushed to 8.x, thanks!

Worth adding a change notice for the addition.

+++ b/core/lib/Drupal/Component/Utility/SortArray.php
@@ -91,6 +91,34 @@ public static function sortByTitleProperty($a, $b) {
+  public static function sortByWeightAndTitleKey($a, $b, $weight_key = 'weight', $title_key = 'title') {
...
+      return static::sortByKeyString($a, $b, $title_key);

+++ b/core/lib/Drupal/Core/Language/Language.php
@@ -205,14 +205,7 @@ public function extend($obj) {
-        return strnatcasecmp($a->name, $b->name);

Why did we switch from an object with a name property to an array with a title key?

Out of curiosity, testing a revert to see what happens.

Let's wait on the change record until we fix this. :) We can also just create a new draft here and publish it when we commit the fixed patch.

I think head is silently failing because of missing tests? So this is needs work to get a patch with tests.

I looked at this and I don't know what is the next step.

ArraySort::sortByWeightAndTitleKey it is useful for avoiding duplication, just maybe rename it to sortByWeightAndLabel key and use label by default (I thought there was an issue for that, but cannot find it right now).

In #2239399: Languages should be sorted by label instead of title we are using an anonymous function in sort() for calling this with additional params, so we could use similar patterns in other places.

#11: I don't know, but it allows to reorder arrays and objects in the same way, and reuse sortByKeyInt et al. We could check if this function is needed for arrays anymore and remove the array casting, which probably is a bad thing in terms of performance.

So, as far as I got, @ŧodos:

1. Write a change notice for commit at #10.
2. Check #11, in a follow-up.
3. Rename sortByWeightAndTitleKey() to sortByWeightAndLabelKey(), in a follow-up (which maybe already exists).

Am I right? Still confused.

Other than language, which is broken, there are no other usages of this in core.
I don't think sortByWeightAndTitleKey is actually that reusable.

Even if we had another use case for it, what if it wanted to sort first by Title and *then* by weight?

I think Language::sort should just use SortArray::sortByKeyInt and SortArray::sortByKeyString in the anonymous function.

Reverting this would let us use the test coverage from the other issue, and I think that's enough.

Why did we switch from an object with a name property to an array with a title key?

Because the class is named SortArray...?

I agree with reverting this.

@tim.plunkett kindly explained the bug/regression to me in IRC.

However, I don't think that really requires a full revert of this patch. The method is useful, but admittedly, it isn't obvious how you can leverage it. The trick is to wrap it into a mini closure, so you can pass custom arguments:

    uasort($languages, function ($a, $b) {
      return SortArray::sortByWeightAndTitleKey($a, $b, 'weight', 'name');
    });

I'm very confident that I'm actively using this method in another issue/patch/branch, but I wasn't able to find it.

that patch is #2239399: Languages should be sorted by label instead of title. but it has test coverage.

Okay, so this is just back to needing a change notice, and we can follow-up in the language issue.

ok.

clarified in issue summary remaining tasks.

Tagging novice for the change record.

This is a (very) minor API addition, not an API change at all, so I don't really think it's worth adding a change notice for.

This is still troubling...