I found this before, but I actually hit this while testing #2113701: ConfigTranslationFormBase should implement BaseFormIdInterface.
I'm pretty sure this fix is what is actually meant by the function, otherwise I don't get it at all.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

Status: Active » Needs review
FileSize
1.16 KB

Here we go.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

No, if the language was not possible to load AND it was English, then we can make it appear like English. If we cannot load the language and it was NOT English, we don't want to make it appear as if it would be English. It should probably throw an exception in that case (if empty($language) && $langcode != 'en', since then you have a config file for a language not present on your site. Eg. you created a View in German and then removed German from the site but not the View, then we don't want it to appear as if it is English. Your patch makes it appear as English...

It would not hurt indeed to have tests for this case, but basically this is a shortcut for lazy config developers who do not include a langcode key in their file, and makes us assume those files are English (even if English is not present on the site as in when you install in German). The same assumption exists in the locale module in core when parsing config files, if there is no langcode, we assume it is English.

tstoeckler’s picture

Hmm... that's very strange, though. I think in that case we should adapt getLangcode() and make the case where no langcode is provided more explicit. I.e. change that check to if (empty($langcode)) or something.

Gábor Hojtsy’s picture

That would only work for the lack of langcode case, not when English is not present on the site such as when installing in German

Gábor Hojtsy’s picture

So this logic is supposed to cover two cases:

- config file without langcode code (lazy module coder) => should assume English
- config file with a langcode not on the system ONLY IF the langcode is 'en' => assume "Built-in English specifically"

The way this is achieved is the following:

- getLangcode() defaults the langcode to 'en' if not present in the file (covers the first point)
- geLanguageWithFallback() attempts to covert the langcode to a language object and if it fails AND the langcode was 'en', it will fake a "Built-in English" for the sake of display

So the combined behavior of getLangcode() and getLanguageWithFallback() is what achieves this logic. In getLanguageWithFallback() the langcode will never be empty, because getLangcode() always sets it. So we'll have a langcode. Then we try to load the language. If it was English, we want to use the runtime configured version of it (eg. user may have changed the label to something else and also then custom config files may be created in English, so those files would not be built-in English). If the language cannot be loaded and it was English then we assume that file was a shipped config file, ie. the user did not create English config files on a site if English is not present... So we say "Built-in English". Then if the langcode was not 'en' but we could not load the language, the return value will be null I think or whatever language_load() returns for an unrecognized langcode. We may or may not want to throw an exception then.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +D8MI, +sprint, +language-config
FileSize
1.67 KB

Here is some docs to help cover this :) What do you think?

Gábor Hojtsy’s picture

Status: Needs review » Fixed
Issue tags: -sprint

Committed these docs, please reopen if you have better ideas :)

Gábor Hojtsy’s picture

Title: ConfigNamesMapper::getLanguageWithFallback() does not work in all cases » ConfigNamesMapper::getLanguageWithFallback() logic not clear

Retitled for accuracy.

Status: Fixed » Closed (fixed)

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