Problem/Motivation

  1. Install in a foreign language. Will download translations and import.
  2. Add English and enable English to be translatable.
  3. Now even if you look at English pages, they will be in the other language UI.

This is due to #2122175: String translation does not honor language fallback. Currently the default language fallback rules are applied to looking up interface translations which means if there is no string translation, it will try to look up the string translation *again* in the same language, then in all other languages in their order and then in the undefined language. Neither of these make sense most of the time. Language fallback for interface translation should be an explicit configuration, ie. if you want to fall back from informal German to German or if you have regional French set up for Canada and France and the Canadian should fall back on the France one or vice versa.

Proposed resolution

Fix the default language fallback setup, so there is no suggested language fallback for interface translation. Either core can implement a UI / setting for this later or contrib can provide one. Add tests to ensure the default values in the hook and that it gets all info necessary for providing fallback langcodes.

Remaining tasks

Review. Commit.

User interface changes

Interface translation will not "randomly" fall back :D

API changes

Context on getFallbackCandidates now contains the langcode key/value instead of taking it as a separate optional argument. Signature change:

-  public function getFallbackCandidates($langcode = NULL, array $context = array());
+  public function getFallbackCandidates(array $context = array());
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Unrelated, but:

3) renamed the "human-readable" "English" to "English_custom", just to be clear it might include some personal preferences (e.g. one might want to have "Users" instead of the rather-counterintuitive, though, perhaps, more "PC", "People".

Unnecessary, English now has a setting that allows it to be translated as well, so you can translate from English as it's written in the source code to English in the UI.

I'm not sure what exactly you see as the bug here, path prefix can only work reliable if everything has a path, if there is none, then negotation falls back to the next method, which might be browser language.

That should however not change based on what you import, but should be stable, you just might not have had translations before. Try to show the language switcher block to see which is the current language.

penyaskito’s picture

I tried to reproduce it, but it worked as expected.

However, in your step 5), the Language select probably had the default language and you didn't change it, so it understood that those translations were for English.

Could you verify that?

One of the new fancy features of D8 UX is that when you select a Drupal.de.po translation file, it automatically pre-selects German (de). This is not the case if the file is called de.po. As this is the default file name when exporting Gettext PO files, this should be supported.

I would rephrase this issue as "Preselect language when uploading translations previously exported"

penyaskito’s picture

Status: Active » Needs review
Issue tags: +JavaScript
FileSize
941 bytes

Attached a patch, which modifies the regular expression so myproject.langcode.po and langcode.po are both supported.

penyaskito’s picture

Issue tags: +language-interface, +sprint
LoMo’s picture

