OOP hooks using event dispatcher

> Hook implementations go into Drupal\modulename\Hook namespace. Familiar pattern from plugins.

+1, good DX.

> These are automatically registered as autowired services

Do they specifically need to be registered as services? They don't need to be used as services, and that's going to add a LOT of bloat to the container.

> All the implementations are stored in a container parameter

Likewise - that's a lot of bloat, surely?

> The attribute supports a priority as well: #[Hook(hook: 'user_cancel', priority: -20)]

I'm torn between using priority to be consistent with Symfony, and suggesting we use weights to be consistent with Drupal-land.

> There's little to no concern given to backwards compatibility. New hook implementations fire after the old ones and they are altered and fired completely separately. It is not possible to mix old and new

That would be doable if we wanted to, though, I think? Have something that discovers procedural hooks, and registers them with the OOP hook handler using the module weight.

We could even smooth the path to conversion by allowing #[Hook] to be used on a procedural function if we wanted, no?

> Do they specifically need to be registered as services?

Well, yes, we use the event dispatcher to fire them and the dispatcher uses services.

Note the full Symfony framework will also register every class it finds under a specified directory which is usually just App\ so that system is designed to take it. Surely by now we would if our array dumper/loader couldn't but can't see why not.

> I'm torn between using priority to be consistent with Symfony, and suggesting we use weights to be consistent with Drupal-land.

You and me, brother. I have no idea what to do here. This is up to community to decide. Code wise, it's trivial to change: change string replace priority to weight and a - sign in HookCompilerPass because the event dispatcher does use priority but that's an internal detail, really.

I neither know nor do I want to figure out how to convert procedural functions into something that the event dispatcher can fire. The entire system is designed around hook implementations being methods on objects. But I have a suggestion. Let's pretend we have a dumper which can dump the service definitions written by autowire. Then:

