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.
Comment | File | Size | Author |
---|---|---|---|
#29 | 2625258-29.patch | 21.31 KB | alexpott |
#29 | 27-29-interdiff.txt | 1.12 KB | alexpott |
#25 | 2625258-25.patch | 21.18 KB | alexpott |
#25 | 23-25-interdiff.txt | 2.78 KB | alexpott |
#25 | 23-25-interdiff.txt | 2.76 KB | alexpott |
Comments
Comment #2
tstoecklerComment #3
tstoecklerComment #4
alexpottSo 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.
Comment #6
Gábor HojtsyThat 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).
Comment #7
alexpottUsing 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
Comment #8
alexpott#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...
Comment #9
alexpottHere'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...
Comment #12
alexpottUpdated IS
Comment #13
alexpottFixing the test fail - nice that we have a test for that - it would have been easy to break.
Comment #14
Gábor HojtsyHow 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.
Comment #15
alexpottImproving test coverage and injecting the service.
Comment #16
alexpottDiscussed 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 outComment #17
alexpottAdding 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.
Comment #18
Gábor HojtsyWe can do the upgrade path in yet another critical, that's fine.
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.
Comment #20
alexpottFixed 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.Comment #21
alexpottCreated the postponed upgrade path issue: #2628004: Create an upgrade path to determine if default_config_hash should be added (2625258)
Comment #22
catchIs 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).
Comment #23
alexpottHere's setter injection - if we agree I'll file a follow up do change this to constructor inject in 8.1.0
Comment #24
catchThat looks less bad than I thought it would :)
Comment #25
alexpottDiscussed 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.
Comment #26
alexpottFiled #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
Comment #27
Gábor HojtsyShould 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.
Comment #28
alexpottre #27 @Gábor Hojtsy I'm not sure about how useful it is to add the link to this?
Comment #29
alexpottLet's just follow the @todo standards until @internal standards exist.
Comment #30
Gábor HojtsyLooks good to me now :) Discoverability++
Comment #31
catchCommitted/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!
Comment #34
Gábor HojtsyYay, thanks!
Comment #35
Wim LeersAll 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:
That's not how it works in the committed patch. All config gets this.
Comment #36
alexpottAll 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 :)
Comment #37
almaudoh CreditAttribution: almaudoh commentedThis 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.Comment #38
alexpott@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.
Comment #39
almaudoh CreditAttribution: almaudoh commentedIt 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.Comment #40
alexpott@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.
Comment #41
Wim LeersHah, 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.
Comment #42
Gábor HojtsyAdded https://www.drupal.org/node/2653358.
Updated issue summary.
Comment #43
alexpottThe CR looks great - thanks @Gábor Hojtsy
Comment #44
Gábor HojtsyUpdated docs at https://www.drupal.org/node/1905070/revisions/view/9279932/9298130
Comment #45
Wim LeersMany thanks for the CR & updated docs!
Comment #46
alexpottUpdated some more docs https://www.drupal.org/node/1809490/revisions/view/9300276/9300330 to document the problem observed with DMU
Comment #48
Wim LeersFollow-up created at #2915414: Omit "_core" key from normalized config entities, which includes the default config hash.