Hook ordering across OOP, procedural and with extra types i.e replace hook_module_implements_alter

I think that introducing (or re-introducing?) 5 different attributes here to control hook order makes the scope of the issue quite large, at least the testing aspect of it.

But before that, I think it'd be helpful to define what the expectation is of how ordering should (or will?) work. I understand the goal is to replace hook_module_implements_alter in a way that works with both procedural and OOP hooks, but I think a full example with the variations of hook implementations and how exactly they can be ordered in a business sense is something missing from the IS.

.

(please continue not crediting me)

I think we should consider to convert all implementations in core of this to verify the concept. There aren't that many and then we can immediately deprecate implementations hook_module_implements_alter().

That said, there are at least two common use cases for hook_module_implements_alter(). One is covered here, it's ensuring that certain hooks are called before/after others. The second is completely replacing hook implementations. This might be more common in custom code for some advanced use cases and maybe this is already possible through a service provider or so, but we need to document that if we want to deprecate the hook.

Looking through common core and contrib implementations as well as those in my project.

core:
* content translation is a trivial first/last (I assume if two modules want to be first then it would keep the order if it would have by default between those two).
* layout_builder is also a trivial last
* layout_builder_test is a before layout_builder.
* navigation has two, page_top is a trivial last, but the second part is an implementation of the second use case, it replaces layout_builder. so that use case does in fact exist in core.
* page_cache_form_test is also a variation of that, it removes system
* common_test_module() I think is just a general test for it, if we have tests for the new system, we could keep it just for BC testing?
* module_test too is more test edge cases that I think we should just use for BC testing.
* workspaces is a fun one: It moves itself first *but* also moves content_moderation "firster". How would that look with the new system?

contrib:
* address: last
* field_group: last
* google_tag: last + remove google_analytics
* metatag: last
* monitoring: remove a requirements hook for test purposes
* paragraphs: last
* webform: last (same hook as paragraphs)
* workbench_email: last (really, after content_moderation)

And one custom implementation that removes a hook.

Looking through this, I'm wondering about the group stuff. It makes sense that we tackled this by looking at the most complex implementation and made sure that we support that. But, I think such a complex thing that's based on two different hook that we have to put in a group is very, very rare. This is literally the only implementation that I found that doesn't just put itself first or last. We could either say that such a special case needs to handle this itself by altering the service container, or much simpler: We just solve it like everyone else does it and put ckeditor5 last.

I didn't review the implementation at all, but I suspect it would remove a considerable amount of complexity if we don't need to support that group thing? Thoughts?

workspaces is a fun one: It moves itself first *but* also moves content_moderation "firster". How would that look with the new system?

Workspaces needs the ability to implement the entity presave hook twice, one that runs first and one that runs last :) The current code that moves CM's implementation before is just a workaround.

> Two hook attributes on the same method, one first, one last.

Or , depending , one hook implementation which runs first and one that runs last -- we now have the ability to implement the same hook twice. Yay.

Removing the group stuff as it would make alter quite a bit simpler and others a bit simpler.

Or , depending , one hook implementation which runs first and one that runs last -- we now have the ability to implement the same hook twice. Yay.

Yup, that's what we need for Workspaces, to split the current implementation in two separate methods.

> Putting it last won't work because it's across extra types, so alter doesn't know how to move it. In fact I think this will solve some unique ordering issues for alter hooks.

I didn't look at the implementation yet. I see that it gets tricky with multiple types either way. I assumed we could just give a very low priority/high weight, but we don't really have that, just an array. And we always process the first hook first, so ckeditor5 would be first since it implements the base hook that's specified first.

One option would be to register the ckeditor5 hook twice, once as add and once as edit form id, instead of using the base form id. Then each could depend on the respective other and they'd be same hook?

Pushed a commit to fix Workspaces' presave implementation, we don't want to run the whole thing twice :)

The current code from HMIA doesn't run the implementation twice, it moves it first then moves CM's implementation before it. I'll look at the test failure tomorrow :)

I looked into it and the test failure is genuine, \Drupal\workspaces\Hook\EntityOperations::entityPresaveLast() runs before \Drupal\content_moderation\Hook\ContentModerationHooks::entityPresave().

