Problem/Motivation
- Install in a foreign language. Will download translations and import.
- Add English and enable English to be translatable.
- 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());
Comment | File | Size | Author |
---|---|---|---|
#34 | interdiff.txt | 2.64 KB | Gábor Hojtsy |
#34 | 2240459-interface-language-fallback-defaults-34.patch | 13.15 KB | Gábor Hojtsy |
#31 | interdiff.txt | 1.15 KB | Gábor Hojtsy |
#31 | 2240459-interface-language-fallback-defaults-31.patch | 12.2 KB | Gábor Hojtsy |
#29 | interdiff.txt | 10.35 KB | Gábor Hojtsy |
Comments
Comment #1
BerdirUnrelated, but:
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.
Comment #2
penyaskitoI 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"
Comment #3
penyaskitoAttached a patch, which modifies the regular expression so myproject.langcode.po and langcode.po are both supported.
Comment #4
penyaskitoComment #5
LoMo CreditAttribution: LoMo commented@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.
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.
Comment #6
penyaskitoForked my comment and patch at #2 - #3 to #2240555: Preselect language when importing translations previously exported while we investigate this issue further.
Comment #7
Gábor HojtsySo 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?
Comment #8
Gábor HojtsyFix topic tag.
Comment #9
Gábor HojtsyIt would be great to get confirmation so work can start :)
Comment #10
Gábor HojtsyAlthough @LoMo disappeared I think we can consider this due to a "feature" not implemented well.
Comment #11
Gábor HojtsyWe reproduced the same problem and made me VERY puzzled in front of an audience at Drupalaton :D Should actually be relatively easy to fix.
Comment #12
Gábor HojtsyA quick try for no fallbacks for now. We should add a settings snippet for setting these fallbacks otherwise it is not overridable.
Comment #13
Gábor HojtsyHelps to name the function right, so it is picked up :D
Comment #14
Gábor HojtsyOh, 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.
Comment #16
Gábor HojtsyFix form error. Should be green.
Comment #17
Gábor HojtsyRetitled. 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.
Comment #21
penyaskitoAgree 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.
Comment #22
Gábor HojtsyMeh, 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.
Comment #23
SiliconMind CreditAttribution: SiliconMind commentedBut do we really need this:
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:
Comment #24
Gábor Hojtsy@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.
Comment #25
SiliconMind CreditAttribution: SiliconMind commented@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?
Comment #26
matsbla CreditAttribution: matsbla commentedI 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.
Comment #27
Gábor Hojtsy@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.
Comment #28
SiliconMind CreditAttribution: SiliconMind commented@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).
Comment #29
Gábor HojtsyNow 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.
Comment #31
Gábor HojtsySo 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 :)
Comment #32
Gábor HojtsyComment #33
penyaskitoDownloading and applied the patch, looks good, just some questions.
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.
Comment #34
Gábor HojtsyThanks for the feedback. Considerably improved docs and changed the condition as suggested.
Comment #35
penyaskitoRTBC then.
Comment #36
webchickGreat catch.
Committed and pushed to 8.x. Thanks!
Comment #38
Gábor HojtsyThanks!