Problem/Motivation

In Drupal 7, the language names in the language list show up as localized to their native names. In Drupal 8 native names don't exist anymore as a concept, however language names are translated to any other language just as well as any other configuration entity. We should restore the behavior of the language switcher block to use the localized language name for that language through a specific language context.

Proposed resolution

Make language list display in language switcher block use the localized language names by loading the language objects in the specific language context from config. This is the same as the Drupal 7 behaviour.

Remaining tasks

Review. Commit.

How to test

  1. Install drupal 8 with default (English) language + standard profile.
  2. Enable language & configuration translation module
  3. Add one or two language from admin/config/regional/language/add
  4. Go to admin/config/regional/language and select 'translate' from the dropdown of the language you want to translate.
  5. Assign language switched block to 'sidebar first' region
  6. Now you can see the localized name (i.e. translated string).

User interface changes

- Language names in language switchers are localized to their respective language (if translation available)

API changes

None.

- #1498880: Theme language switcher for seven theme
- Follow-up: #2135379: Make the language switcher configurable and extensible
- Follow-up: #2232375: Make language switcher block cacheable

CommentFileSizeAuthor
#148 2107427-language-switcher-labels-148.patch10.78 KBGábor Hojtsy
#145 2107427-language-switcher-labels-145.patch10.8 KBGábor Hojtsy
#145 interdiff.txt585 bytesGábor Hojtsy
#143 2107427-language-switcher-labels-143.patch10.8 KBGábor Hojtsy
#143 interdiff.txt585 bytesGábor Hojtsy
#135 languages_added_end.png263.56 KBYesCT
#133 2107427-language-switcher-labels.interdiff.131-133.txt402 bytespenyaskito
#133 2107427-language-switcher-labels-133.patch11.81 KBpenyaskito
#131 2107427-language-switcher-labels.interdiff.127-131.txt1.58 KBpenyaskito
#131 2107427-language-switcher-labels-131.patch11.65 KBpenyaskito
#127 2107427-language-switcher-labels.interdiff.124-127.txt2.32 KBpenyaskito
#127 2107427-language-switcher-labels-127.patch10.83 KBpenyaskito
#124 2107427-language-switcher-labels.interdiff.122-124.txt2.52 KBpenyaskito
#124 2107427-language-switcher-labels-124.patch10.86 KBpenyaskito
#122 2107427-language-switcher-labels.interdiff.119-122.txt1.42 KBpenyaskito
#122 2107427-language-switcher-labels-122.patch11.18 KBpenyaskito
#119 2107427-language-switcher-labels-119.patch11.41 KBpenyaskito
#111 2107427-language-switcher-labels.interdiff.102-111.txt3.36 KBpenyaskito
#111 2107427-language-switcher-labels-111.patch11.39 KBpenyaskito
#102 2107427-language-switcher-labels.interdiff.100-102.txt3.54 KBpenyaskito
#102 2107427-language-switcher-labels-102.patch11.01 KBpenyaskito
#100 2107427-language-switcher-labels-100.patch11.07 KBpenyaskito
#89 2107427-language-switcher-labels-89.patch9.64 KBmashermike
#87 interdiff.2107427-86-87.txt1.65 KBmashermike
#87 2107427-language-switcher-labels-87.patch9.64 KBmashermike
#86 2107427-language-switcher-labels-86.patch11.29 KBYesCT
#79 interdiff.txt1.65 KBWim Leers
#79 2107427-language-switcher-labels-78.patch11.96 KBWim Leers
#75 post-patch.png27.81 KBAron Novak
#74 pre-patch-db.png30.04 KBAron Novak
#74 post-patch-getNative.png27.03 KBAron Novak
#74 post-patch-db.png24.49 KBAron Novak
#74 pre-patch.png27.33 KBAron Novak
#74 post-patch-db.png24.49 KBAron Novak
#74 pre-ab-2.txt1.25 KBAron Novak
#74 pre-ab-1.txt1.25 KBAron Novak
#74 post-ab-2.txt1.25 KBAron Novak
#74 post-ab-1.txt1.25 KBAron Novak
#74 profile_diff.txt.gz699.94 KBAron Novak
#74 cachegrind.d8_local.1396210387.txt.gz753.67 KBAron Novak
#74 cachegrind.d8_local.1396210045.txt.gz751.05 KBAron Novak
#69 interdiff-2107427-57-69.txt1.36 KBmikispeed
#69 2107427-language-switcher-labels-69.patch11.38 KBmikispeed
#62 Screen Shot 2014-03-23 at 20.13.00.png28.64 KBvijaycs85
#62 Screen Shot 2014-03-23 at 20.13.22.png29.1 KBvijaycs85
#62 2107427-diff-57-61.txt765 bytesvijaycs85
#62 2107427-language-switcher-62.patch11.3 KBvijaycs85
#57 interdiff-2107427-52-57.txt6.3 KBYesCT
#57 2107427-language-switcher-labels-57.patch11.38 KBYesCT
#52 2107427-language-switcher-labels-52.patch11.07 KBdclavain
#52 interdiff.txt3.21 KBdclavain
#48 2107427-language-switcher-labels-48.patch24.99 KBdclavain
#48 interdiff.txt13.03 KBdclavain
#44 2107427-language-switcher-labels-44.patch11.96 KBdclavain
#44 interdiff.txt2.34 KBdclavain
#40 2107427-language-switcher-labels-40.patch9.62 KBdclavain
#40 interdiff.txt709 bytesdclavain
#37 2107427-language-switcher-labels-37.patch9.15 KBpenyaskito
#37 interdiff.txt936 bytespenyaskito
#34 2107427-language-switcher-labels-34.patch9.09 KBpenyaskito
#25 2107427-language-switcher-labels-25.patch1.9 KBszato
#23 2107427-language-switcher-labels-23.patch2.07 KBszato
#16 2107427-language-switcher-labels-16.patch4.85 KBpenyaskito
#16 2107427-language-switcher-labels-16.only-test.patch3.02 KBpenyaskito
#16 interdiff.txt794 bytespenyaskito
#8 2107427-language-switcher-labels-8.patch7.33 KBpenyaskito
#8 2107427-language-switcher-labels-8.only-test.patch3.02 KBpenyaskito
#8 interdiff.7-8.txt5.05 KBpenyaskito
#7 2107427-language-switcher-labels-7.patch5.53 KBpenyaskito
#7 interdiff.txt2.94 KBpenyaskito
#2 2107427-language-switcher-labels-2.patch3.72 KBpenyaskito
#2 2107427-language-switcher-labels-2.only-test.patch2.85 KBpenyaskito
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

