Hook ordering across OOP, procedural and with extra types

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.

This issue really isn't ready for review yet, we still need the runtime BC layer, but you can see how it works in ckeditor here:

  #[Hook('form_filter_format_form_alter')]
  #[HookOrderGroup(['form_filter_format_add_form_alter', 'form_filter_format_edit_form_alter'])]
  #[HookAfter(['editor', 'media'])]

We kind of have to do this all at once, we're only replacing the one implementation in this issue though.

(please continue not crediting me)

nicxvan → changed the visibility of the branch 3485896-attempt-simplification to hidden.

Updating the IS an CR.

nicxvan → changed the visibility of the branch 3485896-attempt-simplification to active.

nicxvan → changed the visibility of the branch 3485896-hook-ordering-across to hidden.

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.

@berdir thanks for your review a lot of good points! I'll reply more fully once I've thought through some things.

@amateescu great to know! That would be super easy in this implementation then!

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

> 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. But I do not advocate for doing so, I do not advocate for anything.

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.

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

Even easier, I'll reach out if I have questions.

I think we should consider to convert all implementations in core of this to verify the concept.

I agree, I was concerned about scope, I reached out to @catch on slack and since there are only a few hooks we can convert them here except common_test and module_test

There aren't that many and then we can immediately deprecate implementations hook_module_implements_alter().

Minor correction here, we will not deprecate hook_module_implements_alter in the traditional sense, we will deprecate hook_module_implements_alter without the #[LegacyHook] attribute. In order to support older versions of Drupal contrib / custom will need to keep their implementation and tag it.

The second is completely replacing hook implementations

Good catch! I think we can handle that here as well though with a more simple remove or replace parameter. (What is replace other than remove the other one and add mine? So with this you can remove theirs and on the same attribute provide the replacement #[Hook])

Opinions on the name of the parameter are welcome.

I think functionally it has to be remove, but semantically replace might be easier to understand as a DX

In the case that you only wanted to remove you would just put an empty implementation and replace the hook you wanted to remove.

There is one caveat to this approach that could be addressed in the future followup. Remove for a given module / hook combination would be global, meaning if a module implements a hook multiple times all would be removed if this parameter is passed.

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.

Yes, this does complicate things and was where a lot of the effort went. It is an edge case, but I would say it's worth keeping because it does solve the extra types use case. Most of the comments on the MR from catch were about explaining this so it definitely needs more comments.
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.

Assigning to myself per the assignment policy.
I am working on addressing comments on the MR and converting the rest of core.

I will also take a look at adding the remove / replace parameter name TBD

> 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?

I tried that, it didn't work cause it then gets called multiple times since it's new hooks.

Ok I rebased to fix the navigation performance test.
I also converted all core implementations except common_test and module_test which we've not converted anything for.
I also did not convert module_handler_test_all1 since it is testing hmia.
I did not convert workspaces as I am waiting on: 📌 Cleanup hook implementations in the Workspaces module Active

I'm wondering if replacements is more confusing since it is really just removing those implementations. It could be called removals.

Once a consensus is reached I can update it. In the mean time I finished converting navigation and wrote tests.

I also added a new CR just for this parameter.
https://www.drupal.org/node/3496786 →

Once that workspaces issue is in I'll convert it, I will look at deprecating HMIA too here since we've converted everything but the test ones.

nicxvan → changed the visibility of the branch 3485896-deprecatehmia to hidden.

Workspaces is in, let me convert that one too!

nicxvan → changed the visibility of the branch 3485896-deprecatehmia to hidden.

Ok workspaces has been converted, I also created yet another CR for deprecating hook_module_implements_alter, I also created a new branch that I hid to add the deprecation message where I think it needs to go, I'm running that test now, if we think we should deprecate as part of this issue it's easy to move it over to the canonical branch.

I think I'm going to rename replacements remove.

I think longer term it will be confusing to have it called something it's not doing.

If you would like to see what is needed on top of this change to deprecate hook_module_implements_alter:

https://git.drupalcode.org/issue/drupal-3485896/-/merge_requests/1/diffs

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

That broke Drupal\Tests\content_moderation\Kernel\WorkspacesContentModerationStateTest

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 :)

Ah I misunderstood what you were saying here:

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.

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.

And what does it look like with my version? Cause that passed.

That should do it! I'll keep an eye on tests.

Ok this is ready for review again

Cleaned up the remove hook bit and the override order bit.

I'll update the IS, CRs and docs later tonight or over the weekend. It's still ready for review.

I've updated CRs and the IS.

Renamed to ReOrderHook