@Berdir (Re #1) I know that adding "custom" to the UI name was unnecessary. And I don't think that this caused the effect, but I was saying what I actually *did* do... and in a production site, it might be that admin would want to clarify that the "Site English" is a "tweak strings" translation "language". ;-)

It is still possible (and preferable) to use the site default language with no prefix. Yes, /en/ should work as a prefix, but it should not be necessary to have it in the path. It should be possible (and apparently is possible) to remove the prefix for your "default language". In any case, changing the setting to use a prefix of "en" made no difference. And, if I'm not mistaken, the default for the "default language" is "no path prefix", right?

@penyaskito (Re #2): No. That was the first thing I checked. English has no translations. German and French were about 77% translated (as I used the D7.26 de/fr .po files and uploaded them -- perhaps auto import would have more translations, though I don't know if there are yet any D8-specific .po files). Anyway, the "workaround" for this is to have a "customized" English added with "plain English" next in the "weights", but in some cases it might not work as one would want. I think, ideally, each language should have a "fallback translation chain", e.g. "If no Mexican Spanish translation is available, use Castilian Spanish; if no Castilian Spanish is available, use English (another related language might be more appropriate in some cases, though).

I have made a few changes since I had observed the issue in question, but you can see that I only imported translations in the non-English languages.
Only translations for DE and FR were imported. I did that when Karl (kfritsche) looked at this with me and he determined it had to do with a fallback pattern for the translation function. But, IMHO, if we are not using the language prefix for German or French, just because it was imported and has translations should not be a reason to see it when you are browsing in English. If I was confused by the behavior and found it seemed "buggy", I don't think I'd be the only one.

I'll try to take the time to do a bit more experimenting. Since I've been so uninvolved for so long, due to too many other commitments (and now my wife being out of town each work-week and back at the weekends when I might have had some time before), so... if I can do a bit more, I'm a bit like a "fresh user" who doesn't already know how things *do* work, and will make assumptions about how they *should* work, so I'm kind of the ideal candidate for "usability" studies of D8MI right now.

penyaskito’s picture

Forked my comment and patch at #2 - #3 to #2240555: Preselect language when importing translations previously exported while we investigate this issue further.

Gábor Hojtsy’s picture

Status: Needs review » Postponed (maintainer needs more info)

So I think this may be due to #2122175: String translation does not honor language fallback, although that should theoretically not happen with the language order you are showing on the screenshot. Can you confirm if it happens due to that or not?

Gábor Hojtsy’s picture

Fix topic tag.

Gábor Hojtsy’s picture

It would be great to get confirmation so work can start :)

Gábor Hojtsy’s picture

Priority: Normal » Major
Status: Postponed (maintainer needs more info) » Active

Although @LoMo disappeared I think we can consider this due to a "feature" not implemented well.

Gábor Hojtsy’s picture

Issue tags: +Drupalaton 2014

We reproduced the same problem and made me VERY puzzled in front of an audience at Drupalaton :D Should actually be relatively easy to fix.

Gábor Hojtsy’s picture

Title: Strange fallback in UI translation (translation() function bug?) » String fallback implementation does not make sense
Priority: Major » Critical
Issue summary: View changes
Status: Active » Needs review
FileSize
520 bytes

A quick try for no fallbacks for now. We should add a settings snippet for setting these fallbacks otherwise it is not overridable.

Gábor Hojtsy’s picture

Helps to name the function right, so it is picked up :D

Gábor Hojtsy’s picture

Oh, well, this should totally fail, but the unit test has the language fallback rules baked in in #2122175: String translation does not honor language fallback. So we need a webtest which relies on the actual module setup. Now adding an actual settings UI. Did not make out how to map this down to the language fallback layer.

Status: Needs review » Needs work

The last submitted patch, 14: 2240459-fallback-settings-for-locale-14.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
5.02 KB
715 bytes

Fix form error. Should be green.

Gábor Hojtsy’s picture

Title: String fallback implementation does not make sense » Applying default language fallback rules to interface translation has impressively bad results
Issue summary: View changes
Issue tags: -D8UX usability, -JavaScript, -Needs tests
FileSize
2.56 KB
4.43 KB

Retitled. Here is an entirely new direction. I decided that we should not block a release on a UI for this because there was no UI for this before either. Hah! So we should just provide sane defaults and then can still provide a UI later in a non-release blocking issue. So the solution is to provide sane defaults for locale based fallback lookups, that is no fallback rules by default. The default fallback rules that included falling back on itself and on undefined language were not even performant, let alone make sense...

Also includes a test that there are no default fallback rules. This should fix the bug and prove it is fixed. Actual configurable fallback rules based on a UI improved from #16 can be added later on, not in scope for this patch.

Status: Needs review » Needs work

The last submitted patch, 17: 2240459-interface-language-fallback-defaults.patch, failed testing.

Status: Needs work » Needs review
penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

Agree that we shouldn't assume which languages we can fallback to.
I couldn't find anything stopping this to be committed, and fixes an important DrupalWTF, so let's get this in.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Meh, sorry. Not having an implementation of this hook in fact fired back. The hook does not get any info about the language being looked up, so a hook implementation could not look up proper fallback langcodes for the given language. Either we need to pass that on in the context or another argument on the hook.

SiliconMind’s picture

But do we really need this:

// The first candidate should always be the desired language if specified.
if (!empty($langcode)) {
   $candidates = array($langcode => $langcode) + $candidates;
}

This seems like unnecessary overhead. What for do we need to add current language to the fallback candidates array? What's the benefit? After all this is a fallback candidates for a specific language, right? So why do we include that language anyway?

I think that we should change the way default fallback works. I'd go in this direction:

  1. Make default fallback to fallback only to the default language (or do not fallback if language is default). I really think that the case when you want to fall back through all available languages is very uncommon. For example there is no point in serving Chinese content to the German users. What's the chance that German user will have fonts to at least display that content (not mentioning understanding it) :)
  2. Make clear statement on the language config page that in case of missing content for the particular language, the default language will be used unless user defines his own language fallback chain. And that leads us to point 3:
  3. Add UI for language fallback configuration.
Gábor Hojtsy’s picture

@SiliconMind: it would be dangerous to try and define all the use cases of the fallback system and how it makes sense for content vs. interface in this issue. If you want this resolved in a timely manner that is. And people I showed the current system to have been perplexed why their interface translation appears in totally mixed ways. That is a very real problem that you can easily reproduce now.

Whether a site wants to fall back across all its languages for content display is very situational. If you are in Switzerland where all 3 official languages are supposedly equal, it may totally make sense. Also whether this API should return the langcode itself in the array is out of scope here AFAIS. The existing uses of this API in EntityManager and views Field integration both assume the original langcode is part of the fallbacks, ie. it is not adding the original langcode at first outside of invoking this method. One could argue this is the most flexible way, since only what is generated in this method can be altered with the hook.

SiliconMind’s picture

@Gabor, so we shouldn't make any assumptions and provide at least some fallback behavior switch.

Also I've noticed that LanguageManager::getFallbackCandidates returns only default language but ConfigurableLanguageManager::getFallbackCandidates returns the array of all languages. Why the change/inconsistency? I'm still a bit confused with the D8 architecture and sometimes it's very difficult to track usage of certain functions. When ConfigurableLanguageManager takes over LanguageManager implementation and why the change?

matsbla’s picture

I think it is not a good idea at all to add any automatic fallback chain for languages in core. Drupal should not give any fallback translations unless someone has explicitly configured it to do so, if not you can easily get confusing results, e.g. in front of an audience at Drupalaton ;)

