Posted by xjm

Problem/Motivation

When adding or editing a node the language name in the content language selector is not translated to the local language.

This is a followup issue for #1535868: Convert all blocks into plugins.

Caused by: Languages are in English because the language names are cached in the ConfigurableLanguageManager and it is first invoked so early in the request before the language is negotiated, that we have no chance to have it in the right language if we keep caching like this. Also @alexpott suggested we just rely on the config factory cache.

To reproduce:
Setup Drupal with modules: Language, Content translation, Configuration translation

  1. Add a second langauge.
  2. Translate the Language names of both languages.
  3. Enable the "Show language selector on create and edit pages" setting for Article content types (admin/structure/types/manage/article).
  4. Add an article node in the default language (http://example.com/node/add/article) and observe the languages in the Language select list on the node form. Language names should be in default language.
  5. Add an article node in the other language (http://example.com/[language code]/node/add/article) and observe the languages in the Language select list on the node form. Language names should be in the interface language.

Proposed resolution

Do not cache in the ConfigurableLanguageManager, rely on config factory cache instead.

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

Without patch

With patch

Files: 
CommentFileSizeAuthor
#157 interdiff.txt2.47 KBGábor Hojtsy
#157 1879930.157.patch13.71 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,912 pass(es). View
#155 interdiff.txt4.68 KBGábor Hojtsy
#155 1879930.155.patch13.69 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,710 pass(es). View
#153 1879930-2.153.patch14.05 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,718 pass(es). View
#153 149-153-interdiff.txt8.52 KBalexpott
#149 selectors-1879930-149.patch10.99 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,725 pass(es). View
#149 interdiff-1879930.148.149.txt1.44 KBYesCT
#149 selectors-1879930-149-testonly.patch2.78 KBYesCT
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,569 pass(es), 1 fail(s), and 0 exception(s). View
#148 interdiff.txt2.06 KBGábor Hojtsy
#148 selectors-1879930-148.patch10.99 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,602 pass(es). View
#146 interdiff.txt1.92 KBGábor Hojtsy
#146 selectors-1879930-146.patch10.95 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,602 pass(es). View
#145 interdiff.txt1.22 KBGábor Hojtsy
#145 selectors-1879930-145.patch10.82 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,730 pass(es). View
#142 interdiff.txt4.47 KBGábor Hojtsy
#142 selectors-1879930-142.patch10.72 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,732 pass(es). View
#141 interdiff.txt1.45 KBGábor Hojtsy
#141 selectors-1879930-141.patch11.4 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,727 pass(es). View
#137 interdiff.txt2.32 KBGábor Hojtsy
#137 selectors-1879930-137.patch9.95 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,604 pass(es), 1 fail(s), and 0 exception(s). View
#133 interdiff.txt817 bytesGábor Hojtsy
#133 selectors-1879930-133.patch10.02 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,429 pass(es), 12 fail(s), and 2 exception(s). View
#131 selectors-1879930-131.patch9.87 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,190 pass(es), 13 fail(s), and 16 exception(s). View
#129 interdiff.txt2.56 KBGábor Hojtsy
#129 selectors-1879930-129.patch9.87 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,906 pass(es), 832 fail(s), and 15 exception(s). View
#125 interdiff-123-125.patch5.04 KBSchnitzel
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch interdiff-123-125.patch. Unable to apply patch. See the log in the details link for more information. View
#125 selectors-1879930-125.patch9.13 KBSchnitzel
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,576 pass(es), 18 fail(s), and 21 exception(s). View
#123 selectors-1879930-123.patch10.25 KBSchnitzel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,520 pass(es). View
#120 interdiff-119-120.txt3.23 KBmartin107
#120 selectors-1879930-120.patch10.48 KBmartin107
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,508 pass(es). View
#119 selectors-1879930-118.patch10.39 KBmartin107
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,491 pass(es). View
#119 interdiff-114-118.txt2.39 KBmartin107
#114 interdiff-111-114.txt2.58 KBmartin107
#114 selectors-1879930-114.patch8.96 KBmartin107
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,423 pass(es). View
#111 selectors-1879930-111.patch9.02 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,411 pass(es), 1 fail(s), and 0 exception(s). View
#101 1879930.101.patch9 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1879930.101.patch. Unable to apply patch. See the log in the details link for more information. View
#98 1879930-with-patch-98.png63.44 KBSutharsan
#98 1879930-without-patch-98.png60.89 KBSutharsan
#96 language-selector-translation-1879930-96.patch8.99 KBSutharsan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,049 pass(es). View
#96 interdiff-1879930-95-96.txt911 bytesSutharsan
#95 language-selector-translation-1879930-95.patch8.97 KBSutharsan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,063 pass(es), 1 fail(s), and 0 exception(s). View
#81 interdiff-76-81.txt798 bytesmartin107
#81 1879930.65-mixed-2326885-81.patch49 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#76 1879930.65-mixed-2326885-76.patch48.99 KBfran seva
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#74 reroll-1879930.65-mixed-2326885-71.patch45.88 KBfran seva
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#71 1879930.65-mixed-2326885-71.patch45.86 KBfran seva
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,023 pass(es), 15 fail(s), and 378 exception(s). View
#68 1879930.65-mixed-2326885.patch42.18 KBfran seva
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1879930.65-mixed-2326885.patch. Unable to apply patch. See the log in the details link for more information. View
#65 interdiff.1879930.59.65.txt5.73 KBmon_franco
#65 1879930.65.patch10.18 KBmon_franco
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,201 pass(es), 76 fail(s), and 3,178 exception(s). View
#60 interdiff-1879930-56-59.txt827 bytesfran seva
#60 language-selector-translation-1879930-59.patch10.05 KBfran seva
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,108 pass(es), 73 fail(s), and 3,046 exception(s). View
#56 language-selector-translation-1879930-56.patch9.25 KBfran seva
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,650 pass(es), 96 fail(s), and 4,325 exception(s). View
#54 interdiff-1879930-49-54.txt3.34 KBfran seva
#54 language-selector-translation-1879930-54.patch9.1 KBfran seva
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch language-selector-translation-1879930-54.patch. Unable to apply patch. See the log in the details link for more information. View
#49 interdiff-1879930-47-49.txt2.96 KBfran seva
#49 language-selector-translation-1879930-49.patch9.09 KBfran seva
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,649 pass(es). View
#47 interdiff-1879930-44-47.txt2.93 KBfran seva
#47 language-selector-translation-1879930-47.patch9.14 KBfran seva
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,363 pass(es). View
#44 interdiff-1879930-43-44.txt3.23 KBfran seva
#44 language-selector-translation-1879930-44.patch8.96 KBfran seva
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,666 pass(es). View
#43 interdiff-1879930-42-43.txt2.91 KBfran seva
#43 language-selector-translation-1879930-43.patch5.72 KBfran seva
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,666 pass(es). View
#42 add_content_form_check.png32.2 KBfran seva
#42 content_translation_overview_check.png60.38 KBfran seva
#42 interdiff-1879930-40-42.txt961 bytesfran seva
#42 language-selector-translation-1879930-42.patch3.71 KBfran seva
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,611 pass(es). View
#40 interdiff-1879930-36-40.txt2.99 KBfran seva
#40 language-selector-translation-1879930-40.patch2.77 KBfran seva
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,567 pass(es), 1 fail(s), and 0 exception(s). View
#36 language-selector-translation-1879930-36.patch2.73 KBfran seva
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,348 pass(es), 1 fail(s), and 0 exception(s). View
#36 interdiff-1879930-31-36.txt3.82 KBfran seva
#31 language-selector-translation-1879930-31.patch3.12 KBfran seva
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,333 pass(es), 6 fail(s), and 0 exception(s). View
#28 regional-settings.png44.36 KBfran seva
#24 block-place.png196.48 KBYesCT
#24 article-select.png174.12 KBYesCT
#24 content-langauge.png261.33 KBYesCT
#24 language.png297.08 KBYesCT
#16 block-translate_language-1879930-16.patch906 bytesherom
PASSED: [[SimpleTest]]: [MySQL] 59,652 pass(es). View
#10 block-translate_language-1879930-10.patch861 bytesmarkie
PASSED: [[SimpleTest]]: [MySQL] 59,350 pass(es). View
#7 block-translate_language-1879930-7.patch1.19 KBmarkie
PASSED: [[SimpleTest]]: [MySQL] 58,572 pass(es). View
#6 block-translate_language-1879930-6.patch1.17 KBmarkie
PASSED: [[SimpleTest]]: [MySQL] 58,555 pass(es). View
#4 block-translate_language-1879930-4.patch1.23 KBmarkie
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-translate_language-1879930-4.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

xjm’s picture

Status: Postponed » Active
xjm’s picture

Issue tags: +Block plugins
jibran’s picture

Title: Translate language names in BlockBase::form() » Translate language names in BlockFormController::form()
+++ 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.

markie’s picture

Status: Active » Needs review
FileSize
1.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-translate_language-1879930-4.patch. Unable to apply patch. See the log in the details link for more information. View

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

Status: Needs review » Needs work

The last submitted patch, block-translate_language-1879930-4.patch, failed testing.

markie’s picture

Status: Needs work » Needs review
FileSize
1.17 KB
PASSED: [[SimpleTest]]: [MySQL] 58,555 pass(es). View

Reroll..

markie’s picture

FileSize
1.19 KB
PASSED: [[SimpleTest]]: [MySQL] 58,572 pass(es). View

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

benjy’s picture

Status: Needs review » Needs work
+++ 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()

YesCT’s picture

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

markie’s picture

FileSize
861 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,350 pass(es). View

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

markie’s picture

Status: Needs work » Needs review

run, patch-test, run!

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +sprint, +language-config

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.

markie’s picture

Assigned: Unassigned » markie

at badcamp sprint to finish this off.

markie’s picture

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

herom’s picture

Status: Needs work » Needs review
FileSize
906 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,652 pass(es). View

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

herom’s picture

Issue summary: View changes

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

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs review » Needs work

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.

Gábor Hojtsy’s picture

Issue tags: +Configuration context
penyaskito’s picture

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

Status: Needs work » Postponed

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.

Gábor Hojtsy’s picture

Issue tags: -sprint

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

Gábor Hojtsy’s picture

Title: Translate language names in BlockFormController::form() » Language selectors are not showing localized to the page language
Status: Postponed » Needs work
Issue tags: -Blocks-Layouts, -Block plugins, -Configuration context +sprint

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

YesCT’s picture

FileSize
297.08 KB
261.33 KB
174.12 KB
196.48 KB

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.

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

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.

YesCT’s picture

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.

fran seva’s picture

FileSize
44.36 KB

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

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Status: Needs work » Active

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.

fran seva’s picture

FileSize
3.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,333 pass(es), 6 fail(s), and 0 exception(s). View

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

fran seva’s picture

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

Status: Needs review » Needs work
  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 :)

The last submitted patch, 31: language-selector-translation-1879930-31.patch, failed testing.

Gábor Hojtsy’s picture

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.

fran seva’s picture

FileSize
3.82 KB
2.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,348 pass(es), 1 fail(s), and 0 exception(s). View

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

fran seva’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 36: language-selector-translation-1879930-36.patch, failed testing.

Gábor Hojtsy’s picture

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*

fran seva’s picture

Status: Needs work » Needs review
FileSize
2.77 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,567 pass(es), 1 fail(s), and 0 exception(s). View
2.99 KB

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

Status: Needs review » Needs work

The last submitted patch, 40: language-selector-translation-1879930-40.patch, failed testing.

fran seva’s picture

Status: Needs work » Needs review
FileSize
3.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,611 pass(es). View
961 bytes
60.38 KB
32.2 KB

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

fran seva’s picture

FileSize
5.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,666 pass(es). View
2.91 KB

I posted in the last patch wrong indentations :S

fran seva’s picture

FileSize
8.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,666 pass(es). View
3.23 KB

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 .

Gábor Hojtsy’s picture

Status: Needs review » Needs work
  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.

Gábor Hojtsy’s picture

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.

fran seva’s picture

Status: Needs work » Needs review
FileSize
9.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,363 pass(es). View
2.93 KB

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

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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.

fran seva’s picture

FileSize
9.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,649 pass(es). View
2.96 KB

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

fran seva’s picture

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

Status: Needs review » Needs work

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.

Gábor Hojtsy’s picture

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

fran seva’s picture

Issue tags: +Amsterdam2014
fran seva’s picture

Status: Needs work » Needs review
FileSize
9.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch language-selector-translation-1879930-54.patch. Unable to apply patch. See the log in the details link for more information. View
3.34 KB

Attach the patch :)

