Problem/Motivation

When adding new languages, they do not get a weight assigned. Locked / built-in languages get explicit weights so they are always at the end of the list, but configured languages do not get weights. This means when displaying a language list, the displayed order is different based on translations of languages at the time. That is not expected because sites usually expect languages in a fixed order.

A workaround is to hit save the language admin page once all languages are added, so that each language gets a different weight.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because saving should not reorder the order.
Issue priority Normal because it effects just one piece of functionality: language orders. (not a whole system).
Unfrozen changes Not unfrozen.
Disruption Not disruptive for core/contributed and custom modules/themes because it will not require a BC break/deprecation/internal refactoring/widespread changes.

Proposed resolution

Give a newly added language a weight. Set it higher than the previously added configured language. (Locked languages keep getting weights bigger than this newly added one based on pre-existing code). Add tests.

Remaining tasks

User interface changes

Languages have an expected order out of the box in the order they are added.

API changes

None.

Files: 
CommentFileSizeAuthor
#49 interdiff.txt807 bytescatch
#46 interdiff.txt938 bytesGábor Hojtsy
#46 2350933.46.patch6.43 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,175 pass(es). View
#42 2350933-2.42.patch6.39 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,008 pass(es). View
#42 39-42-interdiff.txt7.42 KBalexpott
#39 2350933-2.39.patch8.33 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,942 pass(es). View
#39 38-39-interdiff.txt714 bytesalexpott
#38 2350933-2.38.patch9.03 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#38 36-38-interdiff.txt9.67 KBalexpott
#36 2350933-2.36.patch8.22 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,930 pass(es). View
#36 34-36-interdiff.txt1.64 KBalexpott
#34 interdiff.txt836 bytesGábor Hojtsy
#34 2350933.34.patch8.12 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,872 pass(es). View
#33 2350933.33.patch8.12 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,265 pass(es). View
#33 25-33-interdiff.txt8.19 KBalexpott
#26 new-language-weight-2350933-25.patch6.82 KBrodrigoaguilera
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,269 pass(es). View
#25 interdiff.txt962 bytesrodrigoaguilera
#24 new-language-weight-2350933-24.patch6.71 KBrodrigoaguilera
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,136 pass(es). View
#17 test-only.patch1.46 KBrodrigoaguilera
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,088 pass(es), 2 fail(s), and 0 exception(s). View
#17 interdiff.txt5.46 KBrodrigoaguilera
#17 new-language-weight-2350933-17.patch6.69 KBrodrigoaguilera
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,095 pass(es). View
#12 new-language-weight-2350933-12.patch11.36 KBrodrigoaguilera
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,099 pass(es). View
#8 new-language-weight-2350933-8.patch3.98 KBrodrigoaguilera
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,094 pass(es), 6 fail(s), and 0 exception(s). View
#5 new-language-weight-2350933-5.patch3.15 KBrodrigoaguilera
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,819 pass(es), 87 fail(s), and 0 exception(s). View

Comments

Gábor Hojtsy’s picture

Issue tags: +SprintWeekend2015Queue
YesCT’s picture

Issue summary: View changes

did beta evaluation according to https://www.drupal.org/contributor-tasks/update-allowed-beta
disruption guess might need to be updated, but for now, this looks to be allowed in the beta.

YesCT’s picture

Issue summary: View changes
Issue tags: +Needs tests
rodrigoaguilera’s picture

I'll work on this one

rodrigoaguilera’s picture

Status: Active » Needs review
FileSize
3.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,819 pass(es), 87 fail(s), and 0 exception(s). View

Here's a patch that adds +1 to the newly added language leaving the default weight when we have the first language addded.

I'm not very sure if that is a very proper way to test.
I think is an overkill to test changing the language of the interface to check if the weight order is respected.

Status: Needs review » Needs work

The last submitted patch, 5: new-language-weight-2350933-5.patch, failed testing.

rodrigoaguilera’s picture

Status: Needs work » Needs review
FileSize
15.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,083 pass(es), 3 fail(s), and 0 exception(s). View

Added a check to ConfigurableLanguageManager so it doesn't try to update the weight on a non-configured-yet language.

rodrigoaguilera’s picture

FileSize
3.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,094 pass(es), 6 fail(s), and 0 exception(s). View

Wrong patch

The last submitted patch, 7: new-language-weight-2350933-7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 8: new-language-weight-2350933-8.patch, failed testing.

rodrigoaguilera’s picture

Status: Needs work » Needs review
FileSize
11.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,101 pass(es). View

