ModuleInstaller::install() should provide more information than simply returning FALSE when a module isn't in the file system

This is a good idea. As well as missing modules, it will also help whenever there are module-name typos (e.g. trying to enable entity_reference in an update hook fails, because the correct name is entityreference).

#5: module_not_found-module_enable-1719648-4.patch queued for re-testing.

Duplicate of #1979508: ModuleHandler::install() silently fails if a module isn't in the file system.

Feel free to switch them if you like -- that one had a better summary and I was lazy :(
You'd need to update the status and title of this one too.

Revised code snippet to remove debugging info

If this is being kept as the main issue, it needs the title and summary updating.

Also, the patch is not the best approach: an exception should be thrown so that this shows up in tests.

Added details about installing multiple modules and Drupal 8 details

combining both issue summaries

I don't see that a logging a watchdog error is useful at all -- the system will already be in an unpredictable state by then, and it's of no use in a testing scenario.

#1979508: ModuleHandler::install() silently fails if a module isn't in the file system was tagged as needing backport, so presumably there's a way to do it.

I'd anything that's trying to enable non-existent modules is broken. This is just a better way to detect the problem.

Figured out the failing tests.

Very much still an issue, but needs a reroll after #2324055: Split up the module manager into runtime information and extension information..

Wow, that patch was nearly a year old :/

Looks like the failures caught some missing modules :)

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -69,9 +69,12 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
+        throw new \Exception(format_string('Unable to enable modules %modules due to missing modules %missing.', array(

I wonder if we should add a new exception type here, so calling code can specifically check for this failure, versus other exceptions that might be thrown during the install process?

Weird. config_existing_default_config_test exists at core/modules/config/tests/config_existing_default_config_test. Why would it claim it's missing? The test in ConfigInstallWebTest is not expecting it to be missing.

> I wonder if we should add a new exception type here, so calling code can specifically check for this failure, versus other exceptions that might be thrown during the install process?

I like the idea, but the logical thing would be to have an ExtensionInstallerException rather than just a ModuleInstallerException, and change ThemeHandler::install() to throw it too... which is patch creep. If we make it just a ModuleInstallerException, then we're introducing a brand new little DrupalWTF, which will need clean-up in 9. But then I suppose that we're already changing the behaviour of missing modules compared to missing themes here anyway.

So let's try that then :)

This should fix the ModuleHandlerTest fail: I can see why that's going wrong.

As for the config test -- no idea. I'm uploading a totally separate patch just adds an assertion to that test, so we can see if it's actually the test that's broken and currently fails silently on 8.0.x.

Ha! #2387669: ConfigInstallWebTest is broken.

Once that's fixed, this should be green.

I've run into this with drush when there was a typo in a module name, or running it on the wrong install, very confusing.

+++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
@@ -31,7 +31,10 @@
+   * @throws Exception

Should be updated to @throws ModuleInstallerException.

Thanks!

Setting back to needs review for other humans to look at, though we know the testbot will fail it and why.

This is a great patch. Just a few nits:

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -69,9 +69,12 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +        throw new ModuleInstallerException(format_string('Unable to enable modules %modules due to missing modules %missing.', array(
   
   +++ b/core/modules/simpletest/src/Tests/SimpleTestTest.php
   @@ -210,7 +210,7 @@ function assertNothing() {
   +    $this->assertAssertion(t('Unable to enable 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()');
   

   "Unable to install modules..." sounds better given we don't "enable" modules anymore.

2. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -913,8 +913,16 @@ protected function setUp() {
   +        $this->assertTrue($success, String::format('Enabled modules: %modules', array('%modules' => implode(', ', $modules))));
   +        $this->assertTrue($success, t('Enabled modules: %modules', array('%modules' => implode(', ', $modules))));
   

   s/Enabled/Installed/ same as above.

Updated the title and added beta evaluation.

Thanks for reviewing!

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -913,8 +913,16 @@ protected function setUp() {
-      $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))));
+        $this->assertTrue($success, t('Enabled modules: %modules', array('%modules' => implode(', ', $modules))));
+      }

That test assertion message was there before; I've merely moved it into the try{} block. There are quite a few occurrences of 'enabled modules' throughout core/modules/simpletest/src, so it's out of scope to fix this here I think. Let's leave that for a follow-on.

Though I have spotted that we have two identical lines there, one using t(), which probably shouldn't be there. So fixing that :)

Can we add a similar check to KernelTestBase::enableModules() ?

We always end up with no longer existing modules/typos in those tests, would be nice to throw an exception instead.

We should revert as part of this patch this change from #2387669: ConfigInstallWebTest is broken, as checking the return of install() is no longer necessary:

+++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
@@ -132,7 +132,8 @@ function testInstallProfileConfigOverwrite() {
     // should be ignored.
-    \Drupal::service('module_installer')->install(array('config_existing_default_config_test'));
+    $status = \Drupal::service('module_installer')->install(array('config_existing_default_config_test'));
+    $this->assertTrue($status, "The module config_existing_default_config_test was installed.");
 

Updated patch.

> Can we add a similar check to KernelTestBase::enableModules() ?

Can that be done in a separate issue? I'm not sure where to change something in KernelTestBase::enableModules, and it's going to need corresponding changes to KernelTestBaseTest, I assume.

Just another little nit:

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -913,8 +913,15 @@ protected function setUp() {
+        $this->assertTrue($success, String::format('Enabled modules: %modules', array('%modules' => implode(', ', $modules))));

s/Enabled/Installed/

Otherwise, this looks RTBC to me.

I really like these detail issues, which really reduces the annoying things on a day to day work.

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -69,9 +69,12 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +        throw new ModuleInstallerException(format_string('Unable to install modules %modules due to missing modules %missing.', array(
   +          '%modules' => implode(', ', $module_list),
   +          '%missing' => implode(', ', $missing_modules),
   +        )));
   

   So in case we install multiple modules at the same time, can we figure out which of the request modules has the unmet dependency? Note: Let's directly use String::format() as format_string() is marked as deprecated.

2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -87,7 +90,9 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +            throw new ModuleInstallerException(format_string('Depency module %name is missing.', array(
   +              '%name' => $module,
   +            )));
   

   Note: Let's directly use String::format() as format_string() is marked as deprecated.

   +

   Let's use 'Dependency module %name is missing.'

New patch with changes requested in #48.

+            throw new ModuleInstallerException(format_string('Depency module %name is missing.', array(
+              '%name' => $module,

Turns out that $module IS the module with the missing dependency, so that message was wrong. Fixed and extended (and typo fixed too).

> +++ b/core/modules/simpletest/src/WebTestBase.php
> s/Enabled/Installed/

That's an assertion that's just getting moved into a try-catch block. There are uses of 'enabled' all over that test, so best all covered up in a follow-up.

Oh I probably need to make the string change in the tests too... :/

Rerolled.

I think this is RTBC.

Bizarre, that was green earlier.

This should be!

(The problem was a missing 'use Drupal\Component\Utility\String;' at the top of ModuleInstaller.php.)

+++ b/core/lib/Drupal/Core/Extension/ModuleInstallerException.php
@@ -0,0 +1,15 @@
+/**
+ * Exception class to throw when modules are missing on install.
+ *
+ * @see \Drupal\Core\Extension\ModuleInstaller::install()
+ */
+class ModuleInstallerException extends \Exception {}

If the exception is specific to that, then we should name it accordingly. MissingDependencyException or so.

Done.

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -916,8 +916,15 @@ protected function setUp() {
+      catch (\Exception $e) {

+++ b/core/modules/system/src/Tests/Extension/ModuleHandlerTest.php
@@ -116,8 +116,14 @@ function testDependencyResolution() {
+    catch (\Exception $e) {

I think these should only catch the expected exception, not all exceptions. Sorry I didn't notice earlier.

Done.

I think this is now RTBC.

@joachim
Just make sure that you do interdiffs in the future.

I love this issue, it is certainly a nice DX improvement.

Committed/pushed to 8.0.x, thanks!

This is marked as a bug for Drupal 7 backport, but I don't see how it's backportable in its current form (could very easily break existing code to throw an exception where returning FALSE was previously expected), nor do I see a bug... I can't reproduce this at all (from the issue summary):

In Drupal 7, when enabling multiple modules, all modules up to the one that is missing will be installed, and the ones after will not be installed. This can cause instability issues.

If someone can reproduce this, please provide more details on how to do so.

I guess we could do a watchdog message instead (i.e. the earlier suggestion in this issue) if people think it would be useful. Generally I would think the caller should decide that kind of thing, but it's hard for the caller to determine and the function already does watchdog logging in other places.