The important thing is that core gives the possibility for other contribution modules to take full control of how these fallback translations are handled. E.g. if you check last dev. version of Language fallback module (https://www.drupal.org/project/language_fallback) it is possible to configure individual fallback languages for each language, and you can even make different fallback languages for one language depending on the country of the visitor. E.g. a person speaking Northern Sami can get Norwegian fallback language if the person is from Norway, Swedish fallback language if the person is located in Sweden, Finish in Finland and Russian in Russia. Then it is possible to make sophisticated logic for fallback languages for many different cases. However, these choices should only be made depending on the website project and in close communication with local people, fallback languages should not be given automatic by core.

For other website projects, e.g. a company in Swiss, with content in Italian/French/German, it could be more relevant to give all translations on the website using an automatic fallback chain. In that case there can be made another, more simple, contribution module doing that. Core should only give possibility to control fallback languages and let other modules do that job.

E.g. today in D7 everyone gets strings in English if they are not translated to UI language, now it will be possible to decide to not give English strings to people not speaking English.

Until Drupal core is able to communicate with locals and make decisions together with them, core should not make any decisions on fallback languages :) I run a website with more than 150 languages, I'm very happy that there is no automatic fallback languages set by core now and hopes it stays that way.

Gábor Hojtsy’s picture

@SiliconMind: The LanguageManager is a core langugae service which is replaced by ConfigurableLanguageManager when languages become configurable (aka when you enable language module). This happens in core/modules/language/src/LanguageServiceProvider.php. BTW language module also provides ConfigurableLanguage class among other things... Anyway, the logic you are concerned about is not used in this patch, so again I don't think your concerns are in scope here.

@matsbla: indeed this patch makes it so that there are no defaults. Core may provide some UI to configure fallbacks later, not with the granularity you refer exists in contrib but at least on the pure language level. It will still be alterable by contrib implementations so no harm done there either.

SiliconMind’s picture

@Gabor, thanks for explanation. I guess D8 docs still needs a lot of work. Very often it's really unclear when and why some classes are used and functions overridden. What's more some API pages are missing the list of calls to some functions (getFallbackCandidates for example).

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
12.15 KB
10.35 KB

Now expanded the patch to pass on langcode in the context. In fact to take langcode in the context. The docs already included that that is how it would be passed in, so surprising it was not used as such. With an optional langcode and a context that has as much useful info, they are better together. Also expanded the locale test to ensure now it gets enough info to decide on a fallback (for a contrib module and/or future core UI @matsbla). I think this is as far as we can go here.

Status: Needs review » Needs work

The last submitted patch, 29: 2240459-interface-language-fallback-defaults-29.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
12.2 KB
1.15 KB

So the hook gets the original string as context too. We cannot be sure what was the last string, so we should only check for operation and langcode. Should now pass. Reviews please :)

Gábor Hojtsy’s picture

Issue summary: View changes
penyaskito’s picture

Downloading and applied the patch, looks good, just some questions.

+++ b/core/modules/language/src/ConfigurableLanguageManager.php
@@ -326,17 +326,24 @@ public function updateLockedLanguageWeights() {
+      if (empty($context['operation']) || strpos($context['operation'], 'locale') !== 0) {

I don't really like special treatment for locale. Why do we need this? I think it was because we added locale_language_fallback_candidates_locale_lookup_alter during the life of this issue, but we don't have that anymore. And if there is only locale_lookup, why doing the strpos and not comparing to locale_lookup?

I'm missing also docs about the possible operations that can appear in the context, at least in core. As far as I found, those are "views_query", "entity_view" and "locale_lookup".

I cannot find any other issue avoiding this to be committed.

Gábor Hojtsy’s picture

Thanks for the feedback. Considerably improved docs and changed the condition as suggested.

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

RTBC then.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great catch.

Committed and pushed to 8.x. Thanks!

  • webchick committed 38587bd on 8.0.x
    Issue #2240459 by Gábor Hojtsy, penyaskito | LoMo: Fixed Applying...
Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks!

Status: Fixed » Closed (fixed)

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