Status: Needs review » Needs work

The last submitted patch, 54: language-selector-translation-1879930-54.patch, failed testing.

fran seva’s picture

Status: Needs work » Needs review
FileSize
9.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,650 pass(es), 96 fail(s), and 4,325 exception(s). View

Attach the reroll :p

YesCT’s picture

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

Status: Needs review » Needs work

The last submitted patch, 56: language-selector-translation-1879930-56.patch, failed testing.

YesCT’s picture

  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.

fran seva’s picture

FileSize
10.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,108 pass(es), 73 fail(s), and 3,046 exception(s). View
827 bytes

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>;
     }
   }
fran seva’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 60: language-selector-translation-1879930-59.patch, failed testing.

mon_franco’s picture

Hi there, I'm working on this issue.

mon_franco’s picture

Assigned: markie » mon_franco
mon_franco’s picture

FileSize
5.73 KB
10.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,201 pass(es), 76 fail(s), and 3,178 exception(s). View
5.73 KB

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

mon_franco’s picture

mon_franco’s picture

Assigned: mon_franco » Unassigned
Status: Needs work » Needs review
fran seva’s picture

FileSize
42.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1879930.65-mixed-2326885.patch. Unable to apply patch. See the log in the details link for more information. View

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.

Status: Needs review » Needs work

