Languages should be sorted by label instead of title

YesCT told me to create this patch. It adds a sorting by label instead of title.

Looks good.

It's curious that this patch passes without having to modify any tests. The test coverage will be added as soon as #2107427: Regression: Language names should display in their native names in the language switcher block lands, so I would unblock that one RTBCing this one.

We need tests on this one.

Also, I'm not sure this is implemented in a sane way.

We are in core, so we can just do this, but how would this be done in a way contrib could do it?

Adding a new method to the Utility class is wrong here.

If this change had been along the right lines, this is the correct change:

  public static function sort(&$languages) {
    uasort($languages, function ($a, $b) {
      return SortArray::sortByWeightAndTitleKey($a, $b, 'weight', 'label');
    });
  }

However, #2151223: Add a method to SortArray to sort by weight and title switched from name to title, and this switches to label, but the Language class does *not* have a label or title!

Totally agree with tim.plunkett, I didn't thought of an anonymous function!

Added phpunit tests, and changed the sort method according to @tim.plunkett advice at #8.

11: 2239399-11.patch queued for re-testing.

11: 2239399-11.patch queued for re-testing.

an unrelated change in newline. just undoing that so the patch from 8.x is cleaner.

note, if this is committed, it makes #2151223: Add a method to SortArray to sort by weight and title not easy to revert.

let's get a patch here that does not use that sortByWeightAndTitleKey()
the tests here are still good.

Nevermind, we're keeping it after all, let's just get this in. Thanks @penyaskito!

ok. super.

17: 2239399-17.patch queued for re-testing.

17: 2239399-17.patch queued for re-testing.

Last one looks random failure, I created #2254011: Drupal\system\Tests\Entity\EntityQueryTest random failure

The fix looks good. Its confusing that language config entities have a label while language runtime objects have a name. That would be resolved with #2246679: Make Language module's LanguageInterface (to be ConfigurableLanguageInterface) extend Core's LanguageInterface which is still a bit off. So we need this fixed in the meantime.

17: 2239399-17.patch queued for re-testing.

17: 2239399-17.patch queued for re-testing.

This is a blocker for #2107427: Regression: Language names should display in their native names in the language switcher block, would be good to get this going so we can move onto that which is in itself a blocker for more language name display issues.

Spent some time debugging the errors without any luck. BTW locally, without the patch (or with SortArray::sortByWeightAndTitleKey($a, $b, 'weight', 'title');), is always green, but with the current patch at #17 sometimes is green, sometimes I got 5 errors.

17: 2239399-17.patch queued for re-testing.

There's no "title" as key when ordering the values. This is why the test is always green I think.

Looks like the problem is here:

    if ($weight_cmp === 0) {
      return static::sortByKeyString($a, $b, $title_key);
    }

If this method (sortByKeyString) returns 0, 1 or -1 the test will be green. Maybe a problem with chars when using strnatcasecmp()... not sure. Anyway I would like to know if everything is correct when the result is -1 fror example.

