Languages don't get weights by default, reordered when displayed translated

I'll work on this one

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.

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

Wrong patch

More test corrections that took assumptions about how languages were ordered

Removed debug call in the test

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

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.

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);

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

first the rerrol

And the changes

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

Retitled once more as appropriate for a bug.

+++ 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?

@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.

+++ 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.

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.

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

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?

Rerolled and improved code comments as per #35.

+++ 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?

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.

Oops.

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.

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.

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

@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.

@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.

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.

Committed and pushed to 8.0.x. Thanks!

One minor docs typo fixed on commit.

Committed/pushed to 8.0.x, thanks!

Thanks!