From 78e9940a30339138a511aa78d2f56fc079fc65bf Mon Sep 17 00:00:00 2001 From: Nathaniel Catchpole Date: Wed, 31 Dec 2014 11:14:07 +0000 Subject: [PATCH] Issue #1719648 by joachim, ttkaminski, kshama_deshmukh: ModuleInstaller::install() silently fails if a module isn't in the file system --- .../Core/Extension/MissingDependencyException.php | 15 +++++++++++++++ .../lib/Drupal/Core/Extension/ModuleInstaller.php | 13 ++++++++++--- .../Core/Extension/ModuleInstallerInterface.php | 5 ++++- .../config/src/Tests/ConfigInstallWebTest.php | 3 +-- .../simpletest/src/Tests/SimpleTestTest.php | 2 +- core/modules/simpletest/src/WebTestBase.php | 11 +++++++++-- .../system/src/Form/ModulesListConfirmForm.php | 4 ++++ .../src/Tests/Extension/ModuleHandlerTest.php | 10 ++++++++-- 8 files changed, 52 insertions(+), 11 deletions(-) create mode 100644 core/lib/Drupal/Core/Extension/MissingDependencyException.php diff --git a/core/lib/Drupal/Core/Extension/MissingDependencyException.php b/core/lib/Drupal/Core/Extension/MissingDependencyException.php new file mode 100644 index 000000000000..1cb07cd19345 --- /dev/null +++ b/core/lib/Drupal/Core/Extension/MissingDependencyException.php @@ -0,0 +1,15 @@ + implode(', ', $module_list), + '%missing' => implode(', ', $missing_modules), + ))); } // Only process currently uninstalled modules. @@ -101,7 +105,10 @@ class ModuleInstaller implements ModuleInstallerInterface { foreach (array_keys($module_data[$module]->requires) as $dependency) { if (!isset($module_data[$dependency])) { // The dependency does not exist. - return FALSE; + throw new MissingDependencyException(String::format('Unable to install modules: module %module is missing its dependency module %dependency.', array( + '%module' => $module, + '%dependency' => $dependency, + ))); } // Skip already installed modules. diff --git a/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php index 95cb25e0878e..bd2923a6d493 100644 --- a/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php @@ -31,7 +31,10 @@ interface ModuleInstallerInterface { * if you know $module_list is already complete. * * @return bool - * FALSE if one or more dependencies are missing, TRUE otherwise. + * TRUE if the modules were successfully installed. + * + * @throws \Drupal\Core\Extension\MissingDependencyException + * Thrown when a requested module, or a dependency of one, can not be found. * * @see hook_module_preinstall() * @see hook_install() diff --git a/core/modules/config/src/Tests/ConfigInstallWebTest.php b/core/modules/config/src/Tests/ConfigInstallWebTest.php index 521297336ea6..4defbd4033e8 100644 --- a/core/modules/config/src/Tests/ConfigInstallWebTest.php +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php @@ -132,8 +132,7 @@ class ConfigInstallWebTest extends WebTestBase { // Turn on the test module, which will attempt to replace the // configuration data. This attempt to replace the active configuration // should be ignored. - $status = \Drupal::service('module_installer')->install(array('config_existing_default_config_test')); - $this->assertTrue($status, "The module config_existing_default_config_test was installed."); + \Drupal::service('module_installer')->install(array('config_existing_default_config_test')); // Verify that the test module has not been able to change the data. $config = $this->config($config_name); diff --git a/core/modules/simpletest/src/Tests/SimpleTestTest.php b/core/modules/simpletest/src/Tests/SimpleTestTest.php index d2949a64cee5..69421197aaad 100644 --- a/core/modules/simpletest/src/Tests/SimpleTestTest.php +++ b/core/modules/simpletest/src/Tests/SimpleTestTest.php @@ -210,7 +210,7 @@ EOD; * Confirm that the stub test produced the desired results. */ function confirmStubTestResults() { - $this->assertAssertion(t('Enabled modules: %modules', array('%modules' => 'non_existent_module')), 'Other', 'Fail', 'SimpleTestTest.php', 'Drupal\simpletest\Tests\SimpleTestTest->setUp()'); + $this->assertAssertion(t('Unable to install modules %modules due to missing modules %missing.', array('%modules' => 'non_existent_module', '%missing' => 'non_existent_module')), 'Other', 'Fail', 'SimpleTestTest.php', 'Drupal\simpletest\Tests\SimpleTestTest->setUp()'); $this->assertAssertion($this->passMessage, 'Other', 'Pass', 'SimpleTestTest.php', 'Drupal\simpletest\Tests\SimpleTestTest->stubTest()'); $this->assertAssertion($this->failMessage, 'Other', 'Fail', 'SimpleTestTest.php', 'Drupal\simpletest\Tests\SimpleTestTest->stubTest()'); diff --git a/core/modules/simpletest/src/WebTestBase.php b/core/modules/simpletest/src/WebTestBase.php index 4109d8381b22..2bd33fc6ddd0 100644 --- a/core/modules/simpletest/src/WebTestBase.php +++ b/core/modules/simpletest/src/WebTestBase.php @@ -916,8 +916,15 @@ abstract class WebTestBase extends TestBase { } if ($modules) { $modules = array_unique($modules); - $success = $container->get('module_installer')->install($modules, TRUE); - $this->assertTrue($success, String::format('Enabled modules: %modules', array('%modules' => implode(', ', $modules)))); + try { + $success = $container->get('module_installer')->install($modules, TRUE); + $this->assertTrue($success, String::format('Enabled modules: %modules', array('%modules' => implode(', ', $modules)))); + } + catch (\Drupal\Core\Extension\MissingDependencyException $e) { + // The exception message has all the details. + $this->fail($e->getMessage()); + } + $this->rebuildContainer(); } diff --git a/core/modules/system/src/Form/ModulesListConfirmForm.php b/core/modules/system/src/Form/ModulesListConfirmForm.php index 2a31dbecc3d9..279b95ebf971 100644 --- a/core/modules/system/src/Form/ModulesListConfirmForm.php +++ b/core/modules/system/src/Form/ModulesListConfirmForm.php @@ -153,6 +153,10 @@ class ModulesListConfirmForm extends ConfirmFormBase { // Install the given modules. if (!empty($this->modules['install'])) { + // Don't catch the exception that this can throw for missing dependencies: + // the form doesn't allow modules with unmet dependencies, so the only way + // this can happen is if the filesystem changed between form display and + // submit, in which case the user has bigger problems. $this->moduleInstaller->install(array_keys($this->modules['install'])); } diff --git a/core/modules/system/src/Tests/Extension/ModuleHandlerTest.php b/core/modules/system/src/Tests/Extension/ModuleHandlerTest.php index 48fbf0e1dd73..130ffd2de3ed 100644 --- a/core/modules/system/src/Tests/Extension/ModuleHandlerTest.php +++ b/core/modules/system/src/Tests/Extension/ModuleHandlerTest.php @@ -116,8 +116,14 @@ class ModuleHandlerTest extends KernelTestBase { \Drupal::state()->set('module_test.dependency', 'missing dependency'); drupal_static_reset('system_rebuild_module_data'); - $result = $this->moduleInstaller()->install(array('color')); - $this->assertFalse($result, 'ModuleHandler::install() returns FALSE if dependencies are missing.'); + try { + $result = $this->moduleInstaller()->install(array('color')); + $this->fail(t('ModuleInstaller::install() throws an exception if dependencies are missing.')); + } + catch (\Drupal\Core\Extension\MissingDependencyException $e) { + $this->pass(t('ModuleInstaller::install() throws an exception if dependencies are missing.')); + } + $this->assertFalse($this->moduleHandler()->moduleExists('color'), 'ModuleHandler::install() aborts if dependencies are missing.'); // Fix the missing dependency.