1. Convert the hook code to the new system
2. Dump the service definition generated and add it to service yml
3. Change the procedural call to manually call the converted code
4. Add a [#HookConverted] attribute to the procedural code. This would be ignored completely by pre-patch Drupals and this patch could change ModuleHandler to skip functions marked so.

This would allow modules to convert to the new system yet remain compatible with old Drupals without maintaining the same code twice. Ordering would need to be doubled up if this is what the community wants to do. Not my call.

>> These are automatically registered as autowired services

> Do they specifically need to be registered as services? They don't need to be used as services, and that's going to add a LOT of bloat to the container.

If they are registered as private services they don't bloat the container as much; during container build they should be inlined into the event dispatcher definition as that is the only place they are needed, they don't need to be publically accessible by any name.

> Note the full Symfony framework will also register every class it finds under a specified directory which is usually just App\ so that system is designed to take it. Surely by now we would if our array dumper/loader couldn't but can't see why not.

Symfony does do this but also services are private by default, and unused private services are then optimised away - so if you don't refer to an automatically discovered class somewhere, it doesn't even exist in the final container. Drupal is still public-by-default and we need to unwind a lot of things to be able to change that.

> A method can have multiple Hook attributes if it implements multiple hooks. For example, node_comment_insert and node_comment_update have the exact same implementation and so they could become

This is nice!
But how would the implementation know which hook is being invoked currently?

We also need to think about how this should affect api.php file documentation. When Hook() attributes become the default way of implementing a hook, an api.php file containing procedural functions won't make much sense.

But how would the implementation know which hook is being invoked currently?

They don't. This feature is for code that doesn't care. Please check node_comment_insert and node_comment_update -- they are literally the same code.

But how would the implementation know which hook is being invoked currently?

Isn't it up to the person writing the code to make it aware of what is triggering the code? Or am I missing something?

I apologize if I am missing something, but:

#[Hook(hook: 'user_cancel')]

feels a little bit duplicated (or is that because of alter hooks being different?).

But I would prefer clearly:

#[Hook('user_cancel')]
#[AlterHook('module_implements')]

or

#[AlterHook('module_implements_alter')]

Therefore we get rid of the magic naming, but don't have: Hook(hook: ) duplicated.

But maybe it is necessary, then please disregard.

1. I am not sure how this works. It's entirely possible you can already drop hook: -- if it just calls the constructor then you already can. I pushed a commit doing this.
2. We could add an AlterHook attribute extending Hook which adds _alter and ... that's it. So Hook('foo_alter') and AlterHook('foo') is the exact same. Is this acceptable?
3. Regarding procedural, this is described in the issue summary: I advocate against the complexity of adding a harness to call the old ones from the new system but of course that's just me. But I also wrote it up in #8 how to integrate them seamlessly using a ProceduralCall class and a magic __call method call whatever procedural call it is passed to.
4. Also I am not seeing the event based hooks take care of module implements alter -- this is explained in the issue summary and an example is provided in the PR. The existing service provider infrastructure is used.

You and me, brother. I have no idea what to do here. This is up to community to decide. Code wise, it's trivial to change: change string replace priority to weight and a - sign in HookCompilerPass because the event dispatcher does use priority but that's an internal detail, really.

There's an old issue at 📌 Add before/after ordering to events Needs work , which with new things, could maybe look like #[Before('whatever')] #[After('whatever')] #[Replaces('whatever') (or an extra param to the hook attribute) then numeric priorities/weights wouldn't need to be interacted with by modules.

Isn't it up to the person writing the code to make it aware of what is triggering the code? Or am I missing something?

It's for cases where exactly the same things needs to happen. i.e. sometimes you have identical code running on insert or update hooks, right now you'd put that in a extra method/function somewhere and call it from both, with being able to annotate the same method for multiple hooks, everything is in one place.

If the implementation needs to be different, then it should two methods with their own hook attributes.

So Hook('foo_alter') and AlterHook('foo') is the exact same. Is this acceptable? Would look like like

I think this might have been discussed in one of the other issues (or even implemented in @donquixote's issue). Alter hooks are special so it's worth doing dedicated for it IMO.

I am all in for adding before / after into the attribute, I see zero challenges here, HookCompilerPass could run the (already documented) min/max as needed, but could we punt this into a followup? My hand is up, I will write it, no problems. I just would like to proceed in minimal slices so we can proceed in parallel after. Likely the conversions made in this issue shouldn't be merged either, they are there to ferret out any problems (and they did).

Because while I see no technical challenges, the exact syntax likely will take quite a bit of discussion to figure out -- is it before/after a module? a list of modules? a list of class-method pairs? how do we indicate I want to be first/last? what if two does say want that? what if the ordering is not doable? A wants to be after B, B wants to be after C, C wants to be after A, opsie. So there's a bit to discuss on that front.

I am all in for adding before / after into the attribute, I see zero challenges here, HookCompilerPass could run the (already documented) min/max as needed, but could we punt this into a followup?

Yes but I think it means we can defer some discussion about weights vs. priorities too.

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.

One thing I liked about the Hux-style approach was that you could just add a Hook attribute to any service and it was not limited to things in the hook namespace - you just tag the service as a 'hook_listener' and it works.

BUT this can and should be a follow-up.

I just want to ensure that this use case CAN still be made to work with the current architecture. (even if we then just later decide for or against it)

Patch looks great so far, but if it was me:

- We only allow one hook implementation per module right now (which is fine)

Why don't we just merge the collected listeners with the procedural when:

moduleHandler->invokeAll() collects the procedural implementations?

Then the whole module_implements_alter(), etc. would still work (without any changes or before/after!).

That would ensure for this FIRST iteration, we get OOP hooks via event listeners and auto-wired services, but moduleHandler really does not change except for discovering these implementations for the hook.

Before this was not possible due to how we did not have getListeners() so ordering became impossible, but now it's natural.

And we still throw an Exception() if a hook is defined more than once in the same way.

I feel - and I might be wrong - this would make for the most minimal patch.

Good idea to figure out what can fit into 10.3.x (deadline - week 29 Apr'24)

Current state looks good starting point but Experimental API needs more discussion

PS: looking at NodeNooks.php implantation it reminds me Action plugin, for example \Drupal\user\Plugin\Action\RemoveRoleUser

I posted a new version folding procedural hooks in resulting in much simplified ModuleHandler -- and the build time parts are not so bad either. We discussed with catch and just throwing in every function as a hook candidate seems a very reasonable plan as already there are not that many functions which are not hooks.

A few tests around install and update are not passing yet, the rest are happy.

The only unrelated change I made is renaming core_field_views_data to views_field_views_data. But since that's a hook implementation I doubt it's a problem. If not desirable, we can revert -- it will work now but when I made the change ModuleHandler::invoke was stricter, requiring $module to be a $module and $hook to be a $hook. Since then I dropped this for more backwards compatibility.

I'm not sure if this is out of scope, but I don't see any special handling for .theme files.
I do see the update for where hook_theme() is invoked in registry, but while there is some code to parse in HookHelper for .module, .inc, and .install, I don't see the same for .theme.

I would punt on themes. While ThemeHandler does add the themes to the PSR-4 namespaces and so it is maybe possible to give themes a similar treatment I would like to proceed in as small chunks as possible to make it easier to get this in. Please note DrupalKernel currently has zero to do with themes and we would need to figure out some sort of theme namespaces during container build which might raise some issues: the patch currently has failures around installation, I am afraid it make even this worse.

Fair enough, I'm ok with that scope, we should be sure to include that carve out in the change record so there is no confusion.

Another note from the slack conversation between Chx, godotislate and myself is the following from godotislate:

it looks like they are all invoked after module handler has its turn so keeping them OOS seems fine

So they are likely not intermingled and can be modified in another issue.

Just gathering the test failures:

Drupal\Tests\demo_umami\Functional\DemoUmamiProfileTest
Drupal\Tests\media\Functional\UrlResolverTest
Drupal\Tests\system\Functional\UpdateSystem\UpdatePathLastRemovedTest
Drupal\Tests\system\Functional\UpdateSystem\UpdateScriptTest
Drupal\Tests\standard\Functional\StandardTest
Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryAuthenticatedPerformanceTest
Drupal\Tests\standard\FunctionalJavascript\StandardPerformanceTest

The cache test is passing for me locally, I am going to try rebasing.

It was already up to date, I'm not sure what is going on, the test passes locally, but it is failing upstream.

I narrowed it down to this assertion in: core/modules/block/tests/src/Functional/BlockCacheTest.php

// Block content not served from cache.
$this->drupalLogout();
$this->drupalGet('user');
$this->assertSession()->pageTextContains($current_content);

It's great to see more movement on this problem space but this new issue has raised a few questions for me. Quick background: I have been using Hux since it's inception, and am arguably one of its biggest users and supporters seeing as my colleague wrote it :)

We now have several implementations of Hux like OOP hooks, it would be great to get a consensus on what one we're going forward with and close out the other MRs/issues ( https://www.drupal.org/project/drupal/issues/2237831 🌱 Allow module services to specify hooks Needs work for example)

I'm also curious what the differences (if any) there are with this solution when compared to Hux. It seems quite similar but I haven't had a chance to review the code yet.

Thanks for all the great work so far!

@acbramley that decision is not mine to make. As far as I can tell, there are two things Hux allows for and the current PR does not and both are easy to add either here if it's blocking or in a followup if desired. I do not yet see the need but I am easy to convince.

1. hooks in subdirectories -- we already have multiple classes, do we need multiple directories too? I can imagine arranging into say NodeUserHooks.php and NodeTermHooks.php but ... subdirectories? Are we really going to need them? I can write them, sure, we already have a recursive iterator, using another is not a biggie.
2. hooks as tagged services -- I think the Hook attribute on a class completely replaces this need. If I am wrong, let me know and I will add this.

Re-reading Hux after writing this was certainly useful because I quickly added the replaces functionality.

This PR has some advantages over Hux. Apologies if I missed some of its functionality.

1. Complete subsuming of procedural hooks resulting in a simplification of ModuleHandler by moving some of the code to discovery time.
2. Support for the rather complex alter ordering for multiple types. I think my support is easier to understand than the current one, even.
3. Speaking of ordering, an equivalent for module implements alter is supported -- without adding any new mechanism even. Just use service provider alters on the new container parameter.

For the future, this patch delegates the actual sorting to the event dispatcher allowing for a followup which adds before/after functionality benefiting both events and hooks automatically.

While more tests certainly need to be added this is now green and certainly ready for review.

@acbramley I've gone ahead and postponed 📌 Hux-style hooks, proof of concept Needs work which has a similar API but ended up with a very complicated bc layer, the discussion on that issue is extremely relevant to here still and we should copy credit over etc. It could also use a proper comparison of the different approaches between these two issues somewhere here, but not going to attempt that on Sunday morning..

I've also postponed 🌱 Allow module services to specify hooks Needs work which didn't have any recent code but was the original, ten year old discussion, that wanted what we ended up with here but didn't have Symfony 6 and attributes (or the experience of Hux) to make it a reality.

Between all the various issues, and can't separate my own very strong opinions here, I think there is the following in common between the two active issues that so far seems non-controversial:

• Allow OOP hooks to be invoked with the current hook invoking APIs, so that conversions to OOP hooks can be done independently from any changes to invocation.
• Continue to support passing 'arbitrary' arguments to hooks, so that e.g. node load implementations type hint NodeInterface $node and get that, not an Event with a ::getNode() method.
• #[Hook] attribute possibly with #[HookAlter] variation, and autowiring, so that hooks can still be defined within a single method in a single file (no requirements for separate YAML or ::getSubscribedEvents()).
• Allow multiple hook attributes on a method (for the insert/update identical logic case).
• Allow multiple classes for hooks so that they can inject only the services they need.

Things that are still to be defined somewhat and may end up in follow-ups:

• Allowing multiple implementations of the same hook in one module vs. not.
• Hook weights vs. priorities vs. before/after/replaces support, how much bc there is for hook_module_implements_alter(), and what any eventual replacement for hook_module_implements_alter() might look like (can we do it all with before/after/replaces attributes + a compiler pass?)

catch also tagged 📌 Replace hook_cron() with a more modern approach Needs work as related. Being able have multiple hooks in a module would definitely solve one aspect that the cron subscriber issue is trying to solve, another is the ability to have cron subscribers in core components. Would this allow to also basically treat each component as a "module"? That would allow to move a lot of the code that's currently in system.module, especially hooks, to the respective component. Similar to how we already support plugin discovery there, each core component could have a Hooks folder as well.

Supporting the full container.namespaces instead of module namespaces to include core is certainly easily doable.

With Hux, finally, we can crush all remnants of the New Republic and the Resistance.

How will hooks whose names include a variable work?

E.g. hook_form_FORM_ID_alter, hook_ENTITY_TYPE_update()?

Could the variable part be a separate value in the attribute?

@joachim I would think that would work the same as now? - i.e. hook_form_FORM_ID_alter() is always implemented as hook_form_my_form_id_alter() same as hook_ENTITY_TYPE_load() is the same as hook_node_load(). If you want to operate on all entities or all forms, then you go back to hook_form_alter() and hook_entity_load()

What I meant is, would we have:

  #[Hook('hook_form_myform_alter')]

or split this out to a parameter:

  #[Hook('hook_form_FORM_ID_alter', form: 'myform')]

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.

Big +1 to just having it as it is now (i.e #[Hook('hook_form_myform_alter')], not #[Hook('hook_form_FORM_ID_alter', form: 'myform')] - I don't even know how that'd work...

> #[Hook('hook_form_FORM_ID_alter', form: 'myform')]

I don't think your proposal makes it any easier to use, it's longer, and you still have to remember the pattern, which tends to be confusing as many forms have a "form" in their form_id (is it form_FORM_ID_alter() or FORM_ID_form_alter()...)

What we *could* add is something like a #[FormAlter('form_id')] or #[EntityHook(entity_type: 'node', hook: 'presave')], specific attribute classes that then build the specific hook name patterns that they know about. That could easily be a follow-up, the discovery already explicitly supports any Hook subclass (for example the already implemented AlterHook), here can be as many as we want, you could even add your own.

> #[Hook('hook_form_FORM_ID_alter', form: 'myform')]
> #[FormAlter('form_id')]

I'm not hung up on the specific syntax, the point was splitting out the variable part for the FORM_ID placeholder into a separate value.

Happy to leave it as a follow-up to keep this issue simple.

We have hook_form_BASE_FORM_ID_alter() too, so any special handling would need to also handle that.

We could take that one step further and turn every hook into its own attribute that extends the base hook class. In some ways that would be nice because you could just use #[Install] for hook_install, or #[Cron] for hook_cron, which removes the magic hook name string, but it would also be fairly annoying that every module would need to declare a whole bunch of hook attributes. Although, this could replace mymodule.api.php so maybe it has some merit.

Oh yes, we need to rethink MODULE.api.php as part of this issue. Otherwise it's not going to make sense.

mstrelan , that's a wonderful idea. Custom code could still just use #[Hook] for an undocumented one-off hook if needed for quick-and-dirty -- the ability to keep the zero fuss on either caller is implementation side is of extreme importance to me (indeed that's why I jumped on this). But clean code could just use create a custom attribute class and put the API documentation on the constructor. A somewhat small change to api.drupal.org, hopefully. I presume it would still all extend the Hook attribute just set the hook name to something relevant. We could use a class constant for that, so it would become ->invokeAll(Cron::HOOK) and #[Cron] on the implementation side which would be the exact same as >invokeAll('hook') and #[Hook('cron')]. I really like this idea.

Re form alters once we've stopped using procedural hooks we could just use the form class name as the form ID, and the ::class constant in hooks, I think? Yo start with this would only be for new forms but we could eventually deprecate form IDs and add some form of BC, maybe.

> So if you have a fixed hook then you could just create a attribute class and put the API documentation on the constructor. A somewhat small change to api.drupal.org, hopefully

Harder to discover by reading code if hook attributes are mixed in with other kinds of attribute in src/Attribute.

Oh, that's easy to solve, just put them in src/Attribute/Hook.

With the possibility of additional/contrib/custom hook classes, it might make sense to add HookInterface and make every relevant attribute class implement that. With the Interface definining some basic setters and getters (setHook(), getHook() etc..

Also, and I haven't thought through the complexities, I wonder if there are any advantages to making Hook attributes support multiple hooks, such as node_update + node_insert in one hook attribute, or form_node_form + form_media_form in one alter hook attribute?

So

#[Hook('node_update')]
#[Hook('node_insert'])]
public function doNodeStuff(...) {}

becomes

#[Hook(['node_update', 'node_insert'])]
public function doNodeStuff(...) {}

or

#[AlterHook('form_node_form')]
#[AlterHook('form_media_form')]
public function alterNodeOrMediaForm(...) {}

#[AlterHook(['form_node_form', 'form_media_form'])]
public function alterNodeOrMediaForm(...) {}

#59 feels like a complication we don't need for now, multiple attributes are supported and feel much cleaner to read.

Tagging for framework manager signoff as this is a significant addition to an existing API (or set of APIs depending on how you look at it)

I'm also not sure we need an interface if everything must descend from the base Hook class anyway.

I don't have any use cases in mind, but the idea for an interface would be to allow some degree of future proofing, in case the hook attribute class properties need amendment at some point.

It would also allow other hook attribute classes to do some logic in getters and setters instead of directly acting on property values.

Note that I don't feel strongly about this, but I thought I'd mention it in case.

#[FormAlter(FormTestAlterForm::FORMID)]

This would probably be better as #[FormAlter(FormTestAlterForm::FORM_ID)], but I like the idea of adding FormAlter.

I'm not sold on removing Hook from the attribute name. I like being able to see at a glance which attributes are for hooks. We could conceivably have other attributes on functions that aren't for hooks, and that would make it harder to see which ones are hooks.

While I did resurrect ModuleHandlerTest it is an extremely strongly coupled unit test which has assumptions about how the implementation exactly works. In particular, ::testInvokeAll presumes addModule followed by invokeAll loads the module while ::testLoadModule asserts addModule does not load the module. There is no rhyme or reason to presume addModule doesn't load the module and so now it does. The system proposed here can't work otherwise because module_implements_alter needs to be called build time and it would be inconsistent if we only loaded the modules implementing it. Since addModule is a very rare, install time only thing, I see no harm in this change.

I just rebased this so it applies.

I'm wondering if the merge request being in draft is intentional?

My understanding is that this is ready for a deeper review with the following comments that all need to be addressed.

1. A framework manager needs to review per 61.
2. The suggestion to change FormAlter(FormTestAlterForm::FORMID) to FormAlter(FormTestAlterForm::FORM_ID) per 63.
3. One question in the MR about handling the profile extension that I think may be addressed.
4. The question in the issue summary about whether additional tests are needed.

Let me know if I missed anything, I added these to the issue summary too.

Thanks for the meta review :)

1. It's in draft because I have no idea what that means. I am just writing patches, here (in two weeks it'll be twenty years). This new workflow is a complete unknown for me.
2. I talked with catch about a review , likely next week.
3. I will
4. Profile was addressed I thought?
5. That's part of #1 I think

Edit: sorry I didn't see the review by solideogloria I will process that too.

@ghost-of-drupal-past If you click on the Merge Request (and make sure you are signed in to GitLab; just click Log In and it will automatically auth you), there should be an option to mark the MR as "Ready". That will remove the Draft status.

In addition, there should be a way to mark each of my comments as "resolved", either now or after you have committed a change that addresses a comment.

For Drupal core MRs, I don't know that marking them as "draft" really matters much, but often on other projects, reviewers are accustomed to skip reviewing draft MRs/PRs, because it's a signifier of WIP product not ready for review or to merge.

FYI, if you are the MR author, you can move it out of draft by clicking on the kebab menu in the upper right and selecting "Mark as ready".

Yes. Also, only the author can select "Mark as ready", I think, so it would be a good idea to do so.

Thanks guys. As far as the profile question it is resolved, I was on my phone so I couldn't see the changes super accurately.

collectModuleHookImplementations takes care of it I think so I've removed that.

I also want to document the .theme discussion from slack here a bit clearer.

From godotislate:

you can implement hook_form_alter, hook_page_attachments_alter, among a few others in .theme
but that is via theme manager, not module handler
it looks like they are all invoked after module handler has its turn
so keeping them OOS seems fine

keeping them OOS

What does "OOS" stand for?

It's what godotislate said in slack, I assume it means Out Of Scope.

Note the FORMID constant was just a discussion in the issue, it's not found in the code yet. The attribution parameter is called form_id.

I marked the PR as non-draft.

I fixed the codesniff issues. I think that only runs once the MR is out of draft which is why we didn't catch it before.

I rebased to try to fix the unrelated failures.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

naughty bot

Thanks! I was about to say the same lol, tests are passing again.

I addressed the two comments on the MR that I could, there are a few comments that @Ghost of Drupal Past should address from @solideogloria.

Two questions about targeting classes: #[\Attribute(\Attribute::TARGET_CLASS | \Attribute::TARGET_METHOD | \Attribute::IS_REPEATABLE)]
And the regex here can module names have pipes | in them: $module_preg = '/^function (?(?' . implode('|', $modules) . ')_(?!preprocess_)(?[a-zA-Z0-9_\x80-\xff]+))\(/m';

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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'm rebasing

Ok I rebased this so it applies again.

@solideogloria's questions have been responded to on the MR.

Two remaining tasks are whether more tests are needed and a framework manager review.

Copying some relevant abridged discussion from slack if others think the class target is worth dropping:

chx (he/him)
if we dont want the class target, dropping it is really easy

chx (he/him)
10 hours ago
one could write the relevant part of getHookAttributesInClass like this to show it doesn't leak

<?
$class_implementations = array_map(function (\ReflectionAttribute $reflection_attribute) use ($class) {
$hook = $reflection_attribute->newInstance();
assert($hook instanceof Hook);
if (!$hook->method) {
if (method_exists($class, '__invoke')) {
$hook->setMethod('__invoke');
}
else {
throw new \LogicException("The Hook attribute for hook $hook->hook on class $class must specify a method.");
}
}
return $hook;
}, $reflection_class->getAttributes(Hook::class, \ReflectionAttribute::IS_INSTANCEOF));

@chx nor I can reproduce the config validation failures locally so any insight would be appreciated.

I pushed a small change which shows there's no need to use loadInclude now. I am not 100% on doing this but it'll go away anyways when node admin is converted to a class which seems inevitable.

Thanks for the discussion about this issue at Dev Days, I feel it was super productive and myself, @alexpott and @catch all agree that this is a great change and will unlock several further improvements in the future.

Added some minor feedback but overall the approach is great and I have no architectural concerns.

We probably also need to be clear that this doesn't apply to preprocess hooks, nor hook_update_N or post_update hooks. Unsure if there are any other special cases on top of these?

As hook_hook_info() is gone we should remove the docs and any implementations and write a separate change record for it? Or, should the HookCollector still use hook_hook_info() to learn what files to scan?

Re #94 - I think (at least this is a limitation of Hux) that no themeing hooks work at all, including hook_theme. Would be good to confirm that too.

Should the HookCollector still use hook_hook_info() to learn what files to scan? No, it shouldn't, it's not adequate, it's possible for modules/includes to include further files and those are hidden from us unless we the full scanning done here. We got our framework review, we have quite a few new things now from DevDaysBurgas, most importantly moving to a compiler pass and the introduction of LegacyHook so I think needs review is the best state right now. I will add a LegacyHook test ASAP.

Adding back tag, still on our radar

There are a few merge conflicts and some missing docblocks but otherwise to me this is ready to go - still needs framework manager signoff though.

Only did a rebase and some docs updates because of request in Slack. I did notice there are more methods that need documentation, that declare strict types hasn't been added and that not all method arguments have type hints. I can't help with that now.

Added strict_types on new files and rebased to head.

@Bhanu951 thank you for your contribution!

In the future I think rebasing is preferred over merging for bringing upstream changes into an MR.

Here is the documentation I usually follow: https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... → .

I've taken care of updating this MR with the most recent changes already so no need to rebase at the moment.

With 📌 Handling update path divergence between 11.x and 10.x Fixed committed this issue reached a breaking point.

If no other help is forthcoming I will won't fix this in a week.

Two things on the recent test failures:

1. I didn't think we were trying to support hooks in .install files (either in this issue, or even in a follow-up), so I am a bit confused why they are causing test failures, is it because .install files are using the new discovery even though oop hooks aren't supported?

2. The conditionally defined functions in update test modules are an extreme edge case and abuse of the update API, and I don't think we need the bc layer to handle them. Either we can use the old discovery for .install hooks, or find a different way to test the upgrade path.

I think it might be easier to go back to the old discovery for update (.install/post_update.php hooks, and only use the tokenizer temporarily), but there might be reasons to not do that I haven't thought of at all.

All MR comments except https://git.drupalcode.org/project/drupal/-/merge_requests/7604#note_332589 are resolved and I am not keen on resolving that one for reasons outlined there. catch already granted a BC break exception to this issue (conditionally defined hooks are not supported), hopefully longwave will grant another.

However, new first/last/before/after functionality just landed. I hope both the documentation and testing will be found adequate.

For both, I think needs review is the correct status at this point.

Given that the static helper is really just syntactic sugar, we could make it even cleaner by moving the before/after/first/last directly to the attribute?

So

class CkEditor5Hooks {
  #[FormAlter('filter_format_form')]
  public function formFilterFormatFormAlter() {}
}

class CkEditor5ServiceProvider extends ServiceProviderBase {
  public function alter(ContainerBuilder $container) {
    HookHelper::after($container, 'form_filter_format_form_alter', CkEditor5Hooks::class . '::formFilterFormatFormAlter', MediaHooks::class .  '::formFilterFormatFormAlter', EditorHooks::class . '::formFilterFormatFormAlter');
  }
}

becomes

class CkEditor5Hooks {
  #[FormAlter('filter_format_form'), after: [
    MediaHooks::class .  '::formFilterFormatFormAlter',
    EditorHooks::class . '::formFilterFormatFormAlter',
  ]]
  public function formFilterFormatFormAlter() {}
}

