Restore simplicity of language list

@miro: the special languages that were hidden were not possible to delete or switch to. The only thing was possible for them was to define their order within select boxes (by dragging them in this list). See screenshot on #1869292: Remove confusing "multiple" language from core (the default radio button is disabled on the special languages, see the code removed). So the only functionality this proposed patch removes is the ability to move special languages around (eg. inbetween the human created languages, before them, etc) which is not a very realistic feature (even if it would make sense, it would probably make sense on a per entity basis, which is not possible).

There is no other functionality removed.

@Bojhan: well, that is not an existing feature in Drupal 7. So we imagined there would be a need but the admittedly limited use of it hinders usability in basic operations, so I think it was legitimately sidelined as an imaginary need :) The special languages do have weight in the DB (and we need to fix their weight when adding new languages, still to be solved in this patch), so anybody in contrib can do a UI that lets people reorder them, or modify this form to make it happen.

The notices were due to language_admin_overview_form_submit() assuming the locked languages will be in the form. They are not. The fail was in Drupal\views\Tests\UI\DisplayTest->testCloneDisplay which I can hardly believe has anything to do with this patch. Fixed the notices, let's see this again!

Still needs code adjustments to make sure newly added languages don't show up after/inbetween locked languages in language dropdowns. Theoretically weights might also be recalculated when languages are reordered (if they were saved differently outside of the form), so I think that case should be covered too.

Any takers?

@klonos: that text is not introduced by this patch and is not yet in core, so it is a good cross-reference, but has no relevance here.

@YesCT: core does not have a way to order languages outside of the order screen (except when a new language is added, I think it does not get any weight and therefore likely shows up at top of the list). Ie. you can test this patch, manually add a couple languages and likely see the locked languages show up at odd places in language select dropdowns (intermixed with human added languages).

Did not intend to remove Needs tests.

Well, it would still have the weight select box on the table, which is equally puzzling I think :)

Oh, so the "Show row weights" feature remains (hides the weight) but the drag feature does not come in. Hm, interesting. This might be a good solution, yeah :)

@vijaycs85: so the more involved problem that this was left open for and needing tests is if you add a couple more languages, those would get weights that are higher than the special languages in the languages table (Not applicable, etc). so the list order would be upset. The weight of the special languages would need to be reset properly when submitting this form or adding new languages or removing languages I think. (Unless the problem I'm explaining is not reproducible, I did not actually test yet :).

@andypost: languages are not yet config entities, so that does not apply (yet).

I don't agree with the weight logic. It seems like the system languages are weighted *before* the user created languages. That was not the behavior prior to the patch, the custom languages were weighted before the system languages, right? Am I missing something?

Also, minor code style comments: 'autoconfiguration' can be left as one word AFAIS and the new API function's phpdoc contains an extra empty line, that should not be there.

Yes, we did agree earlier to remove those references too, when we remove the display of them in the admin list.

All right, so I don't know how that default state comes about, but is not really intended.

If you look at http://api.drupal.org/api/drupal/core%21modules%21language%21language.in... it sets up the default language and *then* adds the system languages with higher weights, ensuring the system languages are at the end of the list. (http://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functio... gets the default language weight so it can set up the system languages with higher weights).

Also, http://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functio... itself, when the language module is not enabled would return the default language and *then* the system languages, again later in the list (and with higher weight).

So the existing code demonstrates at least at two places that we want to have the system languages listed *after* the user created languages through weighting. Therefore it would be important we ensure that behaviour with this patch as well. ie. instead of taking the minimum of the weights for the non-locked languages, take the maximum :)

No, more negative weights are lower, not higher :) -57 is *lower* than -12 :) Weights are simply ordered in order of the number,-57 will be before -12 which will be before 0 which will be before 12 which will be before 57. System languages should have *higher* weight (be a bigger number :) compared to user editable languages.

From IRC review:

[6:47pm] GaborHojtsy: you seem to be updating all existing languages to be one less by weight
[6:47pm] GaborHojtsy: and then setting the system languages all to the same max weight, right?
[6:48pm] GaborHojtsy: I think you could avoid the first update by taking their max weight and then setting the system languages to that + 1
[6:48pm] GaborHojtsy: you don't need to update the locked = 0 languages IMHO
[6:48pm] GaborHojtsy: one less SQL query :)

Thanks! I only looked at the actual code now not yet the test. There are confusions around low/high in the test as well (comments vs. code :) but I assume you'll update the test a bit anyway due to the logic changes that seem to be needed. Thanks for working on this and keep it up :)