The last submitted patch, 68: 1879930.65-mixed-2326885.patch, failed testing.

The last submitted patch, 65: 1879930.65.patch, failed testing.

fran seva’s picture

Status: Needs work » Needs review
FileSize
45.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,023 pass(es), 15 fail(s), and 378 exception(s). View

Attach again :)

Status: Needs review » Needs work

The last submitted patch, 71: 1879930.65-mixed-2326885-71.patch, failed testing.

fran seva’s picture

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

fran seva’s picture

FileSize
45.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

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

fran seva’s picture

Status: Needs work » Needs review
fran seva’s picture

FileSize
48.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Attach the changes based in the previous reroll.

The last submitted patch, 74: reroll-1879930.65-mixed-2326885-71.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 76: 1879930.65-mixed-2326885-76.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 76: 1879930.65-mixed-2326885-76.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
798 bytes

Fix up some isLocked() issues.

Status: Needs review » Needs work

The last submitted patch, 81: 1879930.65-mixed-2326885-81.patch, failed testing.

fran seva’s picture

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

martin107’s picture

Status: Needs work » Needs review
Related issues: +#2340667: Protect Drupal\Core\Language\Language::id, and use getId()
FileSize
49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
969 bytes

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.

Status: Needs review » Needs work

The last submitted patch, 84: 1879930.65-mixed-2326885-84.patch, failed testing.

