Add config_exclude functionality to core

I added it to config_environment because creating a new module requires adding a drupal.org help page and we can do that once the decision is taken that it should be its own module in core.

I think that's a good decision, they are closely tied together so keeping them in one module seems like the easiest solution.

The patch also seems to introduce a few cs violations, those should also be fixed,

1. +++ b/core/modules/config_environment/src/EventSubscriber/EnvironmentModulesEventSubscriber.php
   @@ -0,0 +1,168 @@
   + * The event subscriber preventing development modules to be exported.
   

   not sure if we should mention development modules here. They can be other modules as well, so maybe configured modules?

2. +++ b/core/modules/config_environment/src/EventSubscriber/EnvironmentModulesEventSubscriber.php
   @@ -0,0 +1,168 @@
   +  private $active;
   

   We don't usually add private variables so that we extend classes, but since this is final it seems to be ok.

3. +++ b/core/modules/config_environment/src/EventSubscriber/EnvironmentModulesEventSubscriber.php
   @@ -0,0 +1,168 @@
   +    $this->active = $active;
   

   Maybe this should be activeStorage instead of just active? Because at first it seemed like this property was a boolean flag.

4. +++ b/core/modules/config_environment/src/EventSubscriber/EnvironmentModulesEventSubscriber.php
   @@ -0,0 +1,168 @@
   +   * Make sure excluded modules are not uninstalled by adding them and their
   

   here it is excluded modules, lets use that language everywhere.

5. +++ b/core/modules/config_environment/src/EventSubscriber/EnvironmentModulesEventSubscriber.php
   @@ -0,0 +1,168 @@
   +      // If the core.extension config is not present there is nothing to do.
   +      // This means that probably the storage is empty or non-functional.
   ...
   +      // If the core.extension config is not present there is nothing to do.
   +      // This means some other process has rendered it non-functional already.
   

   This comment should probably be the same.

Awesome, thank you for the clear response, now all this needs is the Change Record. The code already looks great.

Thanks for your work on this. As the author of the Config Exclude module I would obviously like to see functionality like this in core :-)

At first I wasn't sure if this class should be final, but after seeing this talk by Wim Leers (thanks Wim!) and reading #3019332: Use final to define classes that are NOT extension points I agree.

Some remarks about the code:

1. +++ b/core/modules/config_environment/config_environment.services.yml
   @@ -11,3 +11,9 @@ services:
   +  # config_environment services.
   

   I'm not sure what this comment means, or what it tries to clarify here.

2. +++ b/core/modules/config_environment/src/EventSubscriber/ExcludedModulesEventSubscriber.php
   @@ -0,0 +1,169 @@
   +  public function __construct(StorageInterface $active, Settings $settings, ConfigManagerInterface $manager) {
   +    $this->activeStorage = $active;
   

   For consistency, I would prefer $active_storage instead of $active here as well.

3. +++ b/core/modules/config_environment/src/EventSubscriber/ExcludedModulesEventSubscriber.php
   @@ -0,0 +1,169 @@
   +   * Get the modules set as excluded in the drupal settings.
   +   *
   +   * @return string[]
   +   */
   

   Coding standards: This needs a description of the returned value, eg. "An array of module names". Also, "Drupal" deserves a capital "D".

4. +++ b/core/modules/config_environment/src/EventSubscriber/ExcludedModulesEventSubscriber.php
   @@ -0,0 +1,169 @@
   +    return $this->settings->get("config_exclude_modules", []);
   

   Should the name of this setting be namespaced, eg. "config_environment_excluded_modules"?

5. +++ b/core/modules/config_environment/src/EventSubscriber/ExcludedModulesEventSubscriber.php
   @@ -0,0 +1,169 @@
   +   * @return string[]
   +   */
   

   Coding standards: This needs a description of the returned value, eg "An array of configuration names".

I will now go test and try to break the code :-)

OK, I did some manual testing.

Steps performed on a standard-profile D8 site:
- Enable config_environment module.
- Composer-require and enable Devel module.
- Add $settings['config_exclude_modules'] = ['devel']; to settings.php.
- Export config: full export through the admin UI, full export with Drush 9, single-item export of core.extension.yml.
- Check the result: no traces of Devel module should be present.
- Import the exported config again.
- Check the result: although the exported config does not mention Devel module, it should still be enabled.

Results:
- The ExcludedModulesEventSubscriber only kicks in during a full export in the admin UI, not during a single-item export or Drush export. Maybe this was to be expected given the current state of Drupal core and/or Drush? At least as a developer, I was expecting it to work with drush cex too.
- The config export generated in the admin UI indeed does not contain traces of Devel module; not in core.extension.yml, nor Devel's own config files.
- When importing the config .tar.gz archive again (in the admin UI), Devel module remains enabled even though it is not in core.extension.yml.

Here's a patch that fixes points 2, 3 and 5 from comment #8.
You can ignore point 1, I now see it is a response to the @todo higher up in the file.
Point is 4 open for discussion afaic.

Thanks for the explanation Bircher. I'm happy my tests confirm it is working as expected.

Indeed if the final goal is to turn this into a separate module, the setting name doesn't need to be changed.

I like the idea to notify users about excluded config on the import/export page. (And during drush cim / cex too, I suppose.)

So we need a Change Record, and perhaps a re-roll to split this off into a separate module, but code-wise I would call this RTBC.

Shall this module itself be excluded or can be conditionally excluded based on the different environments?