I think we should just deprecate HMIA here, it's only +94 -67 https://git.drupalcode.org/issue/drupal-3485896/-/merge_requests/2/diffs

Tagging for the deprecation question since this is unique.

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.

I will rebase this if necessary but this Needs Review either way.

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 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?

While I agree with your sentiment in the general sense, in this specific case these two attributes are likely going to be super rare.
Further, when they are implemented, they will almost always have a specific hook that they are being enacted FOR, so there is a specific hook they are related to. It will be extraordinarily rare for one of them to be just alone, in that case you can throw them on a hook class with an __invoke method that does nothing.

Evidence that these are rare:
RemoveHook Has one implementation in all of core.
ReOrderHook Also has one implementation in all of core.

The next question of course is if it is so rare why have it, is there another way to do what they are doing?
The answer to that is they both provide a specific piece of functionality that hook_module_implements_alter provided that is crucial we maintain for the modules that maintain it. We spent a significant amount of time exploring options and this is the cleanest we came up with.

If you have an alternate suggestion I'm all ears.

I created this CR @larowlan's request: https://www.drupal.org/node/3505563 →
Turns out it is already covered by BC policy: https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... →

I have requested it get deleted.

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: 📌 Simplify ckeditor5_module_implements_alter() in favor of checking for ckeditor5 in media module Active . 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.

Not to repeat the whole comment, but I think that change should be postponed on this since for now that is a concrete way to show how groups work. I agree long term we can remove it, but since grouping is part of the most complex bit having a concrete example makes this issue easier to grok.

I rebased the main branch, I'll rebase the deprecation later.

nicxvan → changed the visibility of the branch deprecatehmiav6 to hidden.

I recreated the deprecation branch, rebasing has become painful so here is a patch with the changes only.
I'm going to hide it, but I'm attaching it here for posterity.

nicxvan → changed the visibility of the branch deprecatehmiav7 to hidden.

I had missed a couple of test updates, here is an updated patch.

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

Thanks!

@larowlan signed off on the deprecation method on slack so I'm removing framework manager tag. https://drupal.slack.com/archives/C079NQPQUEN/p1740180561937599?thread_t...

From a framework manager POV I'm happy with the approach from a technical perspective. From the POV of should it be one issue or two, it's probably a release manager question. My 2c would be to do it in one issue and sidestep the risk of missing shipping the BC layer.

Leaving the release manager tag for that decision.

I'll update the IS later.

@larowlan signed off on the deprecation method on slack so I'm removing framework manager tag. https://drupal.slack.com/archives/C079NQPQUEN/p1740180561937599?thread_t...

From a framework manager POV I'm happy with the approach from a technical perspective. From the POV of should it be one issue or two, it's probably a release manager question. My 2c would be to do it in one issue and sidestep the risk of missing shipping the BC layer.

Leaving the release manager tag for that decision.

I'll update the IS later.

@donquixote thank you for the thorough review.

I applied several suggestions and renamed a couple of variables you pointed out were confusing.
You're not the first person commenting on the term of Group, it'd be a lot of work, but if someone comes up with a better name for hooks that ordered together, I'm all ears.

There are some questions I want to review and think about.

There were also several suggestions and reorganizations that I think I see what you were going for but I disagree if they are clearer.

Specifically renaming the nested foreach in process and gatherOrderInformation, I don't think we should change those.

Ok I think we need to rename group and the local variable orderGroup.

I asked in slack for suggestions and as of this comment @catch, @longwave and @donquixote participated.

Suggestions were:

• set
• extra_types
• implementations
• grouped_implementations
• implementation_groups
• family

Since they are the extra types we are ordering against in alter, I think the best option is the following:
on the ComplexOrder object: extra_types As a local variable on HookCollectorPass: orderExtraTypes.

Ok I renamed group to extraTypes, $orderExtraTypes and $orderedExtraTypes where applicable, I think that might be a lot clearer.

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.

Great thanks! I'll pull that in and then update the IS

nicxvan → changed the visibility of the branch deprecateHMIAFresh to hidden.

Ok that has been pushed up

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.

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

Most of your comments could be addressed in follow ups that are far more narrowly scoped.
I will create those follow ups shortly with some details to come later.

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.

Yes, hooks are different so the divergence is expected

Explicit vs calculated numeric priorities

These are not subscribers, so again them being different is expected. 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.

Relative ordering

This section only applies when it goes through the legacy ordering, and HMIA can't handle multiple implementations so them getting flattened when run through legacy hmia runtime is acceptable, once all modules convert to this system there will not be an issue.

Module name per implementation

Yes, we do that.

Order lost when grouping by module