More test corrections that took assumptions about how languages were ordered

rodrigoaguilera’s picture

FileSize
11.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,099 pass(es). View

Removed debug call in the test

rodrigoaguilera’s picture

Issue summary: View changes
YesCT’s picture

Thanks for working on this issue.

Cool. it is green. :)

To make it more likely that you will get reviews, please make interdiffs when posting a new patch on an issue that already has a patch.
For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff

Also, Please make a "tests only patch" that does not have the +1 fix, and just has the changes to the test.
That tests only patch will fail (hopefully). And then we can verify that it fails in the correct way. And we will know that this bug will not return in the future.
The contributor task doc on how to write tests at https://drupal.org/contributor-tasks/write-tests has some more info on test only patches.

YesCT’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

there are some fixes to clean up standards in here that are in some of the files being touched, but in lines not being changed for this issue.

So, to make this reviewable, we should only make the changes we need to (or fix standards things in lines we have to change to fix the issue)

Seems like the test coverage here might be good enough, so removing the needs tests tag.

rodrigoaguilera’s picture

rodrigoaguilera’s picture

FileSize
6.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,095 pass(es). View
5.46 KB
1.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,088 pass(es), 2 fail(s), and 0 exception(s). View

Removed the coding standards fixes.
interdiff between #12 and #17
and a test only patch

rodrigoaguilera’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 17: test-only.patch, failed testing.

rodrigoaguilera’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +sprint