What I found is that ModuleHandler::invokeAllWith() executes the callbacks in a foreach keyed by module name, and ModuleHandler::invokeMap['entity_presave'] looks like the screenshot attached.

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.

Some feedback.

I don't like attributes that are not related to the symbol they are attached to.
#[ReOrderHook] and #[RemoveHook] can be placed anywhere with the same effect, so perhaps there should be a different way to do this?

I've given up trying to argue that we shouldn't support the complex grouping stuff, but I still think we can at least get rid of the use case we have in core: #3505641: Simplify ckeditor5_module_implements_alter() in favor of checking for ckeditor5 in media module. It might not save much in lines of code (given that we have to keep support for such use cases) but it makes it IMHO self-contained and easier to understand.

I deleted the CR titled, "Delete this CR".

Unless a good reason emerges which I can't currently think of, I would also lean towards doing the deprecation/bc layer in this issue.

Some higher level thoughts.

Divergence from symfony event system

We are using the symfony event system, but then we are reinventing the way that these events are declared, how we specify ordering, and how we invoke them.

Explicit vs calculated numeric priorities

Atm, if a module uses explicit event subscriber priorities, I suspect it does not really work so well in combination with the dynamic/calculated priorities produced by the hook ordering system proposed here.

If we want to stick closer to the event system, then the default way to specify ordering would just be to specify explicit priorities. There are reasons why we might want to avoid this, because modules will come up with random numbers like -999, -5 or -1000000. But this also applies to events that we already have in Drupal, and where we are ok with just priorities.

We could also consider to order event subscribers without setting a calculated priority.
If we have 10 subscribers with prio = 0, we can just order that array and set it, while the priority remains 0 for each.
We could either do this later in the process, when the tagged services have already been evaluated and applied as a service parameter, or we rely on the order of tagged service definitions in the container. Not sure if this works.

Relative ordering

One thing the symfony event system does not cover is relative ordering.
I remember older discussions where it was said the relative ordering is not really needed.
We only had one such case in core (ckeditor5) and here it was not even really required. But perhaps contrib has such cases.

In the past, with no more than one implementation per module (for regular hooks), relative ordering just meant ordering of module names relative to each other.
Even with advanced multi-hook alter hooks, where you could already have multiple implementations per module, we would group them by module for ordering, to have something in the format we can pass to hook_module_implements_alter().

Now we want to order implementations relative to each other, which is more than what we would need if we only want to preserve the old/existing ordering functionality.

In my older MR, we would order implementations relative to module names, which completely eliminates the possibility of circular conflict.

Module name per implementation

Also we need to know the module name for each implementation, which symfony does not really help us with.
We have to store the impl -> module map in a separate place, outside the actual event subscribers list.

Order lost when grouping by module

Currently in ModuleHandler::getHookListeners(), we are grouping implementations by module:

            $this->invokeMap[$hook][$module][] = $callable;

This means that previous ordering by implementation is destroyed, if we have multiple implementations for one module.

A solution here could be to let ::getHookListeners() return an iterator, which would allow to repeat the same key.

yield $module_A => $module_A_early_implementations;
yield $module_B => $module_B_implementations;
yield $module_A => $module_A_late_implementations;

Of course the `$this->invokeMap` would need to have a different structure in that case. Perhaps it could contain iterator closures, or IteratorAggregate objects.

Ordering conflicts

With the system proposed here, there is always the potential for ordering conflicts.

E.g. A wants to run before B, but B also wants to run before A.
This type of conflict already existed (in theory) with hook_module_implements_alter(), and it almost never was a problem in practice, so I don't see it as a blocker.

And then we can have different implementations that want to run first, in that case the original module order decides.
Again, we already know this edge case problem from hook_module_implements_alter(). But explicit priorities or weights would avoid this fully.

Alternative

I want to recap the ordering logic of my older PR, from what I remember.
This was done without the event system, but in fact it would be very possible to integrate it with the event system, similar to what we do now, but without calculated priorities.

Basically, we have different levels of ordering, similar to semantic versions:

• Explicit numeric priority or weight. This would beat everything else.
• Altered order of modules. This would start with the default module order by module weight, and can be altered with hook_module_implements_alter(). This could either be applied for each weight/prio group, or only for priority = 0.
• Relative ordering relative to a module name. Instead of asking to run before or after a specific implementation, ask to run before or after a specific module's regular implementations, if the priority/weight is the same.