While true, let's handle it in a follow up, multiple implementations are new, going to be rare, and ordering them as a separate entity will be even rarer. That follow up scope will be much more narrow and easier to handle. Let's not hold this up for an edge case of an edge case of a new feature.

Ordering conflicts

I agree with your assessment, this is not a blocker, this can also be solved with module weights, or by using ReOrderHook which was not previously available.

Alternative

I think this is pure follow up material, again I and Ghost have ideas for further clean up too, but as I've mentioned, this issue scope is already huge, and we know it works, let's not get bogged down in perfection.

Created two follow ups

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.

There were two very long discussions in slack relevant to this.
The first regarding naming of extra types and other variables: https://drupal.slack.com/archives/C079NQPQUEN/p1740408731650039
All variables have been renamed.
There is still a point of contention about extraTypes on the order object. I have suggested relatedAlters. Since this is the second rename I will not do this unless there is more consensus. I will post more thoughts on this later.

The second discussion was around higher level feedback most recently about ordering around extraTypes and ordering multiple implementations: https://drupal.slack.com/archives/C079NQPQUEN/p1740590792125979

Before I dive into that further I want to highlight the purpose of this issue and why it is so large.
The purpose of this was to provide the ability to order hooks and deprecate hook_module_implements_alter. In order to deprecate HMIA we need to replace all of it's functionality:

• set a hook first
• set a hook last
• order a hook before another module
• order a hook after another module
• remove another module's hook
• reorder another module's hook
• reorder a hook relative to alter hooks with extra types

That is a huge ask, especially when there is the constraint of handling this in a BC compatible way so current modules that have not converted yet can still order hooks relatively. This issue has been able to achieve that.

I want to also highlight that an inordinate amount of attention has been given to ordering relative to extraTypes (formerly orderGroups). We have to support this to deprecate HMIA because core does do this, and inevitably some module somewhere does to. However, I cannot stress enough how rare this should be. However, as complex as this is, the situation is significantly better because now it is part of a discreet object and parameter so if we need to change it in the future, we can deprecate. As part of HMIA it's all or nothing deprecation so for this stage we must provide that option. I will likely update the IS and CR later highlighting how rare this should be, and if you find yourself using extraTypes, you should take a step back and consider deeply if you really need the approach you are taking. It did take the most time since it's the most complex bit, and it has faced the most scrutiny, but we know it works with ckeditor5 and the tests, and it really should be the least used bit of this. This is also why I will hold off on renaming again unless there is wider consensus, it's a lot of thought and attention something that will almost never be used, and it is named after how it is used in ModuleHandler::alter.

The other point of contention in this thread and slack is that if a module has multiple implementations they want to order that order is lost. However, this should be a follow up, this is a current gap in implementation, and HMIA cannot order against multiple implementations anyway. The scope of this issue is so large we should not take on pre-existing bugs that are for a feature that is brand new and will likely not be used. Again, it is an edge case of an edge case and we have a follow up already for it.

Now to address some specific points in the previous comment:

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.

In general I agree, however I don't think that applies here for two reasons. 1. I strongly doubt we will remove this unless altering with extra types goes away and that's not happening anytime soon. and 2. this situation is much, much better because if we do change it we can deprecate just that one piece of ordering.

Can you remind me what is the "initial OOP hook issue" and how it failed? Maybe I missed it.

The one that is in the IS is where we discuss how it blew up 🐛 Ordering of alter "extra hooks" is a gigantic mess Active
A couple of final comments on the approach you mentioned using priority in the slack message and why it blows up. If you use priority then you have to exclude that hook implementation from hook ordering in HMIA which means there is a line in the sand you cannot legacy order hooks using priority so you force other modules to update if they need to order relative to you. With our relative ordering approach it is far less disruptive.

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.

I didn't want to add too much here since there is already information overload, but I suspect we'll likely go in this direction 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active But we can't start that until this gets in.

To me this MR is incomplete if we don't properly support multiple implementations per module. It could even reveal shortcomings in the approach.

This is out of scope for an issue already this big. This already cannot happen, it remains to be a problem, but once this is in it will be a smaller issue to fix.

I really think this issue is close to being ready, it has been looked at by several people in depth. We converted all core hmia implementations so we know they are covered, and we have significant test coverage. I am starting to worry about attrition for this issue, let's not perfect be the enemy of an issue that replaces one of the most complex and problematic hooks.

donquixote → changed the visibility of the branch 3485896-donquixote-suggested-changes to hidden.

@donquixote did a deep dive and has a lot of suggestions. Some are substantial that I need to review and give them due consideration. First review, I think one or two might be a gap that needs to be addressed before this is merged. I think most of them are just an alternate way of doing things, not sure if they are better, worse, or equivalent without more thought.