or for e.g. last

class CkEditor5Hooks {
  #[FormAlter('filter_format_form'), last: TRUE)]
  public function formFilterFormatFormAlter() {}
}

This doesn't solve the really awkward/dynamic cases which would still need the service provider, but the 90% use case is surely likely to be before/after/first/last and we can make it much easier to understand this way.

Separate attributes could also work, but feel a bit more unintuitive:

class CkEditor5Hooks {
  #[FormAlter('filter_format_form')]
  #[RunAfter([
    MediaHooks::class .  '::formFilterFormatFormAlter',
    EditorHooks::class . '::formFilterFormatFormAlter',
  ])]
  public function formFilterFormatFormAlter() {}
}

Personally I think this feels cleaner

class CkEditor5Hooks {
  #[FormAlter('filter_format_form', after: [
    MediaHooks::class .  '::formFilterFormatFormAlter',
    EditorHooks::class . '::formFilterFormatFormAlter',
  ])]
  public function formFilterFormatFormAlter() {}
}

I'm not sure I like:

class CkEditor5Hooks {
  #[FormAlter('filter_format_form')]
  #[RunAfter([
    MediaHooks::class .  '::formFilterFormatFormAlter',
    EditorHooks::class . '::formFilterFormatFormAlter',
  ])]
  public function formFilterFormatFormAlter() {}
}

