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.