Many are still related to typing and naming, before going through that I'd like a consensus on a couple of questions:

1. A lot of the typing is on parameters that have not changed, I think that would be out of scope for this issue, should we avoid that?
2. Same for constructors being moved to multiline, I prefer this myself and I know coding standards allow this, but I'm not sure it is in scope here.
3. Suggesting renaming ComplexOrder with RelativeOrder or RelativeOrderBase, I'd lean towards RelativeOrderBase.
4. Rename value for ordering to direction or position, I'd lean toward position.

If I get consensus on 1-4 I'm happy to apply them.

donquixote → changed the visibility of the branch 3485896-donquixote-suggested-changes-part1 to hidden.

donquixote → changed the visibility of the branch 3485896-donquixote-suggested-changes-part2 to hidden.

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.

Basic concepts

This section is generally correct.

(Consequence: When the class or method for a hook implementation get renamed, the ordering attributes that target the old name no longer work.)

Yes, but when doing relative ordering you have to know about the implementation you are ordering relative to.

Alternatively, a relative ordering can address module names, which will target all implementations by that module.

For the hook providing the ordering directive.

Conflict resolution

Yes, that is how it works here.

Extra hooks logic Tbh I find it hard to follow the "extra hooks" logic.

Yes, it is hard to follow.

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.

I can almost guarantee that almost no one will ever need this, it's needed in core so we had to provide it, this solves core's usage, proven by tests. That should be the bar for acceptance for this bit of the feature.

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.

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.

Naming of methods and variables

Not going to address anything here beyond what I've said before, if other's agree we should rename anything I am happy to change to a consensus. I'm not going to rename anything further before that. See IS for current options.

Test coverage

There is extensive test coverage, further, we have converted all of core which has some of the most complex HMIA implementations. I'm confident we have enough tests. This is a large enough issue that there are gaps, but we cannot find them without contrib using it which requires merging this.

Follow-up issues?

To be fair my core contribution experience is more limited than yours, but from what I've seen everything I've called out as a follow up falls within standard usage. I mean look at the size of this issue, it's already at least 5 times larger than average and far far more complex. There are some edge cases we know about, the sooner we get this in the sooner we can track those down. Needing to rebase a 2000 line MR to try to solve theoretical edge cases is not the way to progress.

Alternative: Order at call time (cached)

Runtime ordering is where almost all of this complexity comes from, so I would be strongly against adding more of that in the future.

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.

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.

I am 100% confident in how we designed this, but as mention in the MR even though it is an API it is marked internal so we can tweak it. The extreme complexity is limited to only one parameter on a parameter, that gives us the ability to deprecate if we have to change it wholesale, we do not have that ability to do targeted updates now which is why it is so massive.

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.

I literally tried that multiple ways, it does not work and is the whole reason we created extra_types.

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?

The same way it does now if you only move your implementation first without doing any array searching and slicing like ckeditor5 does.

$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.

donquixote → changed the visibility of the branch 3485896-donquixote-suggested-changes-part2 to active.

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.

Can you rebase against the main branch so the fork mr is accurate so I can see your changes.

I reopened those two threads.

However, in the one I responded:

Once HMIA is gone this edge case is gone

Maybe edge case is the wrong word, it's more that once this ordering is the only way there won't be a way to order against unknown hooks so we don't have to fall back to legacy ordering. I closed it because of the comment about legacy ordering.
I would still consider that answered.

The other one I'm not sure is an issue either, the last one should win, just like with hmia now.

All of this has to work reliably and predictably, otherwise it should not be added.

It does, and far more that hmia it is easier to understand how to implement your item ordering. It doesn't have to be perfect we can't achieve that in a world where we need to be bc with hmia.

Form alters are not an edge case.

I never said they were, but ordering across multiple layers of form alter very much is an edge case.

If I could cut that out of this issue I would because 90% of all comments outside of typing and renaming are about extra types, and one hook uses extra types.

If you can find a contrib module doing this type of ordering I will gladly convert it to test this further.
Until we find other uses for it, this is all speculative. Modules can use legacy ordering and file an issue if we didn't meet their edge case, it's not the end of the world, we solved a fews issues since oop hooks came out that we missed.

Feel free to add more tests in your branch we have many tests and we have tons of implicit tests with the hmia conversions we did.

Feel free to experiment with refactoring too, but most of your refactoring I've seen are internal implementation details that would be far easier to get in and scope once this is in.