mgifford’s picture

Gábor Hojtsy’s picture

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.

spearhead93’s picture

FileSize
8.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch language-selector-translation-1879930-88.patch. Unable to apply patch. See the log in the details link for more information. View

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.

spearhead93’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 88: language-selector-translation-1879930-88.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 88: language-selector-translation-1879930-88.patch, failed testing.

Désiré’s picture

Status: Needs work » Needs review
FileSize
8.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,974 pass(es). View

Patch rerolled for current HEAD.

Désiré’s picture

Also tested the patch, it works for me.

Sutharsan’s picture

Status: Needs review » Needs work
FileSize
8.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,063 pass(es), 1 fail(s), and 0 exception(s). View
+++ 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.

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
911 bytes
8.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,049 pass(es). View

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

Sutharsan’s picture

Issue summary: View changes
Sutharsan’s picture

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

Without patch

With patch

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

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.

The last submitted patch, 95: language-selector-translation-1879930-95.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
FileSize
9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1879930.101.patch. Unable to apply patch. See the log in the details link for more information. View

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.

alexpott’s picture

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

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

Status: Needs review » Needs work

The last submitted patch, 101: 1879930.101.patch, failed testing.

YesCT’s picture

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();
Gábor Hojtsy’s picture

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

Status: Needs work » Needs review

Gábor Hojtsy queued 101: 1879930.101.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 101: 1879930.101.patch, failed testing.

Gábor Hojtsy’s picture

Component: block.module » language system
Priority: Normal » Major
Issue tags: +Needs reroll

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

