Problem/Motivation

Found this while working on #2212069: Non-English Drupal sites get default configuration in English, edited in English, originals not actually used if translated. When config entities get imported from shipped defaults, they are resaved after instantiated in the import and therefore the undefined language default in ConfigEntityBase gets applied. So if the original shipped file did not include a language code, the entity will now (incorrectly) be undefined language. While configuration otherwise assumes English defaults if the langcode was not specified (see LocaleConfigManager::get(), ConfigNamesMapper::getLangcode(), etc.), this makes the configuration now *know* undefined instead of keeping to assume English.

We also cannot assume lack of language on ANY config entity because even RDF mappings, and view/form displays which do not have any human language text on them may take third party settings, which are impossible to predict if they would contain human language content or not. So unless we want to require people to set some concrete langcode on config entities when setting a third party setting that includes human language content, we need to keep assuming that all config entities are in a human language (and not 'und' or NULL).

Proposed resolution

Option 1. Fix the concrete files in core and document that config entities will not have the same missing langcode fallback at least while being imported.
Option 2. Fix the import code to add merge langcode: en to loaded default configuration before instantiating entities.
Option 3. Change the default in ConfigEntityBase to English and document it.

Patch implements Option 3 for now.

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Title: Config entities need to ship with language or assumed undefined » Config entities need to ship with language or are assumed undefined
Priority: Normal » Major
FileSize
2.1 KB
Gábor Hojtsy’s picture

Issue summary: View changes
effulgentsia’s picture

While configuration otherwise assumes English defaults if the langcode was not specified (see eg. ConfigNamesMapper::getLangcode())

If ConfigNamesMapper::getLangcode() defaults to English, then seems to me that ConfigEntityBase::$langcode should as well. Which is perhaps an option 3 (or 2b)? Otherwise, if we want undefined to be the default, in order to not bias English, then I think we should do so everywhere (e.g., change ConfigNamesMapper accordingly), and then do option 1.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

The English default is supposed to be a DX helper so people don't need to include a langcode in their file. We can default to 'en' in ConfigEntityBase too and let overriding config entities to set different defaults. For example RDF mappings cannot contain a translatable (AKA human readable) string, so they make sense to default all the time to 'und'.

Gábor Hojtsy’s picture

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

Dropped option 1 and implemented Option 3 then. No interdiff, because no code is left from the prior patch.

Berdir’s picture

Don't we actually set the langcode to the site default now? shouldn't the default value be NULL then?

view/form displays also don't have a label, so they should be set to UND (and forced to that) as well. How do we define that? The problem is that the schema now forces that they have a langcode, even if it is not relevant for them.

Status: Needs review » Needs work

The last submitted patch, 5: 2451885-5.patch, failed testing.

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
0 bytes
2.42 KB

@Berdir: the 'en' default is supposed to be a DX feature, so careless developers don't need to specify 'en'. We do set the langcode to the site default **when you create a new entity** (in ConfigEntityStorage::doCreate()), but the problem is with config import. When we import a config entity that lacks a langcode, the ConfigEntityBase::$langcode applies, which is 'und' now.

As a matter of fact, I realized that we cannot assume lack of language on ANY config entity because even RDF mappings, and view/form displays may take third party settings, which are impossible to predict if they would contain human language content or not. So unless we want to require people to set some concrete langcode on config entities when setting a third party setting that includes human language content, we need to keep assuming that all config entities are in a human language (and not 'und' or NULL).

Gábor Hojtsy’s picture

FileSize
1.78 KB

Patch with more than 0 bytes :D

The last submitted patch, 8: 2451885-8.patch, failed testing.

Gábor Hojtsy’s picture

FileSize
78.31 KB
80.09 KB

Following that thinking, replacing all explicit langcode: und with langcode:en then. Almost all are in test views.

vijaycs85’s picture

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -83,9 +83,18 @@
    +   * Assumed to be English by default. ConfigEntityStorage will set an
    +   * appropriate language when creating new entities. This default applies to
    +   * imported default configuration where the language code is missing. Those
    +   * should be assumed to be English. All configuration entities support third
    +   * party settings, so even configuration entities that do not directly
    +   * store settings involving text in a human language may have such third
    +   * party settings attached. This means configuration entities should be in one
    +   * of the configured languages or the built-in English.
    

    documentation++

  2. +++ b/core/modules/field/tests/modules/field_test_config/config/install/field.field.entity_test.entity_test.field_test_import.yml
    @@ -1,5 +1,5 @@
    +langcode: en
    
    +++ b/core/modules/field/tests/modules/field_test_config/config/install/field.field.entity_test.entity_test.field_test_import_2.yml
    @@ -1,5 +1,5 @@
    +langcode: en
    
    +++ b/core/modules/field/tests/modules/field_test_config/config/install/field.field.entity_test.test_bundle.field_test_import_2.yml
    @@ -1,5 +1,5 @@
    +langcode: en
    

    Do we want to change this to 'en'. Can't we remove the langcode itself? setting it to default en wouldn't cause any problem when installing in other language?

Gábor Hojtsy’s picture

FileSize
81.75 KB
1.66 KB

Adding tests too checking implicit and explicit langcode in config entity imports.

@vijaycs85: so long as the default configuration (may) contain English strings, it should be marked English, not a problem with foreign language installs at all. See more at #2212069: Non-English Drupal sites get default configuration in English, edited in English, originals not actually used if translated.

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

Test looks good and ready to go.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2451885-14.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
3.15 KB
83.07 KB

Let's add the new test config to a new module so it does not affect existing tests, ha.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Seems sensible. Back to RTBC.

effulgentsia’s picture

Leaving this issue at RTBC, because I think the code changes in the patch make sense.

But, regarding this doc:

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -83,9 +83,18 @@
+   * should be assumed to be English. All configuration entities support third
+   * party settings, so even configuration entities that do not directly
+   * store settings involving text in a human language may have such third
+   * party settings attached. This means configuration entities should be in one
+   * of the configured languages or the built-in English.

See #2453551-4: TranslationLanguageRenderer tries to add langcode field to the view for entity types that have no langcode.

If based on further discussion we need to change the doc, we can do so in a follow-up.

Gábor Hojtsy’s picture

@effulgentsia: Config entities at least if they extend from ConfigEntityBase all have a langcode. Unlike content fields themselves, which do have their own langcode (as related to the discussion in #2453551: TranslationLanguageRenderer tries to add langcode field to the view for entity types that have no langcode), third party settings have no way to specify their langcode, they just inherit the langcode provided on the config file level.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed eaffcfc and pushed to 8.0.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks, yay!

  • alexpott committed eaffcfc on 8.0.x
    Issue #2451885 by Gábor Hojtsy: Config entities need to ship with...

Status: Fixed » Closed (fixed)

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