Config collections do not trigger configuration events consistently

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

adding tag for #n12

@alexpott was kind enough to give me some Zoom time today to explain this collection stuff, and expand on why this issue exists. I thought I'd share my understandings here, just for disclosure's sake, and in case anyone else has similar questions as I did.

I'd summarize the problem as this: most code that saves config to non-default collections is entirely bypassing the config events.

In other words: event subscribers that listen to config events are probably NOT being notified of any changes happening to non-default collections!

Indeed, they can only be notified if there's some other event they can listen to. As it happens, core's only use for collections (AFAIK) is config overrides on multilingual sites. And sure enough, the Language module has its own separate event(s) which it dispatches to notify event listeners of changes to the config it keeps in its language-segmented collections.

So if you're an event listener, and you only listen to ConfigEvents::SAVE (and other similiar events), you're only going to hear about things that happen to the default collection. If you want to hear about things happening in other collections, well...you need to hope that whatever is managing the other collections (the Language module, for example; Domain is another one in contrib) dispatches an event you can listen to.

This is ridiculously awkward, to say the least. But it's how it is. We've gotten used to it. So much so, in fact, that basically all subscribers to ConfigEvents::SAVE are rightfully assuming that the config they're looking at is part of the default collection. The default collection being "the collection that affects the way your site works, 100% of the time".

Why can't we just add a method to ConfigCrudEvent which says "hey, this is the collection I'm about to modify"?

Because subscribers to ConfigEvents::SAVE, all across core and contrib, would need to know to check which collection was being changed, and take appropriate action. If they didn't do that, weird things could and would happen. We would, in Alex's words, break the world.

Couldn't a subscriber just, like, subscribe to all the config events of modules which define config collections?

Sure, a subscriber could do that. But that would require that every module which uses config collections defined its own set of events, and dispatched them consistently. And even then, the subscriber would need to know about every single one of those events, and how to handle them correctly. That's not remotely realistic.

Conclusion

We're stuck between a rock and a hard place. We can't change the existing config events, and we can't expect that the entire Drupal ecosystem will have the custom events we need. So we instead need to create a new event that is dispatched when config in a non-default collection is changed. That's the proposed solution.

My opinion on these shenanigans

I think this is very awkward, but it seems like the best choice we have.

To mitigate the awkwardness, I think we need to be excruciatingly clear in the documentation about what collections are and how they're used, why you'd want to subscribe to ConfigEvents::SAVE vs. the new event(s), and why the default collection is so special.

Could someone explain in what exact scenarios the configuration events are not being called consistently? Is this a problem that only occurs in specific modules or configurations?

Probably that's the cause of domain config issue
#3222413: Using Domain Config UI creates *.yml files with duplicate UUIDs.

I bet it also affects how webprofiler contrib module will collect config access stats

I reviewed the entire issue (summary + comments) + Slack + MR in detail.

Problem + fix

Tricky factors at play:

1. this is de facto a critical bug that went undiscovered for nearly a decade
2. the most obvious solution is to tweak ConfigEvents::$op's to also receive a string $collection parameter
3. but this would break BC

So a solution is required that solves the problem without breaking BC, but also in a long-term sustainable way.

The MR/proposed solution is to:

1. create a sibling of ConfigEvents, named ConfigCollectionEvents, with non-default-colllections equivalent events for save/rename/delete
2. when to use which: when config is in StorageInterface::DEFAULT_COLLECTION, use ConfigEvents::$op, otherwise use ConfigCollectionEvents::$op
3. move ConfigEvents::COLLECTION_INFO to ConfigCollectionEvents::COLLECTION_INFO

This looks like a sound approach. 👍

MR

I posted a number of questions, one change that I think is a BC break, but mostly … nits. This is looking quite good already :)

I think this is looking pretty good. Stuff I found is largely nitpicks and RFCs (requests for clarity).

Took a shot at writing the change record.

Adjusting credit.

