LocaleConfigManager::updateConfigTranslations() deletes translations if a config object's name happens to match that of a shipped configuration object

So we need to know if a configuration object was created during extension (module, theme or profile) installation. Talking with @dawehner and @tstoeckler we discussed whether or not we should add a flag to configuration entities, create an installation manifest listing UUIDs or add back in UUIDs into default configuration. There is also an issue that Locale needs to know which version of the default configuration was used. So storing a flag on configuration entities doesn't help with this and neither does writing a manifest. Writing a single manifest will also be painful when merging configuration after multiple developers have done installations. Adding UUIDs could help if the module maintainer changed everytime they changed something but this would be easy to forget. An approach that might work will be store the hash of the default configuration data on the configuration entity. Then, if one is present you know that the configuration originates from default config and you can know which version of default config was used.

So we need to know if a configuration object was created during extension (module, theme or profile) installation.

That sounds like a good direction. I first misread this as needing to know whether the translation was created at that time, which we cannot really be sure about, given that translations may be created later on when the strings become translated in that language for that file. (Empty translations are not created in the meantime).

Using a hash of the default config values might allow us to solve #2428045: Comparing active storage to install storage is problematic, install storage may change anytime

#2411967: Allow config entities to specify the config key <-> entity property mapping means we can't use camel case here yet...

Fixing the test fails...

Here's a patch that includes a fix for detecting shipped configuration and a test.

Now we have to work out if we want to try an upgrade path...

Updated IS

Fixing the test fail - nice that we have a test for that - it would have been easy to break.

How could we detect this for an upgrade path, unless the config is the same as the install version still? (Apart of UUID). That may be covering some cases I guess, but not very universal. I don't think we can do this universally, any modification may have happened to the config.

Improving test coverage and injecting the service.

Discussed with @catch and @Gábor Hojtsy. We agreed that the upgrade path should check all active config and if name does not match - do not add hash. If name matches but has custom config translations - do NOT add hash, otherwise add hash. This way we don;t get data loss and most sites will get localisations for default config.

We also agreed that we should add the key to simple configuration since we need to fix #2428045: Comparing active storage to install storage is problematic, install storage may change anytime for them too.

And lastly, we agreed to put the key inside a mapping _core so we can add further keys if necessary with less disruption - see #2362317: No namespacing of "system" properties in ConfigEntities / yaml files means lockdown after 8.0 is out

Adding the hash to simple configuration as well and moved the hash into a _core mapping for all configuration. @catch suggested splitting the upgrade path into another critical issue.

We can do the upgrade path in yet another critical, that's fine.

+++ b/core/config/schema/core.data_types.schema.yml
@@ -87,12 +87,22 @@ color_hex:
+    _core:
+      type: _core_config_info

Its unfortunate that we did not reserve a key sooner for this, it would have looked a bit nicer. But well, that is what we can do now :)

The update looks good to me.

Fixed all the test fails. The fails in Drupal\config\Tests\ConfigInstallTest are interesting because this is testing installing different configuration collections (for example language overrides). Whilst discussing with @Gábor Hojtsy on IRC I realised that the LocaleConfigManager is only working with default config collection so therefore I think we should park the question of whether configuration created through the installer for other collections should have this information. In fact, the use case for creating configuration in other collections than the default during module install is pretty spurious - however the same can not be said for profile installation where the ability to create config in other collections is very useful.

Created the postponed upgrade path issue: #2628004: Create an upgrade path to determine if default_config_hash should be added (2625258)

