#211182 follow-up by clemens.tolboom, David_Rothstein, tstoeckler: Re-work...

#211182 follow-up by clemens.tolboom, David_Rothstein, tstoeckler: Re-work update depencency check to deal with array_merge_recursive() edge case (with tests).

includes/update.inc

@@ -1100,16 +1100,10 @@ function update_build_dependency_graph($update_functions) {
   }
 
   // Now add any explicit update dependencies declared by modules.
-  $update_dependencies = update_invoke_all('update_dependencies');
+  $update_dependencies = update_retrieve_dependencies();
   foreach ($graph as $function => $data) {
     if (!empty($update_dependencies[$data['module']][$data['number']])) {
       foreach ($update_dependencies[$data['module']][$data['number']] as $module => $number) {
-        // If we have an explicit dependency on more than one update from a
-        // particular module, choose the highest one, since that contains the
-        // actual direct dependency.
-        if (is_array($number)) {
-          $number = max($number);
-        }
         $dependency = $module . '_update_' . $number;
         $graph[$dependency]['edges'][$function] = TRUE;
         $graph[$dependency]['module'] = $module;
@@ -1158,39 +1152,66 @@ function update_already_performed($module, $number) {
 }
 
 /**
- * Invoke an update system hook in all installed modules.
- *
- * This function is similar to module_invoke_all(), except it does not require
- * that a module be enabled to invoke its hook, only that it be installed. This
- * allows the update system to properly perform updates even on modules that
- * are currently disabled.
+ * Invoke hook_update_dependencies() in all installed modules.
  *
- * @param $hook
- *   The name of the hook to invoke.
- * @param ...
- *   Arguments to pass to the hook.
+ * This function is similar to module_invoke_all(), with the main difference
+ * that it does not require that a module be enabled to invoke its hook, only
+ * that it be installed. This allows the update system to properly perform
+ * updates even on modules that are currently disabled.
  *
  * @return
- *   An array of return values of the hook implementations. If modules return
- *   arrays from their implementations, those are merged into one array.
+ *   An array of return values obtained by merging the results of the
+ *   hook_update_dependencies() implementations in all installed modules.
  *
  * @see module_invoke_all()
+ * @see hook_update_dependencies()
  */
-function update_invoke_all() {
-  $args = func_get_args();
-  $hook = $args[0];
-  unset($args[0]);
+function update_retrieve_dependencies() {
   $return = array();
-  $modules = db_query("SELECT name FROM {system} WHERE type = 'module' AND schema_version != :schema", array(':schema' => SCHEMA_UNINSTALLED))->fetchCol();
+  // Get a list of installed modules, arranged so that we invoke their hooks in
+  // the same order that module_invoke_all() does.
+  $modules = db_query("SELECT name FROM {system} WHERE type = 'module' AND schema_version != :schema ORDER BY weight ASC, name ASC", array(':schema' => SCHEMA_UNINSTALLED))->fetchCol();
   foreach ($modules as $module) {
-    $function = $module . '_' . $hook;
+    $function = $module . '_update_dependencies';
     if (function_exists($function)) {
-      $result = call_user_func_array($function, $args);
+      $result = $function();
+      // Each implementation of hook_update_dependencies() returns a
+      // multidimensional, associative array containing some keys that
+      // represent module names (which are strings) and other keys that
+      // represent update function numbers (which are integers). We cannot use
+      // array_merge_recursive() to properly merge these results, since it
+      // treats strings and integers differently. Therefore, we have to
+      // explicitly loop through the expected array structure here and perform
+      // the merge manually.
       if (isset($result) && is_array($result)) {
-        $return = array_merge_recursive($return, $result);
-      }
-      elseif (isset($result)) {
-        $return[] = $result;
+        foreach ($result as $module => $module_data) {
+          foreach ($module_data as $update => $update_data) {
+            foreach ($update_data as $module_dependency => $update_dependency) {
+              // If there are redundant dependencies declared for the same
+              // update function (so that it is declared to depend on more than
+              // one update from a particular module), record the dependency on
+              // the highest numbered update here, since that automatically
+              // implies the previous ones. For example, if one module's
+              // implementation of hook_update_dependencies() required this
+              // ordering:
+              //
+              // system_update_7001 ---> user_update_7000
+              //
+              // but another module's implementation of the hook required this
+              // one:
+              //
+              // system_update_7002 ---> user_update_7000
+              //
+              // we record the second one, since system_update_7001() is always
+              // guaranteed to run before system_update_7002() anyway (within
+              // an individual module, updates are always run in numerical
+              // order).
+              if (!isset($return[$module][$update][$module_dependency]) || $update_dependency > $return[$module][$update][$module_dependency]) {
+                $return[$module][$update][$module_dependency] = $update_dependency;
+              }
+            }
+          }
+        }
       }
     }
   }

modules/simpletest/tests/update.test

@@ -86,3 +86,31 @@ class UpdateDependencyMissingTestCase extends DrupalWebTestCase {
   }
 }
 
+/**
+ * Tests for the invocation of hook_update_dependencies().
+ */
+class UpdateDependencyHookInvocationTestCase extends DrupalWebTestCase {
+  public static function getInfo() {
+    return array(
+      'name' => 'Update dependency hook invocation',
+      'description' => 'Test that the hook invocation for determining update dependencies works correctly.',
+      'group' => 'Update API',
+    );
+  }
+
+  function setUp() {
+    parent::setUp('update_test_1', 'update_test_2');
+    require_once DRUPAL_ROOT . '/includes/update.inc';
+  }
+
+  /**
+   * Test the structure of the array returned by hook_update_dependencies().
+   */
+  function testHookUpdateDependencies() {
+    $update_dependencies = update_retrieve_dependencies();
+    $this->assertTrue($update_dependencies['system'][7000]['update_test_1'] == 7000, t('An update function that has a dependency on two separate modules has the first dependency recorded correctly.'));
+    $this->assertTrue($update_dependencies['system'][7000]['update_test_2'] == 7001, t('An update function that has a dependency on two separate modules has the second dependency recorded correctly.'));
+    $this->assertTrue($update_dependencies['system'][7001]['update_test_1'] == 7002, t('An update function that depends on more than one update from the same module only has the dependency on the higher-numbered update function recorded.'));
+  }
+}
+

modules/simpletest/tests/update_test_1.install

@@ -6,6 +6,35 @@
  * Install, update and uninstall functions for the update_test_1 module.
  */
 
+/**
+ * Implements hook_update_dependencies().
+ *
+ * @see update_test_2_update_dependencies()
+ */
+function update_test_1_update_dependencies() {
+  // These dependencies are used in combination with those declared in
+  // update_test_2_update_dependencies() for the sole purpose of testing that
+  // the results of hook_update_dependencies() are collected correctly and have
+  // the correct array structure. Therefore, we use updates from System module
+  // (which have already run), so that they will not get in the way of other
+  // tests.
+  $dependencies['system'][7000] = array(
+    // Compare to update_test_2_update_dependencies(), where the same System
+    // module update function is forced to depend on an update function from a
+    // different module. This allows us to test that both dependencies are
+    // correctly recorded.
+    'update_test_1' => 7000,
+  );
+  $dependencies['system'][7001] = array(
+    // Compare to update_test_2_update_dependencies(), where the same System
+    // module update function is forced to depend on a different update
+    // function within the same module. This allows us to test that only the
+    // dependency on the higher-numbered update function is recorded.
+    'update_test_1' => 7002,
+  );
+  return $dependencies;
+}
+
 /**
  * Dummy update_test_1 update 7000.
  */

modules/simpletest/tests/update_test_2.install

@@ -8,11 +8,30 @@
 
 /**
  * Implements hook_update_dependencies().
+ *
+ * @see update_test_1_update_dependencies()
+ * @see update_test_3_update_dependencies()
  */
 function update_test_2_update_dependencies() {
+  // Combined with update_test_3_update_dependencies(), we are declaring here
+  // that these two modules run updates in the following order:
+  // 1. update_test_2_update_7000()
+  // 2. update_test_3_update_7000()
+  // 3. update_test_2_update_7001()
+  // 4. update_test_2_update_7002()
   $dependencies['update_test_2'][7001] = array(
     'update_test_3' => 7000,
   );
+
+  // These are coordinated with the corresponding dependencies declared in
+  // update_test_1_update_dependencies().
+  $dependencies['system'][7000] = array(
+    'update_test_2' => 7001,
+  );
+  $dependencies['system'][7001] = array(
+    'update_test_1' => 7001,
+  );
+
   return $dependencies;
 }
 

modules/simpletest/tests/update_test_3.install

@@ -8,6 +8,8 @@
 
 /**
  * Implements hook_update_dependencies().
+ *
+ * @see update_test_2_update_dependencies()
  */
 function update_test_3_update_dependencies() {
   $dependencies['update_test_3'][7000] = array(