Problem/Motivation

#2107427: Regression: Language names should display in their native names in the language switcher block or some other issue might remove the only usage of sortByWeightAndTitle
This issue tracks that the function it self, the test and data provider should be removed.

Quite a few issues have tried to remove this sortByWeightAndTitle() or had trouble with it.
#2107427: Regression: Language names should display in their native names in the language switcher block
#2226533: Changes to the Language class due to the LanguageInterface (followup)
#2315095: EntityTranslationTest webtest random failure in some patches (not HEAD)
#2239399: Languages should be sorted by label instead of title
#2304651: Core Language sort() should not work, needs to use name instead of default title

It was added a while ago in #2151223: Add a method to SortArray to sort by weight and title

Proposed resolution

Once the usage is gone, remove the things.

Also, maybe think about other sorts that access properties.
And, maybe think about if sort should be on some language interface.

Remaining tasks

Wait for #2107427: Regression: Language names should display in their native names in the language switcher block

User interface changes

No.

API changes

Yeah, removing a method.

CommentFileSizeAuthor
#18 interdiff.2332739.11.18.txt3.95 KBYesCT
#18 2332739.18.patch5.75 KBYesCT
#17 interdiff.2332739.10.11.txt902 bytesYesCT
#17 2332739.11.patch9.08 KBYesCT
#13 2332739.10.patch8.81 KBYesCT
#10 interdiff.2332739.9.10.txt2.69 KBYesCT
#10 2332739.10.patch8.81 KBYesCT
#9 2332739.9.patch8.5 KBalexpott
#9 7-9-interdiff.txt4.64 KBalexpott
#7 interdiff.2332739.2.7.txt627 bytesYesCT
#7 2332739.7.patch4.84 KBYesCT
#3 2332739.2.patch4.84 KBYesCT
#2 2226533.279.patch273.76 KBYesCT
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue summary: View changes
YesCT’s picture

YesCT CreditAttribution: YesCT commented
Status: Postponed » Needs review
FileSize
2226533.279.patch273.76 KB

want to see what this would look like.

not sure if the Language::sort() should be typehinted with Language or LanguageInterface.

note #2246679: Make Language module's LanguageInterface (to be ConfigurableLanguageInterface) extend Core's LanguageInterface got in.

Maybe sort() should be on the LanguageInterface. It is not now.

ConfigurableLanguage gets its sort() from ConfigEntityBase. (which accesses the weight and label properties directly.)

YesCT’s picture

YesCT CreditAttribution: YesCT commented
FileSize
2332739.2.patch4.84 KB

oops. totally the wrong file. (I'll cancel that test)
here is the correct one.

The last submitted patch, 2: 2226533.279.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2332739.2.patch, failed testing.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue summary: View changes

collecting other issues I think have delt with this sort.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Status: Needs work » Needs review
FileSize
2332739.7.patch4.84 KB
interdiff.2332739.2.7.txt627 bytes

messed up the brackets.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented 
+++ b/core/lib/Drupal/Core/Language/Language.php
@@ -206,12 +206,16 @@ public function setNegotiationMethodId($method_id) {
+   * @param \Drupal\Core\Language\Language[] $languages
...
+    uasort($languages, function (Language $a, Language $b) {

If we hint on the interface then this can be used to sort configurable languages too. Which will be super useful for #2107427: Regression: Language names should display in their native names in the language switcher block

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
7-9-interdiff.txt4.64 KB
2332739.9.patch8.5 KB

Like so...

YesCT’s picture

YesCT CreditAttribution: YesCT commented
FileSize
2332739.10.patch8.81 KB
interdiff.2332739.9.10.txt2.69 KB

thanks.

read through the changes...

change to the sort to save the weights instead of getWeight each time. ok.

type hinting on the interface, k.

adds configurable language objects to the sort test. seems like a good idea to have a test that sorts an array made up of Language and ConfigurableLanguage. (not sure if we should add that in this issue, but seems fine, as before #2246679: Make Language module's LanguageInterface (to be ConfigurableLanguageInterface) extend Core's LanguageInterface we could not do it. Now we can and since ConfigurableLanguage's are now LanguageInterface things, and we are changing the Language::sort() seems like a good idea to extend the test coverage.)

I made some small changes to the data provider to keep the same pattern as the Language things had before, and keep the tests only testing what they were before (there was a test that had no ties in weight).

---

I dont understand why we have to make a new EntityType to use, instead of configurable_language.

Maybe the test is not really testing what would happen when sorting an array that has some ConfigurableLanguage.

Status: Needs review » Needs work

The last submitted patch, 10: 2332739.10.patch, failed testing.

YesCT’s picture

YesCT CreditAttribution: YesCT commented

random fail due to #2330751: Random test failures everywhere due to ->with(Request::create())

Parameter 0 for invocation Symfony\Component\Routing\Matcher\RequestMatcherInterface::matchRequest(Symfony\Component\HttpFoundation\Request Object (...)) does not match expected value.

Drupal\Tests\Core\UrlTest

YesCT’s picture

YesCT CreditAttribution: YesCT commented
FileSize
2332739.10.patch8.81 KB
YesCT’s picture

YesCT CreditAttribution: YesCT commented
Status: Needs work » Needs review

(uploaded same patch again, so the test results can be preserved in case that is helpful for the random fail issue)

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Title: Remove sortByWeightAndTitle » Remove SortArray::sortByWeightAndTitle
Status: Needs review » Needs work
Issue tags: +language-base, +sprint

Looks good except:

+++ b/core/tests/Drupal/Tests/Core/Language/LanguageUnitTest.php
@@ -97,12 +100,25 @@ public function testGetNegotiationMethodId() {
   public function testSortArrayOfLanguages(array $languages, array $expected) {

Not an array sort anymore?

YesCT’s picture

YesCT CreditAttribution: YesCT commented

yeah, I thought of that, but it does sort an array of languages. :)

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Status: Needs work » Needs review
FileSize
2332739.11.patch9.08 KB
interdiff.2332739.10.11.txt902 bytes

Just a little comment fix about types.

If we are ok with the mock in the test, then I guess this is ready.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
FileSize
2332739.18.patch5.75 KB
interdiff.2332739.11.18.txt3.95 KB

after berdir pointed out that this test lives in the place for core components, and that it was strange to be using something from a module,

I took out the configurablelanguage bit. that can go in its own test. #2334483: add test for sorting an array of languages that are Language and ConfigurableLanguage to show the sort() will work for configurable languages too.

maybe we should mock something that implements the language interface and add it too, which could be added here... but I think this is good enough to be ok for now, and we can expand it in other issue. #2334477: expand Language::sort() test coverage to show it works with other objects that implement the LanguageInterface

So.. I think we can agree on this. Get this in, then work on the others.
This will also unblock at least some things.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Reviewed & tested by the community

Looks good to me, we can expand with more tests later.

  • catch committed edafea1 on 8.0.x 
    Issue #2332739 by YesCT, alexpott: Remove SortArray::...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: -sprint

Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.