Dependent modules are still installed when required modules return errors in hook_requirements('install')

Nice sleuthing. Is there any chance you would refactor this submit handler a bit? It is really hard to follow all the flow. And other code like drush wants to reuse parts of it.

In any case, we look forward to your tests on this.

This needs a re-roll.

#8: system-592800-8.patch queued for re-testing.

re-roll, this was definitely not trivial, I just hope that we have good test coverage of that function :)

Atleast the module test group passes locally for me...

The patch above was missing the test modules...

+++ modules/simpletest/tests/requirements1_test.info
@@ -0,0 +1,9 @@
\ No newline at end of file

+++ modules/simpletest/tests/requirements1_test.install
@@ -0,0 +1,34 @@
\ No newline at end of file

+++ modules/simpletest/tests/requirements1_test.module
@@ -0,0 +1,8 @@
\ No newline at end of file

+++ modules/simpletest/tests/requirements2_test.info
@@ -0,0 +1,9 @@
\ No newline at end of file

+++ modules/simpletest/tests/requirements2_test.module
@@ -0,0 +1,8 @@
\ No newline at end of file

This should not happen.

+++ modules/simpletest/tests/requirements1_test.install
@@ -0,0 +1,34 @@
+/**
+ * Implementation of hook_install().
+ */
+function requirements1_test_install() {
+}
+
+/**
+ * Implementation of hook_uninstall().
+ */
+function requirements1_test_uninstall() {
+}

It seems those can be removed?

+++ modules/simpletest/tests/requirements1_test.install
@@ -0,0 +1,34 @@
+ * Implementation of hook_requirements().

(and elsewhere) The new documentation standard is

Implements hook_HOOK().

+++ modules/simpletest/tests/requirements1_test.install
@@ -0,0 +1,34 @@
+  // Ensure translations don't break at install time
...
+  // Always fails requirements

(and possibly elsewhere) Missing trailing periods.

+++ modules/system/system.admin.inc
@@ -1125,114 +1128,108 @@ function system_modules_submit($form, &$form_state) {
+        $modules[$name]['enabled'] = FALSE;
+        foreach (array_keys($files[$name]->required_by) as $required_by) {
+          $modules[$required_by] = FALSE;
+        }
...
+  foreach ($modules as $name => $module) {
+    if ($module['enabled']) {

The boolean assignment of

$modules[$required_by] = FALSE;

will trigger a PHP notice in the following foreach when trying to access the 'enabled' key.

Not sure why tests didn't throw any exceptions.

Additionally, does this ensure that already enabled modules are not disabled upon attempt to enable a module that requires an already enabled module?

Powered by Dreditor.

Re-roll with the comments updated and unecessary hooks removed.

Additionally, does this ensure that already enabled modules are not disabled upon attempt to enable a module that requires an already enabled module?

I'm not 100% sure that I understood you correctly. I added an additional dependency on comment.module to requirements2_test.module and added a check to confirm that comment.module is still installed after a failed attempt to install requirements2_test.module. Is that what you meant? Then yes, that seems to work correctly.

changed (from previous patch)
$modules[$required_by] = FALSE;
to
$modules[$required_by]['enabled'] = FALSE;

tested by copying requirements1_test and requirements2_test into sites/all/modules and commenting out "hidden=true", enabled both modules, and neither was enabled, also comments which is listed as a dependency of requirements2_test was not disabled.

Why is this issue critical? From the status page: Critical - When a bug renders a module, a core system, or a popular function unusable.

Impact seems limited to extra modules enabled, only if a module fails to install... Am I missing something?

seems limited to extra modules enabled, only if a module fails to install

Read that again? :) It installs modules that cannot be installed.

+++ modules/simpletest/tests/requirements1_test.install	22 Apr 2010 22:34:12 -0000
@@ -0,0 +1,22 @@
Index: modules/simpletest/tests/requirements1_test.module

Index: modules/simpletest/tests/requirements1_test.module
===================================================================

===================================================================
RCS file: modules/simpletest/tests/requirements1_test.module

RCS file: modules/simpletest/tests/requirements1_test.module
diff -N modules/simpletest/tests/requirements1_test.module

diff -N modules/simpletest/tests/requirements1_test.module
--- /dev/null	1 Jan 1970 00:00:00 -0000

--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ modules/simpletest/tests/requirements1_test.module	22 Apr 2010 22:34:12 -0000

+++ modules/simpletest/tests/requirements1_test.module	22 Apr 2010 22:34:12 -0000
+++ modules/simpletest/tests/requirements1_test.module	22 Apr 2010 22:34:12 -0000
@@ -0,0 +1,8 @@

@@ -0,0 +1,8 @@
+<?php
+// $Id$
+

RCS file: modules/simpletest/tests/requirements2_test.info
diff -N modules/simpletest/tests/requirements2_test.info

It would be good to add a small code comment as to why this is an 'empty' module.

Not sure which patch was reviewed by Dries. #16 looks good to me and has a comment block in the "empty" modules.

Still apply (with benin fuzz).

I looked at this again, and decided to committed to CVS HEAD. The code looks good, comes with tests, and is, in fact, a small clean-up. Good job.