Order of alter hooks is not always respected by ModuleHandler

Subscribing. Would be great to upload a test-only patch.

Extracted a test only patch from #9

Possible duplicate of #2917192: hook_form_FORM_ID_alter , hook_form_alter : unexpected execution order of form alters hooks? I totally get that you might want to reorder form alters individually and I could be reading it wrong, but is this introducing a BC-breaking change? If a module uses hook_module_implements_alter() to reorder its form_alter hook, and the module implements both a hook_form_alter() and a hook_form_FORM_ID_alter(), then with HEAD that would reorder both alter hooks, whereas after the patch it would only reorder the form_alter? I guess the need to update ThemeSuggestionsAlterTest might illustrate that we're changing existing behaviour.

Thanks @mxr576! I could be misunderstanding the patch (or the current behaviour!) but

I guess this could be only a problem for form alters if someone [snip] implemented both hook_form_alter() and hook_form_FORM_ID_alter() alter hooks in a module and then changed the weights of both form_alter and FORM_ID_ALTER hooks

Doesn't it also impact the scenario in 14?

If a module uses hook_module_implements_alter() to reorder its form_alter hook, and the module implements both a hook_form_alter() and a hook_form_FORM_ID_alter(), then with HEAD that would reorder both alter hooks, whereas after the patch it would only reorder the form_alter?

ie as I understand it, currently the correct way to set the order of hook_form_MYFORM_alter() (or any other "extra" hook) is by setting the order of the base hook, ie hook_form_alter(). But after this patch would that still work without modifications? Thanks!