So it would look something like this:

// $impl_by_module order has been altered by hook_module_implements_alter().
foreach ($imp_by_prio as $prio => $impl_by_module) {
  foreach ($impl_by_module as $module => $module_implementations) {
    foreach ([...$before[$prio][$module], $module_implementations, ...$after[$prio][$module]] as $impl) {
      yield $impl->module => $impl->callback;
    }
  }
}

The `$module_implementations` would include procedural _and_ OOP implementations, and a derivative of it is passed to hook_module_implements_alter().
This allowed to convert a procedural implementation with no impact on the ordering.

(The MR looked more complex due to micro optimization.)

This would also completely solve "combined" alter hooks, because we can just combine implementations with same prio and module.

Thank you so much for your review and this comment, you've obviously put a lot of thought into this topic over the years and this issue specifically.

I regret the inconsistency in my level of participation and commitment.

At a high level, I don't see anything here that should block this issue, once this is in there is some opportunity for clean up / expansion, but this is already far broader than a typical issue due to the nature of HMIA. I truly think for this issue the thought should be

In the MR for this issue we are introducing the different Order classes, including ComplexOrder with the ->extraTypes, which would be obsolete if we use a different model / architecture for the ordering.
(I need to do a more detailed analysis of these)

We should avoid adding features and extension points that we later remove.
We don't need to do everything in one issue, but we should know in which direction we are moving.

The initial OOP hook issue did use explicit priorities and that blew up which is why it was pulled out, this issue was specifically borne out of a BC way to allow ordering relatively.