+++ b/core/modules/locale/src/LocaleConfigManager.php
@@ -111,13 +119,14 @@ class LocaleConfigManager {
+  public function __construct(StorageInterface $config_storage, StringStorageInterface $locale_storage, ConfigFactoryInterface $config_factory, TypedConfigManagerInterface $typed_config, ConfigurableLanguageManagerInterface $language_manager, LocaleDefaultConfigStorage $default_config_storage, ConfigManagerInterface $config_manager) {

Is it worth adding (@internal) setter injection to the class instead and updating the service definition to set that, so that if someone happened to be extending the class, we wouldn't break that in a patch release - although getDefaultConfigLangcode() might then need to account for the property not being set as well.

We could then have an 8.1.x follow-up to change the constructor etc.

Or we could just decide that the chances of overriding this are so slim, that it's only a theoretical bc break and make the change as-is in a patch release.

(The other two options are an unscheduled minor release, which I don't think this is a disruptive enough change to warrant, or delaying this to the scheduled minor release, which I think this is too severe a bug to delay for).

Here's setter injection - if we agree I'll file a follow up do change this to constructor inject in 8.1.0

That looks less bad than I thought it would :)

Discussed with @catch on IRC we decided to prefix everything with underscores to make the change of any break in 8.0.x extremely unlikely - we should probably document that the _method and _property are reserved for core bug fixes in patch releases. Also made the property private and the getter private final to ensure that no one can or will depend on this.

Filed #2628132: Replace setter inject and internal methods on LocaleConfigManager with constructor injection to remove the @internal layer being introduced in 8.0.x in 8.1.0

+++ b/core/modules/locale/src/LocaleConfigManager.php
@@ -96,6 +97,16 @@ class LocaleConfigManager {
+   *   Will be made protected and renamed to $configManager in 8.1.0.

@@ -121,6 +132,34 @@ public function __construct(StorageInterface $config_storage, StringStorageInter
+   *   Will be replaced by constructor injection in 8.1.0.
...
+   *   Will be replaced by constructor injection in 8.1.0.

Should have a Drupal.org issue link to refer to for the work to #2628132: Replace setter inject and internal methods on LocaleConfigManager with constructor injection I guess :) Otherwise looks good.

re #27 @Gábor Hojtsy I'm not sure about how useful it is to add the link to this?

Let's just follow the @todo standards until @internal standards exist.

Looks good to me now :) Discoverability++

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

Yay, thanks!

All these _core things in exported config make for a very very confusing experience. This could definitely use a change record explaining why these things are a necessary evil (an evil they are, because they pollute our otherwise elegant config files).

This would also benefit from documentation: most people exporting configuration or diffing configuration in git will wonder what this is for.

Also:

We only need to worry about configuration entities since there can be 0 to N of these whilst simple configuration there can be only 1.

That's not how it works in the committed patch. All config gets this.

That's not how it works in the committed patch. All config gets this.

All config created through the configuration installer gets this. Yes - simple configuration too so we can work out what version it came from too.

As it is added during installation it should never appear in a git diff :)

This change has introduced a bug in Drupal Module Upgrader by adding the _core: key in the config file which are loaded directly and turned into plugins. #2635944: InvalidArgumentException' with message '$string ("")

Maybe the _core entry should also be automatically removed when the config is re-loaded (or read) so as to reverse the DX regression.

@almaudoh if the key is removed then how can anything use it - I'm not sure what is happening in #2635944: InvalidArgumentException' with message '$string ("") but adding a config key should not break the Drupal Module Upgrader.

adding a config key should not break the Drupal Module Upgrader

It does break DMU because of the way the config there is turned directly into plugin instances, so the _core entry is also turned into a plugin (with an incomplete definition at that). This throws an Exception when the plugin manager tries to instantiate that plugin.

@almaudoh so that means that the way DMU is working is wrong - every config object it creates should be a config_object or a config_entity which both have the _core key defined.

As it is added during installation it should never appear in a git diff :)

Hah, fair!

I'm in an interesting situation where I need to build an install profile based on a site that was built. So I need to export the configuration from that site. But that site is on a version of Drupal 8 from before this patch.

That's why it does show up in a diff for me.

Nevertheless, you're right that this won't be a concern 99.99999% of the time.

But I still think updated documentation and a CR would be helpful. At the very least, documenting why this value exists in config files to avoid people analyzing config files wondering what it's for.

Added https://www.drupal.org/node/2653358.
Updated issue summary.

The CR looks great - thanks @Gábor Hojtsy

Updated docs at https://www.drupal.org/node/1905070/revisions/view/9279932/9298130

Many thanks for the CR & updated docs!

Updated some more docs https://www.drupal.org/node/1809490/revisions/view/9300276/9300330 to document the problem observed with DMU

Follow-up created at #2915414: Omit "_core" key from normalized config entities, which includes the default config hash.