Other than your comment about module handler add i have not seen an architectural question you've raised we had not already considered which is one of the reasons I'm so confident. I'm not worried about module handler add because it has far deeper architectural issues and it's on its way out and this works just as well as it does in 11.1

I took a look at your branch and just reviewed HCP directly.
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.

I still think it's far more efficient to store legacyImplementationMap and remove the one or two that need to be removed rather than loop over all thousands of hooks just to build it after, but I wouldn't block the issue on that.

Just to make sure I am reviewing everything, I don't think you made any substantial changes beyond what you changed in HCP and the really minor optimization in ModuleHandler:alter right?

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.

I don't have a super strong opinion on this bit, it was more that we already have the reflection class so why not use it.

I know that @godotislate is experimenting with updating this elsewhere too.

It was suggested as a possible performance enhancement on 📌 Investigate possibilities to parse attributes without reflection Active #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.

No objection from me to remove that on your branch, let me know when it's ready for a deeper look.

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.

But for now I want to reduce disruption.

Too late for that lol.
Honestly I don't really care snake case vs camel.
Things seem to be trending toward camel so I generally lean towards camel. In an issue of this size where we are talking about a whole new approach this feels like noise and can be addressed when we settle on which approach.

More tests

Sounds good.

Order and alter hook implementations at call time, especially for multi hook alter calls.

I still feel like this would be a mistake. If you want predictability we want as much ordering during build as possible. This will also be more performant.

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.

Yes, this is a limitation, but in the interest of scope we should try to minimize changes.

In my older proposal, there was the possibility to order relative to a module's default odering position.

Feels out of scope and something that could be added later.
Out of curiosity why would you want this?

Another way to approach this is to define an order of ordering operations.

Same scope concerns and as you pointed out it's just punts down the road.

First impression on the further changes.
I'm 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 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.

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.

We can check that the implementation exists before adding to the order group.
We had considered that so i want to look deeper.

I suspect there is more, including possibly implementations disappearing

What mechanism are you proposing that does that?

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.

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 do not think that's true, but let me work it out, thanks for the tests!

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

Created one off yours to fix the first stage just so we can see the full test suite.

I'll take a look at your tests on my branch in a bit.

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.

nicxvan → changed the visibility of the branch 3485896-nicxvan-order-at-call-time to hidden.

nicxvan → changed the visibility of the branch 3485896-donquixote-suggested-changes-part3 to hidden.

nicxvan → changed the visibility of the branch 3485896-donquixote-tests-nicxvan-FAIL-side-effect to hidden.

nicxvan → changed the visibility of the branch 3485896-donquixote-suggested-changes-part2 to hidden.

nicxvan → changed the visibility of the branch 3485896-donquixote-tests-donquixote-2 to hidden.

nicxvan → changed the visibility of the branch 3485896-donquixote-tests-baseline to hidden.

nicxvan → changed the visibility of the branch 3485896-donquixote-tests-nicxvan to hidden.

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.

Why would this matter?
We key them by class hook and method.
Collection order should not have an effect.
Your tests look like they are failing due to deprecations.

I hid most of the branches.

#[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]

They are just ignored.

The issue may be with the new enum and ComplexOrder object in the new order parameter for hook, I think it does get loaded so if contrib converts to the new ordering then is loaded on Drupal 11.1 then those do not exist so there is a fatal error. (The fact that the parameter is not parser does not matter because those missiles will have kept hmia around for ordering.)

I need to confirm this and look at a solution.
I will also look at the extra type tests you wrote.

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.)

We raised three specific concerns about that piece:
Speed, yes this is only for alters, but form alters can be called many times on a page.
Size/memory, in the beginning there are not many, but this api is much easier than HMIA so adoption will likely go up.
Security, you're hand waving this, but there are escalation attacks related to these functions and classes. why not avoid this concern entirely?

Looking at how you're using it is is so you can just call the ->apply method right?

I think we will be consolidating on the new approach shortly. There is still some refinement to happen, but before I hide the attempt simplification branch and update the CRs and IS to remove mentions of extra_types I wanted to give some notice.

nicxvan → changed the visibility of the branch 3485896-donquixote-order-at-call-time-nicxvan-changes-BAD to hidden.

donquixote → changed the visibility of the branch test-file-scan-order to hidden.

donquixote → changed the visibility of the branch 3485896-donquixote-order-at-call-time-nicxvan-changes-TBD to hidden.

donquixote → changed the visibility of the branch 3485896-test-file-scan-order to hidden.

donquixote → changed the visibility of the branch 3485896-test-file-scan-order to hidden.

nicxvan → changed the visibility of the branch 3485896-attempt-simplification to hidden.