I posted a comment on Sunday, but then the browser session expired while I was writing it on my phone and my comment got eaten and I have not had time to post it since.
But also a lot has happened in the mean time. So I will summarize my original comment and comment on the evolved MR instead.

I think it makes a lot of sense to have different events dispatched for the default collection vs other collections. Only the config in the default collection can be validated and the config in the collections may not be complete or their schema may not be available. And when loading it you may get an object of a different class than Config.
So I don't think it is strange that there would be different events for each. In fact I think it will probably help avoiding bugs because now you have to check the collection if you listen to the regular save event or you may get unexpected data. (and it only happens when deploying config.. very confusing)

I disagree that we should deprecate StorageInterface::deleteAll (this is not proposed any more) because it may be a more efficient way to clear all instead of looping over all names and deleting them individually.
I am not against documenting more, but the storage interface is for all config storages, and the active one is just a special use case of it. But also the storage interface is the wrong layer of the API to change configuration of your Drupal site. So I would rather add a general text to it explaining that manipulating the active config storage directly is the wrong thing to do when one wants to load or save some configuration.

Otherwise I am much in agreement with the direction of the issue.

@bircher in #25:

So I don't think it is strange that there would be different events for each. In fact I think it will probably help avoiding bugs because now you have to check the collection if you listen to the regular save event or you may get unexpected data. (and it only happens when deploying config.. very confusing)

+1

I disagree that we should deprecate StorageInterface::deleteAll (this is not proposed any more) because it may be a more efficient way to clear all instead of looping over all names and deleting them individually.

I never even saw that, so was very confused about this 😆

I am not against documenting more, but the storage interface is for all config storages, and the active one is just a special use case of it. But also the storage interface is the wrong layer of the API to change configuration of your Drupal site. So I would rather add a general text to it explaining that manipulating the active config storage directly is the wrong thing to do when one wants to load or save some configuration.

This probably explains your very brief MR comment on StorageInterface, and seemingly confirms how I interpreted it before finding this high-level comment of yours 👍

AFAICT

1. To create a new set of collection events that are triggered by Config and LanguageConfigOverride
2. Fix ConfigManager::uninstall() to trigger events when it deletes collection data

from the proposed solution have been implemented, so updated the issue summary.

But AFAICT the last one is still missing test coverage? 🤔

The one bit that is not yet done is Improve documentation for StorageInterface::write() / StorageInterface::delete() / StorageInterface::rename() to mention that the caller is responsible for triggering the correct config crud event..

P.S.: the #3406348: Move config import-related events out of ConfigEvents, into a new ConfigImporterEvents that seems to have been confirmed as a good follow-up/sibling issue now has a green MR 👍

Yes I am ok having different classes for the constants. I mean ideally we would have different event classes and use the class name.. but that is a different issue.

I don't know what the best wording is, but for sure it is not only about the deleteAll, maybe yes document it in the docblock on the class level?
Something along the lines of

When mutating the active config storage the caller is responsible for dispatching the corresponding CRUD events.
Use ... for the default collection and ... for non default collections

Great, Improve documentation for StorageInterface::write() / StorageInterface::delete() / StorageInterface::rename() to mention that the caller is responsible for triggering the correct config crud event. is now complete too.

AFAICT the only thing missing is test coverage for Fix ConfigManager::uninstall() to trigger events when it deletes collection data?

Replaced todos in issue summary.

Apart from a few minor suggestions on the documentation, and the missing test coverage, this looks great to me.

This looks like it has sufficient test coverage, and it has also been thoroughly reviewed. I really love the documentation that's being added here.

Looks great to me too! 🤩

Couple of minor comments on the MR and one broader one about whether we want Config to know anything about storage collections

Nice!
Maybe we can subclass the config object for collections, but that should be in a follow up. This needs a whole lot more consideration. And the issue at hand is solved with the current code I think.

So I think this is ready.

Committed to 11.x

Decided against backporting to 10.2 because of new APIs.

Published the change record.

Thanks all

Closed #3248728: Implement getOriginal Method to get the original data on LanguageConfigOverride as a duplicate