Thanks for bearing with me (: I've tried testing locally and I do seem to be seeing the behaviour I described. With test1.module and test2.module both altering the node form with all three types of form alter, if test1.module has the following:

function test1_module_implements_alter(&$implementations, $hook) {
  if ($hook === 'form_alter') {
    $group = $implementations['test1'];
    unset($implementations['test1']);
    $implementations['test1'] = $group;
  }
}

Then without the patch I get:

Executed test2_form_alter()
Executed test2_form_node_form_alter()
Executed test2_form_node_article_form_alter()
Executed test1_form_alter()
Executed test1_form_node_form_alter()
Executed test1_form_node_article_form_alter()

Note that current behaviour is that we can make test1_form_node_form_alter() and test1_form_node_article_form_alter() run after the test2 form alters just by altering the weight on the base hook test1_form_alter().

And with the patch I get:

Executed test2_form_alter()
Executed test1_form_alter()
Executed test1_form_node_form_alter()
Executed test2_form_node_form_alter()
Executed test1_form_node_article_form_alter()
Executed test2_form_node_article_form_alter()

In this case test1_form_node_form_alter() and test1_form_node_article_form_alter() are run before the test2 equivalents - I think this makes it a BC-breaking change?

Separately from that, I think the patch also introduces the desired order described in #2917192-11: hook_form_FORM_ID_alter , hook_form_alter : unexpected execution order of form alters hooks - I'm not sure if that's considered breaking BC or not, but certainly worth highlighting it imho.

Thanks!

This is currently one of the random major bug issue targets for the Bug Smash Initiative.
Hope to have a chance to dive into this sometime soon and see if I can help move it along...

Thanks,
-Derek

Patch needs a re-roll.

This version works on 9.4.5.

Wow, it seems like I've been bitten by this one while getting back field group support in DS in a new branch where I'm refactoring (see https://www.drupal.org/project/ds/issues/3308854#comment-14716099)

When manually testing, moving 'ds' to the end (to end up after field_group), it all works out fine.
However, in the test (FieldGroupTest), the order of the 'entity_view_alter' hook remains ds first, then field_group.

Even when I mimic standard install (with history. comment + layout_builder and no test modules) in the test, ds still remains before field_group.

The only 'clean' workaround I can think off for the test is to swap out EntityViewBuilder and only call alter on entity_view and not ['node_view', 'entity_view']. However, swapping that class in DS is a no go as that would break so many sites out there. I currently solved it by calling field_group_entity_view_alter in DS so the field groups are attached. It's wrapped in a Settings::get call which returns FALSE by default, which the test overrides.

I'm very tempted to mark this critical from a DX perspective, but I'll leave it for now because I'm not even 100% sure it this is the bug I'm facing.

Thanks @swentel, I think that's an important example that's actually more involved than the form altering case. With forms, the more general hook (form_alter) is passed first in the list of hooks to the call to alter, which means

• the general form alters are always called after the more specific;
• you can use hook_module_implements_alter() to ensure your general form_alter is called later/after another.

Between those two, it's always possible to have final say on any form. The problems with form alters (IIUC) are

• You can't use hook_module_implements_alter() to change the weight of an "extra" hook (or "subtype", not sure of the terminology) such as hook_form_FORM_ID_alter() directly, you have to have implemented hook_form_alter() and change the weight of that.
• You can't have a hook_form_alter() and a hook_form_FORM_ID_alter() run with different weights.

However, entity_view_alter is dispatched with the hooks in the reverse order: the more specific comes first, and the more generic comes second. This means node_view_alter will always come after entity_view_alter and furthermore, the only way to affect the order it's run in is to use hook_module_implements_alter() on node_view_alter which IIUC only works if you've actually implemented a hook_node_view_alter, so it's completely unworkable for an entity-agnostic solution.

I'm not sure the patch as it stands will help though. It will let you control when both your entity_view_alter and node_view_alter hooks will run relative to others, but the overall order of node_view_alter being called after entity_view_alter will remain the same. Looking at other examples of multiple hooks being passed to an alter, it looks like the majority put the more general hook first:

• ['block_build', "block_build_" . $plugin->getBaseId()]
• ['block_view', "block_view_$base_id"]
• ['query', 'query_' . $tag]
• ["plugin_filter_{$type}", "plugin_filter_{$type}__{$consumer}"]
• ['form', "form_$base_form_id", "form_$form_id"]
• ['theme_suggestions', 'theme_suggestions_' . $base_theme_hook]
• ['field_widget_complete_form', 'field_widget_complete_' . $this->getPluginId() . '_form']
• ['field_widget_single_element_form', 'field_widget_single_element_' . $this->getPluginId() . '_form']

and the others:

• ["language_fallback_candidates_$operation", 'language_fallback_candidates']
• [$entityType . '_build_defaults', 'entity_build_defaults']
• ["{$entity_type_id}_view", 'entity_view']

I wonder if those last three should have the order switched? (Tho again sadly a BC break IIUC.)

Let's add related tests here, #3369958: Improve unit test coverage for ModuleHandler

I believe changing this as proposed would make some happy and others unhappy.
The impact will become more clear with the new order tests added in #3369958: Improve unit test coverage for ModuleHandler.
I suggest to keep the functionality as it is today, and wait for #3368812: Hux-style hooks, proof of concept which will allow to specify weights for individual implementations.

----

For people who still need this, there _is_ a workaround!
You can invent a fake module, and use hook_module_implements_alter() to insert an implementation on behalf of that fake module.

E.g. Display Suite (ds) can invent a fake module '_ds_late', for hooks that should run late.

Then do this:

function ds_module_implements_alter(array &$implementations, string $hook): void {
  // Always target the the base hook, otherwise implementations will be removed.
  if ($hook === 'entity_view_alter') {
    $implementations['_ds_late'] = FALSE;
  }
}

// This will run last on ->alter(['entity_view', 'node_view']).
function _ds_late_node_view_alter(...) {...}

I am not sure if we will or should support this workaround in #3366083: [META] Hooks via attributes on service methods (hux style), though.

Sorry, links in my previous posts were wrong.
Correct link is #3369958: Improve unit test coverage for ModuleHandler
(I already corrected, but this is for people who only read the emails)

Is this resolved in #3442009: OOP hooks using attributes and event dispatcher ?

I'm pretty sure this is now resolved now that #3485896: Hook ordering across OOP, procedural and with extra types i.e replace hook_module_implements_alter is in.

Maybe we just add a test here.

Needs a rebase, but I have a clean test that shows this is still an issue.

Scratch that this was fixed in 11.2 but it will be good to have explicit test coverage.

I'll clean up the issue summary etc later.

Took a pass at credit and updated the issue summary.

Appears to have an open thread

Replied thanks!

Actually I want to make it less likely that a random test hook will collide with this and some more comments

Ok I made the hook more rare and added some comments, this is ready for review again.

Not quite sure why that new MR is here.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

This needs a rebase on main.

Also I'm not sure what to do with the extra mr since it is failing.