Module uninstall validators are not used to validate a config import

This is running the uninstall validators on configuration import, not the API change yet.

I think from here the next steps are to change ModuleInstallerInterface::validateUninstall() to take a second parameter indicating whether to check for configuration-based validity or not (default is true). To make this possible, ModuleUninstallValidatorInterface must have added to it a method that checks whether the given validator is configuration-based or not.

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -761,6 +761,12 @@ protected function processExtension($type, $op, $name) {
   +        if ($reasons = $this->moduleInstaller->validateUninstall(array($name), FALSE)) {
   
   +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -500,10 +500,13 @@ protected function updateKernel($module_filenames) {
   +  public function validateUninstall(array $module_list, $configuration_based = TRUE) {
   

   I think instead of calling validateUninstall with a flag we could just add the uninstall validators to the ConfigImporter and only add the configuration based ones. Plus I think this validation should be done before the COnfigImporter actually starts importing.

2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
   @@ -74,11 +74,13 @@ public function addUninstallValidator(ModuleUninstallValidatorInterface $uninsta
   +   * @param bool $configuration_based
   +   *   Check with configuration-based validators.
   

   This should @see to the interface where what configuration-based means.

3. +++ b/core/lib/Drupal/Core/Extension/ModuleUninstallValidatorInterface.php
   @@ -13,6 +13,14 @@
   +   * Indicate if this validator is configuration-based.
   

   I think this needs more explanation as to what configuration-based actually means.

Hello

Sorry that I didn't include the previous patch in this one but I think the previous patch does not address the issue in the right place. It runs the uninstall validator on individual modules when they are processed instead of during the validation phase. This is redundant because the uninstalling of modules already validates them and throws the errors. The validation needs to happen in the validation phase.

I discussed the issue with @fago during drupalaton and we concluded that the issue is two fold:

• The validators are not taken into consideration and thus the import results in exceptions being thrown in the process. (see title of issue)
• The some validators check the configuration of for example base fields and these checks are already done by other import validation processes, so running all validators results in the same or very similar checks being run twice.

I attached a test that shows that we currently have a problem. (You can not uninstall the system module during the synchronisation, so its not critical, but since the synchronisation uninstalls the modules one by one some others might have been uninstalled before the error is thrown and the site might end up in a funny state anyway, that would probably be another issue)

I attached two patches that run the uninstall validation, one fixing it in the Importer and one in the EventSubscriber. pick the better one.

The second part of the issue is not addressed in this patch yet, but one could argue that it could be a separate follow-up issue about the duplication of configuration validation.

There would also be a third option, fixing it in the event subscriber but instead of injecting the ModuleInstaller one could make the one from the Importer available.

interesting!

The failing tests for the patches proves that we need to address both problems at the same time.
In fact running the ModuleUninstallValidator during the import validation doesn't make sense because the validation checks the currently active site configuration and not the configuration that will be imported.

What we need to do instead is to make sure that all the validators also have a config import event subscriber that can take the imported configuration into consideration or have an alternative validation that takes the imported configuration and does its checks with that.

More broadly I think it would be nice to have a standard way to validate that a set of configuration is in itself consistent and a way to bypass the validation and enforce it even though it leads to data loss. (for example uninstall the book module even though there is a book outline left, and instead of complaining the process should remove the outline.)

Triaged for bug smash. Given the patch age likely needs a reroll and there's actionable feedback from previous reviews

Rerolled patch against 9.3.x
Attached rerolled diff

Fixed errors
Attached interdiff against #25

Fixed errors
Attached interdiff against #26

Error fixes.
Attached interdiff

I think the fails in Drupal\Tests\field\Kernel\FieldImportDeleteUninstallTest show that we can't just call all the uninstall validators on the modules. Deleting fields and uninstalling the module that provides has to be possible.

Therefore I think we should introduce a new tag that allows uninstall validators to be registered with \Drupal\Core\EventSubscriber\ConfigImportSubscriber to be run. There we can update the core provider uninstall validators appropriately.

Something like this. No interdiff because the approach is quite different.

That said I'm not sure. It's pretty tricky because whatever we tag with config_import.module_uninstall_validator it needs to be aware that configuration might be deleted.

Here are a couple of other services that can be tagged with config_import.module_uninstall_validator.

I think this is an okay idea. We need to improve the documentation on \Drupal\Core\Extension\ModuleUninstallValidatorInterface to explain why you would also add the config_import.module_uninstall_validator tag and we need an issue summary update and a change record.

Here are the validators in core that do not get the tag and why:

• field.uninstall_validator: because otherwise you'd never be able to uninstall a module and delete a field in the same config import. We test this and this is why #29 fails - see https://www.drupal.org/pift-ci-job/2391564 and field_config_import_steps_alter()
• filter.uninstall_validator: because the check depends on config that might be changing during the import
• module_required_by_themes_uninstall_validator: because the config import might be uninstalling the theme too

Given that module uninstall validators run during config import I suspect that filter.uninstall_validator and module_required_by_themes_uninstall_validator mean that you can break config import because of these checks. I'm not sure it is in the scope of this issue to fix and maybe the solution is to add the tag to these so people can take manual steps to fix this prior to import. Hmmm...

I'd like to land this issue prior to #1170362: Install profile is disabled for lots of different reasons and core doesn't allow for that so I'm going to make this issue part of the Distribution initiative.

Forgot to add the patch - lol.

Slightly different approach. Given that uninstall validators are going to run during a configuration import we should normally run them all first - this makes it less likely that config import is going to break because an uninstall validator triggers an error - which is a good thing.

New approach adds an interface \Drupal\Core\Extension\ConfigImportModuleUninstallValidatorInterface that allows an uninstall validator to do different things when it is called prior to a configuration import. This seems more consistent than adding a new tag that 99% of module uninstall validators are going to need to call. It also allows for more flexibility than being called or not.

Updated issue summary and created a change record.

The tests failed due to HEAD being broken.

Rather than have the method name as a variable I think it's better to call the method directly - it's nicer for humans, IDEs and static analysers.

With a bit more typehinting because PHP goodness.

my patch on is issue is so old that I have completely forgotten what was in it, so I looked at this again like a new person.

I like the additional typehints compared to the older interface from #42. Also the change from #41 is much nicer.

I also like the approach with the new interface adding a new method because it defaults to also running the validator early but gives modules a way out if the validation at that point is not needed. Whereas the new tag would mean everyone would have to opt in, which I would assume, is much less likely to happen.

I was think about how #1170362: Install profile is disabled for lots of different reasons and core doesn't allow for that might use this and I realised that it will be useful to pass along either the ConfigImporter object, or the StorageComparer or the Source storage - so that the validator can interrogate the config as it will be after the import. Going to mull over the best option.

what a coincidence! I was thinking about this more last night in bed after I posted the RTBC.
So what we would need is not just a module uninstall validator but a validator for a whole set of configuration.
Like you get passed a config storage, a storage comparer to get the storage from, or the importer to get the comparer from.

Some uninstall validators can run before the actual config import while others just need to pull the emergency break before the module is gone.
I don't know if we want to solve both of these problems at the same time or not but I think it would be useful to do so.

Well for the more generic whole config validation you can subscribe to the \Drupal\Core\Config\ConfigEvents::IMPORT_VALIDATE event. I've woken up thinking that passing the source storage is best here. Because it'll allow the validator to check the future state of the configuration. If it needs the active state it can get that via dependency injection and the only thing it should want from the ConfigImporter is the extension changelist - so I think we should pass that as well.

So I decided to add the source storage to the interface and then convert \Drupal\Core\Extension\ModuleRequiredByThemesUninstallValidator to use it. As it turns out installing or uninstalling a theme with module dependencies via configuration import is quite broken! The fact that themes can now depend on modules means that we need to process extension installs and uninstalls in a very particular order. First we need to uninstall themes and then modules. Then we need to install modules and then themes. This ensures dependency expectations can be met. Secondly, the current check in \Drupal\Core\EventSubscriber\ConfigImportSubscriber::validateThemes() to ensure dependencies are installed is broken if the dependencies contain modules. This is an omission from the original change that allowed themes to depend on modules.

I think given how broken theme install / uninstall via config import is when the theme has module dependencies is means that this issue is now a critical.

A couple of notes on the changes in the latest patch.

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -254,6 +254,7 @@ public function reset() {
        $this->validated = FALSE;
        $this->processedSystemTheme = FALSE;
   +    $this->errors = [];
        return $this;
   

   This should have always been the case. This change helps testing by making the ConfigImporter::reset() clear all errors and recalculate them. This should have been the case the moment we set $this->validated = FALSE above.

2. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -673,13 +674,18 @@ protected function finish(&$context) {
   -    foreach (['module', 'theme'] as $type) {
   -      foreach (['install', 'uninstall'] as $op) {
   +    foreach (['uninstall', 'install'] as $op) {
   +      $types = $op === 'uninstall' ? ['theme', 'module'] : ['module', 'theme'];
   +      foreach ($types as $type) {
   

   This change is slightly scary because it changes how we have been doing things for years. I think it is okay - but it would be good if people have a good think about this. I'm not 100% sure how to fix this if w don't make this change.

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -673,13 +674,18 @@ protected function finish(&$context) {
   /**
    * Gets the next extension operation to perform.
    *
+   * Uninstalls are processed first with themes coming before modules. Then
+   * installs are processed with modules coming before themes. This order is
+   * necessary because themes can depend on modules.

Haven't reviewed the entire patch, but just to say this makes perfect sense. Also ensures that we can never make modules depend on themes.

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -254,6 +254,7 @@ public function reset() {
     $this->validated = FALSE;
     $this->processedSystemTheme = FALSE;
+    $this->errors = [];
     return $this;

Ah we can't do this because we call ->reset() in \Drupal\Core\Config\ConfigImporter::finish() and that removes all the error information. Not good. Let's fix that here but I'm going to open a separate issue to fix this on it's own because otherwise this issue get's lots of unrelated change.

+++ b/core/tests/Drupal/KernelTests/Core/Theme/ConfigImportThemeInstalTest.php
@@ -0,0 +1,82 @@
+    try {
+      $this->configImporter()->validate();
+      $this->fail('ConfigImporterException not thrown; an invalid import was not stopped due to missing dependencies.');
+    }
+    catch (ConfigImporterException $e) {
+      $error_message = 'Unable to uninstall the <em class="placeholder">Test Module Required by Theme</em> module because: Required by the theme: Test Theme Depending on Modules.';
+      $this->assertStringContainsString($error_message, $e->getMessage(), 'There were errors validating the config synchronization.');
+      $error_log = $this->configImporter->getErrors();
+      $this->assertEquals([$error_message], $error_log);
+    }
+
+    // Remove the other module and the theme.
+    unset($extensions['module']['test_another_module_required_by_theme']);
+    unset($extensions['theme']['test_theme_depending_on_modules']);
+    $sync->write('core.extension', $extensions);
+    $this->configImporter()->import();
+

This is why #3284270: Reset \Drupal\Core\Config\ConfigImporter::$errors in ::validate() method is necessary - without that change the second import here fails because the validation error from the earlier call to $this->configImporter()->validate() is still in the ConfigImporter::$errors array.

We could work around this here by building a completely new ConfigImporter object here but that does not look very pretty.

small nit: we don't need to call this $staging any more

$staging = $this->container->get('config.storage.sync');

Also I am not sure I followed the test correctly, but are we also testing the uninstallation?

#3284270: Reset \Drupal\Core\Config\ConfigImporter::$errors in ::validate() method is in... less change here now.

#53 $staging is fixed.

RE the test - I guess you mean testConfigImportWithThemeWithModuleDependencies - yes that tests both uninstalling and installing a theme with dependencies. All the other test, testRequiredModuleValidation, is testing an uninstall because that'd really what is changing here. The theme tests needs to test both install and uninstall because actually both are broken :D

I think this is a pragmatic solution.
Nice catch with the theme order. I guess not many people uninstall themes.
I tested it with my contrib bug but it is also not the cause.

A couple of things in the patch that made me think 'do we really have to do that?' but as far as I can tell, we do in fact really have to do that.

Couple of comment nits:

1. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
   @@ -50,6 +59,16 @@ public function __construct(ThemeHandlerInterface $theme_handler, ModuleExtensio
    
   +  /**
   +   * Adds module a uninstall validator.
   +   *
   

   'Adds a module uninstall validator'?

2. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
   @@ -181,13 +211,28 @@ protected function validateThemes(ConfigImporter $config_importer) {
        foreach ($installs as $theme) {
   -      foreach (array_keys($theme_data[$theme]->requires) as $required_theme) {
   +      $module_dependencies = $theme_data[$theme]->module_dependencies;
   +      // $theme_data[$theme]->requires contains both theme and module
   +      // dependencies keyed by the extension machine names and
   +      // $theme_data[$theme]->module_dependencies contains only modules keyed
   +      // by the module extension machine name. Therefore, we can find the theme
   +      // dependencies by finding array keys for 'requires' that are not in
   +      // $module_dependencies.
   

   Could we split this into two sections, or add a semicolon?

3. +++ b/core/lib/Drupal/Core/Extension/ConfigImportModuleUninstallValidatorInterface.php
   @@ -0,0 +1,34 @@
   + * Special interface for module uninstall validators for configuration import.
   + *
   + * A module uninstall validator the needs different functionality prior to a
   + * configuration import should implement this interface and be defined in
   

   'the needs different' - should 'the' be 'that'?

Thanks for the review @catch - both #56.1 and .2 are c&p's so I've fixed them in the places they were c&p'd from as they are both in very related code.

Committed/pushed to 10.0.x and cherry-picked to 9.5.x, thanks!

I'm not 100% on backporting this to 9.4.x - new interface etc., re-open if you strongly feel it should be.

I think we should consider this for 9.4.0. This is correctly a critical issue due to the theme install uninstall checks - especially as I expect people will be deploying more theme change in 9.4.x - due to claro becoming stable.

I think all the API changes are additions so safer than a change. Plus the uninstall validator will already run on config import - just in a way that potentially puts your site into a completely untested state.

OK fair enough. I've added a release notes snippet, and tagging for 9.4.0 release notes. Cherry-picked there too. Also crediting @xjm for discussing the backport in slack.

It appears this may have resolved #3176625: Install from existing config with a theme that declares module dependencies fails