Gábor Hojtsy’s picture

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

martin107’s picture

Assigned: Unassigned » martin107

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

martin107’s picture

Status: Needs work » Needs review
FileSize
9.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,411 pass(es), 1 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 111: selectors-1879930-111.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: -Needs reroll

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"

martin107’s picture

Assigned: martin107 » Unassigned
Status: Needs work » Needs review
FileSize
8.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,423 pass(es). View
2.58 KB

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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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

martin107’s picture

Assigned: Unassigned » martin107

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

YesCT’s picture

  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)

YesCT’s picture

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

martin107’s picture

Status: Needs work » Needs review
FileSize
2.39 KB
10.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,491 pass(es). View

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

martin107’s picture

FileSize
10.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,508 pass(es). View
3.23 KB

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. :)

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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 :/

martin107’s picture

Assigned: martin107 » Unassigned

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

Schnitzel’s picture

FileSize
10.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,520 pass(es). View

first a reroll

Schnitzel’s picture

Status: Needs work » Needs review
Schnitzel’s picture

FileSize
9.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,576 pass(es), 18 fail(s), and 21 exception(s). View
5.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch interdiff-123-125.patch. Unable to apply patch. See the log in the details link for more information. View

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.

Status: Needs review » Needs work

The last submitted patch, 125: interdiff-123-125.patch, failed testing.

The last submitted patch, 125: selectors-1879930-125.patch, failed testing.

YesCT’s picture

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
9.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,906 pass(es), 832 fail(s), and 15 exception(s). View
2.56 KB

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

Status: Needs review » Needs work

The last submitted patch, 129: selectors-1879930-129.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
9.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,190 pass(es), 13 fail(s), and 16 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 131: selectors-1879930-131.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
10.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,429 pass(es), 12 fail(s), and 2 exception(s). View
817 bytes

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.

Status: Needs review » Needs work

The last submitted patch, 133: selectors-1879930-133.patch, failed testing.

Gábor Hojtsy’s picture

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.

YesCT’s picture

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
9.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,604 pass(es), 1 fail(s), and 0 exception(s). View
2.32 KB

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.

Status: Needs review » Needs work

The last submitted patch, 137: selectors-1879930-137.patch, failed testing.

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
11.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,727 pass(es). View
1.45 KB

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.

Gábor Hojtsy’s picture

FileSize
10.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,732 pass(es). View
4.47 KB

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.

YesCT’s picture

I'll review this now.

YesCT’s picture

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

Gábor Hojtsy’s picture

FileSize
10.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,730 pass(es). View
1.22 KB

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.

Gábor Hojtsy’s picture

FileSize
10.95 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,602 pass(es). View
1.92 KB

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.

YesCT’s picture

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. :)

Gábor Hojtsy’s picture

FileSize
10.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,602 pass(es). View
2.06 KB

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.

YesCT’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,569 pass(es), 1 fail(s), and 0 exception(s). View
1.44 KB
10.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,725 pass(es). View

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.

The last submitted patch, 149: selectors-1879930-149-testonly.patch, failed testing.

YesCT’s picture

that fails perfectly. :) good.

fran seva’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
8.52 KB
14.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,718 pass(es). View

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.

alexpott’s picture

Adding promise xhprof results :)

Gábor Hojtsy’s picture

FileSize
13.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,710 pass(es). View
4.68 KB

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?

alexpott’s picture

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

Gábor Hojtsy’s picture

FileSize
13.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,912 pass(es). View
2.47 KB

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

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

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

vijaycs85’s picture

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.

Gábor Hojtsy’s picture

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

jhodgdon’s picture

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

  • catch committed 51fcd31 on
    Issue #1879930 by fran seva, Gábor Hojtsy, martin107, markie, Schnitzel...
catch’s picture

Status: Reviewed & tested by the community » Fixed

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!

Gábor Hojtsy’s picture

Issue tags: -sprint

Amazing, thanks!

Status: Fixed » Closed (fixed)

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

20 Drei’s picture

Hello, I guess I ran into this bug. I installed the current Drupal release. The translated language names don't show up. Please see the description of my issue here:

https://www.drupal.org/node/2858769

How do I not chache the ConfigurableLanguageManager?

jhodgdon’s picture

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