Document that Config save/delete/rename events may be dispatched during hook_update_N(), what that means for subscribers, and fix core subscribers accordingly

Provide a way to do storage-agnostic configuration updates without firing the event.

What about \Drupal::service('config.storage')->(listAll|read|write)()?

I'm a but confused. Doesn't #2 basically tells us: this works as designed and we have an API for that?

As alternative we could for sure provide an event dispatcher which can be toggled off temporarily

@alexpott, @catch, @effulgentsia, @webchick and I discussed this issue and decided to defer triage on it. If it turns out to be a hard blocker for the block context upgrade path issue in #2528178: Provide an upgrade path for blocks context IDs #2354889 (context manager), then it will remain critical. @alexpott plans to discuss this issue on tomorrow's criticals hangout to get a better issue of what the correct resolution might be and what the implications are.

I think the big question to answer here is what is the intention of ConfigEvents::SAVE?

Should a subscriber to that be able to know the semantics and formatting of the $config being saved? For example, Drupal\language\EventSubscriber\ConfigSubscriber::onConfigSave() assumes that system.site has a default_langcode key and that it is formatted as a language code.

Or should ConfigEvents::SAVE subscribers only deal with generic storage concepts and nothing specific? Such as Drupal\Core\Config\Entity\Query\QueryFactory::onConfigSave(), which implements a separate denormalized storage as a performance optimization, but does so generically based on whatever data is there, and makes no assumptions about the semantics and formatting of specific keys or values.

If the former, then hook_update_N() functions dispatching the event is problematic, because let's say you have mymodule_update_8001() that makes one change and mymodule_update_8002() that makes another change, and a site updates from pre-8001 to 8002. A ConfigEvents::SAVE subscriber that runs in response to a change in 8001 will receive data that is potentially invalid for the subscriber's code assumptions.

However, if we don't dispatch the event, then subscribers such as QueryFactory not running is potentially problematic, because what if what that update function is changing is important to that subscriber? For example, what if the config key being changed by block_update_8001() in #2528178: Provide an upgrade path for blocks context IDs #2354889 (context manager) was a lookup key? I.e., imagine a contrib subscriber that implemented fast lookups for every key, not just what the entity type declared in its annotation.

If we think we must support both use cases (subscribers that assume the saved config is valid, and subscribers that must react to internal format changes as well as to semantically meaningful value changes), then things become harder. Perhaps one possibility is to do all incremental saves without dispatching the event, but keep a queue of which config has been modified, and then dispatch a single event (per config object) after all hook_update_N() functions are done, since at that point, all the modified configs are valid?

We discussed this on the criticals call... https://www.youtube.com/watch?feature=player_embedded&v=g580b-v-8YE#t=753

The consensus we came to was this:

• Config events need to fire during update - there is the possibility that one update might save something in config that would have an event make changes to other configuration and then another update occur that needs these changes to occur.
• We should use the trusted data feature during hook_update_N() to mitigate issues to with schema change during updates. This disables using schema during config save. This makes sense since developers should provide schema compliant values during update. We could even add an assert that checks that the trusted data flag is set during a config save when the maintenance mode is set to update.
• We need to document what is permissible during an Config events... and make it clear that what should be attempted is limited to: cache invalidations, enforcing related config change and marking a container for rebuild

Just to get the ball rolling.

+++ b/core/lib/Drupal/Core/Config/ConfigEvents.php
@@ -10,6 +10,13 @@
+ *  - cache invalidation
+ *  - enforcing related config change
+ *  - marking a container for rebuild

This is interesting, interesting in the sense that it does not cover the KV usecase

I guess we should permit direct interaction with other storage. Like if you have to run a db query for what ever crazy reason it should be okay.

For example, Drupal\language\EventSubscriber\ConfigSubscriber::onConfigSave() assumes that system.site has a default_langcode key and that it is formatted as a language code.

How do we fix that to comply with the docs in #8?

I think it is okay to inspect configuration, especially if the provider of the event is the same as the provider of the config... in the case of Drupal\language\EventSubscriber\ConfigSubscriber::onConfigSave() core provides everything.