I have not used attributes much beyond custom Drush commands so this isn't from a deep perspective, just from reading the comments the former feels clearer to me while the latter feels more disconnected as a separate attribute.

FWIW these before and after additions are essentially making hook implementations public API, so if we wanted to move MediaHooks::formFilterFormatFormAlter to another class, or rename it, we'd need to think about how to deprecate it.

@mstrelan that's a great point. Do we need to implement this stuff in the MVP of OOP hooks? Might be quite a bit of bike shedding for something that probably isn't even used in 90% of sites. I've heavily used Hux since its inception on some very complex sites and never needed before, after, or even reordering. I understand it's to match feature parity with module_implements_alter but do we have to have 100% feature parity for the first iteration?

OK, maybe #111 is a step too far for this first issue. Although I'm not sure how this is really making it API any more than it would be inside a service provider, it's the same end result with a different syntax.

I wasn't specifically talking about attributes, just the concept of before and after in general.

Is it safe to use class names like that in recent comments?

What if an extension declaring the hook is not dependent on those other extensions and classes may be missing?

Is it possible to use just extension names instead?

The ::class constant doesn't invoke the autoloader and it works even if the class doesn't exist.

php > print \Non\Existent\Code::class . '::someMethod';
Non\Existent\Code::someMethod

With OOP hooks an extension can implement the same hook more than once if they want to, so the extension name isn't enough on its own.

