String translation does not honor language fallback

I don't get this patch. If I have German, Norvegian and English on my site in this order, would a German site page potentially contain Norvegian strings (instead of English) due to your proposed fallback logic? How is this useful?

Right, so the proposed logic makes sense for a subset of scenarios. Since there is no configuration proposed, applying it to all scenarios sounds like a problem to me.

This fallback function is very useful, especially when making a website containing minority languages, as it is possible to specify different fallback languages for different languages. E.g. I can have a website in Nortern Sami, but if I don't have the string translated in Northern Sami I would prefer more to have it displayed in Norwegian, than English (Most Sami people understand Norwegian much better than English). As a Norwegian I would also prefer to see strings not translated to Norwegian, in Danish, as Norwegian Bokmål writing is based on Danish; some words and sentences can even be written exactly the same. It can also be practical for other issues, e.g. Chinese simplified and Chinese traditional could benefit from using each other as fallback languages. And we can make it possible for visitors to specify their own fallback languages, so they can adapt the translations used specific for their own need.

So in general the language fallback function is practical for several use cases, but it should be a unified way to handle them. Why only use it for entity translations?

So this is not useful in any way without the logic to specify those fallbacks which is not proposed. A module implementing that logic / UI could also swap the service with an implementation that does the fallback, which is a huge point of the service based architecture in Drupal 8. I'm not sure without proving this full circle is good especially since it harcodes a behaviour but only applies to some scenarios.

There is no field level language fallback system in entities either (in Drupal 8), so the language selected is global to the entity. Much like the language selected for the page is global to the page unless specific parts request otherwise. Same applies to configuration translation. I think the feature you are proposing here is more akin to the field level language fallback in content entities that Drupal 7 supported in entity translation. That is not present in Drupal 8 AFAIK.

Gábor;

If we later want to port the language fallback module to Drupal 8; could we do it without LanguageManager::getFallbackCandidates?

As you say that it makes no sense to implement the function without the UI: how could a "module [Drupal 8] implementing that logic [...] swap the service" ? Maybe you could explain more or share link that explain better how this "is a huge point of the service based architecture in Drupal 8"?

And can you explain better if/why the LanguageManager::getFallbackCandidates exists in Drupal 8?

All right I took another look at LanguageManager::getFallbackCandidates. I have not been involved in getting that into core and have only seen it in relation to content fallback. It does seem like a very generic solution to any kind of language fallback priority generation, however, that would need to segment this use case with its own context. At least that looks like it would segregate this feature to its own corner without the chance of unrelated content fallback setups interfering.

