Config collections do not trigger configuration events consistently

Updating issue summary with latest from the MR.

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 some more detail to the issue summary which also explains why things are totally broken.

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.

Please stop incorrectly setting this issue to needs work @needs-review-queue-bot

adding tag for #note-12

@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 anticipate 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 $0.02
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?

@wilku.net so for example when language overrides are saved in their config collections no ConfigCrudEvents are called (this is a good thing). However when you install the test module config_collection_install_test its config in collections do result in ConfigCrudEvents. That's not consistent.

Additionally due to the way that configuration in overrides are deleted by \Drupal\Core\Config\ConfigManager::uninstall() no events ConfigCrud or even Language override events are called. This doesn't actually matter at the moment because we call very similar code when we remove the real config - see \Drupal\locale\LocaleConfigSubscriber::onConfigSave and \Drupal\locale\LocaleConfigSubscriber::onOverrideChange.

None of this is consistent.

Probably that's the cause of domain config issue
🐛 Using Domain Config UI creates *.yml files with duplicate UUIDs. Needs work

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

@andypost yes this will fix things with Domain. Domain is not using its own StorableConfigBase object - see https://git.drupalcode.org/project/domain/-/blob/8.x-1.x/domain_config/s... - so it will trigger regular config events for it's overrides. Which is incorrect because as with config translations there are only partial representations of configuration and meant to be merged via the override system with active config in the default collection.

Addressed @Wim Leers's review on the MR.

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

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 .

P.S.: the 📌 Move config import-related events out of ConfigEvents, into a new ConfigImporterEvents Needs review 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

I've resolved the feedback from @Wim Leers @bircher and @phenaproxima - thanks for the reviews.

Great, is now complete too.

AFAICT the only thing missing is test coverage for ?

Replaced todos in issue summary.

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

I've added the requested test coverage from #29

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

I've addressed @larowlan's feedback - thanks for the review.

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.

larowlan → closed merge request !5665

Committed to 11.x

Decided against backporting to 10.2 because of new APIs.

Published the change record.

Thanks all

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

smustgrave → credited fromme → .

Closed ✨ Implement getOriginal Method to get the original data on LanguageConfigOverride Needs work as a duplicate