drupal_alter does not always obey module_implements

subscribing.

#1179422: hook_form_form_id_alter does not honor changes made by hook_module_implements_alter was duplicate.

Here is an attempt to solve this via a weighted sort function (not a patch, but a test case):

http://paste.pocoo.org/show/401484/

The basic idea is to cascade the module_implements by adding it to a map like:

Array
(
    [0] => Array
        (
            [m1] => 1
            [m2] => 2
        )

    [1] => Array
        (
            [m1] => 1
            [m3] => 2
            [m2] => 3
            [m4] => 4
        )

    [2] => Array
        (
            [m1] => 1
            [m4] => 2
            [m2] => 3
        )
)

As you can see at index 0, comparing * with m3 or m4 results in NaN (as one of them is undefined), such it goes to the next array to compare the modules.

As you can further see the sort order is conflicting, but the first order is used.

It may be possible to still combine a test case so that the sort function ends up trying to compare against NaN, but I've not yet gotten it so.

Edit: Okay, I found a test case now, which is a little problematic:

My code gives an interesting result with:

module_implements('foo_bar_alter', array('m1', 'm3', 'm2'));
module_implements('foo_xyz_alter', array('m1', 'm2', 'm4'));
test();

This gives the map of:

Array
(
    [0] => Array
        (
            [m1] => 1
            [m2] => 2
        )

    [1] => Array
        (
            [m1] => 1
            [m3] => 2
            [m2] => 3
        )

    [2] => Array
        (
            [m1] => 1
            [m2] => 2
            [m4] => 3
            [m3] => 4
        )
)

The resulting order is: m1,m3,m2,m4

As there is a conflict the absolute order is not well defined:

But on second look the partial sort order m1,m3,m2,m4 is correct, because

m1 < m2 -- OK
m1 < m3 -- OK
m3 < m2 -- OK
m1 < m4 -- OK
m2 < m4 -- OK
m3 < m4 -- FAILED, but this is due to the conflict of m3 < m2 and m3 > m2.

So the problem can be really reduced to:

Combine the above mappings into one global sorting order and use that as $module_weight keys.

I think however that the approach of cascaded search should preserve the search order even in case of conflicts.

Best Wishes,

Fabian

Nice work!

Why does the Simple Test fail?

Best Wishes,

Fabian

The approach in #14 is faulty, because it is assuming the weights are fixed.

Example:

Sorts: 4,3,1,5 and 4,2,0

Sort Order: 0,4,2,3,1,5

With 2 and 0 that need to be changed either the result is 0,4,2,3,1,5 or 2,4,0,3,1,5.

which is obsivously wrong.

New approach (in pseudo code):

$modules_map = array(
  array(4 => 0,3 => 1,6 => 2,1 => 3,5 => 4),
  array(4 => 0,2 => 1,6 => 2, 0 => 3)
);

$all_modules = array( 0, 1, 2, 3, 4, 5, 6);

This is our setup as sort orders. (the current code can get those)

Modules should of course be named and not numeric, so 0,1,2,3,4,5,6 are module names here, so there is that akey hack below ...

Now we sort it:

$ma = array();

foreach ($all_modules as $akey => $val) {
  $key = 'a' . $akey;

  for ($i = 0; $i < count($modules_map); $i++) {
    $ma[$key][$i] = $modules_map[$i][$akey];
  }
}

$last = 0;

foreach ($modules_map[1] as $akey => $val) {
  $key = 'a' . $akey;
  if (!empty($ma[$key][0])) {
    $last = $ma[$key][0];
  }
  else {
    $ma[$key][0] = $last + 1;
  }
}

array_multisort($ma);

print_r($ma);
exit(1);

And thats it. Works of course only for one alter hook at a time, but the previous patch also had that limitation.

Best Wishes,

Fabian

<?php

/*      $modules_map = array(array_flip($modules));

      foreach ($extra_types as $extra_type) {
        $extra_hook = $extra_type . '_alter';
        $extra_modules = module_implements($extra_hook);
        if ($extra_modules) {
          $modules_map[] = array_flip($extra_modules);
        }
      }*/
  
      $modules_map = array(array_flip(array('a4', 'a3', 'a6', 'a1', 'a5')));
      $modules_map[] = array_flip(array('a4', 'a2', 'a6', 'a0'));
      $modules_map[] = array_flip(array('a4', 'a8'));
      $modules_map[] = array_flip(array('a6', 'a7'));

      $all_modules = array( 'a-1', 'a0', 'a1', 'a2', 'a3', 'a4', 'a5', 'a6', 'a7', 'a8');

      $n = count($modules_map);

// The idea is setup a sorted table like so:
//
// name | module_list_weight | module_implement_weight_1 | module_implement_weight_2 | etc.
// a4   | 1                  | 1                         | 1
// a2   | NULL               | 2                         | NULL
// a8   | NULL               | NULL                      | 2
// 

      foreach ($all_modules as $module) {
        $count = 0;
        for ($i = 0; $i < $n; $i++) {
          if (isset($modules_map[$i][$module])) {
            $ma[$module][$i] = $modules_map[$i][$module] + 1;
          }
          else {
            $ma[$module][$i] = '';
            $count++;
          }
        }
        // No hook is mplemented by this module
        if ($count == $n) {
          unset($ma[$module]);
        }
      }

// And now we have the problem that a2 would be sorted first (as NULL is < 1 ), but it should be after a4
// because of the second column. And such the column is traversed and the weight is set to the the last value
// found + 1 (because there is a strict order).
//
// In this case this would be:
//
// Before a2 comes a4 and the weight is in the first column 1, so set it to 2.
//
// name | module_list_weight | module_implement_weight_1 | module_implement_weight_2 | etc.
// a4   | 1                  | 1                         | NULL
// a2   | 2*                 | 2                         | NULL
//
// And the same is true for multi-column a8 in third dimension of module_map:
//
// From the third column the last known is a4 again, so the weight of second column is 2.
// And then again, the last known column is a4 again in first column, so it is also 2.
//
// That way all sorting rules are satisfied.
//
// name | module_list_weight | module_implement_weight_1 | module_implement_weight_2 | etc.
// a4   | 1                  | 1                         | 1
// a2   | 2*                 | 2                         | NULL
// a8   | 2*                 | 2*                        | 2
//  

      for ($i = 1; $i < $n; $i++) {
        $last[$i] = 0;
        foreach ($modules_map[$i] as $module => $val) {
          $j = $i;
          for ($j = $i; $j > 0; $j--) {
            if (empty($ma[$module][$j - 1])) {
              $ma[$module][$j - 1] = $last[$j] + 1;
            }
            else {
              $last[$j] = $ma[$module][$j - 1];
            }
          }
        }
      }

array_multisort($ma);

print_r($ma);
exit(1);

Here is the updated code for n dimensions together with an explanation.

Best Wishes,

Fabian

Assigning to me, so I finally get back to work on this.

#1222190: hook_module_implements_alter does not work in some cases when hook is called using drupal_alter was duplicate.

Bumping this to major, it is causing a lot of duplicates apparently, here's another one #1224014: form_FORM_ID_alter problem with hook_module_implements_alter.

Tagging issues not yet using summary template.

Hello Franz, is there any progress with this issue? Also for 7.x ?

Putting on my queue for this week.

Just pointing out that since #765860: drupal_alter() fails to order modules correctly in some cases was committed to both D7 and D8, this issue may wind up being unnecessary. (But I'm not familiar enough with the code in this issue to know if that's actually the case, plus the other issue is still being heavily discussed and potentially rolled back.)

That patch has been in for a while, and I'm pretty sure it fixes the issue here. Closing as duplicate.