Huge thanks for working on this, this looks generally good, two notes:

  1. +++ b/core/modules/language/src/ConfigurableLanguageManager.php
    @@ -353,10 +353,11 @@ public function updateLockedLanguageWeights() {
    -        $max_weight++;
    -        ConfigurableLanguage::load($language->getId())
    -          ->setWeight($max_weight)
    -          ->save();
    +        if ($language = ConfigurableLanguage::load($language->getId())) {
    

    Under what conditions would a locked language not be present?

  2. +++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
    @@ -120,6 +120,18 @@ public function preSave(EntityStorageInterface $storage) {
    +      // The newly created language will have the weight of the heaviest
    +      // language +1.
    +      $this->setWeight($last_language->getWeight() + 1);
    

    I think it would be important to make the distinction that you only take configurable languages here.

rodrigoaguilera’s picture

About 1.
I did that because of tests failing during the install of locale and language modules at patch in #5
https://qa.drupal.org/pifr/test/951998
For example CKEditorTest.

I'm wild guessing here but maybe the locked languages are not configurable yet until language module is fully installed and configurableLanguageManager trying to change the weights.

About 2.
I'll change it for
$this->getLanguages($this::STATE_CONFIGURABLE);

Gábor Hojtsy’s picture

@rodrigoaguilera: 1. aham, makes sense. 2. I think making sure the comments mention that too would be good.

rodrigoaguilera’s picture

Status: Needs work » Needs review
FileSize
6.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,136 pass(es). View

first the rerrol

rodrigoaguilera’s picture

FileSize
962 bytes

And the changes

rodrigoaguilera’s picture

FileSize
6.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,269 pass(es). View
Gábor Hojtsy’s picture

Title: Give languages weights when they are added, so they do not reorder when switching language (since weight ties, ordered by name) » Give languages weights when they are added, so they do not reorder when displayed translated
Issue summary: View changes

Looks good to me :) Thanks! Also updated issue summary.

Gábor Hojtsy’s picture

Title: Give languages weights when they are added, so they do not reorder when displayed translated » Languages don't get weights by default, reordered when displayed translated

Retitled once more as appropriate for a bug.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/language/src/ConfigurableLanguageManager.php
@@ -353,10 +353,11 @@ public function updateLockedLanguageWeights() {
-        $max_weight++;
-        ConfigurableLanguage::load($language->getId())
-          ->setWeight($max_weight)
-          ->save();
+        if ($language = ConfigurableLanguage::load($language->getId())) {
+          $max_weight++;
+          $language->setWeight($max_weight)
+            ->save();
+        }

Why is this change necessary?

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

@alexpott: I had the same question above in #21 and got answered in #22. In short it is for the brief transitional period when ConfigurableLanguageManager is already loaded but the configuration is not yet imported for those languages.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
@@ -121,6 +121,20 @@ public function preSave(EntityStorageInterface $storage) {
+    // When adding a new language the weight shouldn't be zero to avoid a
+    // reordering of the languages list when their names change i.e. interface
+    // translation.
+    // Fetch all configurable languages ordered by weight.
+    $languages = \Drupal::languageManager()->getLanguages($this::STATE_CONFIGURABLE);
+    // Retrieve the last.
+    $last_language = end($languages);

Let's only do this when $this->isNew() is TRUE and the weight is not zero.

alexpott’s picture

Status: Needs work » Needs review
FileSize
8.19 KB
8.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,265 pass(es). View

The changes in #25 overwrote any weight value already set. Also the question in #30 can be answered differently. If you are doing a kernel test base and the enabling language then you have to install its configuration because the configurable language manager relies upon.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
8.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,872 pass(es). View
836 bytes

Thanks @alexpott for the fixes, makes sense to me. Also fixing a case problem in method invocation and I think this should be good :)

catch’s picture

Status: Reviewed & tested by the community » Needs work

Just comment nits..

  1. +++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
    @@ -121,6 +121,16 @@ public function preSave(EntityStorageInterface $storage) {
    +    if ($this->id != 'en' && $this->isNew() && !$this->isLocked() && $this->getWeight() == 0) {
    

    Why exclude English from here? Could use a comment.

  2. +++ b/core/modules/language/src/Tests/LanguageListTest.php
    @@ -43,6 +47,14 @@ function testLanguageList() {
    +    // heavier and that is French.
    

    What is French?

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.64 KB
8.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,930 pass(es). View

Rerolled and improved code comments as per #35.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/modules/language/src/Tests/LanguageListTest.php
@@ -48,7 +48,7 @@ function testLanguageList() {
+    // heavier and than the last configurable language.

The updated sentence does not make sense. One "and" too much?

alexpott’s picture

Status: Needs work » Needs review
FileSize
9.67 KB
9.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Based on catch's questioning and writing tests I think we need a new approach. This makes the default value NULL and if it is NULL make it the last weight +1. If we want to ensure that ConfigurableLanguages have unique weights then we have to go way further.

alexpott’s picture

FileSize
714 bytes
8.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,942 pass(es). View

Oops.

The last submitted patch, 38: 2350933-2.38.patch, failed testing.

Gábor Hojtsy’s picture

Sounds like a good approach to differentiate explicit and implicit weights.

+++ b/core/modules/language/src/Tests/LanguageConfigurationTest.php
@@ -28,6 +29,9 @@ class LanguageConfigurationTest extends WebTestBase {
+    // Ensure the after installing the language module the weight of the english
...
+    $this->assertEqual(ConfigurableLanguage::load('en')->getWeight(), 0, 'The english language has a weight of 0.');

@@ -103,6 +107,24 @@ function testLanguageConfiguration() {
+    // Ensure that french language has a weight of 1 after being created through the UI.
...
+    // Ensure that french language can now have a weight of 0.

When we mention languages in comments and tests, we still uppercase them. You mostly uppercase them, these are exceptions and therefore inconsistent.

alexpott’s picture

FileSize
7.42 KB
6.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,008 pass(es). View

Another approach :)

Also looking at

  /**
   * Sort language objects.
   *
   * @param \Drupal\Core\Language\LanguageInterface[] $languages
   *   The array of language objects keyed by langcode.
   */
  public static function sort(&$languages) {
    uasort($languages, function (LanguageInterface $a, LanguageInterface $b) {
      $a_weight = $a->getWeight();
      $b_weight = $b->getWeight();
      if ($a_weight == $b_weight) {
        return strnatcasecmp($a->getName(), $b->getName());
      }
      return ($a_weight < $b_weight) ? -1 : 1;
    });
  }

I think sorting on something that is translatable might be problematic here.

Gábor Hojtsy’s picture

Most things that people would sort by traditionally are translatable, especially titles. Even creation dates and last updated dates on content entities :)

Gábor Hojtsy’s picture

@alexpott: looking at the newest approach patch, why would we not do the weight adjustment on creating a new language with the API? The latest patch only deals with creation on the UI.

alexpott’s picture

@Gábor Hojtsy with the API you are free to do what you like. Also there is no API for saving multiple languages at once.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
6.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,175 pass(es). View
938 bytes

Ok, I agree that to resolve the UX problem, solving it in the form is fine too. Attached is a minor doc fix. I think this is RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 1f2d857 on 8.0.x
    Issue #2350933 by rodrigoaguilera, alexpott, Gábor Hojtsy, YesCT:...
catch’s picture

FileSize
807 bytes

One minor docs typo fixed on commit.

Committed/pushed to 8.0.x, thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks!

Status: Fixed » Closed (fixed)

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