1. +++ b/core/modules/locale/lib/Drupal/locale/LocaleLookup.php
   @@ -102,6 +103,27 @@ protected function resolveCacheMiss($offset) {
   +    ¶
   ...
   +          ¶
   ...
   +    ¶
   

   Extra whitespaces should not be here.

2. +++ b/core/modules/locale/lib/Drupal/locale/LocaleLookup.php
   @@ -102,6 +103,27 @@ protected function resolveCacheMiss($offset) {
   +      $fallbacks = \Drupal::languageManager()->getFallbackCandidates($this->langcode);
   

   1. Pass on a context array with an operation of 'locale_lookup' or some other specific string.

   2. Ensure no code in core does language fallback altering without checking for specific contexts, so this does not become a huge performance nightmare like it looks like :)

Also no core committer would consider committing this without test coverage. Basically multiple local languages set up with different sets of strings, a test module implementing a language fallback alter hook for this context and then a test to ensure the right fallbacks are applied.

@matsbla: If needed, how to override services is described in Altering existing services, providing dynamic services and but the way this is the example mentioned there.

Just thinking out loud, I was thinking about getFallbackCandidates for string translation too, but in the case that we need different behaviors for string and content.

I can think of scenarios where we want to fallback strings from Spanish (Venezuela) to Spanish, but for content I want a different behavior (like a Not found or Forbidden) if the content has not translation.

Gábor pointed on IRC that we have $context['operation'] for that and we can provide different fallbacks based on 'locale_lookup' for the locale case and 'entity_view' for the other. We still have also hook_language_fallback_candidates_OPERATION_alter in place too.

So I'm +1 for trying to get this in as makes everything more consistent (although it took me a while to understand).

I'm going to work on this.

I added the 3 required properties as variables because they were out of scope.

Fix tag.

Found several code style problems:

1. +++ b/core/modules/locale/tests/Drupal/locale/Tests/LocaleStringFallbackTest.php
   @@ -0,0 +1,214 @@
   +  /**
   +   * Instance of LocaleTranslation class.
   +   * @var \Drupal\locale\LocaleTranslation
   +   */
   ...
   +  /**
   +   * A mocked user object built from AccountInterface
   +   * @var \PHPUnit_Framework_MockObject_MockObject
   +   */
   

   Code style: add newlines before @var. (More instances not done here).

2. +++ b/core/modules/locale/tests/Drupal/locale/Tests/LocaleStringFallbackTest.php
   @@ -0,0 +1,214 @@
   +    $this->user = $this->getMock('Drupal\Core\Session\AccountInterface');
   +    $this->user->expects($this->any())
   +                ->method('getRoles')
   +                ->will($this->returnValue(array('anonymous')));
   ...
   +    list($user, $config_factory, $language_manager) = array($this->user, $this->config_factory,$this->language_manager);
   +    $this->container->expects($this->any())
   +                    ->method('get')
   +                    ->will($this->returnCallback(function ($arg) use ($user, $config_factory, $language_manager)  {
   +                      $map = array(
   +                          'current_user' => $user,
   +                          'config.factory' => $config_factory,
   +                          'language_manager' => $language_manager
   +                      );
   +
   +                      return $map[$arg];
   +                    }));
   

   I don't think this indentation is Drupal code style compatible. Also lots more elsewhere.

3. +++ b/core/modules/locale/tests/Drupal/locale/Tests/LocaleStringFallbackTest.php
   @@ -0,0 +1,214 @@
   +  /**
   +   * Test string translation with fallback.
   +   * Note that context is irrelevant here.
   +   * It is not used but it is required.
   +   */
   +  public function  testStringTranslationWithFallback(){
   

   Code style: first function comment line should be on its own line, one space between 'function' and function name.

4. +++ b/core/modules/locale/tests/Drupal/locale/Tests/LocaleStringFallbackTest.php
   @@ -0,0 +1,214 @@
   +  /**
   +   * Helper method that returns different fallback chains
   +   * depending on requested language.
   

   Function doc comment should be at most up to 80 chars, not wrapped.

5. +++ b/core/modules/locale/tests/Drupal/locale/Tests/LocaleStringFallbackTest.php
   @@ -0,0 +1,214 @@
   +   * @param string $arg
   +   *  Language code
   +   *
   +   * @return array
   +   *  Array of language codes
   ...
   +   * @param array $arg
   +   *  Array containing three keys: language, source and context.
   ...
   +   *
   +   * @return StdClass|boolean
   +   *  An object containing one property 'translation' if translations is found; otherwise TRUE.
   

   Not indented properly.

ok Gábor Hojtsy, I going review the problems that you comments.

@Gabor I fixed all problems that you comments. Could you review it?

Found a typo in a docblock, otherwise looks good to me. Hope that Gábor agrees.

+++ b/core/modules/locale/tests/Drupal/locale/Tests/LocaleStringFallbackTest.php
@@ -30,24 +30,29 @@ class LocaleStringFallbackTest extends UnitTestCase {
+   * A mocked config facktory built with UnitTestCase::getConfigFactoryStub

*factory

thanks for your comment @penyaskito , I added new patch that resolve this typo.

+++ b/core/modules/locale/tests/Drupal/locale/Tests/LocaleStringFallbackTest.php
@@ -0,0 +1,241 @@
+    list($user, $config_factory, $language_manager) = array(
+      $this->user,
+      $this->config_factory,
+      $this->language_manager,
+    );

This may look cooler, but doing the three assignments on their own lines would be 2 lines less and less cycles (no intermediary array).

Thank you @Gábor. I added new patch.

Looks good to me.

saw a few nits while checking if this still applied. went a head and fixed them.

[edit removed some dreditor mistake]

1. +++ b/core/modules/locale/lib/Drupal/locale/LocaleLookup.php
   @@ -11,6 +11,7 @@
   +use Drupal\Core\Language\LanguageManager;
   

   IDE says this use is unused. It was added in patch 6 (comment #7) when \Drupal::languageManager()->getFallbackCandidates() call was put in the patch. But it might not need to be "used" for that.

2. +++ b/core/modules/locale/lib/Drupal/locale/LocaleLookup.php
   @@ -102,6 +103,27 @@ protected function resolveCacheMiss($offset) {
   +    // If there is no translation available for current language
   +    // then use language fallback to try other translations
   

   wrapping these to 80 chars and using period to end the sentence.

   "Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over, with a few exceptions..."

   "All documentation and comments should form proper sentences, use proper grammar and punctuation..."

   https://drupal.org/node/1354#drupal

3. +++ b/core/modules/locale/tests/Drupal/locale/Tests/LocaleStringFallbackTest.php
   @@ -0,0 +1,239 @@
   + * Definition of Drupal\locale\Tests\LocaleStringFallbackTest.
   

   This is Contains now with \Drupal... probably copied from an older example test . https://drupal.org/node/1354#file

4. +++ b/core/modules/locale/tests/Drupal/locale/Tests/LocaleStringFallbackTest.php
   @@ -0,0 +1,239 @@
   +   * A mocked storage to use when instantiating LocaleTranslation objects.
   +   *
   +   * @var \PHPUnit_Framework_MockObject_MockObject
   ...
   +   * A mocked user object built from AccountInterface.
   +   *
   +   * @var \PHPUnit_Framework_MockObject_MockObject
   

   should these and others be type hinted with | (or) like in PermissionsHashTest?

5. +++ b/core/modules/locale/tests/Drupal/locale/Tests/LocaleStringFallbackTest.php
   @@ -0,0 +1,239 @@
   +   * Instance of LocaleTranslation class.
   +   *
   +   * @var \Drupal\locale\LocaleTranslation
   +   */
   +  protected $translation;
   

   I think we dont want the type in the one line summary. Changing that to The translation.

6. +++ b/core/modules/locale/tests/Drupal/locale/Tests/LocaleStringFallbackTest.php
   @@ -0,0 +1,239 @@
   +    $this->user->expects($this->any())
   +                ->method('getRoles')
   ...
   +    $this->container->expects($this->any())
   +    ->method('get')
   +    ->will($this->returnCallback(function ($arg) use ($user, $config_factory, $language_manager) {
   

   I think we just indent the next lines of -> with two spaces. I looked for other examples in core, and 2 spaces seemed to be what was done. Also suggested in #1516460: Define coding standard for chained method calls.

7. +++ b/core/modules/locale/tests/Drupal/locale/Tests/LocaleStringFallbackTest.php
   @@ -0,0 +1,239 @@
   +   * @param string $arg
   ...
   +  public static function getFallbackChain($arg) {
   ...
   +    return $chains[$arg];
   

   :) I dont think we usually use $arg like this. Changed to $langcode since that is what it is.

8. +++ b/core/modules/locale/tests/Drupal/locale/Tests/LocaleStringFallbackTest.php
   @@ -0,0 +1,239 @@
   +   * @return StdClass|boolean
   +   *   An object containing one property 'translation' if translations is found;
   +   *   otherwise TRUE.
   

   a. StdClass doesn't seem like the best type hint here. but I'm not sure we dont have that many phpunit examples yet. But if it's just a object then.. hm. "object (NOT "stdClass")" https://drupal.org/node/1354#types

   b. Also, should be bool

   "bool (NOT "boolean" or "Boolean")" https://drupal.org/node/1354#types

   c. why return *true* if you *dont* find the translation?

9. also added a newline before the closing brace of the class https://drupal.org/node/608152

ran the tests locally per https://drupal.org/node/2116263
before patch, (1 test, 2 assertions)
after patch (3 tests, 28 assertions)

removing the needs tests tag, since the patch has all the tests it needs I think.

All changes look good, superb cleanups.

49: add_language_fallback_to_string_translation-2122175-49.patch queued for re-testing.

ok. opened another issue we can look at later. #2188715: Document return value in \Drupal\locale\LocaleTranslation::getStringTranslation

Cleaned up the unit test. We usually (always?) use an anonymous function with returnCallback().

Here are some notes I took while I tried to do that... but my attempt (in the interdiff attached) was not as elegant. :)
------
actual callback static methods
->will($this->returnCallback('Drupal\locale\Tests\LocaleStringFallbackTest::stringStorageMockFindTranslation'));

->will($this->returnCallback('Drupal\locale\Tests\LocaleStringFallbackTest::getFallbackChain'));

should be inline anonymous functions,

since then
they would be
easier to understand, and
move the code closer to where it is called

Also, we dont have static methods for any other returnCallback in core, but we do do the inline anonymous function 15 times.
See ag "returnCallback\(" --ignore-dir vendor

Looked at examples in
MigrateTestCase::getMigration()
FormBuilderTest::testSetErrorByName()

------------------

a. tim's probably more up on the @group stuff's so that's fine.
b. Note that a separate novice issue can be made to change the other lowercase group to be Locale also.
c. And adding the 2 explicit properties was something I thought of doing to, so that looks good. (could add the one line summary to them. I'll do that.)
d. Ah, I missed those camelCase property names. good.
e. it was the use, using the translations array in the returnCallback, that I saw in the examples, but couldn't quite make the leap to do. very cool.

f. Not sure about the container thing. It's probably obvious to tim, but maybe a little explanation as to why that's better would help me spot a similar situation in the future.

--
without patch
./vendor/bin/phpunit --group Locale
(4 tests, 16 assertions)

with
(6 tests, 40 assertions)

so that still looks good also.

--
should be green and I can't see anything else to improve. rtbc from me. (didn't try it manually though)

the two fails reported on patch 56 are unrelated/random and in the views_ui tests

In the test results, in the collapsed review log fieldset can see burried there:

Fatal error: Allowed memory size of 268435456 bytes exhausted (tried to allocate 268435456 bytes) in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php on line 336

Let's see how 57 does.

Discussed more with @YesCT, we're testing the wrong class here.
We should be testing LocaleLookup in isolation.
In order to do so, we should properly inject everything.

The interdiff is vaguely useless, since I had to move the test.

Doh!

sweet! that is the same interdiff I was just about to upload. I might be getting the hang of this!

64: locale-2122175-64.patch queued for re-testing.

Great, improvements, thanks! Looks good to me.

64: locale-2122175-64.patch queued for re-testing.

locale-2122175-64.patch no longer applies.

error: patch failed: core/modules/locale/lib/Drupal/locale/LocaleTranslation.php:63
error: core/modules/locale/lib/Drupal/locale/LocaleTranslation.php: patch does not apply

reroll

that applies, removing needs reroll tag.

Thanks for the reroll.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleLookup.php
@@ -99,18 +120,46 @@ protected function resolveCacheMiss($offset) {
+        foreach($fallbacks as $langcode) {

Minor nitpick to be fixed on commit/reroll: missing white space before the parenthesis.

71: locale-2122175-71.patch queued for re-testing.

71: locale-2122175-71.patch queued for re-testing.

this is rtbc, but the issue summary is out of date.
I'm starting to work on that.

ok. no major changes to the summary. I think the proposed resolution is pretty much the same.

Committed and pushed to 8.x. Thanks!

Superb, thanks!

This issue ended up sparking a critical bug. Applying the default content language fallback rules to interface translation is nonsensical. See #2240459: Applying default language fallback rules to interface translation has impressively bad results for a solution that removes the incorrect defaults but keeps the extensibility. @SiliconMind, @dclavain, @plach, @YesCT: reviews welcome there at your convenience :)