+++ b/core/modules/language/language.moduleundefined
@@ -514,6 +514,9 @@ function language_save($language) {
+  // Update weight of locked system languages.
+  language_update_default_weight();
+

@@ -560,6 +563,9 @@ function language_delete($langcode) {
+    // Update weight of locked system languages.
+    language_update_default_weight();
+

I think a better name for this would be language_update_locked_weights(). It updates multiple weights and does not relate to the default language :)

+++ b/core/modules/language/language.moduleundefined
@@ -855,3 +861,17 @@ function language_set_browser_drupal_langcode_mappings($mappings) {
+/**
+ * Updates system languages weight.
+ *
+ */

We can say locked languages here, that is correct internal terminology.

+++ b/core/modules/language/language.moduleundefined
@@ -855,3 +861,17 @@ function language_set_browser_drupal_langcode_mappings($mappings) {
+ // Update system languages weight.
+  db_update('language')
+    ->fields(array('weight' => $max_weight + 1))
+    ->condition('langcode', array(LANGUAGE_NOT_SPECIFIED, LANGUAGE_NOT_APPLICABLE), 'IN')
+    ->execute();

Minor: The comment is not indented properly.

Major: there could be any other system languages. It is possible that contrib modules add any other locked languages. So locked = 1 would be a more accurate condition here.

Also, it would be ideal if we could retain the existing order of locked languages. By setting all of them to the same weight, they will be listed in alphabetic ordering after the user configured languages. What I'd do is to use language_list(LANGUAGE_LOCKED) (or equivalent SQL) to get a full list of locked languages and update them in their order as returned with weights increasingly higher than the highest configured language. So that way they would retain their order and all be higher weighted compared to the configured languages.

Hope that is clear :)

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageConfigurationTest.phpundefined
@@ -23,7 +23,7 @@ class LanguageConfigurationTest extends WebTestBase {
-      'name' => 'Language negotiation autoconfiguration',
+      'name' => 'Language negotiation auto configuration',

Once again, this is fine as "autoconfiguration" IMHO. It looks like an odd word, but "auto configuration" makes less sense.

Looking better, thanks! Most comments are now on the test :)

+++ b/core/modules/language/language.moduleundefined
@@ -855,3 +861,21 @@ function language_set_browser_drupal_langcode_mappings($mappings) {
+/**
+ * Updates locked system languages weight.
+ *
+ */

This still has an empty newline. Also I'd pluralize the weights and not languages, so:

"Updates locked system language weights."

+++ b/core/modules/language/language.moduleundefined
@@ -855,3 +861,21 @@ function language_set_browser_drupal_langcode_mappings($mappings) {
+    $max_weight ++;

Why the space between the variable and ++? :)

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageConfigurationTest.phpundefined
@@ -99,4 +99,54 @@ function testLanguageConfiguration() {
+    $this->assertTrue($this->hasHighWeight(), 'System languages has high weight after adding new language');
...
+    $this->assertTrue($this->hasHighWeight(), 'System languages has high weight after re-ordering languages');
...
+    $this->assertTrue($this->hasHighWeight(), 'System languages has high weight after deleting a language');

"System languages has ..." => "System languages have ..."

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageConfigurationTest.phpundefined
@@ -99,4 +99,54 @@ function testLanguageConfiguration() {
+  /**
+   * Helper to check high weight of system languages;
+   *
+   * @return bool
+   */
+  protected function hasHighWeight() {
+    $system_lang_weight = db_query('SELECT weight FROM {language} WHERE locked = 1 limit 1')->fetchField();
+    return $system_lang_weight >= $this->getHighWeight();
+  }

The inclusion of "limit 1" in the query is pretty MySQL specific, that should not be used as-is in a database compatible fashion :)

Anyway I'd make this a bit more specific, eg. do a drupal_static_reset('language_list') on top of this method to ensure the language list is reset and then compare weight of all items in language_list(LANGUAGE_LOCKED) to the high weight of the configurable languages. That would ensure all of them got a properly high weight.

Also I'd rename to hasCorrectWeights() or something, because we are not giving the same weight to all languages anymore, so the method name is misleading.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageConfigurationTest.phpundefined
@@ -99,4 +99,54 @@ function testLanguageConfiguration() {
+  /**
+   * Helper to get high weight of unlocked languages list.
+   *
+   * @return int
+   */
+  protected function getHighWeight(){
+    return db_query('SELECT MAX(weight) FROM {language} WHERE locked = 0')->fetchField();
+  }

I'd rename to getHighestConfigurableLanguageWeight() or something descriptive like that :)

Thanks!

+++ b/core/modules/language/language.moduleundefined
@@ -855,3 +861,21 @@ function language_set_browser_drupal_langcode_mappings($mappings) {
+ * Updates locked system language weights.
+ *

This is the 3rd time I'm making a note of this extra unneeded newline.

+++ b/core/modules/language/language.moduleundefined
@@ -855,3 +861,21 @@ function language_set_browser_drupal_langcode_mappings($mappings) {
+  // Get minimum weight to update the system languages to keep it on top.

keep them on top

+++ b/core/modules/language/language.moduleundefined
@@ -855,3 +861,21 @@ function language_set_browser_drupal_langcode_mappings($mappings) {
+    // Update system languages weight.

language weights

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageConfigurationTest.phpundefined
@@ -99,4 +99,62 @@ function testLanguageConfiguration() {
+    $this->checkConfigurableLanguageWeight('after re-ordering');
+    // Remove predefined language.

You've accidentally removed a line here.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageConfigurationTest.phpundefined
@@ -99,4 +99,62 @@ function testLanguageConfiguration() {
+   * Helper to validate all system languages have higher weight.
+   *

Higher weight compare to what? I'd write:

"Validates system languages are ordered after configurable languages."

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageConfigurationTest.phpundefined
@@ -99,4 +99,62 @@ function testLanguageConfiguration() {
+   * @param string $event
+   *   Event of check. e.g. reoder, save, delete.
...
+    foreach (language_list(LANGUAGE_LOCKED) as $language) {
+      $replacements['%language'] = $language->name;
+      $this->assertTrue($language->weight > $system_lang_weight, t('System language %language has higher weight than configurable languages @event', $replacements));
+    }

Naming this an $event is pretty misleading, Drupal has the concept of events which is unrelated to how you use it here. I'd name it $state or something along those lines, sounds more natural.

Also t() is not allowed in assert() messages, you can use format_string() as a replacement that does not translate text.

Looks good to go now! Thanks for sticking to it and producing quick iterations :)

Superb, thanks!