Language selectors are not showing localized to the page language

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
@@ -18,7 +18,163 @@ class BlockFormController extends EntityFormController {
+      // Fetch languages.
+      $languages = language_list(LANGUAGE_ALL);
+      foreach ($languages as $language) {
+        // @todo $language->name is not wrapped with t(), it should be replaced
+        //   by CMI translation implementation.
+        $langcodes_options[$language->langcode] = $language->name;
+      }

This method is moved to BlockFormController in #1927608: Remove the tight coupling between Block Plugins and Block Entities.

Thought I would take a stab at this with the TranslationManager. Feel free to point and laugh if I went the wrong way.

Reroll..

updated the patch so the reference to the TranslationManager is only created once.

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
@@ -178,10 +179,10 @@ public function form(array $form, array &$form_state) {
+        $lang = $t->translate($language->name);

Please use $this->t()

I'm not sure how this relates to the change notice: Alternative way of string translation for injected classes https://drupal.org/node/2079611

ah.. I didn't see that it extended.... sure makes this request super easy...

run, patch-test, run!

Looks good.

This is not a CMI translation implementation, sorry... The config_context_enter/leave() functions should be used to enter proper language contexts and the language loaded with that to load proper translations. Same problem in #2107427: Regression: Language names should display in their native names in the language switcher block.

at badcamp sprint to finish this off.

@Gabor
Michael (@Schnitzel) and I worked on this today and we found there was no translation for the languages until we added the configuration translation module and updated the language name. However even at that point, the language_list function returns a drupal_static list of languages, which returns the original (english) language name. If we add a reset the configured translation shows.

diff --git a/core/modules/block/lib/Drupal/block/BlockFormController.php b/core/modules/block/lib/Drupal/block/BlockFormController.php
index e2692a4..fbb2024 100644
--- a/core/modules/block/lib/Drupal/block/BlockFormController.php
+++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
@@ -170,8 +170,8 @@ public function form(array $form, array &$form_state) {
     // Configure the block visibility per language.
     if ($this->moduleHandler->moduleExists('language') && $this->languageManager->isMultilingual()) {
       $configurable_language_types = language_types_get_configurable();
-
       // Fetch languages.
+      drupal_static_reset('language_list');
       $languages = language_list(Language::STATE_ALL);
       $langcodes_options = array();
       foreach ($languages as $language) {

I am not sure which way to go from this point and would love some direction.

@Gabor
wouldn't this suffice in this case? (translates each language name to its native form.)

Removing myself from the author field so that I can unfollow the issue. --xjm

As I've said above t() is not to be used with language names. The comment itself says we don't use t() because that should not be used. This @todo is not to be resolved by removing the comment advising against it and using t() anyway. Heh.

Language names are user editable/configurable and not code sourced. t() is for code sourced strings. Please read and follow my suggestions in #13. I think we would be better off if we'd have utility functions to (a) return language code => names all translated to a specific language (b) return language code => names all translated to their native names and use the proper ones at appropriate places. There are related issues for other appearances of language lists.

I think this needs to be postponed on #1862202: Objectify the language system which similarly postponed #2107427: Regression: Language names should display in their native names in the language switcher block.

#1862202: Objectify the language system landed!

Does this make sense to work in parallel while #2107427: Regression: Language names should display in their native names in the language switcher block is in the works? I'm not sure.

Removing from sprint since this was postponed for so long....

Looking at https://api.drupal.org/api/drupal/core%21modules%21language%21src%21Conf... I am not sure this is still needed? Looks like language_list() which delegates the call to that method would use loadMultiple() on configFactory() which would load languages localized, no?

Same true for all other language selectors, eg. when selecting languages for a menu or node. Looking at language_process_language_select(). So we may only need to verify that this works as expected and close it as cannot reproduce :)

manually testing this. (too bad it didn't have a test. :P )

turning on all the translation modules...
adding languages: af, es, fr

in the UI translation page, translated "English" into AFEnglish, Inglés, anglais

(searching for French or Spanish does not show a interface string to be translated)

then,

1. language admin
==== uses the localized language label

2. language and content translation overview

3. article add - language select

4. block place (language switcher) with a language list

---------------
next I'll config translate the names and retest.

Yeah the listing of the languages loads the entity in the page language. I think I remember why language_list() will not work. It is called way too early in the request and then it "caches" the language list on a class variable, so the further calls, although the config language context changed, they would still get the "cached" language list from earlier which is all English. Either we should remove this "caching" and just look up the languages all the time or introduce a new method like #2107427: Regression: Language names should display in their native names in the language switcher block did for translation languages.

Probably if you remove the if ($this->languages) wrapping from ConfigurableLanguageManager::getLanguages() that will “fix” the issue :D then need to figure out how best to *fix* it and write tests.

Re #24 ... English was showing at traslatable in the interface translation because English is shipped config. So that was really config translation. :) @Gábor Hojtsy clarified in the d8mi meeting that the UI finds config strings (and nicely, magically) translators can use the interface translation UI if they like.

I'm working in tests but before start to code it I'm reviewing which interfaces should be tested. I have checked all UI commented in #24 by @YesCT but I have found one more in [1], "default language"

[1] es/admin/config/regional/settings

@franSeva: yeah common in those UIs is they use LanguageManagerInterface::getLanguages() **I think**. So not sure all of them need to be tested, but if there is any technological difference, then it would make sense to test separately. Eg. the language selector on an entity and a language configuration selector on a content type are different "wrapper" codes for eventually calling LanguageManagerInterface::getLanguages(). The former is a language_select (see language_process_language_select()) the later is language_configuration type (language_configuration_element_process()) in form API.

Discussed this with @alexpott today for some guidance:

[12:06pm] GaborHojtsy: alexpott: btw you may be able to help with some guidance in https://www.drupal.org/node/1879930 :)
[12:06pm] Druplicon: https://www.drupal.org/node/1879930 => Language selectors are not showing localized to the page language #1879930: Language selectors are not showing localized to the page language => 30 comments, 8 IRC mentions
[12:07pm] GaborHojtsy: alexpott: this one is about the language selectors generally not showing in the page language, so not the languae switcher where each language was translated to its own native name but language selectors which should be translated to page language :)
[12:07pm] GaborHojtsy: alexpott: they use LanguageManager::getLanguages() which “caches” the language list at first call
[12:07pm] GaborHojtsy: alexpott: which would be so early to be English
[12:07pm] GaborHojtsy: alexpott: and then just keeps returning that
[12:08pm] GaborHojtsy: alexpott: so the general question is to skip this caching altogether and read it from config storage all the time (it has cache) or cache per language :)
[12:08pm] alexpott: GaborHojtsy: remove the cache
[12:08pm] alexpott: GaborHojtsy: and add tests and then if performance here is an issue we'll have to add a proper cache per language
[12:09pm] GaborHojtsy: alexpott: ok
[12:10pm] GaborHojtsy: alexpott: getLanguages() is using similar low level config API that you eradicated from getNativeLanguages :D
[12:10pm] alexpott: GaborHojtsy: yep I was jsut thinking that
[12:11pm] alexpott: GaborHojtsy: ConfigurableLanguage::loadMultiple() - no need for the listAll etc..
[12:12pm] alexpott: GaborHojtsy: also no need for getDefaultLockedLanguages :)
[12:13pm] GaborHojtsy: alexpott: right
[12:13pm] alexpott: GaborHojtsy: also we'll need to refactor the filtering into a protected method for LanguageManager::getLanguages() and ConfigurableLanguageManager::getLanguages() to use
[12:29pm] GaborHojtsy: alexpott: yeah that may or may not need ot happen in this issue, but probably easy enough to do it here
[12:29pm] GaborHojtsy: alexpott: will paste this feedback on the issue :)
[12:30pm] alexpott: GaborHojtsy: i think it does because we call the parent in ConfigurableLanguageManager
[12:30pm] alexpott: GaborHojtsy: which checks $this->languages :)
[12:36pm] GaborHojtsy: alexpott: oh, ok

