From ba827d468c40731d3c4fcebfb7e29e765df7a6ea Mon Sep 17 00:00:00 2001
From: Alex Pott <alex.a.pott@googlemail.com>
Date: Sat, 23 Dec 2023 21:49:48 +0100
Subject: [PATCH] Issue #3103962 by godotislate, smustgrave, alexpott,
larowlan: ConfigInstaller::isSyncing() is not reset before module preinstall
---
.../Drupal/Core/Extension/ModuleInstaller.php | 30 ++++++++-----
core/lib/Drupal/Core/Extension/module.api.php | 17 ++++++-
.../modules/system_test/system_test.module | 30 +++++++++++--
.../Core/Config/ConfigImporterTest.php | 44 +++++++++++++++++++
4 files changed, 104 insertions(+), 17 deletions(-)
diff --git a/core/lib/Drupal/Core/Extension/ModuleInstaller.php b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
index 4e5734bb6bca..289ecffe41fb 100644
--- a/core/lib/Drupal/Core/Extension/ModuleInstaller.php
+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -179,9 +179,6 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
/** @var \Drupal\Core\Config\ConfigInstaller $config_installer */
$config_installer = \Drupal::service('config.installer');
$sync_status = $config_installer->isSyncing();
- if ($sync_status) {
- $source_storage = $config_installer->getSourceStorage();
- }
$modules_installed = [];
foreach ($module_list as $module) {
$enabled = $extension_config->get("module.$module") !== NULL;
@@ -268,7 +265,7 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
}
// Allow modules to react prior to the installation of a module.
- $this->moduleHandler->invokeAll('module_preinstall', [$module]);
+ $this->moduleHandler->invokeAll('module_preinstall', [$module, $sync_status]);
// Now install the module's schema if necessary.
$this->installSchema($module);
@@ -326,12 +323,7 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
// Install default configuration of the module.
$config_installer = \Drupal::service('config.installer');
- if ($sync_status) {
- $config_installer
- ->setSyncing(TRUE)
- ->setSourceStorage($source_storage);
- }
- \Drupal::service('config.installer')->installDefaultConfig('module', $module);
+ $config_installer->installDefaultConfig('module', $module);
// If the module has no current updates, but has some that were
// previously removed, set the version to the value of
@@ -467,7 +459,7 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
}
// Allow modules to react prior to the uninstallation of a module.
- $this->moduleHandler->invokeAll('module_preuninstall', [$module]);
+ $this->moduleHandler->invokeAll('module_preuninstall', [$module, $sync_status]);
// Uninstall the module.
$this->moduleHandler->loadInclude($module, 'install');
@@ -604,10 +596,17 @@ function ($definition) {
/**
* Updates the kernel module list.
*
- * @param string $module_filenames
+ * @param string[] $module_filenames
* The list of installed modules.
*/
protected function updateKernel($module_filenames) {
+ // Save current state of config installer, so it can be restored after the
+ // container is rebuilt.
+ /** @var \Drupal\Core\Config\ConfigInstallerInterface $config_installer */
+ $config_installer = $this->kernel->getContainer()->get('config.installer');
+ $sync_status = $config_installer->isSyncing();
+ $source_storage = $config_installer->getSourceStorage();
+
// This reboots the kernel to register the module's bundle and its services
// in the service container. The $module_filenames argument is taken over as
// %container.modules% parameter, which is passed to a fresh ModuleHandler
@@ -619,6 +618,13 @@ protected function updateKernel($module_filenames) {
$this->moduleHandler = $container->get('module_handler');
$this->connection = $container->get('database');
$this->updateRegistry = $container->get('update.update_hook_registry');
+
+ // Restore state of config installer.
+ if ($sync_status) {
+ $container->get('config.installer')
+ ->setSyncing(TRUE)
+ ->setSourceStorage($source_storage);
+ }
}
/**
diff --git a/core/lib/Drupal/Core/Extension/module.api.php b/core/lib/Drupal/Core/Extension/module.api.php
index 62f0bd65f937..ed735b833928 100644
--- a/core/lib/Drupal/Core/Extension/module.api.php
+++ b/core/lib/Drupal/Core/Extension/module.api.php
@@ -161,8 +161,15 @@ function hook_system_info_alter(array &$info, \Drupal\Core\Extension\Extension $
*
* @param string $module
* The name of the module about to be installed.
+ * @param bool $is_syncing
+ * TRUE if the module is being installed as part of a configuration import. In
+ * these cases, your hook implementation needs to carefully consider what
+ * changes, if any, it should make. For example, it should not make any
+ * changes to configuration objects or configuration entities. Those changes
+ * should be made earlier and exported so during import there's no need to
+ * do them again.
*/
-function hook_module_preinstall($module) {
+function hook_module_preinstall($module, bool $is_syncing) {
mymodule_cache_clear();
}
@@ -252,8 +259,14 @@ function hook_install($is_syncing) {
*
* @param string $module
* The name of the module about to be uninstalled.
+ * @param bool $is_syncing
+ * TRUE if the module is being uninstalled as part of a configuration import.
+ * In these cases, your hook implementation needs to carefully consider what
+ * changes to configuration objects or configuration entities. Those changes
+ * should be made earlier and exported so during import there's no need to
+ * do them again.
*/
-function hook_module_preuninstall($module) {
+function hook_module_preuninstall($module, bool $is_syncing) {
mymodule_cache_clear();
}
diff --git a/core/modules/system/tests/modules/system_test/system_test.module b/core/modules/system/tests/modules/system_test/system_test.module
index e52b030abb1d..c30a87e81f46 100644
--- a/core/modules/system/tests/modules/system_test/system_test.module
+++ b/core/modules/system/tests/modules/system_test/system_test.module
@@ -35,12 +35,20 @@ function system_test_modules_installed($modules) {
/**
* Implements hook_modules_uninstalled().
*/
-function system_test_modules_uninstalled($modules) {
+function system_test_modules_uninstalled($modules, $is_syncing) {
if (\Drupal::state()->get('system_test.verbose_module_hooks')) {
foreach ($modules as $module) {
\Drupal::messenger()->addStatus(t('hook_modules_uninstalled fired for @module', ['@module' => $module]));
}
}
+
+ // Save the config.installer isSyncing() value to state to check that it is
+ // correctly set when installing module during config import.
+ \Drupal::state()->set('system_test_modules_uninstalled_config_installer_syncing', \Drupal::service('config.installer')->isSyncing());
+
+ // Save the $is_syncing parameter value to state to check that it is correctly
+ // set when installing module during config import.
+ \Drupal::state()->set('system_test_modules_uninstalled_syncing_param', $is_syncing);
}
/**
@@ -132,14 +140,30 @@ function system_test_filetransfer_info() {
/**
* Implements hook_module_preinstall().
*/
-function system_test_module_preinstall($module) {
+function system_test_module_preinstall($module, bool $is_syncing) {
\Drupal::messenger()->addStatus('system_test_preinstall_module called');
\Drupal::state()->set('system_test_preinstall_module', $module);
+
+ // Save the config.installer isSyncing() value to state to check that it is
+ // correctly set when installing module during config import.
+ \Drupal::state()->set('system_test_preinstall_module_config_installer_syncing', \Drupal::service('config.installer')->isSyncing());
+
+ // Save the $is_syncing parameter value to state to check that it is correctly
+ // set when installing module during config import.
+ \Drupal::state()->set('system_test_preinstall_module_syncing_param', $is_syncing);
}
/**
* Implements hook_module_preuninstall().
*/
-function system_test_module_preuninstall($module) {
+function system_test_module_preuninstall($module, bool $is_syncing) {
\Drupal::state()->set('system_test_preuninstall_module', $module);
+
+ // Save the config.installer isSyncing() value to state to check that it is
+ // correctly set when uninstalling module during config import.
+ \Drupal::state()->set('system_test_preuninstall_module_config_installer_syncing', \Drupal::service('config.installer')->isSyncing());
+
+ // Save the $is_syncing parameter value to state to check that it is correctly
+ // set when installing module during config import.
+ \Drupal::state()->set('system_test_preuninstall_module_syncing_param', $is_syncing);
}
diff --git a/core/tests/Drupal/KernelTests/Core/Config/ConfigImporterTest.php b/core/tests/Drupal/KernelTests/Core/Config/ConfigImporterTest.php
index bf17339b8b69..9f3e9fd9a294 100644
--- a/core/tests/Drupal/KernelTests/Core/Config/ConfigImporterTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigImporterTest.php
@@ -829,6 +829,50 @@ public function testIsSyncingInHooks() {
$this->assertTrue($state['entity_state::predelete'], 'ConfigEntity::isSyncing() returns TRUE');
$this->assertTrue($state['global_state::delete'], '\Drupal::isConfigSyncing() returns TRUE');
$this->assertTrue($state['entity_state::delete'], 'ConfigEntity::isSyncing() returns TRUE');
+
+ // Test that isSyncing is TRUE in hook_module_preinstall() when installing
+ // module via config import.
+ $extensions = $sync->read('core.extension');
+ // First, install system_test so that its hook_module_preinstall() will run
+ // when module_test is installed.
+ $this->container->get('module_installer')->install(['system_test']);
+ // Add module_test and system_test to the enabled modules to be imported,
+ // so that module_test gets installed on import and system_test does not get
+ // uninstalled.
+ $extensions['module']['module_test'] = 0;
+ $extensions['module']['system_test'] = 0;
+ $extensions['module'] = module_config_sort($extensions['module']);
+ $sync->write('core.extension', $extensions);
+ $this->configImporter->reset()->import();
+
+ // Syncing values stored in state by hook_module_preinstall should be TRUE
+ // when module is installed via config import.
+ $this->assertTrue(\Drupal::state()->get('system_test_preinstall_module_config_installer_syncing'), '\Drupal::isConfigSyncing() in system_test_module_preinstall() returns TRUE');
+ $this->assertTrue(\Drupal::state()->get('system_test_preinstall_module_syncing_param'), 'system_test_module_preinstall() $is_syncing value is TRUE');
+
+ // Syncing value stored in state by uninstall hooks should be FALSE
+ // when uninstalling outside of config import.
+ $this->container->get('module_installer')->uninstall(['module_test']);
+ $this->assertFalse(\Drupal::state()->get('system_test_preuninstall_module_config_installer_syncing'), '\Drupal::isConfigSyncing() in system_test_module_preuninstall() returns FALSE');
+ $this->assertFalse(\Drupal::state()->get('system_test_preuninstall_module_syncing_param'), 'system_test_module_preuninstall() $is_syncing value is FALSE');
+ $this->assertFalse(\Drupal::state()->get('system_test_modules_uninstalled_config_installer_syncing'), '\Drupal::isConfigSyncing() in system_test_modules_uninstalled returns FALSE');
+ $this->assertFalse(\Drupal::state()->get('system_test_modules_uninstalled_syncing_param'), 'system_test_modules_uninstalled $is_syncing value is FALSE');
+
+ // Syncing value stored in state by hook_module_preinstall should be FALSE
+ // when installing outside of config import.
+ $this->container->get('module_installer')->install(['module_test']);
+ $this->assertFalse(\Drupal::state()->get('system_test_preinstall_module_config_installer_syncing'), '\Drupal::isConfigSyncing() in system_test_module_preinstall() returns TRUE');
+ $this->assertFalse(\Drupal::state()->get('system_test_preinstall_module_syncing_param'), 'system_test_module_preinstall() $is_syncing value is TRUE');
+
+ // Uninstall module_test via config import. Syncing value stored in state
+ // by uninstall hooks should be TRUE.
+ unset($extensions['module']['module_test']);
+ $sync->write('core.extension', $extensions);
+ $this->configImporter->reset()->import();
+ $this->assertTrue(\Drupal::state()->get('system_test_preuninstall_module_config_installer_syncing'), '\Drupal::isConfigSyncing() in system_test_module_preuninstall() returns TRUE');
+ $this->assertTrue(\Drupal::state()->get('system_test_preuninstall_module_syncing_param'), 'system_test_module_preuninstall() $is_syncing value is TRUE');
+ $this->assertTrue(\Drupal::state()->get('system_test_modules_uninstalled_config_installer_syncing'), '\Drupal::isConfigSyncing() in system_test_modules_uninstalled returns TRUE');
+ $this->assertTrue(\Drupal::state()->get('system_test_modules_uninstalled_syncing_param'), 'system_test_modules_uninstalled $is_syncing value is TRUE');
}
/**
--
GitLab