Can you remind me what is the "initial OOP hook issue" and how it failed? Maybe I missed it.
(I assume we are not talking about my older issue here, which mostly suffered from being overly ambitious. I don't recall that anything "blew up".)

This said, I can imagine how this could have failed, depending how it is done.

Yes, hooks are different so the divergence is expected

My point is the more we diverge and the less we reuse from symfony events, the more we have to ask what we gain from using the symfony system, or if it would have been better to do something custom.
Maybe it is too late now.

multiple implementations are new, going to be rare, and ordering them as a separate entity will be even rarer.

The MR provides a way to order specific implementations relative to each other.
If we assume all implementations of one module to have the same ordering position, then all we would need is to order relative to a module, not relative to an implementation.
To me this MR is incomplete if we don't properly support multiple implementations per module. It could even reveal shortcomings in the approach.

Another higher-level recap.

Basic concepts

(these are just observations to verify my understanding.)

The new attributes and attribute properties allow to specify ordering information for hook implementations, or to entirely remove specific implementations.
An implementation can be first, last, or before/after specific other implementations.
For relative ordering and for removal, other implementations are addressed by hook name, class name and method name.
(Consequence: When the class or method for a hook implementation get renamed, the ordering attributes that target the old name no longer work.)
Alternatively, a relative ordering can address module names, which will target all implementations by that module.

Ordering rules from attributes are applied _after_ the implementations have been reordered with hook_module_implements_alter().

Ordering rules from attributes are applied one by one, in the order in which they were discovered, but with the order on regular Hook attributes being applied first, then the order from ReOrderHook attributes.
An ordering rule applied later will overwrite / win over a conflicting order by a previous rule.

A relative ordering rule can have "extra hooks" specified, which means that this ordering rule should be applied for ->alter() calls with multiple hooks. (more on that below)

Conflict resolution

The resolution of conflicts is quite simple:
Any "later" ordering rule will overwrite any "earlier" ordering rule.

If we want to control this further in the future, we should provide ways to prioritize the ordering rules.
(This could bring us to a similar situation as explicit priorities, but we can discuss that in the future)

Overall this basic concept is fine!
It is different from what I proposed in my older MR, but it is a very valid way to order hook implementations.

Extra hooks logic

Tbh I find it hard to follow the "extra hooks" logic.
I mean I think I understand the intention, but then I have trouble to follow what exactly is happening in the code.

I also suspect that developers who want to apply order attributes may not fully understand the consequences of adding or not adding hooks in the order extra types.

At the same time I had the suspicion that it might not behave as we expect in many scenarios.
But it is hard to point out possible flaws when I might just not correctly understand it.
This is why my earlier comments were primarily focused on docs, naming and possible refactoring.

Naming of methods and variables

(these are generic comments, for specific comments see the review)

My comments about naming may appear nitpicky and pedantic.

In general there is no "perfect" naming, but there are some mistakes we can avoid.

Overall, I like it if the same thing has the same name throughout a subsystem, and more so within the same method, and different things to have different names.
E.g. when I see something like `foreach ($priorities as $weight)` or `foreach ($trees as $forest)`, I get confused.

Of course it is much easier to point out flaws than to propose better names.

Sometimes, the difficulty to find a good name can point to a design issue, where the variable or method should not really exist in that way.

Test coverage

Actually I have not looked at all into the tests.
But I suspect we need more, given that some possible problems I found in the code did not cause test failures.

If we could isolate the calculation some more, it might become possible to cover a lot of stuff with unit tests, which are much faster than

Follow-up issues?

To me, follow-up issues should be for additional functionality, or to fix things that we left unchanged.
I don't like to plan follow-ups to change things from a current MR.

We still have some time until the next release, so we _could_ change or rewrite parts of this ordering in follow-ups, but I would prefer if we can avoid it.

Alternative: Order at call time (cached)

I can imagine one alternative that (I think) would allow to drop the "extra hooks" or "extra types" in the order object added to an attribute.

We could simply apply the ordering rules from attributes at runtime:

At container build time:
- Calculate the order for regular single hook.
- Store ordering rules per hook.

At runtime for a multi-hook alter call:
- Load and merge the implementations for each hook.
- Group by module, apply module order, call HMIA with the primary hook (as was done in older versions of Drupal).
- Undo the grouping by module, so we have a list of individual implementations, by identifier.
- Apply the ordering rules for each of the hooks from the ->alter() call.
- Cache the result.

A tricky edge case here would be if the same class + method implements multiple hooks that are invoked together.
So, if something calls `->alter(['A', 'B'], ..)`, and we have `#[Hook('A_alter'), Hook('B_alter')]` on one method, possibly with different ordering information, then the targeting with class + method would become ambiguous.
To avoid this ambiguity there could be an additional parameter in the Order object, to determine whether we order relative to A_alter or B_alter.
But, I think it would be much better to just disallow that, or to order relative to both.

This alternative will lead to a situation where we order "combined" hooks at call time, but regular hooks at container build time.
We _could_ instead just do all the ordering at call time, which would probably reduce code duplication.

Note that in the current MR we already do ordering at call time for a multi-hook, if we do not find a pre-ordered combined hook.

I can also all but guarantee that anybody trying to use this will probably not need it and it will be wrong.
We can't build this feature for what might be or how people might use it, it's far, far too much of an edge case. If someone opens a bug against it I will happily chase it down and try to find a solution, but we can't make guesses in this area, if it were not for HMIA supporting something like it, we would not have built it in the first place. As it stands we do need it unfortunately.
The bar for this really should be does it work in core, can we address contrib if they need it? The truth is if there is a contrib module doing some ordering on extra types this doesn't work for they can keep their procedural implementation and we can work on solving their use case before Drupal 12.

We are designing a public API here, and anything we create will be very hard to remove later.
We can fix things before a release, but in general we should only commit what we are ready to release.

From what I remember, ckeditor5 would work just fine with "Order::Last" instead of "Order::After".
The main reason why we would try to support this case in core instead of replacing it with "Order::After" is that there are likely more such cases in contrib, and we keep this one as a canary.
(Or maybe we are concerned about BC impact of moving this hook priority? I don't think so.)

What we don't want is add a bunch of complex code and complex extension points, and then if we are worried it might not work we say nobody will use it anyway. If we only care about one specific case in core, we better to some kind of one-off hack.

This makes me wonder: Order::First and Order::Last do not allow to specify additional hook names.
How will first and last work if alter is called with multiple hooks?

To be fair my core contribution experience is more limited than yours

To be clear I don't have any authority and have been a bit out of the loop in the last years.
I mostly express what makes sense to me.
I welcome any opinions from others.

$snake_case vs $camelCase

The 11.x version of HookCollectorPass and other classes has a mix of snake and camel case in local variables and parameters, with snake case being the majority.
The MR introduces new variables that are mostly lower camel case, although in HookPriority I see new snake case variables.

I have a personal preference which has shifted over time, currently leaning towards snake case.
But this should not be relevant here.
The coding standards regarding this have been relaxed. I even saw discussions to soften the rule of not mixing within a file. But this does not mean it should be arbitrary in Drupal core.

But I also agree with @nicxvan to avoid renaming until there is broader consensus. Don't rename things just for me.

https://git.drupalcode.org/project/drupal/-/merge_requests/11344/diffs
The current version of this (commit af51ab5bb1ef81e35658ffa2dd1514d7757669c2) shows how we can calculate everything first and _then_ write to the container, in HookCollectorPass.
(It also contains some unrelated changes.)

It passes existing tests.
However, it is very possible that we just don't have enough coverage.
In that case, we should improve the tests.

To improve test coverage for different combinations, we can create a hook with a bunch of implementations with different ordering info, where each does "return __METHOD__;", or sets `$arg[] = __METHOD__` for an alter hook.
Then, instead of asserting something in $GLOBALS, we can assert the return value which gives us a list of all the methods that were called, and their order.

There are still more possible simplifications we can do.

Once HMIA is gone this edge case is gone.

We can cross that bridge when we come to it, if we ever do.
Realistically we won't need to. This only takes affect if they are form alters.

To me these are not good answers that justify to "resolve" a review comment.
We are designing a new system, and we want to support BC.
All of this has to work reliably and predictably, otherwise it should not be added.
Form alters are not an edge case. HMIA is not an edge case, it has to be 100% supported until we remove it in 12.x.
Also, the time spent on an issue does not determine the mergeability.

The next steps I see for me:
- Add tests to cover more combinations.
- Further cleanup and refactoring of the code in this issue.

Hi
Just a quick clarification.

If I read it right you can remove Hook priority entirely right? I'd have to take some more time to look at it, but it looks interesting.

Yes, we can remove HookPriority class, I just did not get to it yet.

I don't think you made any substantial changes beyond what you changed in HCP

The main change here was to change the algorithm within HookCollectorPass.
I wanted less interaction with the container, and only deal with priorities once everything is finished.
Once this is complete, we can run the main part of the sorting logic in a clean unit test without too much mocking.

I did try to keep the behavior of the existing MR.
This was successful as far as existing tests, _but_ as mentioned, it could be that additional tests will reveal functional changes.

In this version, we cannot remove the $order->extraHooks / ->extraTypes.

In a future version (which I think we can still do in this issue), we can try sorting at call time, which would allow us to get rid of $order->extraTypes.
(I say in this issue, because I much prefer not add something than to add and then remove.)

I refactored this based on a discussion with @GhostOfDrupalPast to use the container reflections that Symfony caches, this should be much faster.

What do we actually gain from this?
Worst case is that a ReflectionClass is created again, after it was already created in symfony.
I suspect the main cost of "new ReflectionClass" is just to load and parse the class file. Once the class is known to php, it will be very fast.
See also https://gist.github.com/mindplay-dk/3359812

symfony ContainerBuilder::getReflectionClass() actually does some crazy magic that we don't need, so it might actually be slower than just creating the reflection class again.
It definitely increases complexity and mysteriousness.

I would like to remove it to reduce the places that get passed a container.

It was suggested as a possible performance enhancement on #3395260: Investigate possibilities to parse attributes without reflection #16. I have not done any metrics on it.

Let's keep this as follow-up, with actual measurements.
As said, I suspect the main cost is loading the file. After that, I suspect we can call "new ReflectionClass" and "$reflector->getAttributes()" repeatedly without much extra cost.

Atm I think we should drop everything that smells like additional complexity.
In this case, dropping that symfony reflection cache allows to do most of the calculations without a container.

The third MR extracts a new class HookCollector from HookCollectorPass which now does most of the logic and is mostly independent from the container.
https://git.drupalcode.org/project/drupal/-/merge_requests/11415/diffs

If you want a cleaner diff, look at v2 instead:
https://git.drupalcode.org/project/drupal/-/merge_requests/11344/diffs

I would still like to do more changes:
- I think we should use $snake_case for all new local variables and non-promoted parameters. But for now I want to reduce disruption. We can do this once we agree.
- I would like to rename more variables, but again want to reduce disruption.
- More tests.
- Order and alter hook implementations at call time, especially for multi hook alter calls.

Some thoughts about relative ordering

In hook_module_implements_alter(), relative ordering always means to order one module relative to another module.
With the new code, we can order specific implementations relative to other specific implementations, OR to all implementations of a module.

As mentioned earlier, ordering relative to a specific implementation easily breaks when that implementation gets renamed.
Ordering relative to all implementations of a module might have unintended effects if one of these as a "first" or "last" order applied.

In my older proposal, there was the possibility to order relative to a module's default odering position.
So, even if the implementations of that module get moved around with first, last, before or after ordering, we still only order relative to the original position.

So, let's say:
- We have modules A, B, C, D, in that order.
- We have hook implementations A1, B1, B2, B3, C1, D1.
- We have ordering info "B1 first" and "D1 before B*".

With the current proposal we get D1, B1, A1, B2, B3, C1.
If "before B*" is changed to "before the default position of B", we get B1, A1, D1, B2, B3, C1.
We can even make this work if none of the B* implementations remain in the default position.

Another way to approach this is to define an order of ordering operations.
E.g. we could apply all relative ordering (before/after) before all absolute ordering (first/last). But this only gets us so far.

I created new branches with tests.
For now I created an MR only for one of them, for tests that include the 3485896-attempt-simplification and do not include my own changes.

In general I think this is direction that these tests should take:
We call a hook or an alter hook, we get a list of called implementations as from __FUNCTION__ or __METHOD__, and we compare that to the expected order.

The test has some additional magic where we want to check that `->alter(['a', 'b', 'unknown'], ...)` produces the same call list as `->alter(['a', 'b'])`.
Currently this is more like a proof of concept, we definitely don't want to merge as-is.

The tests confirm my suspicion:
An order operation with "extra" types has side effects beyond that one implementation being reordered.
We could probably craft something even more clear to confirm this.

The tests would become a lot more interesting if we fix the grouping by module problem in ModuleHandler.
I know this is currently planned as follow-up, but we might actually want to do this first.

Should we take this out of needs review for a bit?

Done.

I was on the fence about splitting hook collector out, but honestly pass is getting huge. I wonder if we have more files, one that does collection, one with parsing and ordering and we leave the registration in hcp?

I am undecided about how exactly to split the classes.
For now I just wanted to see that we _can_ split this up, so that ModuleHandler no longer has to deal with a compiler pass directly.
A separate HookCollector feels like a more generic class to use in different places.
Currently we still have lots of stuff in that class. So indeed we might want to move some of it back into HookCollectorPass, anything that deals with the container.

But, any decision we take now about the final shape of HookCollector might become obsolete with further changes.

New branch to prove the side effect:
https://git.drupalcode.org/issue/drupal-3485896/-/compare/11.x...3485896...
This commit:
https://git.drupalcode.org/issue/drupal-3485896/-/commit/404bc461f0d3fa5...

So we add a #[ReOrderHook] that targets a non-existing implementation in a non-existing module.
This should have no effect on the order of the other implementations.
But the extraTypes messes things up.
We see the first fail, but I suspect there is more, including possibly implementations disappearing.

We can check that the implementation exists before adding to the order group.

This is missing the point.
I just pushed another version of the side effect test, which is using existing implementations.

What mechanism are you proposing that does that?

The "extra types" mechanism is just fundamentally flawed.
For an implementation to ask "I want to be ordered together with X" inevitably has an effect on other implementations and other groupings that can exist at call time. This can be mitigated to some extent, but the basic flaw is always present.

The correct thing is to order at call time.
Otherwise we would have to predict every possible combination of hooks that can be passed to alter().

To explain it differently:
The sorting done in one specific group of hooks can only be used when ->alter() is called with those exact hook names.
But this is not what the MR is doing.

Instead, when you call ->alter() with hooks/types ['a', 'b', 'c'], you might get the pre-ordered list from ['b', 'c', 'd'] or from ['b', 'x'], which gives you not only the wrong order, but can also give you the wrong implementations. Even if we correct the list of implementations at call time, by adding and removing implementations, the order would still be wrong.

There is no way to fix this within the current proposal, because naturally there won't always be a pre-ordered list for the current combination.

I created a separate MR with call time ordering.
Still a bit rough, but this is what I have for now.

I found that objects in a container service parameter definition are no good, it prevents dumping of the container. So I changed it to serialize them explicitly.

Now we have HookOrderTest failing in the pipeline but not in my local.

The reason seems to be that RecursiveDirectoryIterator / DirectoryIterator does not produce a predictable order.
https://stackoverflow.com/questions/29102983/order-in-filesystemiterator
And the way we look for hooks, we mix procedural and oop in the same scan operation.
This means it is not clear whether OOP hooks are first or procedural hooks are first, if from the same module.

#[LegacyModuleImplementsAlter] problem

We found one problem with #[LegacyModuleImplementsAlter], or the idea behind it.

The idea was that a contrib module can support Drupal 11.1.x and Drupal 11.2.x+ by:

• Creating a MYMODULE_module_implements_alter() with #[LegacyModuleImplementsAlter] so that it is only called in Drupal < 11.2.0.
• Adding OOP hook implementations with e.g. #[Hook('my_hook', order: Order::First)], where the "order" part would only be picked up in Drupal 11.2.0 and up.

Problem:
In 11.1.x, the hook attributes will be discovered, but then the `order: Order::First` part will blow up because:
- The enum for Order::First does not exist.
- The named parameter `order: ` does not exist.

Collection order should not have an effect.

It matters because if a module has multiple implementations, their order can be different on different system.
We key by class and method, but we don't sort.
Also, if we would sort by class, we would get a weird position for procedural based on the alphabetic order of ProceduralCall class.

Instead, we should decide whether procedural implementations should be always before or always after oop implementations from the same module (if no other order specified).

We found one problem with #[LegacyModuleImplementsAlter], or the idea behind it.

The solution could be a polyfill in a separate package outside of Drupal core.

There is no issue with:
#[LegacyModuleImplementsAlter]
#[RemoveHook]
#[ReOrderHook]

Correct, technically these work as designed.
The problem is not with the #[LegacyModuleImplementsAlter] attribute but the greater purpose behind it.
Of course technically the problem is with the new order property in #[Hook].

We want modules to be able to use the new order property of the #[Hook] attribute, while at the same time keeping HMIA around to support older Drupal versions.
This will cause problems as described above in 11.1.x.

What these modules could do is declare compatibility with 10.x and 11.2.x but not with 11.1.x.

We could actually create a separate issue where we add a part of HookOrderTest for HMIA as baseline, to be merged _before_ this one.
This will make it more visible if or how the ordering changes.

Some people in chat were concerned about serialization of order operations, I think mostly for security.
I am not strongly attached to serialize/unserialize, but I think the security should be ok if we use a hard-coded list of very simple classes for 'allowed_classes'.

From https://www.php.net/manual/en/function.unserialize.php:

Warning
Do not pass untrusted user input to unserialize() regardless of the options value of allowed_classes. Unserialization can result in code being loaded and executed due to object instantiation and autoloading, and a malicious user may be able to exploit this. Use a safe, standard data interchange format such as JSON (via json_decode() and json_encode()) if you need to pass serialized data to the user.

If you need to unserialize externally-stored serialized data, consider using hash_hmac() for data validation. Make sure data is not modified by anyone but you.

In this case the data is not "untrusted".
But, even if it was, what exactly can go wrong if the classes we allow are as simple as here?
I would like to know.
(and don't tell me "if the class has a wakeup method...". the specific classes we are looking at don't have it.)

I am proposing this change to avoid relying on two synced arrays in ModuleHandler:
https://git.drupalcode.org/project/drupal/-/merge_requests/11676/diffs?c...

I started to review the comments here and have not finished. The main conclusion I have reached is that documentation for this should be in module.api.php. Then, the many references to the change record(s) should direct to the relevant section in module.api.php.

Thanks for the explanations and the prompt updates. Also kudos to @nicxvan for understanding my intent despite my typos.

I focused on just resolving comments and ignoring minor problems that don't effect meaning. Next, for me, is to think about adding this to module.api.php.

I created a branch 3485896-parts and MR https://git.drupalcode.org/project/drupal/-/merge_requests/11783
This has a cleaned-up incremental history, which should make it easier to review step by step.
Ideally, if I did things correctly, each commit should have passing tests. But I have not really tried this, and I don't want to spam the pipelines.
We do lose original authorship.

In the end this is meant to help with reviews, the other branch is still the main review target.

The 'Defining and 'Invoking' sections have moved to the bottom. I think the section order in core.api.php should be this so the more 'advances' uses are last.

 * @section implementing Implementing a hook
 * @section defining Defining a hook
 * @section invoking Invoking a hook
 * @section ordering_hooks Ordering hook implementations
 * @section ordering_other_module_hooks Ordering other module hook implementations
 * @section removing_hooks Removing hook implementations

I asked @nicxvan for some clarification about the CR, Procedural hooks are gathered before object oriented hooks for a given module, in Slack. They said that this was for an edge case, so the CR may not be needed. I lean to not publishing it as well. We want to keep the CRs relevant and not create too many that they are ignored.

I added an item to the remaining tasks to consider not publishing that one.

They said that this was for an edge case, so the CR may not be needed.

Whether this is an edge case is in the eye of the beholder.
It applies when a module implements the same hook twice, one time as a function and another time as a method, and without using the #[LegacyHook] attribute. So it would already be doing something deprecated.

Not quite, procedural hooks are not deprecated yet.

Oh, you are right.

But I struggle to see the use case where one would have both an oop and procedural for the same hook in the same module.

Quite simple, you have the procedural implementation which has not been migrated yet, then you add the OOP implementation which may be needed to cover a different problem.

E.g. you might have a mymodule_node_update() and then you add a \Drupal\mymodule\Hook\MyHook::nodeUpdate() to do something unrelated. You don't want to touch mymodule_node_update() because you were asked (and paid) to work on a very specific problem, and changing existing code is out of scope.
In most cases the order of these two won't matter, if they are for unrelated problems. But you could have funky side effects that depend on the order.

I read through the CRs, which all look great.

I then read through all of the code changes except the test coverage. It is pretty dense, so I am not confident that I would have spotted many issues, but I didn't spot any big ones, just a few comment nits and one or two small questions. I'm still behind on issue comments, semi-deliberately to read the MR with less context.

Everything looks really well organised and well documented, there's just quite a lot of it. However, even though it's adding quite a lot, it hugely simplifies the actual ordering implementations for modules and removes (well, soft deprecates) one of Drupal's biggest hacks.

Given the amount of review/iteration this has had, I'm fairly happy that I'm not spotting issues because they aren't there, rather than failing to spot issues that are, but it's going to take another read through to feel even a bit more on top of it. This shouldn't stop any other committers from having a look and/or committing if they're happy.

Left comments on the MR.
The visibility of a couple of the new methods are commit blocking but many others are personal preferences
Very close here.

@nicxvan @larowlan Can you confirm that the only "commit-blocking" changes are for the two methods becoming protected instead of public?

Oh, never mind, saw #218.

That being said, the validatable config job is showing a warning. Not sure if anything needs to be looked at there.

That being said, the validatable config job is showing a warning. Not sure if anything needs to be looked at there.

Tried rerunning the job, got the same warning.

I also tried rebasing, but got this: "Rebase failed: Rebase locally, resolve all conflicts, then push the branch. Try again."

PHPCS errors now.

#3521450: [pp-1] Add tests for passing both identifiers and modules to order against. says tests are needed for both OrderAfter and OrderBefore, but I see only a test for OrderAfter added here. Is the OrderBefore test still needed?

lgtm

Committed to 11.x and published change records.
Added a release note snippet.

There's a small problem with mixing test annotations and attributes on the same level, here. Impacting #3497431: Deprecate TestDiscovery test file scanning, use PHPUnit API instead. Opened #3521815: Fix test annotations for tests introduced by #3485896 for a fix.

from https://docs.phpunit.de/en/10.5/attributes.html:

PHPUnit will first look for metadata in attributes before it looks for annotations in comments. When metadata is found in attributes, metadata in comments is ignored.

Thanks @mondrake

opened this follow up today