penyaskito’s picture

Assigned: Unassigned » penyaskito
Issue summary: View changes
penyaskito’s picture

Use config contexts for localizing the labels, like we used at the Date localization clean-up in Drupal/Core/Datetime/Date.php dateFormat method). If locale is not enabled we will show the English language names. Not sure if this is desirable.

For the tests, I needed an example of managing config files for testing that I found at the config_translation module (ConfigTranslationUiTest). The tests needed enabling locale.

Status: Needs review » Needs work

The last submitted patch, 2: 2107427-language-switcher-labels-2.only-test.patch, failed testing.

penyaskito’s picture

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

Status: Needs review » Needs work

Functionally looks good, thanks! Now only some code organization :)

  1. +++ b/core/modules/language/language.negotiation.inc
    @@ -394,9 +394,13 @@ function language_switcher_url($type, $path) {
    +    config_context_enter('Drupal\Core\Config\Context\LanguageConfigContext')->setLanguage($language);
    +    $config = \Drupal::config('language.entity.' . $language->id)->get();
    +    $localized_language = new Language($config);
    +    config_context_leave();
    

    I think this should be a utility function (or an option to language_list(), which should maintain a cached list of languages with localized names as well). It looks odd that we'd need to avoid language_load() or any entity method for that matter to do this, but the sole reason is language_load() is dependent on language_list() which caches original entity values, right?

  2. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSwitchingTest.php
    @@ -145,4 +153,10 @@ function testLanguageLinkActiveClass() {
    +  private function translateLanguage($langcode, $label) {
    +    $file_storage = new FileStorage($this->configDirectories[CONFIG_ACTIVE_DIRECTORY]);
    +    $file_storage->write('locale.config.' . $langcode . '.language.entity.' . $langcode,
    +        array('label' => $label));
    +  }
    

    Add phpdoc on this :)

penyaskito’s picture

Issue summary: View changes

Reduced scope for sticking to the regression, moved improvement proposals to #2135379: Make the language switcher configurable and extensible.

penyaskito’s picture

FileSize
2.94 KB
5.53 KB

#5.1 Yes, the issue is caching. I'm not sure how do we want to maintain the localized names in every language. drupal_static an array of arrays with the full language objects? Caching labels separately? Caching full objects multiple times where the only difference is the label?
BTW language_list() is caching in request negotiated language, so not sure what happens if we need different language labels in the same requests if we don't use overrides with language contexts there. I think this is something that won't happen because we will use language contexts e.g. sending mails in different languages, but maybe is worth to consider something different.

#5.2 Not addressed yet.

If the attached is the way forward, we need to update language_list docblock too.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
5.05 KB
3.02 KB
7.33 KB

#5.1 I'm not very happy with the final implementation but this includes caching of both translated and untranslated languages list.
#5.2 and language_list docblock have been fixed.

Status: Needs review » Needs work

The last submitted patch, 8: 2107427-language-switcher-labels-8.only-test.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review

Tests failed/passed as expected.

Gábor Hojtsy’s picture

I agree this is not very elegant, but not sure how best to do it. We can introduce yet another function that does similar things... or refactor part of the logic to another function and then #1862202: Objectify the language system would refactor that again. I'd invite reviewers from #1862202: Objectify the language system :)

penyaskito’s picture

I'm tempted of asking for an RTBC here for the sake of the added test and care about separating to two different methods in LanguageManager in #1862202: Objectify the language system, where we could add a private abstract method for avoiding code duplication, which I don't know how to avoid here if we split in two different methods.

penyaskito’s picture

Status: Needs review » Postponed

After chat with Gabor, we think that we should postpone this on #1862202: Objectify the language system

Gábor Hojtsy’s picture

Issue tags: +Configuration context

Tagging with a new tag for tracking config context related issues.

penyaskito’s picture

Status: Postponed » Needs work
penyaskito’s picture

Status: Needs work » Needs review
FileSize
794 bytes
3.02 KB
4.85 KB

The interdiff is only of the tests, for the rest of the patch I started from scratch. I'm not sure if this is the way to go, but tests are green.

The last submitted patch, 16: 2107427-language-switcher-labels-16.only-test.patch, failed testing.

penyaskito’s picture

The last submitted patch, 16: 2107427-language-switcher-labels-16.only-test.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

It sounds problematic to me to change the default behaviour of the language list without even an option to go differently or another way to get the language list. I'd add an option and use that for the switcher, not change the listing method wholesale to return the localized language all the time.

estoyausente’s picture

Assigned: penyaskito » estoyausente
Issue tags: +D8SVQ, +sprintweekend20m
penyaskito’s picture

Issue tags: -D8SVQ, -sprintweekend20m +#SprintWeekend2014, +#D8SVQ
szato’s picture

Status: Needs work » Needs review
FileSize
2.07 KB

Sorry, we forgot to re-assign the issue. We have a solution, please review.

csg’s picture

I have tested the patch in #23, and it works. The code looks good too, apart from some seemingly unnecessary blank lines.

szato’s picture

Blank lines removed.

gloob’s picture

Issue tags: -#SprintWeekend2014, -#D8SVQ +SprintWeekend2014, +D8SVQ
dermario’s picture

Status: Needs review » Reviewed & tested by the community

The patch a applies an language switcher seems to work as proposed.

penyaskito’s picture

Assigned: estoyausente » Unassigned
Status: Reviewed & tested by the community » Needs work

I think Gábor's comment #20 is still an issue, and adding an undocumented name_native property to languages that could or not be set is not a good thing for DX.

Gábor Hojtsy’s picture

I think the proposed patch is good for continuing with the discussion :) So we have at least three use cases for languages:

1. Installer language selector, that is already covered with the predefined languages.
2. Need to display a language list translated to the interface language. Happens if you are editing a block, creating a node, etc. The language selector should contain all language names in the interface language.
3. Need to display the native name of all languages. Occurs in the language switcher block (only).

This issue is to cover 3. (1) already works well. And then we have issues for 2 eg. #1879930: Language selectors are not showing localized to the page language. For both (2) and (3) the cache nature of the language list is an issue. Because the language list is cached in the form first found. So the proposed patch above resolved that problem for (3) by adding this "new" native name property (similar to how it is in Drupal 7).

There are countless other ways to solve these. Eg. alternately we can also introduce a method to get the language list localized and have it support (2) and (3) and cache for (3) and each variant of (2) separately, so we get the right localized versions. I guess we should pick a way and then get it in :) We can go around in circles with different approaches definitely :)

penyaskito’s picture

I don't have a really strong opinion on any of the options, but let me dump my feelings here.