+++ b/core/lib/Drupal/Component/Utility/SortArray.php
@@ -114,7 +114,7 @@ public static function sortByWeightAndTitleKey($a, $b, $weight_key = 'weight', $
-      return static::sortByKeyString($a, $b, $title_key);
+      return -1;

This is incorrect. Please remove this change.
Your other change should have been enough.

Failures at #37 are about Sort Array and this is normal. @tim.plunkett I only wanted to check the test results.

Failures at #17 were about EntityQuery and looks like the problem could be the explained at #37.

With only this change:

return SortArray::sortByWeightAndTitleKey($a, $b, 'weight', 'label');

the test will fail with the same errors (#17). So needs work :)

Sorry for not providing feedback, looks like we are all in the same issue but not in the same page.

I asked tstoeckler for help and we have met today for taking a look together. What was clear that there is a random fail, because running the test with patch at #17 repeatedly shows errors, but not consistently:

Drupal\system\Tests\Entity\EntityQueryTest                    92 passes                                      
Drupal\system\Tests\Entity\EntityQueryTest                    87 passes   5 fails                            
Drupal\system\Tests\Entity\EntityQueryTest                    87 passes   5 fails                            
Drupal\system\Tests\Entity\EntityQueryTest                    92 passes                                      
Drupal\system\Tests\Entity\EntityQueryTest                    87 passes   5 fails           

I'm debugging this tonight and I think I found the cause. IMO the problem is in HEAD, it is just not exposed because we are sorting by "label", but there is no label on languages, so we are comparing "" with "" and that works.

However, the random fails happen when the language name meet concrete conditions. I'm now investigating which conditions would that be.

I'm working on this now at the Drupal London Code Sprint.

cool. hop in #drupal-contribute and #drupal-i18n on irc.freenode.net :)

This patch fixes the failed tests in patch #37. It does the following:

• Fixes \Drupal\Component\Utility\SortArray::sortByWeightAndTitleKey() as suggested in comment #38.
• In the test, provides a label property for Language objects, so that they can be tested for sorting by weight and label - the main reason for creating this issue. Patch #37 did not provide a label property for test Language objects, which caused the tests to fail.

However this brings up a few questions which I hope someone can answer:

1. The test data provider in providerTestSortLanguages() created objects of class \Drupal\Core\Language\Language (the "Core\Language" class) for sorting by weight and label. However, these objects do not have a label property upon creation by default, unless a specific label key is passed in. Why not?
2. Should a "Core\Language" object exist without a label property? If so, what is the point of sorting them by weight and label, since an object might not have a label?
3. I found that the \Drupal\language\Entity\Language class (the "Entity\Language class") has a label property. Should this class have been used to create the test objects instead? However the test was for the Language class's sort() method. The "Entity\Language" class does not have a sort() method for testing, which means we don't have anything to test.

This patch was created with much advice and clarification from @vijaycs85 at Drupal London Code Sprint May 2014. Thanks Vijay!

Unassigning myself as I've created and uploaded the patch in #44.

Patch itself is RTBC.

Just verified, though, that this still results in random failures in EntityQueryTest. Thus marking postponed on #2254011: Drupal\system\Tests\Entity\EntityQueryTest random failure :-/.

I dont see a label property on core Language/Language
was this assuming language module Entity/Language ?

either or both of these might help prevent this confusion.

There is a sort on Entity/Language inherited from ConfigEntityBase

  /**
   * Helper callback for uasort() to sort configuration entities by weight and label.
   */
  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);
    }
    return ($a_weight < $b_weight) ? -1 : 1;
  }

#2304651: Core Language sort() should not work, needs to use name instead of default title is in. So we should try and redo this on top of that. This is high on my priority list, but if someone works on it before me that is ok also. Be sure to post on the issue or ping me on irc.

My worry when I looked at this last week was that #47.

We did the rename. #2271005: Rename Language module's LanguageInterface to ConfigurableLanguageInterface and Language to ConfigurableLanguage
so let's come back to this.

but sort is not required by the interface. Maybe sort should specified on the (core) LanguageInterface. That seems like a separate issue....

Looks like, this issue become one line patch.

How is this tested?

ok, we don't have label anymore. it is just 'name'. It means, all part of this issue has been covered by #2271005: Rename Language module's LanguageInterface to ConfigurableLanguageInterface and Language to ConfigurableLanguage. this issue can be closed as duplicate (sadly). Would be great to get the commit credit for the people contributed in this issue in another followup
eugenesia, marthinal, YesCT, penyaskito, mashermike | alimac

How is this tested?

LanguageUnitTest::testSortArrayOfLanguages covering it already.

Yeah I agree it is sad that the people working on this were not credited :/ On the other hand, #2107427: Regression: Language names should display in their native names in the language switcher block may also be worked on now (unpostponed!), which will be amazing to have in core finally :) Thanks all for contributing here and don't let the process hurdles let you down.