? system-modules-submit-refactor-with-tests-01.patch
Index: modules/simpletest/tests/requirements1_test.info
===================================================================
RCS file: modules/simpletest/tests/requirements1_test.info
diff -N modules/simpletest/tests/requirements1_test.info
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ modules/simpletest/tests/requirements1_test.info	30 Oct 2009 01:32:46 -0000
@@ -0,0 +1,9 @@
+; $Id$
+name = Requirements 1 Test
+description = "Tests that a module is not installed when it fails hook_requirements('install')."
+package = Core
+version = VERSION
+core = 7.x
+files[] = requirements1_test.install
+files[] = requirements1_test.module
+hidden = TRUE
\ No newline at end of file
Index: modules/simpletest/tests/requirements1_test.install
===================================================================
RCS file: modules/simpletest/tests/requirements1_test.install
diff -N modules/simpletest/tests/requirements1_test.install
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ modules/simpletest/tests/requirements1_test.install	30 Oct 2009 01:32:46 -0000
@@ -0,0 +1,34 @@
+<?php
+// $Id$
+
+/**
+ * Implementation of hook_install().
+ */
+function requirements1_test_install() {
+}
+
+/**
+ * Implementation of hook_uninstall().
+ */
+function requirements1_test_uninstall() {
+}
+
+/**
+ * Implementation of hook_requirements().
+ */
+function requirements1_test_requirements($phase) {
+  $requirements = array();
+  // Ensure translations don't break at install time
+  $t = get_t();
+
+  // Always fails requirements
+  if ('install' == $phase) {
+    $requirements['requirements1_test'] = array(
+      'title' => $t('Requirements 1 Test'),
+      'severity' => REQUIREMENT_ERROR,
+      'description' => $t('Requirements 1 Test failed requirements.'),
+    );
+  }
+
+  return $requirements;
+}
\ No newline at end of file
Index: modules/simpletest/tests/requirements1_test.module
===================================================================
RCS file: modules/simpletest/tests/requirements1_test.module
diff -N modules/simpletest/tests/requirements1_test.module
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ modules/simpletest/tests/requirements1_test.module	30 Oct 2009 01:32:46 -0000
@@ -0,0 +1,8 @@
+<?php
+// $Id$
+
+/**
+ * @file
+ * Tests that a module is not installed when it fails
+ * hook_requirements('install').
+ */
\ No newline at end of file
Index: modules/simpletest/tests/requirements2_test.info
===================================================================
RCS file: modules/simpletest/tests/requirements2_test.info
diff -N modules/simpletest/tests/requirements2_test.info
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ modules/simpletest/tests/requirements2_test.info	30 Oct 2009 01:32:46 -0000
@@ -0,0 +1,9 @@
+; $Id$
+name = Requirements 2 Test
+description = "Tests that a module is not installed when the one it depends on fails hook_requirements('install)."
+dependencies[] = requirements1_test
+package = Core
+version = VERSION
+core = 7.x
+files[] = requirements2_test.module
+hidden = TRUE
\ No newline at end of file
Index: modules/simpletest/tests/requirements2_test.module
===================================================================
RCS file: modules/simpletest/tests/requirements2_test.module
diff -N modules/simpletest/tests/requirements2_test.module
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ modules/simpletest/tests/requirements2_test.module	30 Oct 2009 01:32:46 -0000
@@ -0,0 +1,8 @@
+<?php
+// $Id$
+
+/**
+ * @file
+ * Tests that a module is not installed when the one it depends on fails
+ * hook_requirements('install').
+ */
\ No newline at end of file
Index: modules/simpletest/tests/system_test.module
===================================================================
RCS file: /cvs/drupal/drupal/modules/simpletest/tests/system_test.module,v
retrieving revision 1.19
diff -u -p -r1.19 system_test.module
--- modules/simpletest/tests/system_test.module	18 Oct 2009 05:28:42 -0000	1.19
+++ modules/simpletest/tests/system_test.module	30 Oct 2009 01:32:46 -0000
@@ -219,6 +219,9 @@ function system_test_system_info_alter(&
       $info['version'] = '7.x-2.4-beta3';
     }
   }