Native name is better DX++ for people coming from D7, but then we should add this to the Language class itself so its less obscure.
But in the other hand I think this would be incoherent with how we handle other translated config, so DX--.

If we are sure that we only need to display the native name of all languages in the language switcher block, then I will discard the flaggy last parameter in #7 and just add a getNativeLanguages to the LanguageManager for simplicity.

We are going around in circles, and I finished thinking that Gábor's first suggestion at #5 is the way to go. Sorry for the mess.

Gábor, do you still think the same?

Gábor Hojtsy’s picture

Yeah. getLocalizedLanguageNames() with a 'param' of native or 'localized' to langcode (specify langcode as well) would be fine. Or put these on the getLanguages() as arguments directly (and cache there specifically).

Something like:

function getLocalizedLanguageNames($type, $langcode = NULL) {
  if ($type == self::LIST_NATIVE_NAMES) {
    // now ignore $langcode and translate each language to itself => native names
  }
  else {
    // now use $langcode as target language to translate => all languages translated to $langcode
  }
  return ...;
}

Or the same as an extension of getLanguages().

penyaskito’s picture

Assigned: Unassigned » penyaskito
penyaskito’s picture

@szato: I noticed that your patches lacked the test modification added in previous patches. I'm not sure how this happened to you.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
9.09 KB

This grew more than I expected.

After discussion with Gábor on IRC, I don't find that switching depending on a parameter is a good idea, so I added a getNativeLanguages() method to the LanguageManagerInterface. The code there is mostly the same than at #16, without caching.

We use that method only on LanguageNegotiationSession::getLanguageSwitchLinks.

After changing that tests were failing, and the culprit was Language::sort. This calls SortArray::sortByWeightAndTitleKey($a, $b, $weight_key = 'weight', $title_key = 'title') with the default optional parameters. The title key is "title" when it should be "label" or "name". I found that we are not calling that passing the optional params anywhere, and by using uasort there is no way of passing that, so I just cloned it to sortByWeightAndLabelKey. Probably we need to remove those params if they are unused and refactor the duplicated code, but I would leave that for a follow-up.

Sorry for not providing an interdiff, but this is mostly a new approach than the patches above.

Gábor Hojtsy’s picture

Yeah we can say the config factory does the caching of these translated entities, so we don't need to do the caching I guess here. The data derivative generation we do is not very expensive at all. I think we could use the getLanguages() return value though for the base objects and just adjust the label values to use native names to avoid some duplicate code (eg. default identification), but then we'd need to load the translated objects anyway. So yeah. I think here we need to pick one of many ugly options either way.

Just two code comment issues then:

  1. +++ b/core/lib/Drupal/Component/Utility/SortArray.php
    @@ -119,6 +119,25 @@ public static function sortByWeightAndTitleKey($a, $b, $weight_key = 'weight', $
    +   * Sorts a structured array firstly by weight, then by label.
    

    "firstly" => "first" is correct English :)

  2. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSwitchingTest.php
    @@ -283,4 +291,16 @@ protected function doTestLanguageLinkActiveClassAnonymous() {
    +   * @param string $langcode The language code.
    +   * @param string $label The native name of the language
    

    Docs code style is not good.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
penyaskito’s picture

Status: Needs work » Needs review
FileSize
936 bytes
9.15 KB

#35.1: This was copied from sortByWeightAndTitleKey. Searched for it and looks like "firstly" is an English word. See the Oxford dictionary.

#35.2: Fixed.

Status: Needs review » Needs work

The last submitted patch, 37: 2107427-language-switcher-labels-37.patch, failed testing.

Gábor Hojtsy’s picture

@penyaskito: still working on this? I think you said you know the fails :)

dclavain’s picture

Status: Needs work » Needs review
FileSize
709 bytes
9.62 KB

The translated language name is in the first position of the array instead of the zero position.

Status: Needs review » Needs work

The last submitted patch, 40: 2107427-language-switcher-labels-40.patch, failed testing.

penyaskito’s picture

@dclavain: The problem is somehow at the ordering. The failing tests are because we expect "English|Francais" and we get "Francais|English". Initially, this was because we were ordering with sortByWeightAndTitleKey, which title was not defined, and in #37 we added sortByWeightAndLabelKey, which used the label property which is the native language name.

We should debug that this is working properly and take into account that maybe the weights could be different (we expect them to be equal) and this is the root of failures.

Let me know if you want to finish it or if should I give a try.

Gábor Hojtsy’s picture

@penyaskito: looks like it would be great if you could give this a try, the issue is still assigned conveniently to you :)

dclavain’s picture

Status: Needs work » Needs review
FileSize
2.34 KB
11.96 KB

@penyaskito I think that the problem is in the test.

-    $this->assertText('French', 'Language added successfully.');
+    $this->assertText('Français', 'Language added successfully.');

The ordering method is correct.

Status: Needs review » Needs work

The last submitted patch, 44: 2107427-language-switcher-labels-44.patch, failed testing.

The last submitted patch, 44: 2107427-language-switcher-labels-44.patch, failed testing.

Berdir’s picture