Re #13: I'm not sure why you would want to exclude config_exclude itself, but it is possible. I ran a quick test and found that when config_exclude is listed in $settings['config_exclude_modules'], it remains enabled and functional when you import config that does not list Config Export (actually, config_environment with the latest patch) in core.extension.yml. In other words, you could enable it on your local development environment, together with other excluded modules, without any effect on the git repo you commit your config to.

While doing that quick test I did find a minor bug: When you change $settings['config_exclude_modules'] without making any other configuration changes, it does not take effect. This happens because a change to $settings['config_exclude_modules'] does not trigger one of the ConfigEvent events, so ExportStorageManager::onConfigChange() is not called and $this->state->set(self::NEEDS_REBUILD_KEY, TRUE); is not executed. Back to "needs work"!

+++ b/core/modules/config_environment/src/Core/Config/ExportStorageManager.php
@@ -83,7 +83,12 @@ public function __construct(StorageInterface $active, StateInterface $state, Con
+    if (!$rebuild) {
+      // @todo: Use ConfigEvents::STORAGE_EXPORT_REBUILD in #2991683
+      $rebuild = $this->eventDispatcher->dispatch('config.export.rebuild')->isPropagationStopped();
+    }

Let's not use event propagation as a signal here. Let's add an event with methods like setRebuildNeeded() and isRebuildNeeded().

1. +++ b/core/modules/config_environment/src/EventSubscriber/ExcludedModulesEventSubscriber.php
   @@ -0,0 +1,201 @@
   +    if ($this->state->get("config_exclude_modules") != $this->getExcludedModules()) {
   

   I thought about namespacing this key a bit - something like config.exclude_modules - but the current value is nice because it matches the settings.php key. That said there's no reason we can't make this string a class constant and use it for state and settings.

2. +++ b/core/modules/config_environment/src/EventSubscriber/ExcludedModulesEventSubscriber.php
   @@ -0,0 +1,201 @@
   +    if (!$storage->exists('core.extension')) {
   +      // If the core.extension config is not present there is nothing to do.
   +      // This means that probably the storage is empty or non-functional.
   +      return;
   +    }
   

   Does this happen?

3. +++ b/core/modules/config_environment/src/EventSubscriber/ExcludedModulesEventSubscriber.php
   @@ -0,0 +1,201 @@
   +    $modules = $this->settings->get("config_exclude_modules", []);
   +    if (!is_array($modules)) {
   +      return [];
   +    }
   

   Why the is array check - there's the default value - it seems overly cautious.

4. +++ b/core/modules/config_environment/src/EventSubscriber/ExcludedModulesEventSubscriber.php
   @@ -0,0 +1,201 @@
   +  private function getConfigNames() {
   

   I think this could be better named. Something like getDependentConfigNames()

5. +++ b/core/modules/config_environment/src/EventSubscriber/ExcludedModulesEventSubscriber.php
   @@ -0,0 +1,201 @@
   +    foreach ($this->manager->findConfigEntityDependents('config', array_unique($config)) as $dependent) {
   +      $config[] = $dependent->getConfigDependencyName();
   +    }
   

   Maybe a comment above saying that now we're finding any configuration dependent on such the configuration.

   Also I'm deeply suspicious that this is too simplistic looking at \Drupal\Core\Config\ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval() - there are config entities that adapt themselves on module uninstall for example. I've often felt we should not allow a module to be excluded if there are these sort of indirect dependencies.

The code here looks very solid and this is a very useful new feature. I think this is good addition to make the CMI-2 initiative even more better.

I can't find any reason why this shouldn't go in.

1. +++ b/core/modules/config_environment/src/EventSubscriber/ExcludedModulesEventSubscriber.php
   @@ -0,0 +1,206 @@
   +  const EXCLUDED_MODULES_KEY = "config_exclude_modules";
   ...
   +    return $this->settings->get("config_exclude_modules", []);
   

   Can use the class constant here too.

2. +++ b/core/modules/config_environment/src/EventSubscriber/ExcludedModulesEventSubscriber.php
   @@ -0,0 +1,206 @@
   +    // Find all configuration that depends on the configuration found above.
   +    foreach ($this->manager->findConfigEntityDependents('config', array_unique($config)) as $dependent) {
   +      $config[] = $dependent->getConfigDependencyName();
   +    }
   

   I'm not sure that there is a test of this situation yet. It looks like only direct dependencies are being tested.

Excluded modules and all the configuration that directly and indirectly depends on them are not exported through in the tarball and through tools using the config.storage.export service to export the configuration.
When importing the configuration the excluded modules and their configuration are not removed.
It is possible to exclude required modules, which means that the exported configuration can not be imported any more.
This is an advanced feature and using it means opting out of some of the guarantees the configuration synchronisation provides.

What does a required module mean? A module that another module depends on that is not excluded?

@johndevman both system and user are required modules. I.e they have required: true in their .info.yml files. It might also be possible to do weird things by excluding a module that supplies a field type with data (for example telephone). But in all cases we have config import validation and the ability to tell the user exactly what is going to change on import.

I did some more manual testing, everything works as expected. Great work @bircher and thanks for your input @alexpott! All feedback since #25 has been addressed, so going back to RTBC.

Adding review credits.

Reviewed this and the only thing that I think is missing is docs in example.settings.php for the new setting.

Can we get an example entry for this in example.settings.php with some documentation of how to use it etc.

Discussed ^ with @xjm and @bircher

Because this is an experimental module, we don't need to do that now, but we need to do it before the module is marked stable.

So tagging as needs follow-up - can you add a follow-up issue to add those docs to settings.php and add it to the config_environment stable roadmap?

Committed 4e8f977 and pushed to 8.8.x. Thanks!

Published change record

🥇

A related Drush issue https://github.com/drush-ops/drush/issues/4194