The patch in #16 is totally outdated so moving to needs work.

Attacht a first test but needs review because is not translating the work English, seems no to find it in translation UI.

1. +++ b/core/modules/language/src/Tests/LanguageSelectorTranslatableTest.php
   @@ -0,0 +1,94 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   

   I don't think these need docs as per our coding standard.

2. +++ b/core/modules/language/src/Tests/LanguageSelectorTranslatableTest.php
   @@ -0,0 +1,94 @@
   +  /**
   +   *
   +   */
   

   This one does though :)

3. +++ b/core/modules/language/src/Tests/LanguageSelectorTranslatableTest.php
   @@ -0,0 +1,94 @@
   +    // Translate the string English in Spanish (Inglés).
   +    $name = 'English';
   +    $name_translation = 'Inglés';
   +    $search = array(
   +      'string' => $name,
   +      'langcode' => 'es',
   +      'translation' => 'untranslated',
   +    );
   +    $this->drupalPostForm('admin/config/regional/translate', $search, t('Filter'));
   +    $this->assertText($name, 'Search found the string as untranslated.');
   +
   +    // Assume this is the only result, given the random name.
   +    $textarea = current($this->xpath('//textarea'));
   +    $lid = (string) $textarea[0]['name'];
   +    $edit = array(
   +      $lid => $name_translation,
   +    );
   +    $this->drupalPostForm('admin/config/regional/translate', $edit, t('Save translations'));
   +    $this->assertText(t('The strings have been saved.'), 'The strings have been saved.');
   

   I think this will not work because the source string DB is empty at this time, so "English" will not be in there. You can just directly translate the English config entity by doing this one line code (untested):

   \Drupal::languageManager()->getLanguageConfigOverride(‘es’, ‘language.entity.en’)->set(‘label’, $translated_name)->save();

4. +++ b/core/modules/language/src/Tests/LanguageSelectorTranslatableTest.php
   @@ -0,0 +1,94 @@
   +    // Get en language from selector
   +    $elements = $this->xpath('//select[@id=:id]//option[@value=:option]', array(':id' => 'edit-settings-node-node-settings-language-langcode', ':option' => 'en'));
   +
   +    // Check the language text is translated.
   +    $this->assertEqual((string) $elements[0], $name_translation, 'Checking the option string English is translated to Spanish.');
   

   This will of course still not pass, because the bug is not yet fixed :)

Yeah as expected it failed to even find/set the translation for English that is due to locale not finding it. Just set it with config translation as suggested above.

@GáborHojtsy I've attached the interdiff and test. I have added an assertion to check the config entity translation for English is correctly set.