+++ b/core/modules/language/lib/Drupal/language/ConfigurableLanguageManager.php
@@ -293,6 +293,48 @@ public function getLanguages($flags = Language::STATE_CONFIGURABLE) {
+
+    // Retrieve the config storage to list available languages.
+    $prefix = 'language.entity.';
+    $storage = $this->configFactory->get($prefix . Language::LANGCODE_NOT_SPECIFIED)->getStorage();
+    $config_ids = $storage->listAll($prefix);
+
+    $langcodes = array_map(
+      function($value) use ($prefix) { return str_replace($prefix, '', $value);},
+      $config_ids
+    );
+    // Instantiate languages from config objects.
+    $original_language = $this->configFactory->getLanguage();
+    $i = 0;
+
+    foreach ($config_ids as $config_id) {
+      $langcode = $langcodes[$i];
+      $this->configFactory->setLanguage(new Language(array('id' => $langcode)));
+      $data = $this->configFactory->get($config_id)->get();
+      // 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'];
+      $language = new Language($data);
+      if (!$language->locked) {
+        $languages[$langcode] = $language;
+      }
+      ++$i;
+    }
+    $this->configFactory->setLanguage($original_language);
+
+    // Sort the language list by weight.

The dance of language objects, language entity and config gets crazier every day :(

Can't we use the Entity API here? For example, execute an Entity Field Query to get the ID's, and then load entity objects?

We might not be able to inject that, but we also should not be messing with config entity data directly, so we lose anyway ;)

This could also be pretty slow, as changing the language multiple times is a bit ugly. So it's definitely a candidate for being uploaded upfront, there's a config issue related to that, and we should post there that the language config entities should definitely be added there, including translations.

dclavain’s picture

Status: Needs work » Needs review
FileSize
13.03 KB
24.99 KB
Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Language/Language.php
@@ -191,7 +191,7 @@ public function __construct(array $options = array()) {
-          $this->name = $predefined[$this->id][0];
+          $this->name = $predefined[$this->id][1];

I don't get this change an all related test changes. How is this related at all? Has nothing to do with the language switcher block. It is not even the correct change AFAIS.

dclavain’s picture

The last submitted patch, 37: 2107427-language-switcher-labels-37.patch, failed testing.

dclavain’s picture

Status: Needs work » Needs review
FileSize
3.21 KB
11.07 KB

In testLanguageBlock function I have changed the order of block creation and in getLanguageSwitchLinks function I translate name of language, this change is not necesary for pass test.

penyaskito’s picture

+++ b/core/modules/language/lib/Drupal/language/Plugin/LanguageNegotiation/LanguageNegotiationSession.php
@@ -135,7 +135,7 @@ function getLanguageSwitchLinks(Request $request, $type, $path) {
-        'title' => $language->name,
+        'title' => t($language->name, array(), array('langcode' => $language->id)),

+++ b/core/modules/language/lib/Drupal/language/Plugin/LanguageNegotiation/LanguageNegotiationUrl.php
@@ -191,7 +191,7 @@ function getLanguageSwitchLinks(Request $request, $type, $path) {
     foreach ($this->languageManager->getNativeLanguages() as $language) {

Why is t() needed? The title should already be in the native language ($this->languageManager->getNativeLanguages())

dclavain’s picture

No, if you add one language, in the switch language block the title isn't in native language. You need add translate file and clear cache for that the title is in native language.

Gábor Hojtsy’s picture

Assigned: penyaskito » Unassigned
Status: Needs review » Needs work

In IRC @penyaskito said to unassign this. Also here are my review points. I agree t() should not appear in any added code here, the whole point of the issue is not to use t().

  1. +++ b/core/modules/language/lib/Drupal/language/ConfigurableLanguageManager.php
    @@ -293,6 +293,48 @@ public function getLanguages($flags = Language::STATE_CONFIGURABLE) {
    +    // Retrieve the config storage to list available languages.
    

    This does not really retrieve the storage itself, but the list of files with the given prefix instead.

    Maybe:

    // Retrieve list of languages directly from configuration storage.

  2. +++ b/core/modules/language/lib/Drupal/language/Plugin/LanguageNegotiation/LanguageNegotiationSession.php
    @@ -135,7 +135,7 @@ function getLanguageSwitchLinks(Request $request, $type, $path) {
             'href' => $path,
    -        'title' => $language->name,
    +        'title' => t($language->name, array(), array('langcode' => $language->id)),
    
    +++ b/core/modules/language/lib/Drupal/language/Plugin/LanguageNegotiation/LanguageNegotiationUrl.php
    @@ -188,10 +188,10 @@ public function processOutbound($path, &$options = array(), Request $request = N
    -    foreach ($this->languageManager->getLanguages() as $language) {
    +    foreach ($this->languageManager->getNativeLanguages() as $language) {
           $links[$language->id] = array(
             'href' => $path,
    -        'title' => $language->name,
    +        'title' => t($language->name, array(), array('langcode' => $language->id)),
    

    Indeed the whole point of the native language names retrieval is we would not use t(). The translations of languages is in the config system not to be accessed with t().

    In the first instance it is not using the native names method even.

  3. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSwitchingTest.php
    @@ -42,23 +43,26 @@ function setUp() {
    +    // Enable the language switching block..
    +    $block = $this->drupalPlaceBlock('language_block:' . Language::TYPE_INTERFACE, array(
    +        'id' => 'test_language_block',
    +        // Ensure a 2-byte UTF-8 sequence is in the tested output.
    +        'label' => $this->randomName(8) . '×',
    +      ));
    

    I think I understand why is the indentation of this is as-is but it looks very odd.

YesCT’s picture

I'll look into this.

YesCT’s picture

Status: Needs work » Needs review
FileSize
11.38 KB
6.3 KB

addressed #55
used #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines as a guide.

also formatted array_map like
core/modules/field/lib/Drupal/field/Tests/Views/FieldUITest.php
core/modules/config_translation/lib/Drupal/config_translation/ConfigNamesMapper.php

some other nits.

renamed translateLanguage since it was really saving a label in config, not for t().

Tried this manually. By installing in Spanish, but the Espanol does not show in the language switcher.
I cleared the caches.

Maybe the language does not "come" with the language entity label config file?
Also, I do not see an extra setting on the language switcher block.

I need to look more into this, and also get some help.

dclavain’s picture

Tried this manually. By installing in Spanish, but the Espanol does not show in the language switcher.
I cleared the caches.

@YesCT for this reason I had added t in 'title' => t($language->name, array(), array('langcode' => $language->id)).

comment #54

Gábor Hojtsy’s picture

@YesCT/@dclavain: translations of language names to other languages should be downloaded with the .po file in installation. If the language you installed with did not contain a translation for its own language name, then you can enter it using the Interface translation UI or the config translation module by translating the language itself to other languages (than English, which all languages originally are).

vijaycs85’s picture

vijaycs85’s picture

Issue summary: View changes
vijaycs85’s picture

Tested patch in #57 and looks like working as expected. Updated issue summary with 'how to test'.

one question is, current fix shows the language name in their own language no matter what site's current language. For example, French language in language block displays as 'français' for all language sites (i.e. /fr or /ta or /en). IMHO, 'français' should be displayed only on FR site (i.e. /fr). If this is correct, here is the fix for this. otherwise, use the one in #57.

Attaching screenshots of this fix.

Status: Needs review » Needs work

The last submitted patch, 62: 2107427-language-switcher-62.patch, failed testing.

Gábor Hojtsy’s picture

No, if you are French and you are looking at a Spanish page, you would only notice French in French, not French in Spanish :) So the idea to have always the native names of languages in the block is exactly so people can recognize the right one based on the native name of their language they know. So all languages should be localized to their own language at all times.

vijaycs85’s picture

Status: Needs work » Needs review

Okay, in that case, +1 for making #57 RTBC

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

All right, this is definitely not the nicest code in Drupal 8 but looks like some of the hacks need to be done to make this effective and performant. So let's go with this then. We have been arguing for long enough and tried a whole host of other approaches before. Let's fix the bug for once :)

Berdir’s picture

This is a pretty slow operation that sites with a language switcher will obviously have to do on every page.

I don't think we can use block caching, as the blocks have different links on every site. Should have caching within the language manager for the labels? We have 2 config storage loads per language and they can not be preloaded as preloading will not be able to handle the language switch.

Gábor Hojtsy’s picture

I did not profile this. My gut feeling was the config factory will cache the lookups already, so it would be a pretty shallow cache. Did not profile it (once again).

mikispeed’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
11.38 KB
1.36 KB

Patch from #57 marked in #65 as RTBC didn't worked against current HEAD. Here is the updated patch that makes it work with the changes from issue #2219499: Generalize language config override responsibility.

pp’s picture

Status: Needs review » Reviewed & tested by the community

I do the steps in issue description. Without the patch the language name is English. Whit patch the language name is translated.

rodrigoaguilera’s picture

I applied the patch too and fix the bug but I don't know how to do profiling.
If someone can send some link to do this I'll be grateful

webchick’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +needs profiling

It sounds like this still needs profiling, so knocking back to "needs review" and tagging. If you think that is wrong, catch is right there in Szeged, so please ask him. :D

Aron Novak’s picture

Assigned: Unassigned » Aron Novak

i am going to profile it

Aron Novak’s picture

Status: Needs review » Needs work
Issue tags: -needs profiling
FileSize
751.05 KB
753.67 KB
699.94 KB
1.25 KB
1.25 KB
1.25 KB
1.25 KB
24.49 KB
27.33 KB
24.49 KB
27.03 KB
30.04 KB

I did the profiling, i followed the "How to test" procedure in the initial post to configure the test site, then, i issued drush cache-rebuild executed apachebench three times with the following command, ab -n 20 http://d8.local/hu. After that, i loaded http://d8.local/hu?XDEBUG_PROFILE twice in the browser as admin user. Then, i applied the patch, double checked that the issue is solved (yes), and repeated the procedure. I always used the 2nd (3rd) measurement. You can see the attached files.

My gut feeling was the config factory will cache the lookups already

My relevant screenshots from KCachegrind:
Pre-patch

Post-patch


My feeling is that given the fact that we have 10 extra queries, it might need work.

Aron Novak’s picture

FileSize
27.81 KB

one missing file from the previous comment

vijaycs85’s picture

Status: Needs work » Needs review

Just had a look at this additional calls through xdebugger. The additional calls are coming from Drupal\language\ConfigurableLanguageManager::getNativeLanguages() to Drupal\Core\Cache\DatabaseBackend::getMultiple for every language. This can't be optimised at this method level as the loading should happen in the language currently overridden. So setting it to 'needs review' to get feedback on how to move forward with this.

Gábor Hojtsy’s picture

@vijaycs85: yeah well the question is would caching inside the method help, so further calls would not be a problem. I'm not sure that would help for 80% cases as there is likely one call to this method per request (only 1 block displayed). However having a cached list of native names somewhere (state?) that is cleared out if translations / languages change may belp. Not sure that much complication is worth it for the benefit, but we cannot see how much benefit in time that would have right now...

I think this is Aron's/webchick's/berdir's point.

Gábor Hojtsy’s picture

Title: Language names don't show up localized in language switcher anymore » Regression: Language names should display in their native names in the language switcher block

Retitled since there were some misunderstanding of what localized would mean.

Wim Leers’s picture

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSwitchingTest.php
@@ -42,23 +43,26 @@ function setUp() {
+    $this->saveNativeLanguageName('fr', 'Français');

This is actually wrong, French is called "français", not "Français" in its native form. I.e. lowercased, not uppercased. Fixed.


Berdir asked me to chime in from a perf POV. We ended up having an IRC discussion:

WimLeers: berdir: This would be easier to fix if we had something like 'destination' for languages: if we had e.g. 'redirect_to_language=DE', then it'd be globally cacheable, at the cost of a redirect
WimLeers: berdir: I assume there are no such plans?
berdir: WimLeers: uh, how so?
berdir: WimLeers: you need to know the current path
WimLeers: berdir: but "current path" is not aliased. Then we could just have a placeholder in the rendered HTML for the current path in the globally cached block, replace that placeholder with the actual current path in a #post_render_cache callback, and voila
WimLeers: berdir: but since there is now 1) pluggable language negotiators (e.g. language could depend on URL path, or URL scheme, or Accept-Language hader), 2) linking to the current page, which may have per-language URL aliases, 3) hook_language_switch_links_alter()… 1+2+3 imply caching is only possible at per-URL level
berdir: WimLeers: yeah, the alter is fun. 1) could be solved by adding the current language to the cache, that is easy, we know that at that point.
WimLeers: berdir: And 4) the language switcher block itself may have a translated label, if that weren't the case, then we could at least cache on a per-system path basis (since /en/node/1, /fr/node/1 and /de/node/1 would all have the same block)
berdir: WimLeers: no, the labels are always the same
WimLeers: the *block* label may differ
WimLeers: the *language link* labels are indeed always the same
berdir: right
berdir: so it's either per url and language or no caching on block level
WimLeers: So yeah I think the only way to guarantee correctness is to cache the block per-URL *and* per-language.
WimLeers: Precisely.
berdir: not sure if that is worth it
WimLeers: Indeed
WimLeers: Lots of cache entries
berdir: and not sure if caching on the language manager is helpful either
berdir: it would only get interesting if you have bunch of languages, and a fast cache backend like php storage or apcu should make it less of a problem
berdir:  I was mostly worried because this would be impossible to preload in a useful way, but looks like we're possibly going a different route there anyway
WimLeers: berdir: if you solely rely on Accept-Language, then it *is* fully cacheable. The tricky thing is that you essentially have different caching capabilities depending on the language negotiation method. So it seems that for now, it might be better to *not* use block caching for that block.

So, you could make LanguageBlock implement getRequiredCacheContexts() and let it return array('cache_context.url', 'cache_context.language'), and that would work just fine. The question is whether that's worth it: you'd end up with a lot of cache entries. (And that's in the case of the default language negotiation mechanism, if you use another one, you'll want to return something else there.)

penyaskito’s picture

Thanks all for the awesome profiling work and comments.

How do we move next? I'm with Gábor that this will be called only once per request in 80% cases, so the only improvement I see is moving the native list to state, but I don't think it is worth to do that.

Should we just move forward with patch at #79?

Wim Leers’s picture

To clarify: I don't think there's anything left to be done in this patch performance-wise, because doing so would significantly expand the scope of the issue.

Must-have performance improvement follow-up

However, I think a must-have follow-up would be to have a "language redirect" route, i.e. /language-redirect/<language code>. So e.g. "français" would link to /language-redirect/fr?destination=node/42. Because then everything except for the destination parameter could be cached globally. The destination parameter could be set in JS, just like contextual.js already does today:

    // Set the destination parameter on each of the contextual links.
    var destination = 'destination=' + Drupal.encodePath(drupalSettings.path.currentPath);
    $contextual.find('.contextual-links a').each(function () {
      var url = this.getAttribute('href');
      var glue = (url.indexOf('?') === -1) ? '?' : '&';
      this.setAttribute('href', url + glue + destination);
    });

To remain JS-less for anonymous users, we could still generate the entire block dynamically for anonymous users, because Drupal assumes that anonymous users get served cached pages anyway. This would then be in line with what we do for active link handling: we set the active class in PHP (on the server side) for anonymous users, but in JS (on the client side) for authenticated users.

That's the best of both worlds.

Gábor Hojtsy’s picture

Wim: I'm not sure that followup would be so simple, especially with outbound link altering per language being a supported feature (eg. a translation of a node may be a view or a panel :D), but that is better debated in that followup :)

Wim Leers’s picture

#82: I don't follow, but yes, let's discuss that in a follow-up: #2232375: Make language switcher block cacheable.

YesCT’s picture

I'm going to review this.

YesCT’s picture

Issue summary: View changes

Adding follow-up from #83 to issue summary

YesCT’s picture

needed a reroll. it was an easy one. automatic 3 way merge with no conflicts. (still reviewing)

mashermike’s picture

YesCT dragged me into a room and asked me to make this patch. This is my first patch and drupal contribution so be nice... (please?)

The sorting function in the previous patch is not pertinent to this issue. It will be included in a separate one.

Status: Needs review » Needs work

The last submitted patch, 87: 2107427-language-switcher-labels-87.patch, failed testing.

mashermike’s picture

Ok, I did the diff in the wrong direction so the patch did not work. This version should work.

mashermike’s picture

Status: Needs work » Needs review

Changing status to send the patch to the bot.

Status: Needs review » Needs work

The last submitted patch, 89: 2107427-language-switcher-labels-89.patch, failed testing.

penyaskito’s picture

Status: Needs work » Postponed

Nice work, @mashermike!

This three failures looks familiar to me. It's because #2239399: Languages should be sorted by label instead of title is required for this one to pass. I'm postponing on that one.

YesCT’s picture

we should have paid attention to berdir in #47

The dance of language objects, language entity and config gets crazier every day :(

  1. +++ b/core/modules/language/lib/Drupal/language/ConfigurableLanguageManager.php
    @@ -306,6 +306,47 @@ public function getLanguages($flags = Language::STATE_CONFIGURABLE) {
    +  public function getNativeLanguages() {
    

    this is messed. but not the fault here. cause getLanguages() is messed.

  2. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSwitchingTest.php
    @@ -283,4 +291,19 @@ protected function doTestLanguageLinkActiveClassAnonymous() {
    +  private function saveNativeLanguageName($langcode, $label) {
    

    this should just be protected, in case someone wants to subclass this. (not likely, but still)

head is silently doing things we dont expect and should be broken.

#2151223: Add a method to SortArray to sort by weight and title might get a revert, or an issue to fix it.
or an issue to fix getLanugage: #2239497: [Meta] Fix ConfigurableLanguageManager getLanguages() [edit: added link to issue]

Aron Novak’s picture

Assigned: Aron Novak » Unassigned
YesCT’s picture

Issue tags: -medium
Gábor Hojtsy’s picture

Status: Postponed » Needs work

#2239399: Languages should be sorted by label instead of title was just closed down as fixed/duplicate. Should unpostpone this one finally!

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 89: 2107427-language-switcher-labels-89.patch, failed testing.

Gábor Hojtsy’s picture

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSwitchingTest.php
@@ -283,4 +291,19 @@ protected function doTestLanguageLinkActiveClassAnonymous() {
+  /**
+   * Saves the native name of a language entity in configuration as a label.
+   *
+   * @param string $langcode
+   *   The language code of the language.
+   * @param string $label
+   *   The native name of the language.
+   */
+  private function saveNativeLanguageName($langcode, $label) {
+    $file_storage = new FileStorage($this->configDirectories[CONFIG_ACTIVE_DIRECTORY]);
+    $file_storage->write('language.config.' . $langcode . '.language.entity.' . $langcode,
+      array('label' => $label)
+    );
+  }
+

This will certainly not work anymore, you should now use something like:

\Drupal::service('language.config_factory_override')->getOverride($langcode, 'language.entity.' . $langcode)->set('label', $label)->save();
penyaskito’s picture

Status: Needs work » Needs review
FileSize
11.07 KB

Sorry, no interdiff. This is mostly #89 with #93.2 corrected and following #99. We do have doTestLanguageBlockAnonymous() and doTestLanguageBlockAuthenticated() now, so I added assertions in both.

Green locally!

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Looks great. Only minor things:

  1. +++ b/core/modules/language/src/ConfigurableLanguageManager.php
    @@ -302,6 +302,45 @@ public function getLanguages($flags = BaseLanguageInterface::STATE_CONFIGURABLE)
    +    // Retrieve the config storage to list available languages.
    +    $prefix = 'language.entity.';
    +    $config_ids = $this->configFactory->listAll($prefix);
    

    Comment not true anymore. The storage is not directly involved anymore.

  2. +++ b/core/modules/language/src/ConfigurableLanguageManager.php
    @@ -302,6 +302,45 @@ public function getLanguages($flags = BaseLanguageInterface::STATE_CONFIGURABLE)
    +      if (!$language->locked) {
    

    Looks like this can be done earlier, otherwise, we instantiated a language and then threw away immediately.

  3. +++ b/core/modules/language/src/ConfigurableLanguageManager.php
    @@ -302,6 +302,45 @@ public function getLanguages($flags = BaseLanguageInterface::STATE_CONFIGURABLE)
    +    // Sort the language list by weight.
    +    Language::sort($languages);
    

    And by name :) Maybe don't add that comment just let the implementation speak for itself.

  4. +++ b/core/modules/language/src/Tests/LanguageSwitchingTest.php
    @@ -36,23 +37,26 @@ function setUp() {
    -    // Add language.
    +// Add language.
         $edit = array(
    

    Wrong indent.

  5. +++ b/core/modules/language/src/Tests/LanguageSwitchingTest.php
    @@ -36,23 +37,26 @@ function setUp() {
    +        'id' => 'test_language_block',
    +        // Ensure a 2-byte UTF-8 sequence is in the tested output.
    +        'label' => $this->randomMachineName(8) . '×',
    +      ));
    

    I don't think we double-indent like this elsewhere?!

penyaskito’s picture

Handled #101 points.

Gábor Hojtsy’s picture

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

Looks good now. Updated issue summary and looked at the followups as well. Not sure all are necessary, but we can discuss there. While loading all the localized language overrides is not nice, we could not think of nicer solutions unfortunately.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 102: 2107427-language-switcher-labels-102.patch, failed testing.

penyaskito’s picture

Status: Needs work » Reviewed & tested by the community

RTBC per #103, tests errors were because of a db connection issue.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 102: 2107427-language-switcher-labels-102.patch, failed testing.

penyaskito’s picture

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

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/language/src/ConfigurableLanguageManager.php
@@ -302,6 +302,44 @@ public function getLanguages($flags = BaseLanguageInterface::STATE_CONFIGURABLE)
+    foreach ($config_ids as $config_id) {
+      $langcode = $langcodes[$i];
+      $this->setConfigOverrideLanguage(new Language(array('id' => $langcode)));
+      $data = $this->configFactory->get($config_id)->get();
+      if (!$data['locked']) {
+        // 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'];
+        $language = new Language($data);
+        $languages[$langcode] = $language;
+      }
+      ++$i;
+    }

We need to watch the performance of this. Can't think of a better way of doing it though. That said these links appear in a block that should be permanently cached after #2232375: Make language switcher block cacheable is done. So I think we should be loading the language entities here and not be side stepping that.

penyaskito’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
11.39 KB
3.36 KB

This patch changes the strategy to load the language entities.
I had to change the negotiation classes as we still have some languages with name and some others with label. It will be cleaned up in #2246679: Make Language module's LanguageInterface (to be ConfigurableLanguageInterface) extend Core's LanguageInterface.

Status: Needs review » Needs work

The last submitted patch, 111: 2107427-language-switcher-labels-111.patch, failed testing.

The last submitted patch, 111: 2107427-language-switcher-labels-111.patch, failed testing.

The last submitted patch, 111: 2107427-language-switcher-labels-111.patch, failed testing.

The last submitted patch, 111: 2107427-language-switcher-labels-111.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
11.41 KB

Rerolled (automatic 3-way merge)

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks like what @alexpott asked for :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/language/src/ConfigurableLanguageManager.php
    @@ -302,6 +303,37 @@ public function getLanguages($flags = BaseLanguageInterface::STATE_CONFIGURABLE)
    +    $i = 0;
    +
    +    foreach ($config_ids as $config_id) {
    +      $langcode = $langcodes[$i];
    +      $this->setConfigOverrideLanguage(new Language(array('id' => $langcode)));
    +      $language = ConfigurableLanguage::load($langcode);
    +      if (!$language->locked) {
    +        $languages[$langcode] = $language;
    +      }
    +      ++$i;
    +    }
    

    This is unnecessarily complex - just loop the langcodes array and there is no need for $i - no?

  2. +++ b/core/modules/language/src/Tests/LanguageSwitchingTest.php
    @@ -7,6 +7,7 @@
    +use Drupal\Core\Config\FileStorage;
    

    Not used

penyaskito’s picture

Right. Much cleaner now.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/language/src/ConfigurableLanguageManager.php
    @@ -302,6 +303,34 @@ public function getLanguages($flags = BaseLanguageInterface::STATE_CONFIGURABLE)
    +    // Retrieve the list of languages defined in configuration.
    +    $prefix = 'language.entity.';
    +    $config_ids = $this->configFactory->listAll($prefix);
    +    $langcodes = array_map(function ($value) use ($prefix) {
    +      return str_replace($prefix, '', $value);
    +    }, $config_ids);
    

    If we want to API-ify all the things, then we can just as well do $languages = $this->getLanguages() here... Looks like @alexpott is not a fan of our microoptimizations here...

  2. +++ b/core/modules/language/src/ConfigurableLanguageManager.php
    @@ -302,6 +303,34 @@ public function getLanguages($flags = BaseLanguageInterface::STATE_CONFIGURABLE)
    +    // Instantiate languages from config objects.
    

    This comment is now outdated.

  3. +++ b/core/modules/language/src/Tests/LanguageSwitchingTest.php
    @@ -36,23 +36,26 @@ protected function setUp() {
    +    // Enable the language switching block..
    

    One dot should be enough here. I know this was copied over but...

penyaskito’s picture

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

All right, as simple as it can get now :) Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

So this patch is displaying some of the current problems we have because ConfigurableLanguage and Language don't share a common interface. They really should but that is not the fault of this issue. The suggestions below are a work around for this.

  1. +++ b/core/lib/Drupal/Core/Language/LanguageManagerInterface.php
    @@ -85,6 +85,15 @@ public function getDefaultLanguage();
    +   * @return \Drupal\Core\Language\LanguageInterface[]
    

    This is not correct at the moment since ConfigurableLanguage does not implement this however we should fix this by making the suggested changes below.

  2. +++ b/core/modules/language/src/ConfigurableLanguageManager.php
    @@ -302,6 +303,25 @@ public function getLanguages($flags = BaseLanguageInterface::STATE_CONFIGURABLE)
    +      $natives[$langcode] = $native;
    

    lets create language objects from the config entities here... $native->toLanguageObject()

  3. +++ b/core/modules/language/src/ConfigurableLanguageManager.php
    @@ -302,6 +303,25 @@ public function getLanguages($flags = BaseLanguageInterface::STATE_CONFIGURABLE)
    +    uasort($natives, array('\Drupal\language\Entity\ConfigurableLanguage', 'sort'));
    

    Need to change this to use the language object sort

  4. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationSession.php
    @@ -131,11 +131,11 @@ function getLanguageSwitchLinks(Request $request, $type, $path) {
    -        'title' => $language->name,
    +        'title' => $language->label,
    
    +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationUrl.php
    @@ -190,10 +190,10 @@ public function processOutbound($path, &$options = array(), Request $request = N
    -        'title' => $language->name,
    +        'title' => $language->label,
    

    and now you can use getName()

penyaskito’s picture

Status: Needs review » Needs work

The last submitted patch, 127: 2107427-language-switcher-labels-127.patch, failed testing.

penyaskito’s picture

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

penyaskito’s picture

Using Language::sort requires to reimplement it, as the ArraySort methods use properties instead of methods. This leaves SortArray::sortByWeightAndTitleKey() unused, so maybe we should remove it. At some point it was already considered, see #2151223: Add a method to SortArray to sort by weight and title.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Oh well... This at least means that the "use .... SortArray" in Language.php is now unused and should be removed. Otherwise I think this issue has been a land of compromise and not agreeing on said compromises, so we can just as well go with this approach (once the use is fixed) and see if we can agree :D

penyaskito’s picture

I always forget to review use statements...

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Lets go with this one then :)

YesCT’s picture

FileSize
263.56 KB

similar to #2226533: Changes to the Language class due to the LanguageInterface (followup) this removes the only usage of sortByWeightAndTitle.

Ug. I read #131 but I dont why that sort change is needed for this issue though.

We can remove that method (and the test and dataprovider) in a follow-up. (or maybe, maybe, changes issue might get in first, but I doubt that.) #2332739: Remove SortArray::sortByWeightAndTitle

I tried this manually, there is one difference that was probably un-intended. The order a language is added in the language list used to be in the order by the english name of the language. Now, new languages are added always to the end of the language list. I'm not sure this was an intended change. Makes me wonder what weight languages get when they are added. Maybe before this patch they used to all get the same weight when added, so they got then sorted by title?

Did this change the weight when languages get added?

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

@YesCT: @penyakisto said we need to use our own sorting code because sortArray uses public properties to sort:

  public static function sortByWeightAndTitleKey($a, $b, $weight_key = 'weight', $title_key = 'title') {
    $a = (array) $a;
    $b = (array) $b;

    $weight_cmp = static::sortByKeyInt($a, $b, $weight_key);

    if ($weight_cmp === 0) {
      return static::sortByKeyString($a, $b, $title_key);
    }

    return $weight_cmp;
  }

When casting to an array, only public properties will end up in the array.

Looks like both Language and ConfigurableLanguage have their label/weight and name/weight keys public though. The difference is Language has name while ConfigurableLanguage has label. So since @penyaskito attempts to use the sort method on Language to sort ConfigurableLanguage-es, the method cannot be used directly because the properties are named differently.

I think clearly introducing a sort method on ConfigurableLanguage would be better then. Marking needs work for this.

As for why the weighting of that list changes, that looks like a classic method of you saving the order of the languages at one point, from then on languages are not ordered alphabetically anymore. That could have happened before or after this patch either way, this patch does not change that behaviour, you just saved the order once.

alexpott’s picture

Hmmm... i think configurable and non configurable languages should be able to be sorted together. Especially now they have a common interface - perhaps we should postpone on #2332739: Remove SortArray::sortByWeightAndTitle

alexpott’s picture

Also actually I can not repeat YesCT's tests which show the languages always being added to the end. I suspect that during the test YesCT dragged a language on the language overview screen - this will change all the weights to be negative and therefore new languages will always be added to the end. This behaviour is completely unaffected by this patch.

YesCT’s picture

Yes. I could not reproduce the order issue either. Sorry for the noise. (but glad to learn about the save effect)

Gábor Hojtsy’s picture

Status: Needs work » Postponed

@alexpott: While its great we may be fixing most language architecture issues before this patch, there are also others postponed on this one and this is not supposed to be that hard an issue to work on for a year and 140 comments. Probably a good example for https://twitter.com/webchick/status/507919029034835968 :/

YesCT’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Status: Postponed » Needs work

#2332739: Remove SortArray::sortByWeightAndTitle landed. Unpostponing. I still think it would make most sense to sort a list of ConfigurableLanguage objects with the sort method on ConfigurableLanguage, that is most straightforward. In fact, they already have it properly inherited from ConfigEntityBase:

  public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b) {
    $a_weight = isset($a->weight) ? $a->weight : 0;
    $b_weight = isset($b->weight) ? $b->weight : 0;
    if ($a_weight == $b_weight) {
      $a_label = $a->label();
      $b_label = $b->label();
      return strnatcasecmp($a_label, $b_label);
    }
    return ($a_weight < $b_weight) ? -1 : 1;
  }

This is the behaviour we want. Also it did not need any of #2332739: Remove SortArray::sortByWeightAndTitle.

While we can keep using Language::sort() to sort ConfigurableLanguage objects, the two objects do not inherit from each other, so the best we could do is to ensure they have identical implementations with some tests ensuring that happens. #2334483: add test for sorting an array of languages that are Language and ConfigurableLanguage to show the sort() will work for configurable languages too. is about to add tests to ensure that the two in fact completely independent implementations on the two classes work identical. We have the option to postpone on that one (LOL) or just use the sort method on the class we are actually using :)

Other places eg. the admin config entity list will also order using the sort method on the config entity not some class that it only shares one of its implemented interfaces, so making use of the same sort method elsewhere makes most sense IMHO.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
585 bytes
10.8 KB

Rerolled. The Language.php change did not apply anymore due to #2332739: Remove SortArray::sortByWeightAndTitle doing the same change. The one line change for the sort included. Please review.

Status: Needs review » Needs work

The last submitted patch, 143: 2107427-language-switcher-labels-143.patch, failed testing.

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
585 bytes
10.8 KB

LOL, this is too funny. The sort() method on Language takes an array Of LanguageInterface objects and returns the array sorted. The sort method on ConfigurableLanguage takes TWO objects and returns their relative order (to be used in a uasort()) like Language::sort(). Haha. Hahaha. Hahahahaha. Ha.

So we may either sort with the ConfigurableLanguage::sort as

uasort($natives, array('Drupal\language\Entity\ConfigurableLanguage', 'sort'));

(which eg. ConfigEntityListBuilder does as uasort($entities, array($this->entityType->getClass(), 'sort'))).

Or do the less ugly looking

Language::sort($natives)

which was in the prior patch, although Language::sort() has even less to do with ConfigurableLanguage::sort() than we thought. Good thing about these small patches is we can talk about API design line by line.

Status: Needs review » Needs work

The last submitted patch, 145: 2107427-language-switcher-labels-145.patch, failed testing.

penyaskito’s picture

#145 is the same than #143. Applying the interdiff should fix it.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
10.78 KB

Right, sorry, this should have been #145.

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

looks good. nothing to complain. let's get this in!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Finally :) Committed 67c8a34 and pushed to 8.0.x. Thanks!

  • alexpott committed 67c8a34 on 8.0.x
    Issue #2107427 by penyaskito, dclavain, Gábor Hojtsy, mashermike, Aron...
Gábor Hojtsy’s picture

Yay, thanks! I finally reopened #1879930: Language selectors are not showing localized to the page language which was postponed on this one 7 months ago.

Gábor Hojtsy’s picture

Issue tags: -sprint

Amazing, thanks.

Status: Fixed » Closed (fixed)

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