It also works with use https://3v4l.org/QBPtj

Moving it into the attribute instead of a service provider is an easy followup.

If we want to discuss further, moving HookHelper into a followup is a possibility too. It is not required at all, it's completely independent. Yes, using class names and methods is unfortunate but there's just no way around it as noted. But I imagine we could write another helper which gathers the implementation of a hook by a given module. Such a thing wouldn't even need to iterate the listeners, the hook_implementations_map parameter has the info necessary, it's not a lot of code. Most of the time the returned array would have 0 or 1 elements. I consider it a followup because of syntax, possible integration with before/after etc.

#[\Attribute(\Attribute::TARGET_CLASS | \Attribute::TARGET_METHOD | \Attribute::IS_REPEATABLE)]
class FormAlter extends Hook {

There is a core standard that attribute classes go in an Attribute namespace.

These are from the global PHP class
https://www.php.net/manual/en/language.attributes.classes.php

I think joachim asked Hook and FormAlter to live in Drupal\Core\Hook\Annotation instead of Drupal\Core\Hook and that seemed like a reasonable request -- every core attribute I can find is in an Annotation namespace indeed. I moved over and rebased on 11.x, The change record didn't have the FQCN, now it does.

Yup, that was it -- sorry for the unclear snippet. When we started adding attributes to core, we came up with a policy on where to put them -- 📌 [policy, no patch] Decide on a namespace for PHP Attribute classes Fixed .

> Oh yes, we need to rethink MODULE.api.php as part of this issue. Otherwise it's not going to make sense.

This appears to have been forgotten about.

We also need to figure out how to document the hook classes -- see my comments on the MR.

It would be great to get this in for 11.1, but it's frustrating when work on docs and standards is neglected, and then the fact that doing it would hold up an issue getting fixed is used as a reason to handwave it out of the way.

@joachim, do you have any specifics on what you think needs to change about MODULE.api.php?

Assigning to myself to take care of rebasing and addressing a lot of the comment feedback since it's pretty straightforward.

Rebasing took care of a bunch of failures, there is still one job failing.

I replied to a few comments for clarification and took care of all of the simple documentation comments tense etc.

The failures seem to be unrelated, each run they are different tests.

I reran the test and it passed.

I am done.

I'm taking another pass.

There is a comment mentioning that install hooks are not included. I am not sure what the exact makeup of install hooks are.

Initially I thought it was the following:

• hook_install()
• hook_install_tasks()
• hook_install_tasks_alter()
• hook_uninstall()
• hook_schema()
• hook_update_N()
• hook_requirements()

There is a comment in the IS that hook_requirements may not be included in the list.

Also there are a bunch more hooks in module.api.php. If someone can give insight into which hooks explicitly are excluded I can update docs.

I also identified the theme hooks that are not included in the CR.
The CR will need to be updated once we identify the excluded hooks.

As I was writing this @joachim found this: https://github.com/drupal-code-builder/drupal-code-builder/blob/b0fc30b5...

I'll update the cr and docs based on this list.

> @joachim, do you have any specifics on what you think needs to change about MODULE.api.php?

Well, EVERYTHING.

Because at the moment, those functions can be discovered from the 'Implements' docblocks. And also because documenting OO hooks as functions is going to look silly.

So what we need is a pathway from this code:

  #[Hook('user_cancel')]
  public function userCancelBlockUnpublish($edit, UserInterface $account, $method): void {

to the hook_user_cancel() function.

The simplest thing is to put in the the 'Implements hook_user_cancel()' line.

Here is the list of hooks I added documentation that will remain procedural.
* - hook_install()
* - hook_uninstall()
* - hook_schema()
* - hook_update_N()
* - hook_update_last_removed()
* - hook_requirements()

Let me know if there are others such as hook_requirements_alter, hook_install_tasks, or hook_install_tasks_alter

I've added some remaining tasks, three are documentation and one is from a comment on this thread.

@nicxvan what about hook_post_update_NAME?

@andypost, I don't know. I think the main question is what are the criteria for what an install hook is.

I think the true exclusion is hooks not processed by moduleHandler.

Those seem to be theme and install hooks.

If it includes all hooks in module.api.php then we're missing quite a few, but I don't have the answer.

I honestly don't understand why there is a distinction, but I suspect it's the following:

Included

core/lib/Drupal/Core/Extension/ModuleHandler.php

Not included

core/lib/Drupal/Core/Extension/ModuleInstaller.php
core/lib/Drupal/Core/Theme/Registry.php

Further investigation, I think anytime $this->invoke or $this->invokeAll is called in module installer would be excluded:

$this->invokeAll('module_preinstall', [$module, $sync_status]);
$this->invoke($module, 'update_last_removed')
$this->invoke($module, 'install', [$sync_status])
$this->invokeAll('modules_installed', [$modules_installed, $sync_status]);
$this->invokeAll('module_preuninstall', [$module, $sync_status]);
$this->invoke($module, 'uninstall', [$sync_status]);
$this->invokeAll('modules_uninstalled', [$module_list, $sync_status]);
$this->invokeAll('cache_flush');
// Called from installSchema
$tables = $this->invoke($module, 'schema') ?? [];
// Called from uninstallSchema
$tables = $this->invoke($module, 'schema') ?? [];

invokeAll just loops over hooks:
foreach ($this->moduleHandler->getModuleList() as $module => $extension) {
$this->invoke($module, $hook, $args);
}

Reading this list it looks like the new final list is:

• module_preinstall
• update_last_removed
• install
• modules_installed
• module_preuninstall
• uninstall
• modules_uninstalled
• schema

Question is do the alter versions of the listed hooks get excluded too?

E.g. we exclude hook_install, do we exclude hook_install_tasks and hook_install_tasks_alter. I don't think we do.

> @andypost, I don't know. I think the main question is what are the criteria for what an install hook is.

I assume it's because install hooks are invoked before the container can discover class-based hooks.

But that needs documenting.

@catch mentions this above:

1. I didn't think we were trying to support hooks in .install files (either in this issue, or even in a follow-up), so I am a bit confused why they are causing test failures, is it because .install files are using the new discovery even though oop hooks aren't supported?

I think I've taken care of most documentation other than the why install hooks are not included. I'll wait for someone else to weigh in before doing any more.

After digging further due to some questions in slack from @catch.

hook_update_N is part of the exclusion since it's called from the installer by the UpdateHookRegistry.

I don't think hook_post_update_NAME need to be excluded since this is part of the documentation:
* These updates are executed after all hook_update_N() implementations. At this
* stage Drupal is already fully repaired so you can use any API as you wish.

I've updated the comments for hook_update_N

Will API module work with hook methods in classes?

If it doesn't, moving node hooks to a class will break api.d.o.

Ok after discussion in Slack @ghost of drupal past clarified exclusions.

The following hooks will likely will never be OOP, there are significant barriers to converting.
hook_install
hook_uninstall
hook_update_N

The following hooks can be split into a procedural part and an OOP part, but is OOS for this issue:
hook_requirements

The following hooks are invoked by the module installer and will require a follow up to determine if they can be replaced.
In preliminary testing significant numbers of tests break when these are converted so they are most likely not able to be converted either.
hook_module_preinstall()
hook_module_preuninstall()
hook_modules_installed()
hook_modules_uninstalled()
hook_schema()
hook_update_last_removed()
hook_post_update_NAME()

Theme hooks are not affected by this MR.
hook_theme()
hook_theme_suggestion_HOOK()
hook_preprocess_hook()
hook_process_hook()
hook_theme_suggestions_HOOK_alter()

I will add post updates to the exclusion list.

Until hooks actually start getting replaced module.api.php does not need to be changed I think.

This just enables the replacement. Once hooks are replaced, the documention will be removed from module.api.php and the documentation will reside in the respective modules new hook attribute.

I am also testing: $theme_registry = $this->themeRegistry->getRuntime();

Ok I did a quick test and reverted the change:
$theme_registry = $this->themeRegistry->getRuntime(); to put it after where ti was before.

That causes several exceptions in kernal tests from this line:

// If called before all modules are loaded, we do not necessarily have a
// full theme registry to work with, and therefore cannot process the theme
// request properly. See also \Drupal\Core\Theme\Registry::get().
if (!$this->moduleHandler->isLoaded() && !defined('MAINTENANCE_MODE')) {
throw new \Exception('The theme implementations may not be rendered until all modules are loaded.');
}

It's not clear why running $theme_registry = $this->themeRegistry->getRuntime(); before the exception works but after does not. It requires some investigation.

Tests are passing and all comments have been addressed, I think this is ready for review again.

@nicxvan thanks for translating my somewhat cryptic questions from slack on my phone into actual docs and answers here.

Re #149 I think the 'install hooks' part of this we should never attempt to do a 1-1 conversion to OOP hooks and instead defer that to issues like #3167625: Deprecate/replace hook_update_N() in favor of an object-oriented approach similar to Laravel migrations → , 🌱 [meta] Replace hook_schema() implementations with ::ensureTableExists() Active , 📌 Move the on-demand-table creation into the database API Needs work etc. that will eventually replace those hooks with something else entirely. hook_update_N() and post updates we don't only have the issue of them being in the .install or post_update.php file but that the entire API relies on procedural function names (getting the schema number from post updates, storing all the post update function names in key/value). I guess we could add some kind of meta for that even if only as something to point to in case someone attempts to do the 1-1 conversion, like '[meta] Replace all remaining procedural-only hooks with something else' - and that can include allowing OOP implementations if/when we find out they're fine.

Theme hooks like preprocess, I think we should add a direct follow-up for this issue to try to allow OOP implementations, but that's going to be a whole complex task in itself so too much to try to do here.

Ghost of drupal past did some additional investigation into why the exception was being thrown when $this->themeRegistry->getRuntime(); was being called later.

I pushed his work from the test issue here: https://git.drupalcode.org/project/drupal/-/merge_requests/7604/diffs?co...

It does the following:

a) reverts ThemeManager to 11.x
b) removes the loadAll() from invoke() (yay!)
c) fixes the failing tests and any weird code which would call ThemeManager()->render() after a rebuildContainer() call while not in install.