This fails on the right thing now, so let's put the fix in place as well :) Read #30 for guidance as to how to fix. Basically getLanguages() should use config API and not cache the language list in the class property. Other methods should call this method when they need the language list.

Other than that, code/grammar cleanup notes:

1. +++ b/core/modules/language/src/Tests/LanguageSelectorTranslatableTest.php
   @@ -0,0 +1,76 @@
   +use Drupal;
   ...
   +    $override = Drupal::languageManager()->getLanguageConfigOverride('es', 'language.entity.en')->set('label', $name_translation)->save();
   

   The Drupal 8 style is not to "use" Drupal but just refer directly to \Drupal::...

2. +++ b/core/modules/language/src/Tests/LanguageSelectorTranslatableTest.php
   @@ -0,0 +1,76 @@
   + * Tests the Content translation settings using the standard profile.
   

   Copy paste? :) It is not using the standard profile certainly.

3. +++ b/core/modules/language/src/Tests/LanguageSelectorTranslatableTest.php
   @@ -0,0 +1,76 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   

   Still don't need docs.

4. +++ b/core/modules/language/src/Tests/LanguageSelectorTranslatableTest.php
   @@ -0,0 +1,76 @@
   +   * Test Content Translation language selectors are correctly translate.
   ...
   +    $this->assertEqual($override->get('label'), $name_translation, 'Language label for English is translate into Spanish.');
   

   translate*d*

I have applied the changes you comment in #39, but phpcs said that should be a comment.

I changed ConfigurableLanguageManager::getLanguages to not use cache ($this->language). It seems to work (see attachment).

I posted in the last patch wrong indentations :S