+  if ($file->name == 'requirements1_test' || $file->name == 'requirements2_test') {
+    $info['hidden'] = FALSE;
+  }
 }
 
 /**
Index: modules/system/system.admin.inc
===================================================================
RCS file: /cvs/drupal/drupal/modules/system/system.admin.inc,v
retrieving revision 1.216
diff -u -p -r1.216 system.admin.inc
--- modules/system/system.admin.inc	19 Oct 2009 23:28:40 -0000	1.216
+++ modules/system/system.admin.inc	30 Oct 2009 01:32:48 -0000
@@ -875,7 +875,7 @@ function system_modules_confirm_form($mo
   $form['validation_modules'] = array('#type' => 'value', '#value' => $modules);
   $form['status']['#tree'] = TRUE;
 
-  foreach ($storage['more_modules'] as $info) {
+  foreach ($storage['more_required'] as $info) {
     $t_argument = array(
       '@module' => $info['name'],
       '@required' => implode(', ', $info['requires']),
@@ -902,9 +902,11 @@ function system_modules_confirm_form($mo
  */
 function system_modules_submit($form, &$form_state) {
   include_once DRUPAL_ROOT . '/includes/install.inc';
+
+  // Builds list of modules.
   $modules = array();
-  // If we're not coming from the confirmation form, build the list of modules.
   if (!isset($form_state['storage'])) {
+    // If we're not coming from the confirmation form, build the module list.
     foreach ($form_state['values']['modules'] as $group_name => $group) {
       foreach ($group as $module => $enabled) {
         $modules[$module] = array('group' => $group_name, 'enabled' => $enabled['enable']);
@@ -912,122 +914,101 @@ function system_modules_submit($form, &$
     }
   }
   else {
-    // If we are coming from the confirmation form, fetch
-    // the modules out of $form_state.
+    // If we coming from the confirmation form, fetch modules from $form_state.
     $modules = $form_state['storage']['modules'];
   }
 
-  // Get a list of all modules, it will be used to find which module requires
-  // which.
+  // Collect data for all modules to be able to determine dependencies.
   $files = system_rebuild_module_data();
 
-  // The modules to be enabled.
-  $modules_to_be_enabled = array();
-  // The modules to be disabled.
-  $disable_modules = array();
-  // The modules to be installed.
-  $new_modules = array();
-  // Modules that need to be switched on because other modules require them.
-  $more_modules = array();
-  // Go through each module, finding out if we should enable, install, or
-  // disable it. Also, we find out if there are modules it requires that are
-  // not enabled.
+  // Sorts modules by weight.
+  $sort = array();
+  foreach (array_keys($modules) as $module) {
+    $sort[$module] = $files[$module]->sort;
+  }
+  array_multisort($sort, $modules);
+
+  // Makes sure all required modules are set to be enabled.
+  $more_required = array();
   foreach ($modules as $name => $module) {
-    // If it's enabled, find out whether to just
-    // enable it, or install it.
     if ($module['enabled']) {
-      if (drupal_get_installed_schema_version($name) == SCHEMA_UNINSTALLED) {
-        $new_modules[$name] = $name;
-      }
-      elseif (!module_exists($name)) {
-        $modules_to_be_enabled[$name] = $name;
-      }
-      // If we're not coming from a confirmation form, search for modules the
-      // new ones require and see whether there are any that additionally
-      // need to be switched on.
-      if (empty($form_state['storage'])) {
-        foreach ($form['modules'][$module['group']][$name]['#requires'] as $requires => $v) {
-          if (!$modules[$requires]['enabled']) {
-            if (!isset($more_modules[$name])) {
-              $more_modules[$name]['name'] = $files[$name]->info['name'];
-            }
-            $more_modules[$name]['requires'][$requires] = $files[$requires]->info['name'];
-          }
-          $modules[$requires] = array('group' => $files[$requires]->info['package'], 'enabled' => TRUE);
+
+      // Checks that all dependencies are set to be enabled.  Stores the ones
+      // that are not in $dependencies variable so that the user can be alerted
+      // in the confirmation form that more modules need to be enabled.
+      $dependencies = array();
+      foreach (array_keys($files[$name]->requires) as $required) {
+        if (!$modules[$required]['enabled']) {
+          $dependencies[] = $files[$required]->info['name'];
+          $modules[$required]['enabled'] = 1;
         }
       }
+
+      // Stores additional modules that need to be enabled in $more_required.
+      if (!empty($dependencies)) {
+        $more_required[$name] = array(
+          'name' => $files[$name]->info['name'],
+          'requires' => $dependencies,
+        );
+      }
     }
   }
-  // A second loop is necessary, otherwise the modules set to be enabled in the
-  // previous loop would not be found.
-  foreach ($modules as $name => $module) {
-    if (module_exists($name) && !$module['enabled']) {
-      $disable_modules[$name] = $name;
-    }
+
+  // Redirects to confirmation form if more modules need to be enabled.
+  if (!empty($more_required) && !isset($form_state['values']['confirm'])) {
+    $form_state['storage'] = array('more_required' => $more_required, 'modules' => $modules);
+    return;
   }
-  if ($more_modules) {
-    // If we need to switch on more modules because other modules require
-    // them and they haven't confirmed, don't process the submission yet. Store
-    // the form submission data needed later.
-    if (!isset($form_state['values']['confirm'])) {
-      $form_state['storage'] = array('more_modules' => $more_modules, 'modules' => $modules);
-      return;
-    }
-    // Otherwise, install or enable the modules.
-    else {
-      foreach ($form_state['storage']['more_modules'] as $info) {
-        foreach ($info['requires'] as $requires => $name) {
-          if (drupal_get_installed_schema_version($name) == SCHEMA_UNINSTALLED) {
-            $new_modules[$name] = $name;
-          }
-          else {
-            $modules_to_be_enabled[$name] = $name;
-          }
+
+  // Invokes hook_requirements('install').  If failures are detected, makes sure
+  // the dependent modules aren't installed either.
+  foreach ($modules as $name => $module) {
+    // Only invoke hook_requirements() on modules that are going to be installed.
+    if ($module['enabled'] && drupal_get_installed_schema_version($name) == SCHEMA_UNINSTALLED) {
+      if (!drupal_check_module($name)) {
+        $modules[$name]['enabled'] = 0;
+        foreach (array_keys($files[$name]->required_by) as $required_by) {
+          $modules[$required_by] = 0;
         }
       }
     }
   }
-  // Now we have installed every module as required (either by the user or
-  // because other modules require them) so we don't need the temporary
-  // storage anymore.
-  unset($form_state['storage']);
 
-  $old_module_list = module_list();
+  // Initializes array of actions.
+  $actions = array(
+    'enable' => array(),
+    'disable' => array(),
+    'install' => array(),
+  );
 
-  // Enable the modules needing enabling.
-  if (!empty($modules_to_be_enabled)) {
-    $sort = array();
-    foreach ($modules_to_be_enabled as $module) {
-      $sort[$module] = $files[$module]->sort;
-    }
-    array_multisort($sort, $modules_to_be_enabled);
-    module_enable($modules_to_be_enabled);
-  }
-  // Disable the modules that need disabling.
-  if (!empty($disable_modules)) {
-    $sort = array();
-    foreach ($disable_modules as $module) {
-      $sort[$module] = $files[$module]->sort;
-    }
-    array_multisort($sort, $disable_modules);
-    module_disable($disable_modules);
-  }
-
-  // Install new modules.
-  if (!empty($new_modules)) {
-    $sort = array();
-    foreach ($new_modules as $key => $module) {
-      if (!drupal_check_module($module)) {
-        unset($new_modules[$key]);
+  // Builds arrays of modules that need to be enabled, disabled, and installed.
+  foreach ($modules as $name => $module) {
+    if ($module['enabled']) {
+      if (drupal_get_installed_schema_version($name) == SCHEMA_UNINSTALLED) {
+        $actions['install'][] = $name;
+      }
+      elseif (!module_exists($name)) {
+        $actions['enable'][] = $name;
       }
-      $sort[$module] = $files[$module]->sort;
     }
-    array_multisort($sort, $new_modules);
-    drupal_install_modules($new_modules);
+    elseif (module_exists($name)) {
+      $actions['disable'][] = $name;
+    }
   }
 
-  $current_module_list = module_list(TRUE);
-  if ($old_module_list != $current_module_list) {
+  // Gets list of modules prior to install process, unsets $form_state['storage']
+  // so we don't get redirected back to the confirmation form.
+  $pre_install_list = module_list();
+  unset($form_state['storage']);
+
+  // Installs, enables, and disables modules.
+  drupal_install_modules($actions['install']);
+  module_enable($actions['enable']);
+  module_disable($actions['disable']);
+
+  // Gets module list after install process, displays message if there are changes.
+  $post_install_list = module_list(TRUE);
+  if ($pre_install_list != $post_install_list) {
     drupal_set_message(t('The configuration options have been saved.'));
   }
 
@@ -1046,7 +1027,7 @@ function system_modules_submit($form, &$
   // Notify locale module about module changes, so translations can be
   // imported. This might start a batch, and only return to the redirect
   // path after that.
-  module_invoke('locale', 'system_update', $new_modules);
+  module_invoke('locale', 'system_update', $actions['install']);
 
   // Synchronize to catch any actions that were added or removed.
   actions_synchronize();
Index: modules/system/system.test
===================================================================
RCS file: /cvs/drupal/drupal/modules/system/system.test,v
retrieving revision 1.90
diff -u -p -r1.90 system.test
--- modules/system/system.test	23 Oct 2009 00:45:51 -0000	1.90
+++ modules/system/system.test	30 Oct 2009 01:32:49 -0000
@@ -159,6 +159,35 @@ class EnableDisableTestCase extends Modu
 }
 
 /**
+ * Tests failure of hook_requirements('install').
+ */
+class HookRequirementsTestCase extends ModuleTestCase {
+  public static function getInfo() {
+    return array(
+      'name' => 'Requirements hook failure',
+      'description' => "Attempts enabling a module that fails hook_requirements('install').",
+      'group' => 'Module',
+    );
+  }
+
+  /**
+   * Assert that a module cannot be installed if it fails hook_requirements().
+   */
+  function testHookRequirementsFailure() {
+    $this->assertModules(array('requirements1_test'), FALSE);
+
+    // Attempt to install the requirements1_test module.
+    $edit = array();
+    $edit['modules[Core][requirements1_test][enable]'] = 'requirements1_test';
+    $this->drupalPost('admin/config/modules', $edit, t('Save configuration'));
+
+    // Makes sure the module was NOT installed.
+    $this->assertText(t('Requirements 1 Test failed requirements'), t('Modules status has been updated.'));
+    $this->assertModules(array('requirements1_test'), FALSE);
+  }
+}
+
+/**
  * Test module dependency functionality.
  */
 class ModuleDependencyTestCase extends ModuleTestCase {
@@ -195,6 +224,25 @@ class ModuleDependencyTestCase extends M
     $this->assertTableCount('languages', TRUE);
     $this->assertTableCount('locale', TRUE);
   }
+
+  /**
+   * Tests enabling a module that depends on a module which fails hook_requirements().
+   */
+  function testEnableRequirementsFailureDependency() {
+    $this->assertModules(array('requirements1_test'), FALSE);
+    $this->assertModules(array('requirements2_test'), FALSE);
+
+    // Attempt to install both modules at the same time.
+    $edit = array();
+    $edit['modules[Core][requirements1_test][enable]'] = 'requirements1_test';
+    $edit['modules[Core][requirements2_test][enable]'] = 'requirements2_test';
+    $this->drupalPost('admin/config/modules', $edit, t('Save configuration'));
+
+    // Makes sure the modules were NOT installed.
+    $this->assertText(t('Requirements 1 Test failed requirements'), t('Modules status has been updated.'));
+    $this->assertModules(array('requirements1_test'), FALSE);
+    $this->assertModules(array('requirements2_test'), FALSE);
+  }
 }
 
 /**