system.module and language.module are both in the Drupal core package, but they are different providers. If we ship core with language module implementing an onConfigSave() that is implicitly coupled to the schema of a system module config object, then contrib will think it's ok to do the same and will therefore also have contrib subscribers coupled to core or other contrib config objects. And that's a problem, because let's say a contrib module does something similar to language module and implements a subscriber that directly introspects some key/value in system.site. Now, say there are 2 updates (8001 and 8002) that change something about that key or value. If the contrib code is simultaneously updated to its latest release (one that's based on the schema of 8002), then that subscriber can error (loudly or silently) when its onConfigSave() runs during the 8001 update, since the $config values at that time won't conform to the 8002 schema.

I think there are a couple options for fixing it though:

• Make event subscribers that do such introspection responsible for graceful handling of invalid data. For example, Drupal\language\EventSubscriber\ConfigSubscriber::onConfigSave() already does sanity checking that $language exists, which given the current implementation of LanguageManager::getLanguage(), means we're already failing gracefully if the 'default_langcode' value doesn't exist or isn't a valid langcode. We can make that explicit by either adding docs to LanguageManagerInterface::getLanguage() that it must not throw an exception, regardless of the value of $langcode, or by making onConfigSave() validate $default_langcode itself.
• We could provide some mechanism for the subscriber to request to not be fired for invalid $config, or be able to ask the event or config if it's valid. Then a subscriber that chooses to introspect has a means to not run its code during an incremental update that has $config in a temporarily invalid state.

Not sure if this question belongs here or in a new issue, but what's the division of responsibility between update functions and config overrides? In #2528178: Provide an upgrade path for blocks context IDs #2354889 (context manager), we're lucky that the config value being updated in block_update_8001() is not translatable, so we don't have to worry about language overrides of it. But what if a contrib module provided a UI to language-override any value, not only translatable ones? LanguageConfigFactoryOverride::onConfigSave() doesn't handle that, so as a result any language overrides of the block visibility context mappings would be invalid unless the contrib module that provided that UI implemented an update function or a config save subscriber to deal with those. Similarly, in contrib, DomainConfigOverrider does not implement an onConfigSave() to react to block_update_8001()'s update of context mappings in the case that that is overridden for a given domain. Should it, and if not, then who should?

In a way, this is similar to the config sync/import flag but not the same.

I agree with just documenting that those events *are* called during update functions since we declare that config API is safe API for updates. And that they are responsible for being able to deal with old versions of the configuration in case the structure changes (This part could probably be a bit more explicit).

I'm not sure we should even define what's allowed and what not, that's up for the implementation.

For overrides, I'm not sure what to do. Basically what's said above. They need to cope with changes. A module that provides an override should probably implement some kind of structure check for existing overrides on config save.. if there's a mismatch, drop the override or something like that.

I agree with @Berdir. Let's document that the event will be called during update.

In another tangent I've been thinking about the default language change event. We kind of have an interesting problem here for a couple of reasons:

1. If the language module is not installed we should actually prevent changing the default language. To run Drupal in a language other than 'en' you need to the Language module installed. Since we have no validation event should we have a system subscriber that trigger a warning and keeps the language as English?
2. In the language module's subscriber we check if the language exists, but we use the Language manager to do this. Is this wrong? Doing this will fire entity hooks because it is loading the language through the config entity subsystem.

Oh and there's more from that language event. It calls language_negotiation_url_prefixes_update() which loads all languages through the LanguageManager and then saves related config. There are issues that remove that method and replace it will direct calls to config.

Here's a patch based on @Berdir's suggestion.

+++ b/core/lib/Drupal/Core/Config/ConfigEvents.php
@@ -10,6 +10,15 @@
+ * The following actions are examples that are safe:
+ * - Cache invalidation.
+ * - Enforcing related config change.
+ * - Marking a container for rebuild.

Can we include examples of unsafe actions? Examples are for example: trigger a router rebuild.

I think we should document most of this in hook_update_N and point to it from ConfigEvents. Also ConfigEvents has events that will not be run on an update so moving documentation to the ones that are likely to be (SAVE, DELETE and RENAME).

I've also tried to make events that listen to the ConfigEvents comply with the rules :)