It seems that Drupal was handling this situation differently in tests vs production:
It sets this: $this->setInstallProfile('standard');
Which does this: $this->container->get('kernel')->rebuildContainer();
Which resets the loadAll.

There is a follow up issue that can be created:

Investigate moving loadAll from preHandle and (new) rebuildContainer into initializeContainer (all this on DrupalKernel). This is somewhat tricky because there are a lot of calls to preHandle(). But initializeContainer() is called from boot() which is always (?) called before preHandle(). Given preHandle() uses $this->container I would say it is indeed mandatory to call boot() before it. So the issue would be to 1) add a note to preHandle() doxygen to call boot() before it 2) add a guard into preHandle , there's a $this->booted flag 3) move the loadAll() calls , both , into intializeContainer()

@catch, I agree with your assessment.

Setting needs review, I'll bump it back to needs work if tests don't pass.

I think the Hook attribute docs should look more like this:
I think the test hook section can be removed, but included it for review.

/**
 * Defines a Hook attribute object.
 *
 * This allows defining a hook implementation using attributes.
 * Hook implementations needs to be marked with a Hook attribute.
 * Either on a method #[Hook('user_cancel')] or on the class
 * #[Hook('user_cancel', method: 'UserCancel')].
 * If the class Hook doesn't specify a method, __invoke is the default method.
 *
 * The attribute supports a priority as well: #[Hook('user_cancel', priority: -20)].
 * The priority defaults to such values as to keep module order.
 *
 * Reordering hook implementation is done by manipulating the kernel listeners
 * in service alter providers.
 * A HookHelper providing setBefore/setAfter/setFirst/setLast/setPriority
 * functionality is included.
 *
 * Old style procedural calls are integrated into the new system.
 * See LegacyHook for additional information.
 *
 * These are automatically registered as autowired services with the
 * class name as the service id.
 * If autowire doesn't suffice -- should be very rare -- they can be
 * registered manually as well, the service id is the class name.
 *
 * The edge case of "implementing a hook on behalf of another module"
 * is also supported by specifying the module in the attribute.
 * It defaults to the defining module.
 *
 * Multiple implementations are allowed on multiple axis:
 * one method can implement multiple hooks by adding a Hook attribute for each.
 * One module can implement a hook as many times as it wants in as many
 * classes as it wants.
 * This allows, for example, adding form_alter implementations firing on
 * other conditions than form id without modifying any existing implementations.
 *
 * Exceptions:
 * Hooks fired via ModuleHandler::invoke
 * - library_build_info
 * - mail
 * - help
 *
 * Help expects a string return, it is not clear what multiple implementations
 * would mean in this case.
 *
 * Also, ModuleHandler::invoke is used as a replacement for a failure-tolerant
 * $function() call, it is not at all clear what multiple implementations
 * would mean.
 * - mass_update
 * - node_update_index
 * - field_views_data
 *
 * Test hooks:
 * - test_hook
 * - test1
 * - test2
 * - views_data
 * - views_data_alter
 * - views_query_substitutions
 * - views_form_substitutions
 * - views_analyze
 * - views_pre_view
 * - views_pre_build
 * - views_post_build
 * - views_pre_execute
 * - views_post_execute
 * - views_pre_render
 * - views_post_render
 * - views_query_alter
 * - views_invalidate_cache
 * - hook
 */