I have refactored the filtering into a protected function in LanguageManager:: languageFilter (I'm not sure about the naming).
This function is being used in LanguageManager::getLanguages ( $filtered_languages = $this->languageFilter($flags); ).
Also is being used in ConfigurableLanguageManager::getLanguages() through parent::getLanguages($flags);
I've attached a patch with the proposal .

1. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -383,4 +366,38 @@ public function getConfigOverrideLanguage() {
   +   * Filter the full list of languages based on the value of the $all flag.
   

   Filters ...

   $all flag?!

2. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -383,4 +366,38 @@ public function getConfigOverrideLanguage() {
   +   * By default we remove the locked languages.
   

   By default removes the locked languages.

3. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -383,4 +366,38 @@ public function getConfigOverrideLanguage() {
   +  protected function languageFilter($flags = LanguageInterface::STATE_CONFIGURABLE) {
   

   I would make this function take the array and returned the filtered one. Then you can use this in ConfigurableLangaugeManager and LanguageManager equally and NOT call the parent::getLanguages from the configurable one, because there would be no use anymore. That was the point of refactoring to its own function.

Also #2339435: Default no longer needs to be a property on Language or ConfigurableLanguage just got committed, so where the ConfigurableLanguageManager sets $data['default'] that should also be removed. I thought the patch at #2339435: Default no longer needs to be a property on Language or ConfigurableLanguage removed it, but for some reason not.

@Gaborhojtsy
1. Done. $all flags was copy and paste :S
2. Done
3. I have change the function to take 2 arguments, an array with all language (to be filtered), and the $flag, the filter params.

protected function languageFilter(array $languages = array(), $flags = LanguageInterface::STATE_CONFIGURABLE)

I'm not sure if it's necessary force the $languages parameter to be an array, if not will be needed check the parameter to be sure is an array.

About #46 I'm reviewing the patch.

Thanks, moving in the right direction :) We'll need a couple more steps to clean this up, but we are going good :)

1. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -140,7 +140,7 @@ public function getLanguages($flags = LanguageInterface::STATE_CONFIGURABLE) {
   -    $filtered_languages = $this->languageFilter($flags);
   +    $filtered_languages = $this->languageFilter($this->languages, $flags);
    
        return $filtered_languages;
   

   You can return the language list direct like you do in configurable language manager.

2. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -380,9 +383,8 @@ public function getConfigOverrideLanguage() {
   +  protected function languageFilter(array $languages = array(), $flags = LanguageInterface::STATE_CONFIGURABLE) {
   

   I don't think we should default the language list to anything. Its not like someone would call this function without a language list. That would be pointless.

3. +++ b/core/modules/language/src/ConfigurableLanguageManager.php
   @@ -295,7 +295,7 @@ public function getLanguages($flags = LanguageInterface::STATE_CONFIGURABLE) {
        // Sort the language list by weight then title.
        Language::sort($this->languages);
   ...
   -    return parent::getLanguages($flags);
   +    return $this->languageFilter($this->languages, $flags);
   

   I believe we wanted to get rid of $this->languages entirely in the configurable language manager, not just not use it if even if it is set, but not even set it. Now that we are still setting it, it will contain "randomly" localized versions of languages depending on when/how the language list was last invoked. That is not reliable so bet not kept. Just use a local $languages array and do not keep it as a property on the class AFAIS.

@GaborHojtsy I have applied all commented changes or that's what I hope :P

Got close. I think these are the last ones to do, basically do some cleanup :) These are not fixing issues just making the code cleaner.

+++ b/core/modules/language/src/ConfigurableLanguageManager.php
@@ -267,37 +267,35 @@ public function setNegotiator(LanguageNegotiatorInterface $negotiator) {
+    // Prepopulate the language list with the default language to keep things
+    // working even if we have no configuration.
+    $default = $this->getDefaultLanguage();
+    $languages = array($default->id => $default);
 
-      // Add locked languages, they will be filtered later if needed.
-      $this->languages += $this->getDefaultLockedLanguages($weight);
+    // Retrieve the list of languages defined in configuration.
+    $prefix = 'language.entity.';
+    $config_ids = $this->configFactory->listAll($prefix);
 
-      // Sort the language list by weight then title.
-      Language::sort($this->languages);
+    // Instantiate languages from config objects.
+    $weight = 0;
+    foreach ($this->configFactory->loadMultiple($config_ids) as $config) {
+      $data = $config->get();
+      $langcode = $data['id'];
+      // Initialize default property so callers have an easy reference and can
+      // save the same object without data loss.
+      $data['default'] = ($langcode == $default->id);
+      $data['name'] = $data['label'];
+      $languages[$langcode] = new Language($data);
+      $weight = max(array($weight, $languages[$langcode]->weight));
     }
 
-    return parent::getLanguages($flags);
+    // Add locked languages, they will be filtered later if needed.
+    $languages += $this->getDefaultLockedLanguages($weight);

So @alexpott will ask to simplify this even if I don't do it. Better do it ahead :D He had some tips above to make this simpler.

1. Theoretically no need to call getDefaultLockedLanguages at all because there are entities for all locked languages (and they cannot be deleted). Also the language forms ensure those have a higher weight compared to the other languages, so this double ensurance is not needed.

2. The default property is not needed to be set, I think I pointed this out above.

3. When the configurable language manager is used, there should be configurable languages, so the initialization with the default language is probably not needed. If it is needed, it should be done later on a condition if the language list was empty.

3. The lower level config calls would ideally be replaced with a simple entity_load_multiple('configurable_language') which should already have the right weights, good codes and instances of ConfigurableLanguage. You can still sort them with Language::sort() since ConfigurableLanguage also implements LanguageInterface and can be sorted with that.

BTW updateLockedLanguageWeights() takes care of making sure the locked languages are after the configurable ones already (as an addendum to #51/1).

Attach the patch :)

Attach the reroll :p

I'm reviewing this. @mon_franco is going to fix a few small things.

1. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -134,32 +134,13 @@ public function getLanguages($flags = LanguageInterface::STATE_CONFIGURABLE) {
        // Filter the full list of languages based on the value of the $all flag. By
        // default we remove the locked languages, but the caller may request for
        // those languages to be added as well.
   

   I think we dont need this comment, or we can simplify it. Since it is now describing the implementation of languageFilter(). (There used to be a big code hunk this was documenting.)

2. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -383,4 +364,41 @@ public function getConfigOverrideLanguage() {
   +   * @return array
   +   *   An associative array of languages, keyed by the language code.
   +   */
   +  protected function languageFilter(array $languages, $flags = LanguageInterface::STATE_CONFIGURABLE) {
   

   I think we can typehint here LanguageInterface[]

3. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -383,4 +364,41 @@ public function getConfigOverrideLanguage() {
   +      $default = isset($default) ? $default : $this->getDefaultLanguage();
   

   I wonder if we can just do $default = $this->getDefaultLanguage();

   Since this is not part of a larger function anymore, there is no $default that could have already been set.

=====
@fran seva and I also were looking at the test fails. :) Patch coming.

After review the code we think the problem is in the use of $language->name in language_process_language_select.
We should use getName in()stead name because we changed the the way to get the languages in ConfigurableLanguageManager::getLanguages(). Now, we use entity_load_multiple('configurable_language') to get all languages.

@@ -148,7 +148,7 @@ function language_process_language_select($element) {
   if (!isset($element['#options'])) {
     $element['#options'] = array();
     foreach (\Drupal::languageManager()->getLanguages($element['#languages']) as $langcode => $language) {
-      $element['#options'][$langcode] = $language->locked ? t('- @name -', array('@name' => $language-><strong>name</strong>)) : $language-><strong>name</strong>;
+      $element['#options'][$langcode] = $language->locked ? t('- @name -', array('@name' => $language-><strong>getName()</strong>)) : $language-><strong>getName()</strong>;
     }
   }

Hi there, I'm working on this issue.

I'm here at Amsterdam with @YesCT, we have fixed some standars errors and added some comments.

I attach a patch that mix this issue with #2341341: Change public 'name' property access on languages to getName() and add back setName(). The reason is that the current issue needs that all occurrences of ->name have to be changed to use a function to get the language name, ->getLangue(). The issue that do it is #2341341: Change public 'name' property access on languages to getName() and add back setName() so, to check if the changes are correct, I post this patch to check it and continue with the work in case get red.

Attach again :)

This patch needs rerol and changes in:

- core/modules/config_translation/src/FormElement/Textfield.php
- core/modules/language/src/Form/NegotiationUrlForm.php
- core/modules/locale/src/Form/TranslationStatusForm.php
- core/modules/views/src/Plugin/views/PluginBase.php

Attach the reroll. The patch still needs works. I'm on int :)

Attach the changes based in the previous reroll.

Fix up some isLocked() issues.

@martin107 good catch!!
I think the problem is still in the filter function, we have a $default->name and should be $default->get name().
In other hand we are setting name attribute directly and we should use a set function to avoid use the attribute directly.

Nibbling at the edges ... the bulk of the error will remain...

I fixed up another isLocked issue..

There are lots of id issues here ... my gut says postpone until the language module starts using id in a way consistent with the rest of core

and the attached issue is resolved.

Discussed with @franseva, the refactoring takes us so far that its not natural to include it here. There are also other issues for that. So we propose we pursue the fix that does not clear everything up, but fixes the bug at least.

So essentially back to #49 and the rest should be cleaned up later AFAIS.

I've reapplied the patch and as #49 didn't apply, I had to change LanguageManager.php to use

$this->languageFilter($this->languages, $flags);

I've also updated a few calls to the protected language attribute locked with the proper method isLocked.

Patch rerolled for current HEAD.

Also tested the patch, it works for me.

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -150,7 +150,7 @@ public function getLanguages($flags = LanguageInterface::STATE_CONFIGURABLE) {
-      $filtered_languages[LanguageInterface::LANGCODE_SITE_DEFAULT] = $default;
+      $filtered_languages['site_default'] = $default;

This should not be in the reroll, it is not contained in the #88 patch.
Attaching new reroll of #88 patch.

Replacing the magic string in $filtered_languages['site_default'] = $default;.

Screen shots for http://example.com/nl/node/add/article.

Without patch

With patch

I think this changes mostly only the required things for this patch (it does refactor the filtering to its own function which was not strictly required, but we would have made the code more ugly if not doing this). Further refactorings of the entity loading code are in order but as we found after #49, those require far reaching changes in things using this API (due to widespread use of public properties instead of getters). So to do those, we have followup issues, eg. #2341341: Change public 'name' property access on languages to getName() and add back setName().

I opened #2350913: Don't use low level configuration API in ConfigurableLanguageManager::getLanguages for cleaning up the getLanguages() method further, but that may or may not be interpreted as API change depending on how we think of public property access (booo).

The solution here looks good for the bugfix and we have plenty side issues to fix the other aspects, on which #2350913: Don't use low level configuration API in ConfigurableLanguageManager::getLanguages is postponed on. Let's get the fix in.

This patch introduces an interesting conflict between ConfigurableLanguageManager and LanguageManager. LanguageManager has

  /**
   * An array of all the available languages keyed by language code.
   *
   * @var \Drupal\Core\Language\LanguageInterface[]
   */
  protected $languages;

This patch makes this not true for ConfigurableLanguageManager which extends LanguageManager.

Not sure what to do here. I'm also not sure about the performance impact of this patch since we are removing a layer of static caching. The config factory static cache should take the load here (i think). Maybe we should statically cache by language by the current langcode.

boo - I didn't mean to post my own reroll :(

Do we need to get a ConfigurableLanguageManager in testLanguageStringSelector()? (right now, we are getting a LanguageManager)

    $language_manager = \Drupal::languageManager();
    $override = $language_manager->getLanguageConfigOverride('es', 'language.entity.en')->set('label', $name_translation)->save();

@alexpott: we are still returning LanguageInterface instances like before the patch, just instances of a different implementation.

Needs yet another reroll and may still fail on the prior issue.

@alexpott I realise my response to #105 was not actually on topic :D It is true we are not populating the class variable for languages anymore and although the configurable language manage inherits that class variable, we don't deal with it. We can avoid having that class variable in LanguageManager also and just rely on the t() cache, which is pretty much the only thing that it layers on top of.

I will take a look at rerolling this tonight ( say in 3 hours )

Straight reroll...

There was only a little bit of work needed... but it is in such a sensitive area, that I am not confident all tests will pass..

[ the best test for the area of concerned is DRUPAL\TESTS\LANGUAGE\UNIT\CONFIGURABLELANGUAGEUNITTEST which passes locally ]

I will stay on this until green :)

Same fail as above:

Fatal error: Call to a member function getPath() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/config_translation/src/ConfigNamesMapper.php on line 226
FATAL Drupal\language\Tests\LanguageSelectorTranslatableTest: test runner returned a non-zero error code (255).

I think this is fixable easily by not relying on the config translation module (which the test enables but does not actually use). This may be indicative of some bug of route associations in the config translation module but is unrelated to this patch.

1. +++ b/core/modules/language/src/Tests/LanguageSelectorTranslatableTest.php
   @@ -0,0 +1,80 @@
   +    'config_translation',
   

   No need for this module AFAIS

2. +++ b/core/modules/language/src/Tests/LanguageSelectorTranslatableTest.php
   @@ -0,0 +1,80 @@
   +  public $administrator;
   

   Document property.

3. +++ b/core/modules/language/src/Tests/LanguageSelectorTranslatableTest.php
   @@ -0,0 +1,80 @@
   +  }
   +  /**
   

   Add empty line between.

4. +++ b/core/modules/language/src/Tests/LanguageSelectorTranslatableTest.php
   @@ -0,0 +1,80 @@
   +   * Test Content Translation language selectors are correctly translate.
   

   Not correct English. Either "are correctly translatable" or "correctly translate" without the "are".

5. +++ b/core/modules/language/src/Tests/LanguageSelectorTranslatableTest.php
   @@ -0,0 +1,80 @@
   +    // Check label is set correctly.
   +    $this->assertEqual($override->get('label'), $name_translation, 'Language label for English is translated into Spanish.');
   

   I don't think we need to check this here, this has other tests ensuring it works. We can skip this.

6. +++ b/core/modules/language/src/Tests/LanguageSelectorTranslatableTest.php
   @@ -0,0 +1,80 @@
   +    // Check the language text is translated.
   

   "that the language text"

Applied all the fixes from #113.... it appears #113.1 does fix the test ... thank you Gábor..that one would have taken me sometime to see :)

Plus I fixed a couple of minor nit-picks

a) inheritDoc applied to the setUp() function.

b) $overrride assigned but never used!

This should come back green.

@martin107: thanks a lot! Maybe you can also help with the outstanding remark from @alexpott and remove the $languages property on LanguageManager altogether even in the base class (just assemble and return the language list in getLanguages()). Sounds like that is the outstanding task.

Ok, I am on it .... well I will take a look tonight..

1. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -384,4 +366,41 @@ public function getConfigOverrideLanguage() {
   +   * @param array $languages
   +   *    Array with languages to be filtered.
   ...
   +   * @return array
   +   *   An associative array of languages, keyed by the language code.
   

   I think we can typehint with LanguageInterface[]

2. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -384,4 +366,41 @@ public function getConfigOverrideLanguage() {
   +      $default = isset($default) ? $default : $this->getDefaultLanguage();
   

   I remember seeing this pattern before, and I think we decided that there is always a default so we dont need the ternary conditional.

3. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -384,4 +366,41 @@ public function getConfigOverrideLanguage() {
   +      // Rename the default language. But we do not want to do this globally,
   +      // if we're acting on a global object, so clone the object first.
   +      $default = clone $default;
   +      $default->name = $this->t("Site's default language (@lang_name)", array('@lang_name' => $default->name));
   

   and we wont be able to rename it like this once #2341341: Change public 'name' property access on languages to getName() and add back setName() goes in.

   Maybe it is that issue where this section was reworked.

   So, I guess since this is just moving the code...and not making new code, we can leave it. (and reroll either one as needed)

4. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -384,4 +366,41 @@ public function getConfigOverrideLanguage() {
   +    return $filtered_languages;
   +  }
    }
   

   before adding languageFilter() method to the end of this class, there was a newline before the } class closing bracket. Let's keep that newline. https://www.drupal.org/node/608152#indenting

5. +++ b/core/modules/language/src/Tests/LanguageSelectorTranslatableTest.php
   @@ -0,0 +1,86 @@
   +  public static $modules = array(
   

   might as well put a @var doc on this.

6. +++ b/core/modules/language/src/Tests/LanguageSelectorTranslatableTest.php
   @@ -0,0 +1,86 @@
   +    $this->drupalLogin($this->administrator);
   +
   +  }
   

   this newline looks like it is extra.

7. +++ b/core/modules/language/src/Tests/LanguageSelectorTranslatableTest.php
   @@ -0,0 +1,86 @@
   +      array('translate interface',
   +            'administer content translation',
   +            'create content translations',
   +            'update content translations',
   +            'delete content translations',
   +            'administer languages'
   +      )
   

   indent on this is wrong.

8. +++ b/core/modules/language/src/Tests/LanguageSelectorTranslatableTest.php
   @@ -0,0 +1,86 @@
   +  /**
   +   * Test Content Translation language selectors are correctly translatable.
   +   */
   

   this should be third person verb Tests.

WAIT didn't we do these fixes already... in #65 ?

Ah, they were lost in #88

Let's put them back in please. (Go look at #65 I might not have mentioned them all in this comment)

@martin107 also, when you get to this, note #2355573: Use English "or" in some LanguageInterface copy and paste docs. means the similar hunk in here should get corrected also.

@YesCT, I see your changes... but tonight I just want to keep things simple and remove the $languages property from the languageManager, and resolve the caching thing.

So here is my attempt to chart the life-cycle of LanguageManager::languages ( after application of #114 ) based on its read and writes ( as reported phpStorm )
a) It is ONLY ever read within LanguageManager::getLanguages....
b) There seems a redundant reset within ConfigurableLanguageManager::reset()

So its is easy to resolve

From #101
.

Not sure what to do here. I'm also not sure about the performance impact of this patch since we are removing a layer of static caching. The config factory static cache should take the load here (i think). Maybe we should statically cache by language by the current langcode.

So now the question as to whether the caching is adequate, err without any profiling please take my comment with a pinch of salt...

I will try and lay out the arguments so that others can take it apart.

This reduces to finding the potentially expensive operation and caching if appropriate..

So I would rephrase the question to

Does the output of ConfigurableLanguageManager::getLanguages() need to be stored in a cache?

The potentially expensive sequence is :-

listAll 'language.entity' objects
load and initialize all language objects
sort by weight and title
filter by flags

I presume a site with 10 languages would be in the extreme, so the time to execute the all the foreach loops should be small.
Caching the output of getLanguages would have to be keyed by its input $flags, but for say 10 languages and a handful of flag variants it would not increase the memory requirement by much.

In short, given we are already picking values from a config cache, I would be surprised if it is needed. I think It could be done but it depends solely on the number of times getLanguages is called. I can't images more than a few hundred :)

Speed is a concern with D8, its early days, and I think any bottleneck introduced here will easily be spotted by general purpose evidence based hunt.

with regard to #117

1 yep I agree two changes made.
2 (fixed) I also think we always have a default language, but I analyse this slightly differently, $default is a undefined local variable and so only one branch of the ternary operator will ever be used.
3 Yep now that setName() is set to be reintroduced the other issue can use clone again.
4 Fixed
5 Fixed - used conventional boiler plate common to other classes extending WebTestBase
6 Fixed
7 Fixed
8 Fixed
err 9! - Stitch back lost changes from #65 ... sorry busy with other things tonight but... I will come back to this. :)

Good news, #2341341: Change public 'name' property access on languages to getName() and add back setName() landed. "Bad news" is now this can "easily" do the refactoring that we abandoned in #87. So essentially we should revisit the changes between #49 and #87 and apply the simplification of ConfigurableLanguageManager::getLanguages(). With #2341341: Change public 'name' property access on languages to getName() and add back setName() landed, I don't think this would be accepted without that :/

Ok...thats not somthing I am very familiar with, so someone else should pick up the batton

first a reroll

So I basically went through #49-#87, tried to find the changes there from the patches and applied them on top of my #123 reroll.

Things I found:
- #54 Simplification of ConfigurableLanguageManager::getLanguages()
- #65 some codestyle adaptions

Some things which where in this range where already fixed:
- #81
- #84

Let's see what testbot thinks.

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -382,4 +354,40 @@ public function getConfigOverrideLanguage() {
+  protected function languageFilter(array $languages, $flags = LanguageInterface::STATE_CONFIGURABLE) {

maybe we should name this filterLanguages()

a verb for what it does.

Agreed with YesCT, but it would also be vital to resolve the fails :) Looking at the more serious ones, looks like there is circular dependency with the following test preinstall hook:

function language_test_module_preinstall() {
  \Drupal::state()->set('language_test.language_count_preinstall', count(\Drupal::languageManager()->getLanguages()));
}

Given this is testing code, I think we can skip this and just use the config API directly to gather the list of languages from config. We only care about the count here. Something like:

function language_test_module_preinstall() {
  \Drupal::state()->set('language_test.language_count_preinstall', count(\Drupal::configFactory->listAll('language.entity')));
}

The interdiff included the configurable language manage change but the patch did not. Duh. So this should have been 129.

Adding the default languages is not necessary in the configurable language manager because we get them from config storage. But required in the non-configurable one because we don't have any storage to get them from. This is just two lines of code that was there before the patch, we need to keep them. This leads to lots of fails around 'und' missing I believe.

The LanguageListModuleInstallTest fails on something that is not anymore true. We need to dig up why it is testing calling the language list when language module is installed, this will not work with this patch... Need to dig into the blame log. We may need to add some protection for this case.

#2166915: Remove uses of deprecated language functions in tests and procedural code just did

-    $this->assertEqual(\Drupal::state()->get('language_test.language_count_preinstall', 0), 1, 'Using language_list() returns 1 language during Language installation.');
+    $this->assertEqual(\Drupal::state()->get('language_test.language_count_preinstall', 0), 1, 'Using LanguageManager::getLanguages() returns 1 language during Language installation.');

before that, #2171015-7: Drupal 8 HEAD broken: installing Language module fails, after that cannot install any other module is the one that added the whole test.

Yeah we need to solve that. In fact that is the reason for other fails also. So better solve it now. The configurable language manager needs to be more graceful for when the entity type is not yet there or when the default configuration is not yet imported. That is what that test is about. So before this patch we had the default language and the locked languages in the list even if there was no configurable language installed yet and did not use the entity API to avoid that the entity may not be there. A more elegant way is to catch the exception for the missing entity and fall back on the default list if we got that exception or there was no default config yet. So no need to change the test module. This should fix a few things.

So yay only one remaining fail! That test times out for me badly even if I only run the setUp() or even only parts of setUp(). Some help here would be highly appreciated.

Well, the testbot has similar problems:

Fatal error: Allowed memory size of 335544320 bytes exhausted (tried to allocate 30211 bytes) in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Cache/DatabaseBackend.php on line 85
FATAL Drupal\comment\Tests\CommentLanguageTest: test runner returned a non-zero error code (255).

Probably an infinite loop in setting up the test somewhere...

So the reason is comment_test has a hook_entity_info_alter() which uses languageManager::isMultilingual() which uses getLanguages() which tries to load the languages as entities, which then invokes the entity API which then invokes alters, which.... #endlessloop

We can retain a static variable to not let us be invoked from that cycle, which will let the altering work only in cases they were not known to come from THIS cycle. I think its slightly dangerous that an innocent looking use of the language manager in an entity API hook will result in an infinite loop but if we want to use the entity API to load the language list, which I believe we do, then it has these side effects...

The attached patch does not time out anymore for me locally, let's see.

Talked to @alexpott about the use of entity API resulting in circular dependencies, and he said we should back out of that. That we did already on this issue before, so no problem...

[2:45pm] alexpott: GaborHojtsy: will need a nice big comment
[2:45pm] GaborHojtsy: alexpott: otherwise https://www.drupal.org/files/issues/interdiff_7674.txt :/
[2:47pm] alexpott: GaborHojtsy: I think we will have to eschew using the config entities in the Manager
[2:48pm] GaborHojtsy: alexpott: makes sense cool :)
[2:48pm] alexpott: GaborHojtsy: the language manager is too low level
[2:53pm] GaborHojtsy: alexpott: superb, thanks
[2:59pm] alexpott: GaborHojtsy: justification is that the entity system depends on Language
[2:59pm] alexpott: GaborHojtsy: therefore Language can not depend on the entity system
[3:00pm] alexpott: GaborHojtsy: GaborHojtsy fortunately there is CMI which both can depend on

So going back to using the config factory directly and not instantiating config entities on this low level... That is what it takes. Now has a lengthy comment on why all the strange looking things happen. Now of course requires no changes to the comment test module because there is no circular dependency.

I'll review this now.

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -130,35 +123,17 @@ public function getDefaultLanguage() {
-    if (!isset($this->languages)) {
-      // No language module, so use the default language only.
...
+    // No language module, so use the default language only.
+    $default = $this->getDefaultLanguage();

This comment made more sense when it was after the condition !isset($this-languages).

And.. we dont really only have the default as we add the system ones in the next lines.

If this language manager is used, then there are no configurable languages / no language module. So the comment is correct for what is going on there. It could indeed be better. Attached updates to comments on this method.

So one final thing I noticed here is if we call the parent getLanguages() to avoid needing to copy that logic (like it was before), then the filtering will run twice, and the default language may be added twice also. That is not very elegant. It does not break anything, but does a tiny bit more work then needed. So we can just call the parent with a STATE_ALL and not even go into filtering if STATE_ALL was provided. That will result in filtering the language list at the end only once.

wow. that's a really great catch!

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -131,7 +131,7 @@ public function getLanguages($flags = LanguageInterface::STATE_CONFIGURABLE) {
-    return $this->filterLanguages($languages, $flags);
+    return $flags == LanguageInterface::STATE_ALL ? $languages : $this->filterLanguages($languages, $flags);

if it is still "correct" I think this would be better to have the logic in the method. that way we dont have to copy and paste this logic around whenever we call the filterLanguage().

and putting it there give us a bit more room where the code might be more readable, and we can add a comment. :)

We can centralize that sure. That means the method will be called but skipped returned from immediately. Not calling that would be a microoptimization that is probably not worth the copy-paste hassle.

read the whole patch a couple times. (found a few nits in new/changed lines and fixed them. they are small enough I'm comfortable rtbc'ing)

the changes make sense to me.
and the comments explain well what is going on.
the test looks like it is testing the bug well.

uploading a test only patch.

that fails perfectly. :) good.

The changes looks good:
1. The typo translate has been fixed to translated in line 69 in LanguageSelectorTranslatableTest
2. At a comma at last perm defined in the test in line 63 in LanguageSelectorTranslatableTest
3. Remove an empty line in a comment before describe the comments in line 367 in LanguageManager

RTBC for me

The only performance regression here is the repeated language sorting. I think this is avoidable. In testing I discovered that with standard install and language module enabled - without any content and 20 languages created - the getLanguages method is called 24 times alone on the frontpage.

Whilst I think that xhprof is amplifying the results there is no doubt that the patch in #149 is a performance regression. See xhprof screenshots.

Patch also ensures that properties like the UUID are not transferred to the runtime language object (thus saving memory and preventing code from abusing the constructor to add random stuff to the object).

Additionally unlike HEAD I've chosen to cache by flags as well since language objects are tiny and looping again and again over the array to remove the same non configurable languages all the time seems pointless.

I think the remaining task is to improve my comment on

  /**
   * An array of arrays the available languages keyed by language code. The top
   * level array is keyed by the language the language names are translated in.
   * The next level is keyed by the flags provided to self::getLanguages().
   *
   * @var \Drupal\Core\Language\LanguageInterface[]
   *
   * @see \Drupal\Core\Language\LanguageManager::getLanguages()
   */
  protected $languages = array();

Maybe this is a situation where less is more - something along the lines of A static cache of the result of self::getLanguages() keyed by language and the flags provided to the method.

Adding promise xhprof results :)

• 149
• 153

I think using the nested array structure in getLanguages() all the time makes the code significantly harder to read.

Also you copied the parent code to the extending class, which is fine now that the caching is key. But that made the filtering run twice now, once for the base languages, then for the complete list again. It is enough to run once.

As for the property comment, I think we can do a shorter and a longer version as well :) Best of both worlds?

#155 Nice! like those changes. I was concentrating on getting it working :) and really nice catch on the double filtering.

@YesCT found that the comments in the configurable language manager now exceeded 80 chars. Just rewrapped that. No other changes.

I read the interdiffs and the whole patch again.
the caching and the changes since then look great.

Overall,it looks very nice (esp. filterLanguages method).

one minor question/concern.

+++ b/core/lib/Drupal/Core/Language/Language.php
@@ -80,7 +80,9 @@ class Language implements LanguageInterface {
+      if (property_exists($this, $key)) {
+        $this->{$key} = $value;
+      }

Go silent, if the property doesn't exist? also wonder how it become part of this issue scope.

@vijaycs85: that came in to optimise the size of arrays cached by the ConfigurableLanguageManager. The language objects would get data that should not be present in the runtime language object like uuid and label (while the object uses 'name'). This change ensures only keys that are actually on the object will get populated, resulting in more controlled size arrays to cache. Tests are also added to cover this optimisation.

+1 for committing this as soon as possible.

I didn't test the bug reported here, but this patch also fixes the language listing part of #2310735: Advanced search form doesn't translate languages and node type names.

I had minor questions (same as vijaycs85), and the circular dependency workaround is saddening but we know there are other issues trying to resolve that.

Committed/pushed to 8.0.x, thanks!

Amazing, thanks!

Regarding comment #166, that seems like a completely different issue to me.