#151452 by David_Rothstein, Dave Reid, tstoeckler: Fixed uninstalling modules...

#151452 by David_Rothstein, Dave Reid, tstoeckler: Fixed uninstalling modules does not follow dependencies.

includes/install.inc

@@ -175,6 +175,9 @@ function drupal_set_installed_schema_version($module, $version) {
     ->fields(array('schema_version' => $version))
     ->condition('name', $module)
     ->execute();
+
+  // Reset the static cache of module schema versions.
+  drupal_get_installed_schema_version(NULL, TRUE);
 }
 
 /**
@@ -608,12 +611,53 @@ function drupal_install_system() {
 }
 
 /**
- * Calls the uninstall function and updates the system table for a given module.
+ * Uninstalls a given list of modules.
  *
  * @param $module_list
  *   The modules to uninstall.
+ * @param $uninstall_dependents
+ *   If TRUE, the function will check that all modules which depend on the
+ *   passed-in module list either are already uninstalled or contained in the
+ *   list, and it will ensure that the modules are uninstalled in the correct
+ *   order. This incurs a significant performance cost, so use FALSE if you
+ *   know $module_list is already complete and in the correct order.
+ *
+ * @return
+ *   FALSE if one or more dependent modules are missing from the list, TRUE
+ *   otherwise.
  */