+++ b/core/modules/language/src/EventSubscriber/ConfigSubscriber.php
@@ -55,12 +67,18 @@ public function __construct(LanguageManagerInterface $language_manager, Language
+        $this->configFactory->getEditable('language.negotiation')
+          ->set('url.prefixes.' . $old_default_langcode, $old_default_langcode)
+          ->set('url.prefixes.' . $new_default_langcode, '')
+          ->save(TRUE);

We should document why we can't call out to language_ne...update()

The logic in language_negotiation_url_prefixes_update() had some logic that I missed.

1. +++ b/core/lib/Drupal/Core/Config/ConfigEvents.php
   @@ -10,6 +10,7 @@
     * Defines events for the configuration system.
     *
   + *
   

   Not needed.

2. +++ b/core/lib/Drupal/Core/Config/ConfigEvents.php
   @@ -21,11 +22,20 @@
   +   * \Drupal\Core\Config\Config::save() and pass schema correct data so that
   +   * configuration schema are not used whilst saving configuration.
   

   I don't think this is clear... "schema correct" means "data that is correct against its schema"? also "so that config schema are not used" etc. means *because*, not *so*? Same repeats multiple times.

Thanks @Gábor Hojtsy

1. +++ b/core/lib/Drupal/Core/Config/ConfigEvents.php
   @@ -21,11 +21,20 @@
   +   * regardless of the structure of the underlying configuration. When changing
   +   * configuration values, the value must be the correct data type.
   +   * Configuration must be saved with the $trusted_data flag set to TRUE so that
   +   * configuration schema are not used.
   

   s/$trusted_data/$has_trusted_data/. But also, instead of repeating all this for each event, what about simply saying that because this can be fired during a hook_update_N(), subscribers must only perform actions that are safe during a hook_update_N(), and to see those docs for details about that.

2. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -464,11 +464,25 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
   + * - Calling Node::save().
   

   What about saying "Loading, saving, or performing any other operation on an entity"?

3. +++ b/core/modules/language/src/EventSubscriber/ConfigSubscriber.php
   @@ -55,12 +67,31 @@ public function __construct(LanguageManagerInterface $language_manager, Language
   +      $default_language = $this->configFactory->get('language.entity.' . $new_default_langcode);
   ...
   +          $negotiation_config->set('url.prefixes.' . $old_default_langcode, $old_default_langcode);
   

   What if an update happens by which $new_default_langcode or $old_default_langcode are strings, but not language codes (for some strange reason, we decide to format the config value string in some other way), or perhaps, maybe not even strings? Maybe the above operations are harmless for invalid language codes, but would it be better for us to either explicitly validate them as language codes or comment why we don't have to?

4. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -464,11 +464,25 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
   + *   Implementations must use the $trusted_data argument for
   + *   \Drupal\Core\Config\Config::save() and pass schema correct data so that
   + *   configuration schema are not used whilst saving configuration.
   

   Why must schema-correct data be passed? An individual hook_update_N() might not be able to ensure that. E.g., mymodule_update_8001() might call $config->save() with data that is no longer schema correct by the time mymodule_update_8002() exists.

4. I think what this means to say is that data needs to be correct according to the schema when that update function was written. And trust on following update functions to correct the schema according to their knowledge.

Looks good, patch aligns with the conversation in the criticals meeting (love that they're recorded).

1. +++ b/core/lib/Drupal/Core/Config/ConfigEvents.php
   @@ -21,11 +21,20 @@
   +   * configuration values, the value must be the correct data type.
   
   @@ -38,11 +47,20 @@
   +   * configuration values, the value must be the correct data type.
   
   @@ -55,10 +73,19 @@
   +   * configuration values, the value must be the correct data type.
   
   +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -464,11 +464,25 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
   + *   \Drupal\Core\Config\Config::save() and pass schema correct data so that
   

   Yes I agree with earlier comments that we should clarify what 'correct data type' means - suggest 'correct data type as specified in the schema at the time the update hook is written. If the data type changes in a subsequent code change, a subsequent update hook is responsible for ensuring the final data-type aligns with the ultimate schema.' or similar?

2. +++ b/core/lib/Drupal/Core/Config/ConfigEvents.php
   @@ -21,11 +21,20 @@
   +   * configuration schema are not used.
   
   @@ -38,11 +47,20 @@
   +   * configuration schema are not used.
   
   @@ -55,10 +73,19 @@
   +   * configuration schema are not used.
   
   +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -464,11 +464,25 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
   + *   configuration schema are not used whilst saving configuration.
   

   schema or schemas?

How about checking that the schema is as we expect in the event...

Patch hopefully addresses the other feedback from @larowlan, @Berdir and @effulgentsia - thanks everyone.

Talked with @catch on IRC he didn't like the schema checking in the config event and thought that the comment was enough. Let's not over complicate this - if schema changes then a config event needs to be updated to handle both schema.

@catch indeed.

Saw the related #2521776: Update documentation for hook_update_N() for Drupal 8 today.

+++ b/core/modules/language/src/EventSubscriber/ConfigSubscriber.php
@@ -34,33 +36,67 @@ class ConfigSubscriber implements EventSubscriberInterface {
+   * then this event must changed to work with both the old and new schema
+   * definition so this event is update safe.

must *be* changed?

Also while there is a theoretic possibility of schema change, we agreed to not change existing schema by policy after release at least, or more precisely we are supposed to only change additively. So as a theoretic problem to be explained in core, it makes sense, as an actual practical problem, we are not supposed to face that (or we are facing a lot more problems with config schema versioning in general that we deferred to Drupal 9).

Thanks @Gábor Hojtsy and good point about previous agreements wrt to schema change.

This looks great to me. Down to nits.

1. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -464,11 +464,30 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
   + *   - not make any assumption that the config data is valid.
   

   Capitalize first letter for this and the other list items below?

2. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -464,11 +464,30 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
   + *   - use be the correct data type when changing configuration values as
   

   s/be//?

3. +++ b/core/modules/language/src/EventSubscriber/ConfigSubscriber.php
   @@ -34,33 +36,67 @@ class ConfigSubscriber implements EventSubscriberInterface {
   +   * This event assumes that the new default langcode and old default langcode
   +   * are valid langcodes. If the schema definition of either
   +   * system.site:default_langcode or language.negotiation::url.prefixes changes
   +   * then this event must be changed to work with both the old and new schema
   +   * definition so this event is update safe.
   

   Awesome. Thank you. But s/this event/this event (subscriber|listener)//g?

4. +++ b/core/modules/language/src/EventSubscriber/ConfigSubscriber.php
   @@ -34,33 +36,67 @@ class ConfigSubscriber implements EventSubscriberInterface {
   +        // Directly update language negotiation settings instead of calling
   +        // language_negotiation_url_prefixes_update().
   

   Perhaps a comment on why not use language_negotiation_prefixes_update()?

So as a theoretic problem to be explained in core, it makes sense, as an actual practical problem, we are not supposed to face that

Yep, agreed that that applies to non-entity core config, such as system.site. Not sure it necessarily applies to entity config if the corresponding properties are protected. And it definitely doesn't apply to contrib modules, especially during contrib betas or when the contrib module does a major version update (e.g., 8.x-1.x to 8.x-2.x), so I'm glad the patch is documenting what it is.

1. I pondered this too - I was not sure if it was meant to be read with first bit "Implementations must:" - but fixing cause i think this is the standard
2. Fixed
3. Fixed
4. Fixed

:)

SIGH. I was working on the hook_update_N() docs on #2521776: Update documentation for hook_update_N() for Drupal 8. Did you have to change them here too???? Very tough to merge.

It's OK, we're handling the merge on the other issue. I was just short of time this morning and it surprised me. Sorry for the big sigh. We'll deal with it. :)

What I should have said was "THANK YOU for adding this very important information to the documentation!" :)

Yep, agreed that that applies to non-entity core config, such as system.site. Not sure it necessarily applies to entity config if the corresponding properties are protected. And it definitely doesn't apply to contrib modules, especially during contrib betas or when the contrib module does a major version update (e.g., 8.x-1.x to 8.x-2.x), so I'm glad the patch is documenting what it is.

Not sure what exactly you meant there but based on how the schema system was architected (lacking versioning, etc) modules are supposed to namespace their schemas (like global functions in D7) and they are not supposed to make any backwards incompatible change to their schemas whatsoever, because there is no versioning, so its impossible to decide which version of their config corresponds to which version of their schema. See #1977498: Add version tracking for configuration schema and data which was last postponed to 8.1.x.

Re #48: #2543150: Document consequences of contrib changing config schema without core's API supporting config version tracking