Problem/Motivation

If you delete shipped configuration and create a new configuration object with the same name, \Drupal\locale\LocaleConfigManager::updateConfigTranslations(), which gets called when installing modules or themes, deletes translations of that configuration in some cases. This constitutes data loss, thus, this is marked critical.

Proposed resolution

Introduce a _core key in configuration files for core data storage. Modules should not conflict with that. During installation of configuration add the default configuration hash to configuration files under the default_config_hash key under _core, which is a hash of the default configuration used during installation. This means we can determine if a piece of configuration is based on default configuration or not and compare if it is still based on the default configuration we have.

Remaining tasks

Upgrade path in #2628004: Create an upgrade path to determine if default_config_hash should be added (2625258).

User interface changes

None

API changes

None

Data model changes

New _core key added to configuration for storing core managed values.

Comments

tstoeckler created an issue. See original summary.

tstoeckler’s picture

Title: LocaleConfigManager::getTranslatableDefaultConfig() reads config from uninstalled modules » \Drupal\locale\LocaleConfigManager::updateConfigTranslations() deletes translations if a config object's name happens to match that of a shipped configuration object
tstoeckler’s picture

Title: \Drupal\locale\LocaleConfigManager::updateConfigTranslations() deletes translations if a config object's name happens to match that of a shipped configuration object » LocaleConfigManager::updateConfigTranslations() deletes translations if a config object's name happens to match that of a shipped configuration object
alexpott’s picture

Status: Active » Needs review
Issue tags: +Needs tests, +D8MI
FileSize
2.82 KB

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.

Status: Needs review » Needs work

The last submitted patch, 4: 2625258-4.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +language-config, +sprint

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).

alexpott’s picture

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.52 KB
5.07 KB

#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...

alexpott’s picture

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...

The last submitted patch, 9: 2625258-9.test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: 2625258-9.patch, failed testing.

alexpott’s picture

Issue summary: View changes

Updated IS

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.26 KB
9.88 KB

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

Gábor Hojtsy’s picture

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.

alexpott’s picture

Improving test coverage and injecting the service.

alexpott’s picture

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

alexpott’s picture

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.

Gábor Hojtsy’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 17: 2625258-17.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
5.11 KB
21.36 KB

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.

alexpott’s picture

catch’s picture

+++ 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).

alexpott’s picture

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

catch’s picture

That looks less bad than I thought it would :)

alexpott’s picture

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.

alexpott’s picture

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

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ 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.

alexpott’s picture

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

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.12 KB
21.31 KB

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

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me now :) Discoverability++

catch’s picture

Status: Reviewed & tested by the community » Fixed

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

  • catch committed dd8c0e6 on 8.1.x
    Issue #2625258 by alexpott: LocaleConfigManager::...

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks!

Wim Leers’s picture

Status: Closed (fixed) » Active
Issue tags: +Needs issue summary update, +Needs change record, +Needs Documentation

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.

alexpott’s picture

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 :)

almaudoh’s picture

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.

alexpott’s picture

@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.

almaudoh’s picture

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.

alexpott’s picture

@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.

Wim Leers’s picture

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.

Gábor Hojtsy’s picture

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

alexpott’s picture

Status: Active » Fixed
Issue tags: -Needs Documentation

The CR looks great - thanks @Gábor Hojtsy

Gábor Hojtsy’s picture

Wim Leers’s picture

Many thanks for the CR & updated docs!

alexpott’s picture

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

Status: Fixed » Closed (fixed)

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