-function drupal_uninstall_modules($module_list = array()) {
+function drupal_uninstall_modules($module_list = array(), $uninstall_dependents = TRUE) {
+  if ($uninstall_dependents) {
+    // Get all module data so we can find dependents and sort.
+    $module_data = system_rebuild_module_data();
+    // Create an associative array with weights as values.
+    $module_list = array_flip(array_values($module_list));
+
+    $profile = drupal_get_profile();
+    while (list($module) = each($module_list)) {
+      if (!isset($module_data[$module]) || drupal_get_installed_schema_version($module) == SCHEMA_UNINSTALLED) {
+        // This module doesn't exist or is already uninstalled, skip it.
+        unset($module_list[$module]);
+        continue;
+      }
+      $module_list[$module] = $module_data[$module]->sort;
+
+      // If the module has any dependents which are not already uninstalled and
+      // not included in the passed-in list, abort. It is not safe to uninstall
+      // them automatically because uninstalling a module is a destructive
+      // operation.
+      foreach (array_keys($module_data[$module]->required_by) as $dependent) {
+        if (!isset($module_list[$dependent]) && drupal_get_installed_schema_version($dependent) != SCHEMA_UNINSTALLED && $dependent != $profile) {
+          return FALSE;
+        }
+      }
+    }
+
+    // Sort the module list by pre-calculated weights.
+    asort($module_list);
+    $module_list = array_keys($module_list);
+  }
+
   foreach ($module_list as $module) {
     // First, retrieve all the module's menu paths from db.
     drupal_load('module', $module);
@@ -660,6 +704,8 @@ function drupal_uninstall_modules($module_list = array()) {
     // Call hook_module_uninstall to let other modules act
     module_invoke_all('modules_uninstalled', $module_list);
   }
+
+  return TRUE;
 }
 
 /**

includes/module.inc

@@ -466,6 +466,7 @@ function module_disable($module_list, $disable_dependents = TRUE) {
     // Create an associative array with weights as values.
     $module_list = array_flip(array_values($module_list));
 
+    $profile = drupal_get_profile();
     while (list($module) = each($module_list)) {
       if (!isset($module_data[$module]) || !$module_data[$module]->status) {
         // This module doesn't exist or is already disabled, skip it.
@@ -477,7 +478,7 @@ function module_disable($module_list, $disable_dependents = TRUE) {
       // Add dependent modules to the list, with a placeholder weight.
       // The new modules will be processed as the while loop continues.
       foreach ($module_data[$module]->required_by as $dependent => $dependent_data) {
-        if (!isset($module_list[$dependent]) && !strstr($module_data[$dependent]->filename, '.profile')) {
+        if (!isset($module_list[$dependent]) && $dependent != $profile) {
           $module_list[$dependent] = 0;
         }
       }

modules/simpletest/tests/module.test

@@ -115,8 +115,14 @@ class ModuleUnitTest extends DrupalWebTestCase {
    * Test dependency resolution.
    */
   function testDependencyResolution() {
+    // Enable the test module, and make sure that other modules we are testing
+    // are not already enabled. (If they were, the tests below would not work
+    // correctly.)
     module_enable(array('module_test'), FALSE);
     $this->assertTrue(module_exists('module_test'), t('Test module is enabled.'));
+    $this->assertFalse(module_exists('forum'), t('Forum module is disabled.'));
+    $this->assertFalse(module_exists('poll'), t('Poll module is disabled.'));
+    $this->assertFalse(module_exists('php'), t('PHP module is disabled.'));
 
     // First, create a fake missing dependency. Forum depends on poll, which
     // depends on a made-up module, foo. Nothing should be installed.
@@ -125,14 +131,65 @@ class ModuleUnitTest extends DrupalWebTestCase {
     $this->assertFalse($result, t('module_enable() returns FALSE if dependencies are missing.'));
     $this->assertFalse(module_exists('forum'), t('module_enable() aborts if dependencies are missing.'));
 
-    // Now, fix the missing dependency. module_enable() should work.
+    // Now, fix the missing dependency. Forum module depends on poll, but poll
+    // depends on the PHP module. module_enable() should work.
     variable_set('dependency_test', 'dependency');
     $result = module_enable(array('forum'));
     $this->assertTrue($result, t('module_enable() returns the correct value.'));
     // Verify that the fake dependency chain was installed.
     $this->assertTrue(module_exists('poll') && module_exists('php'), t('Dependency chain was installed by module_enable().'));
-    // Finally, verify that the original module was installed.
+    // Verify that the original module was installed.
     $this->assertTrue(module_exists('forum'), t('Module installation with unlisted dependencies succeeded.'));
+    // Finally, verify that the modules were enabled in the correct order.
+    $this->assertEqual(variable_get('test_module_enable_order', array()), array('php', 'poll', 'forum'), t('Modules were enabled in the correct order by module_enable().'));
+
+    // Now, disable the PHP module. Both forum and poll should be disabled as
+    // well, in the correct order.
+    module_disable(array('php'));
+    $this->assertTrue(!module_exists('forum') && !module_exists('poll'), t('Depedency chain was disabled by module_disable().'));
+    $this->assertFalse(module_exists('php'), t('Disabling a module with unlisted dependents succeeded.'));
+    $this->assertEqual(variable_get('test_module_disable_order', array()), array('forum', 'poll', 'php'), t('Modules were disabled in the correct order by module_disable().'));
+
+    // Disable a module that is listed as a dependency by the install profile.
+    // Make sure that the profile itself is not on the list of dependent
+    // modules to be disabled.
+    $profile = drupal_get_profile();
+    $info = install_profile_info($profile);
+    $this->assertTrue(in_array('comment', $info['dependencies']), t('Comment module is listed as a dependency of the install profile.'));
+    $this->assertTrue(module_exists('comment'), t('Comment module is enabled.'));
+    module_disable(array('comment'));
+    $this->assertFalse(module_exists('comment'), t('Comment module was disabled.'));
+    $disabled_modules = variable_get('test_module_disable_order', array());
+    $this->assertTrue(in_array('comment', $disabled_modules), t('Comment module is in the list of disabled modules.'));
+    $this->assertFalse(in_array($profile, $disabled_modules), t('The installation profile is not in the list of disabled modules.'));
+
+    // Try to uninstall the PHP module by itself. This should be rejected,
+    // since the modules which it depends on need to be uninstalled first, and
+    // that is too destructive to perform automatically.
+    $result = drupal_uninstall_modules(array('php'));
+    $this->assertFalse($result, t('Calling drupal_uninstall_modules() on a module whose dependents are not uninstalled fails.'));
+    foreach (array('forum', 'poll', 'php') as $module) {
+      $this->assertNotEqual(drupal_get_installed_schema_version($module), SCHEMA_UNINSTALLED, t('The @module module was not uninstalled.', array('@module' => $module)));
+    }
+
+    // Now uninstall all three modules explicitly, but in the incorrect order,
+    // and make sure that drupal_uninstal_modules() uninstalled them in the
+    // correct sequence.
+    $result = drupal_uninstall_modules(array('poll', 'php', 'forum'));
+    $this->assertTrue($result, t('drupal_uninstall_modules() returns the correct value.'));
+    foreach (array('forum', 'poll', 'php') as $module) {
+      $this->assertEqual(drupal_get_installed_schema_version($module), SCHEMA_UNINSTALLED, t('The @module module was uninstalled.', array('@module' => $module)));
+    }
+    $this->assertEqual(variable_get('test_module_uninstall_order', array()), array('forum', 'poll', 'php'), t('Modules were uninstalled in the correct order by drupal_uninstall_modules().'));
+
+    // Uninstall the profile module from above, and make sure that the profile
+    // itself is not on the list of dependent modules to be uninstalled.
+    $result = drupal_uninstall_modules(array('comment'));
+    $this->assertTrue($result, t('drupal_uninstall_modules() returns the correct value.'));
+    $this->assertEqual(drupal_get_installed_schema_version('comment'), SCHEMA_UNINSTALLED, t('Comment module was uninstalled.'));
+    $uninstalled_modules = variable_get('test_module_uninstall_order', array());
+    $this->assertTrue(in_array('comment', $uninstalled_modules), t('Comment module is in the list of uninstalled modules.'));
+    $this->assertFalse(in_array($profile, $uninstalled_modules), t('The installation profile is not in the list of uninstalled modules.'));
   }
 }
 

modules/simpletest/tests/module_test.module

@@ -55,5 +55,25 @@ function module_test_hook_info() {
  * Implements hook_modules_enabled().
  */
 function module_test_modules_enabled($modules) {
+  // Record the ordered list of modules that were passed in to this hook so we
+  // can check that the modules were enabled in the correct sequence.
   variable_set('test_module_enable_order', $modules);
 }
+
+/**
+ * Implements hook_modules_disabled().
+ */
+function module_test_modules_disabled($modules) {
+  // Record the ordered list of modules that were passed in to this hook so we
+  // can check that the modules were disabled in the correct sequence.
+  variable_set('test_module_disable_order', $modules);
+}
+
+/**
+ * Implements hook_modules_uninstalled().
+ */
+function module_test_modules_uninstalled($modules) {
+  // Record the ordered list of modules that were passed in to this hook so we
+  // can check that the modules were uninstalled in the correct sequence.
+  variable_set('test_module_uninstall_order', $modules);
+}

modules/system/system.admin.inc

@@ -1213,30 +1213,42 @@ function system_modules_uninstall($form, $form_state = NULL) {
     return $confirm_form;
   }
 
-  // Pull all disabled modules from the system table.
-  $disabled_modules = db_query("SELECT name, filename, info FROM {system} WHERE type = 'module' AND status = 0 AND schema_version > :schema ORDER BY name", array(':schema' => SCHEMA_UNINSTALLED));
-  foreach ($disabled_modules as $module) {
-    // Grab the module info
-    $info = unserialize($module->info);
-
-    // Load the .install file, and check for an uninstall or schema hook.
-    // If the hook exists, the module can be uninstalled.
-    module_load_install($module->name);
-    if (module_hook($module->name, 'uninstall') || module_hook($module->name, 'schema')) {
-      $form['modules'][$module->name]['name'] = array('#markup' => $info['name'] ? $info['name'] : $module->name);
-      $form['modules'][$module->name]['description'] = array('#markup' => t($info['description']));
-      $options[$module->name] = '';
+  // Get a list of disabled, installed modules.
+  $all_modules = system_rebuild_module_data();
+  $disabled_modules = array();
+  foreach ($all_modules as $name => $module) {
+    if (empty($module->status) && $module->schema_version > SCHEMA_UNINSTALLED) {
+      $disabled_modules[$name] = $module;
+    }
+  }
+
+  // Only build the rest of the form if there are any modules available to
+  // uninstall.
+  if (!empty($disabled_modules)) {
+    $profile = drupal_get_profile();
+    uasort($disabled_modules, 'system_sort_modules_by_info_name');
+    $form['uninstall'] = array('#tree' => TRUE);
+    foreach ($disabled_modules as $module) {
+      $module_name = $module->info['name'] ? $module->info['name'] : $module->name;
+      $form['modules'][$module->name]['#module_name'] = $module_name;
+      $form['modules'][$module->name]['name']['#markup'] = $module_name;
+      $form['modules'][$module->name]['description']['#markup'] = t($module->info['description']);
+      $form['uninstall'][$module->name] = array(
+        '#type' => 'checkbox',
+        '#title' => t('Uninstall @module module', array('@module' => $module_name)),
+        '#title_display' => 'invisible',
+      );
+      // All modules which depend on this one must be uninstalled first, before
+      // we can allow this module to be uninstalled. (The install profile is
+      // excluded from this list.)
+      foreach (array_keys($module->required_by) as $dependent) {
+        if ($dependent != $profile && drupal_get_installed_schema_version($dependent) != SCHEMA_UNINSTALLED) {
+          $dependent_name = isset($all_modules[$dependent]->info['name']) ? $all_modules[$dependent]->info['name'] : $dependent;
+          $form['modules'][$module->name]['#required_by'][] = $dependent_name;
+          $form['uninstall'][$module->name]['#disabled'] = TRUE;
+        }
+      }
     }
-  }
-
-  // Only build the rest of the form if there are any modules available to uninstall.
-  if (!empty($options)) {
-    $form['uninstall'] = array(
-      '#type' => 'checkboxes',
-      '#title' => t('Modules'),
-      '#title_display' => 'invisible',
-      '#options' => $options,
-    );
     $form['actions'] = array('#type' => 'actions');
     $form['actions']['submit'] = array(
       '#type' => 'submit',
@@ -2586,10 +2598,20 @@ function theme_system_modules_uninstall($variables) {
   // Display table.
   $rows = array();
   foreach (element_children($form['modules']) as $module) {
+    if (!empty($form['modules'][$module]['#required_by'])) {
+      $disabled_message = format_plural(count($form['modules'][$module]['#required_by']),
+        'To uninstall @module, the following module must be uninstalled first: @required_modules',
+        'To uninstall @module, the following modules must be uninstalled first: @required_modules',
+        array('@module' => $form['modules'][$module]['#module_name'], '@required_modules' => implode(', ', $form['modules'][$module]['#required_by'])));
+      $disabled_message = '<div class="admin-requirements">' . $disabled_message . '</div>';
+    }
+    else {
+      $disabled_message = '';
+    }
     $rows[] = array(
       array('data' => drupal_render($form['uninstall'][$module]), 'align' => 'center'),
       '<strong><label for="' . $form['uninstall'][$module]['#id'] . '">' . drupal_render($form['modules'][$module]['name']) . '</label></strong>',
-      array('data' => drupal_render($form['modules'][$module]['description']), 'class' => array('description')),
+      array('data' => drupal_render($form['modules'][$module]['description']) . $disabled_message, 'class' => array('description')),
     );
   }
 

modules/system/system.module

@@ -113,7 +113,7 @@ function system_help($path, $arg) {
       }
       return $output;
     case 'admin/modules/uninstall':
-      return '<p>' . t('The uninstall process removes all data related to a module. To uninstall a module, you must first disable it on the main <a href="@modules">Modules page</a>. Not all modules support this feature.', array('@modules' => url('admin/modules'))) . '</p>';
+      return '<p>' . t('The uninstall process removes all data related to a module. To uninstall a module, you must first disable it on the main <a href="@modules">Modules page</a>.', array('@modules' => url('admin/modules'))) . '</p>';
     case 'admin/structure/block/manage':
       if ($arg[4] == 'system' && $arg[5] == 'powered-by') {
         return '<p>' . t('The <em>Powered by Drupal</em> block is an optional link to the home page of the Drupal project. While there is absolutely no requirement that sites feature this link, it may be used to show support for Drupal.') . '</p>';

modules/system/system.test

@@ -350,6 +350,40 @@ class ModuleDependencyTestCase extends ModuleTestCase {
     // Check the actual order which is saved by module_test_modules_enabled().
     $this->assertIdentical(variable_get('test_module_enable_order', FALSE), $expected_order, t('Modules enabled in the correct order.'));
   }
+
+  /**
+   * Tests attempting to uninstall a module that has installed dependents.
+   */
+  function testUninstallDependents() {
+    // Enable the forum module.
+    $edit = array('modules[Core][forum][enable]' => 'forum');
+    $this->drupalPost('admin/modules', $edit, t('Save configuration'));
+    $this->assertModules(array('forum'), TRUE);
+
+    // Disable forum and taxonomy. Both should now be installed but disabled.
+    $edit = array('modules[Core][forum][enable]' => FALSE);
+    $this->drupalPost('admin/modules', $edit, t('Save configuration'));
+    $this->assertModules(array('forum'), FALSE);
+    $edit = array('modules[Core][taxonomy][enable]' => FALSE);
+    $this->drupalPost('admin/modules', $edit, t('Save configuration'));
+    $this->assertModules(array('taxonomy'), FALSE);
+
+    // Check that the taxonomy module cannot be uninstalled.
+    $this->drupalGet('admin/modules/uninstall');
+    $checkbox = $this->xpath('//input[@type="checkbox" and @disabled="disabled" and @name="uninstall[taxonomy]"]');
+    $this->assert(count($checkbox) == 1, t('Checkbox for uninstalling the taxonomy module is disabled.'));
+
+    // Uninstall the forum module, and check that taxonomy now can also be
+    // uninstalled.
+    $edit = array('uninstall[forum]' => 'forum');
+    $this->drupalPost('admin/modules/uninstall', $edit, t('Uninstall'));
+    $this->drupalPost(NULL, NULL, t('Uninstall'));
+    $this->assertText(t('The selected modules have been uninstalled.'), t('Modules status has been updated.'));
+    $edit = array('uninstall[taxonomy]' => 'taxonomy');
+    $this->drupalPost('admin/modules/uninstall', $edit, t('Uninstall'));
+    $this->drupalPost(NULL, NULL, t('Uninstall'));
+    $this->assertText(t('The selected modules have been uninstalled.'), t('Modules status has been updated.'));
+  }
 }
 
 /**