Ok, got a review in slack from @catch for the comment, see the MR for the final.

We cut the section about:

ModuleHandler::invoke is sometimes used as a replacement for a failure-tolerant $function() call.

This should be addressed in a follow-up.

The test hooks have been removed from the comment.

Defines a Hook attribute object.
 *
 * This allows defining a hook implementation using attributes.

Merge these into:

> Hook attribute for defining a class method hook implementation.

> Either on a method ...

Would be clearer as a bullet list of the 3 techniques.

> * is supported by specifying the module in the attribute.

A code example would be good.

> * It defaults to the defining module.

I think we can drop this.

> * See LegacyHook for additional information.

Classes in docs must be FQCN to make links. (Others too)

> * Old style procedural calls are integrated into the new system.

Don't say 'new'. Docs should not be time-based.

> * Multiple implementations are allowed on multiple axis.

I have no idea what this means. (and 'axes' plural)

> * Hooks that must remain procedural:

'must' and 'remain' -- wrong words to use here. Docs are not about 'doing' or 'changing', they are about what IS.

> * Exceptions to multiple implementations.

Needs a colon. Probably a heading, and nested lists. In general this needs a clean-up.

Which order does priority go in?

It follows Symfony's priority since it's using the event dispatcher I think from comment 8 https://www.drupal.org/project/drupal/issues/3442009#comment-15560211 📌 OOP hooks using event dispatcher Needs review .

So it's the opposite of weight.

Accidental status change.

What happens if you try to implement hook_install or hook_theme with a class method?
We should probably throw an exception for DX.

Rather than brute force finding ALL php files in collectModuleHookImplementations(), I think we should have kept hook_hook_info(), as that specifically tells us which PHP files to load.

Rather than brute force finding ALL php files in collectModuleHookImplementations(), I think we should have kept hook_hook_info(), as that specifically tells us which PHP files to load.

Don't think that would work.

The discovery uses StaticReflectionParser, which can't handle situations like a .module that does manual require_once of a file with a set of hooks in it (which used to be a very common pattern ten years ago, probably not so much now but who knows). I don't like having to look for procedural hooks everywhere either, but it's a bc layer we can eventually remove, so can live with it.

What happens if you try to implement hook_install or hook_theme with a class method?
We should probably throw an exception for DX.

This might be possible with a deny list in hook discovery?

I've added support for Module Builder for generating attribute-based hooks (with legacy hook implementations too).

To try it out, use the 4.0.x branch of Module Builder, and branch `feature-oo-hooks` of the drupal-code-builder/drupal-code-builder package.

I think the deny list might work for the install hooks:

    hook_install()
    hook_module_preinstall()
    hook_module_preuninstall()
    hook_modules_installed()
    hook_modules_uninstalled()
    hook_post_update_NAME()
    hook_schema()
    hook_uninstall()
    hook_update_last_removed()
    hook_update_N()

It's more complicated for theme hooks since themes can define their own hooks, but the standard ones are:
hook_theme()
hook_theme_suggestion_HOOK()
hook_preprocess_hook()
hook_process_hook()
hook_theme_suggestions_HOOK_alter()

> It's more complicated for theme hooks since themes can define their own hooks, but the standard ones are:

I don't think that complicates things, since themes can't define attribute hooks anyway.

The deny list would be for the attribute hook discovery to throw an exception if it finds a Hook('procedural_only_hook') attribute.

(oops, accidental status change)

I edited the list of theme hooks. hook_theme is a module hook and is covered by this change, I updated the comment. in the MR

I am going to attempt the deny list.

Ok I got the full list of status for hooks that need to remain procedural and need to remain procedural for this issue but could possibly be updated in a future update. I'm going to update the exception now too:

I'm going to do a direct comparison and regex for the update ones.
I will also update the docs and the CR

Cannot be OOP because the module namespace is not in the namespaces yet:
install
module_preinstall
schema

Excluded for this issue but may be OOP in a followup:
module_preuninstall
modules_installed
modules_uninstalled
requirements (note this can be split into install requirements which must be procedural and runtime requirements which can be OOP)
uninstall
Would take significant effort to convert:
update_last_removed
post_update_NAME
update_N
Likely simple followup for theme hooks
preprocess_hook (needs to be functional for now, but an easy followup: ModuleHandler needs to expose "hook exists" functionality to replace function_exists($prefix . '_preprocess') in Registry, not a biggie, the first line of invoke() is it)
process_hook (see preprocess, also _hook here is a "theme hook" which is not at all the same thing as a hook, see sec_theme_hooks in theme.api.php. Suggestion: use preprocess_HOOK and process_HOOK in this list to indicate it's a theme hook)
theme_suggestions_HOOK ($suggestions = $this->moduleHandler->invokeAll('theme_suggestions_' . $base_theme_hook, [$variables]); it's an ordinary hook made from the prefix theme_suggestions_ and a theme HOOK just to make sure everyone is confused)
theme_suggestions_HOOK_alter (that's an ordinary alter once again cooked from fixed strings and a theme HOOK)

I added the deny list and a refactor of invoke that was suggested by Ghost.

I paused writing the test because locally I'm getting a white screen with this fork so I'm going to investigate that for a bit.

I think there might be an issue in the following code:

if (!class_exists($class)) {
      return [];
    }
    $reflection_class = new \ReflectionClass($class);
    $class_implementations = [];
    foreach ($reflection_class->getAttributes(Hook::class, \ReflectionAttribute::IS_INSTANCEOF) as $reflection_attribute) {
      $hook = $reflection_attribute->newInstance();

I can't seem to figure out how to get the code in the foreach to run.

The @defgroup hooks section in core.api.php is going to need a major rewrite too.