OOP hooks using attributes and 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?

>> 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. But private-by-default here would be a good starting point, to try and get people used to the fact that you can't just retrieve everything from the container (unless you explicitly inject it into something else).

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

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?

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

--

2. What I personally liked about Kris' approach was that the module calling had been converted to the new style as well.

Once we have hooks in the event_lister and drupal_hook.[name] is a good one, then we can indeed register modules like that as well.

I think it is worth it, because running first the class based hooks and then the module hooks might lead to weird side effects.

Also I am not seeing the event based hooks take care of module implements alter, but maybe I am missing something?

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 #2352351: Add before/after ordering to events, 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?

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 implementation it reminds me Action plugin, for example \Drupal\user\Plugin\Action\RemoveRoleUser

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.

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 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 I've gone ahead and postponed #3368812: Hux-style hooks, proof of concept 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 #2237831: Allow module services to specify hooks 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 #3383487: Add CronSubscriberInterface so that services can execute cron tasks directly 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.

This comment was not relevant, so I removed it. Move along.

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.

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.

With the possibility of additional/contrib/custom hook attribute classes, it might make sense to add HookInterface and make every relevant attribute class implement that. With the Interface defining 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.

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

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.

(I might be chasing ghosts because of having to deal with provider vs multiple providers in migrate source plugin attributes.)

#[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 distinguish at a glance which ones are hooks.

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.

@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".

ETA. Oops, what @solideogloria just said.

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.

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 (?<function>(?<module>' . implode('|', $modules) . ')_(?!preprocess_)(?<hook>[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.

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.

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 synced from 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.

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.

In general, I don't think we should be supporting conditionally defined hooks in general - I've only ever seen this in the depths of update path testing, not even in the weirdest production code, if someone is out there doing that, then congratulations we just made it even more conditional.

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 using named arguments?

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() {}
}

before and after could take string or array of strings to make the syntax work easily for one or multiple other implementations.

Similarly for first/last:

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

This doesn't solve any awkward, dynamic, or on-behalf-of-other-module 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.

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

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 -- #3344303: [policy, no patch] Decide on a namespace for PHP Attribute classes.

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

I'm also not sure how the various documentation standards issues with the MR aren't getting picked up by Coder.

@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'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.
Note, list is updated below
* - 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:

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

I think these two can be removed:
hook_update_N
hook_requirements

and to answer Andy, I'm sure hook_post_update_NAME is not excluded either.

Checking in Registry:

$this->moduleHandler->invokeAllWith('theme', function (callable $callback, string $module) use (&$cache) {
        $this->processExtension($cache, $module, 'module', $module, $this->moduleList->getPath($module));
      });

if ($type === 'module') {
      $result = $this->moduleHandler->invoke($name, 'theme', $args);
    }
    else {
      $function = $name . '_theme';
      if (function_exists($function)) {
        $result = $function(... $args);
      }
    }

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.

Also to add to the confusion:
This hook should be implemented in a .module file, not in an .install file. is a comment on function hook_modules_installed.

But I think the issue is more where the hook is executed than where it is defined.

> @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 it 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.');
    }

Theme register calls invoke which loads all modules so the exception is likely no longer needed at all.

I added a comment and am testing the removal.

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, #3221051: [meta] Replace hook_schema() implementations with ::ensureTableExists(), #2371709: [PP-x] Move the on-demand-table creation into the database API 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 values that preserve 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 does not 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 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 (With some exceptions below).
 * This allows, for example, adding form_alter implementations firing on
 * other conditions than form id without modifying any existing implementations.
 *
 * Exceptions to multiple implementations.
 * 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.
 *
 * Hooks that must remain procedural:
 * 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()
 *
 * Theme hooks
 * - hook_theme()
 * - hook_theme_suggestion_HOOK()
 * - hook_preprocess_hook()
 * - hook_process_hook()
 * - hook_theme_suggestions_HOOK_alter()
 *
 * ModuleHandler::invoke is sometimes used as a replacement for a failure-tolerant
 * $function() call.
 * It is not 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.

Priority goes highest to lowest.
So it's the opposite of weight.

From Symfony:

There is an optional attribute for the kernel.event_listener tag called priority, which is a positive or negative integer that defaults to 0 and it controls the order in which listeners are executed (the higher the number, the earlier a listener is executed). This is useful when you need to guarantee that one listener is executed before another. The priorities of the internal Symfony listeners usually range from -256 to 256 but your own listeners can use any positive or negative integer.
https://symfony.com/doc/current/event_dispatcher.html

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 to 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_requirements()
    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_suggestion_HOOK()
    hook_preprocess_hook()
    hook_process_hook()
    hook_theme_suggestions_HOOK_alter()

Edited to remove hook_theme() since that is covered by Attributes.
Registry explicitly changes to moduleHandler: $this->moduleHandler->invoke($name, 'theme', $args);

> 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: (POC below)
install
module_preinstall
schema
update_last_removed

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

These are NOT excluded
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.
False alarm, my local environment was just broken. Reinstalling fixed it.

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.

I tried manually creating an install attribute hook since I was having trouble getting the test to work as expected.

That didn't create the exception expected either. So I began moving the exception up further in the chain. It didn't start throwing the exception until it got outside of the foreach.

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

Ghost may have figured out how to make install hooks OOP, here is a poc for the eventual followup: https://git.drupalcode.org/project/drupal/-/merge_requests/7620/diffs?co...

@joachim, for now that probably just needs a reference to the hooks attribute docs, that won't have to be fully rewritten until procedural hooks are deprecated / removed.

> for now that probably just needs a reference to the hooks attribute doc

If we want people to implement OO hooks, then it needs to be rewritten to say that's the new official way of doing it, and procedural hooks are for a small subset of hooks and OO.

Could use to followup to upgrade status bot to notify and convince a rector rule to apply to existing code for contrib at least

@andypost yep, and the rector rule is already written!

Got a fix for the the exclusion and a test I'll be pushing up shortly.

I'll take a quick pass at those docs too after that.

Ok I've added the deny list and took a pass at the core docs implementation section.

The previous run of the tests passed and this is just a documentation change so I'm setting needs review, but I'll keep an eye on it.

I'm happy to make the change once there is consensus.

This issue really wants to sneak in status and assignment changes...

Echoing what I said back in #37 this is awesome to see this issue get so close, I really hope we can get to a state with the documentation that we can move some things to follow ups (that could block next minor) and get this thing in. Kudos to @nicxvan for the incredible effort recently!

I would love to test this on some of our projects that heavily use Hux and see what the migration path would look like. I'm hoping it's as simple as a namespace change on the attributes (and probably a class name change). I don't think that'll be possible though until the sites are on Drupal 11.

@acbramley, I've not used HUX but looking at the syntax, it looks like it's a matter of matching the new Hook attribute unless I'm missing something, but it definitely needs testing.

On another note, I got excited and I created a follow up for converting core hook implementations.
Naive estimate is there are 1382 implementations over 283 hooks.

Obviously not all hooks are implemented and not all implementations can be converted, but that gives scope once this is in.

I know it's premature to flesh out that plan, but we have a place for it.

Thankfully there is a rector rule to help out already.

Reading through the CR, I wonder why we recommend creating a service for compatibility with older Drupal versions. What prevents usage of \Drupal::classResolver(), which is already the recommended way to call OOP implementations from hooks and does not require creating a service for each hook (class), thus providing better DX?

Okay, answering my own question here, I guess it is about the autowiring part. I can see how creating a service is then easier, if you don't have an existing implementation of a "hook class" already. Probably also more fool proof to ensure that the autowiring actually works later even without a create method. I just shouldn't post to issues this early in the morning... Rock on and sorry for the noise.

@feyp with ClassResolver, you need to add a create() method if you want DI, so that's rather more boilerplate code to write.

> Perhaps change the classname instead, HookHelper is more of a development artefact than anything else. HookOrder for example.

+1 to HookOrder. Clearer than 'helper'

> setAsFirst perhaps

+1 to that too.

Gonna get some missing renames

That should be good.

> To clarify what happened here, after the doxygen for this patch got absurd level of scrutiny far above what anyone else gets

I file a LOT of documentation bugs, and a good portion of them are nitpicks. You can check: I tag them as 'Novice' if they're really simple nitpicks.

But I admit I did give the docs for this more scrutiny than other issues, though I didn't realise at the time. There is a reason though, and it's not personal at all. Unlike the Variation Cache issue (which is about something I barely understand) I needed to understand this issue's code, and RIGHT NOW. Because I wanted Module Builder to be able to generate code for the new style of hooks as soon as this is committed to core.

So I was reading MR and its docs to try to understand how to use it, and immediately spotting lots of gaps in the docs.

And then because as I was working on Module Builder, I was generating hook class code that was trying to match the NodeHooks class in the MR here, I was looking at the code as a new developer would, and spotting issues there too. For example that without the "Implements ..." docblock on the method, it won't join up to the documentation.

> When the documentation gate is rewritten to be about developers instead of the API module

The API module is for developers. It's important that documentation is to be written in a way that can be parsed, just as PHP code has to be written so PHP can parse it.

And this issue is adding completely new pieces of code structure. Variation cache added some classes and services, and API module can already handle those. Here we're adding new stuff and we need to get it right.

And as I have repeatedly pointed out on this issue, if we commit this without a corresponding update in the API module, then we will be BREAKING the API site.

Ah I see now, there is an issue for the api project #3471763: API-docs support for OO hooks implementations in classes.

In my opinion that issue is not a blocker here since it won't be effected until we start converting implementations on a RELEASED version of Drupal.

There is one implementation converted as part of this so work can begin on that issue for testing.

Re #199 please stop spreading this lie:

AccessPolicyInterface.php doesn't explain what's a $scope or a policy

You disrespectfully complained about it before on Slack and when I pointed out that you had failed to read the official documentation, you deleted the thread. As I mentioned in said thread:

Access policies are considered internal code, so their in-code documentation is limited. The AccessPolicyProcessor, however, goes into great detail in the code comments.

If you want to stop working on something for whatever reason, that's your prerogative, but don't drag other people's work into it.

On the subject of doxygen, I do agree that some aspects of core are more documented in code and others are more documented on the website. If you want to see that changed, you could create (or find) an issue for it, link to it here and then use that as a starting point to figure out whether or not this issue's MR should already adhere to what's being discussed.

Again, if you'd rather not have that discussion and stop working on this MR altogether because of it, that's also fine. No-one is forcing anyone to work on something, we're mostly all volunteers here.

Thanks everyone for working on this issue!

No need to be personal. Last comments seem to be a bit on the edge, so please focus on finishing the issue, so that we all can benefit from it. Thanks!

> got even more absurd demands, this time to provide documentation about a subsystem I have not been involved with for more than a decade.

I assure you was not at all what I meant, though I can see that it could have been misread like that. I could have phrased it more clearly, and I'm sorry for the misunderstanding.

What I was trying to say is that FormAPI is one example (of many!!!) where there is knowledge about how the system works which is ONLY in the heads of a small number of people, and not written down in the docs. This is bad because some developers don't know who to ask and give up, some developers waste time doing code archaeology, and some developers bother the small number of people for answers.

I used this example because just the other week I was asking about FormAPI innards and you responded on Slack. The point I was trying to make was that getting the docs right is important because this is a situation we should avoid creating more of.

> According to joachim, I am a diva

I try hard to criticise *behaviour* rather than *people*. If I failed in that, I'm sorry.

Would be great to get it in before Barcelona'24 to polish follow-ups at sprints

For high level documentation discussion, see #3463660: [policy, no patch] Document high-level API concepts in an easier format which is specifically trying to deal with the mismatch between doxygen/api.drupal.org and d.o - I think that's the issue to have that discussion in.

I'll try to review this again soon, it would be nice to reserve the remaining comments for reviews and not docs meta discussion since we are already over 200 here.

I rebased for the conflict, it was a performance test conflict.

I will keep an eye on the tests.

There are a couple of suggestions from #3471763: API-docs support for OO hooks implementations in classes

Suggestion 1:
Update the constructor for the hook attribute to assign:

$this->hook = static::PREFIX . $hook . static::POSTFIX;

This will allow extending it to be cleaner.

Suggestion 2:
Remove FormAlter or rename FormAlter to something more resembles what it really is: hook_form_FORM_ID_alter.

I'm making change 1, then renaming FormAlter to FormAlterId then overriding the prefix and postfix.
FormAlter and FormAlterId both need some work I think, but they are up for discussion.

I then implemented FormAlter.

Let me know if something should change.

I think the downside of this approach is the hook parameter in the constructor is not overridden so I left that in. Let me know if there are any opinions on direction. I think this is likely the same problem that is highlighted in the mentioned issue.

I can see a future where we use form classes instead of ids, so I'd rather not use FormAlterId. Plus it sounds like you're altering the id rather than the form. Would be nice to just pass the relevant form class to the form alter hook, and it can alter any subclass.

I agree. This probably should move to a follow up to be honest, but what about something like

FormAlter
BaseFormAlter
SpecificFormAlter

To match:

hook_form_alter
hook_form_BASE_FORM_ID_alter
hook_form_FORM_ID_alter

I'm inclined to postpone this to a follow up.

Edited to add, for the hook discovery we need to pass the form id for this implementation so that it can find: hook_form_FORM_ID_alter and hook_form_BASE_FORM_ID_alter.
So we need to decide do we want formAlter to pass the full hook name?
#[FormAlter('form_FORM_ID_alter')]
or the extra work we did so that
#[FormAlter('FORM_ID')]
is converted to a call to hook_form_FORM_ID_alter.

I'm tempted to say we should remove it entirely and leave them all in the hook attribute for now.
so hook_form_alter becomes:
#[Hook('form_alter')]

hook_form_node_edit_alter becomes:
#[Hook('form_node_edit_alter')]

This is less clean, but it's more consistent for the first implementation of the Attribute based implementations.
I vote for removing FormAlter and FormAlterId from the scope of this issue. The extended hook attribute is just a convenience. It complicates the interim documentation and naming as well.

Also your note for passing in the form class:
That's definitely a followup but the long term goal.

We could also name if FormAlterById

Ok I think I figured out a clean way to do this.

I added an IMPLEMENTS constant we can use in the documentation issue.

I also updated FormAlter so it can only implement hook_form_alter.
I also updated FormAlterById so that it takes the form id as a parameter.

I went through this again and it's incredibly compact given what it does. Obviously we have all the conversions to follow later!

Mostly only had comment nits and +1s.

I want to separately open an issue to look into creating a filecache-backed class attribute getter - i.e. given a file, get the attributes/an attribute from the file - but where we could use filecache to only actually do the reflection work once. However that issue is orthogonal to this one (and also to annotation/attribute conversion), we can work on all this in parallel. However I'll link that issue here once I've actually opened it.

I'll address the comments, most are pretty straightforward!

I've addressed @catch's comments.

Leaving in Needs Review

Added a question about the form alter hook attributes.

Also the change record says the API is not yet fixed, but it should match what's in the MR in order for this to be RTBC - I think it's pretty close depending on the answer to the above question.

Let me know if that clarifies things, essentially when exploring #3471763: API-docs support for OO hooks implementations in classes it became clear the IMPLEMENTS was useful which kind of implies needing just one hook per Attribute.

If this is complicating things I can drop FormAlter and move that to a follow up issue.

FormAlterById only handles the one hook.
I added some more information in another comment in gitlab.

Edited to add: I updated the CR so it should be up to date and ready.

Took another look at the CR again. Removing the tag now that I think it's ready, please let me know if it needs additional work.

After discussing on slack I've removed both FormAlter and FormAlterById. They require further discussion on naming, I'll create a follow up.

I also removed IMPLEMENTS that requires follow up on: #3471763: API-docs support for OO hooks implementations in classes

I've updated the CR.

Follow up has been created for FormAlter Implementation #3479141: Implement FormAlter attribute

Thanks, I think this is all the contentious parts removed and this is ready to land in 11.x - we can expand on this feature in followups.

The new phpstan return type rule is catching out some of the new code, needs those sorting out (hopefully straightforward).

Otherwise agreed this looks ready to go.

Saving (possibly imperfect, but not an easy one to guage) issue credit for now.

I got it

This is ready for review again!

Thanks - changes are minimal, back to RTBC.

This seems to have spawned from #3366083: [META] Hooks via attributes on service methods (hux style), do we want to close that one and credit the contributors here?

Marked #2237831: Allow module services to specify hooks as duplicate and transferred some credit (probably imperfectly, very old and long issue) over.

Also moved #3366083: [META] Hooks via attributes on service methods (hux style) to fixed.

removing credit as requested.

Couple of minor things

All of your questions have been replied to, please let us know if you need anything else.

All comments have been addressed.

All the answers to @larowlan's questions look reasonable to me, the one contentious thing is the module handler bc dance, but given the chances of someone overriding it are minimal, and that if they are, the bc dance is very unlikely to help them fix their code, we're better finding out sooner than later if someone really is doing that - so should leave things like they are in the MR.

Retrospectively crediting @alexpott for some discussions at devdays.

Really, really please to be able to commit this one.

Committed/pushed to 11.x, thanks!

@berdir asked about a 10.4.x backport, answered in slack but writing it up here in case other people have the same question.

In principle, we could backport this to 10.4.x, however it was not easy to get this into 11.x, and if we've somehow missed something which will affect contrib modules, it would be more disruptive if we cause a 10.4.x regression than an 11.1.x regression still (since there are a lot of sites on <= 10.3.x that are likely to want to update to 10.4.x before they update to Drupal 11). There are several follow-ups open and it is probably going to be more useful to spend the time here.

More importantly, contrib and custom modules can start using this with full 10.x compatibility now via the LegacyHook attribute, which allows the procedural and service hooks to co-exist. Only the service hook will be called in >=11.1, and only the procedural hook will be called in <=11.0.x.

This means modules can begin the conversion (hopefully using rector), and while you can't rm -rf example.module that will be the only step when dropping 10.x compatibility, and the only thing in it will be thin wrappers around the OOP hooks. And it's possible literally today, with full compatibility back to 10.2.x and earlier, not only when 10.3.x support is dropped in 9 months which would be the case if using the new API was dependent on a 10.4.x backport.

So by doing it this way, the legacy hooks will need to stay around until 10.x support is completely dropped, but all the other work can happen sooner.

Actually..
I have concerns about the order of legacy hooks.
(nothing new, but I changed my mind about whether we should care)

The order imposed by this change puts all OOP implementations first, and legacy/procedural implementations second.

This means:
If you want your implementation to run last, after all other implementations from contrib, you need to make it procedural.
As a contrib author, to be maximally compatible, you still need to follow this advice even if a lot of contrib has converted to OOP.

An alternative could be to have a dedicated event listener for the procedural hooks, with priority 0.
This way, OOP hooks can run before or after all procedural hooks.

Why should we care?
- Cost of upgrading major versions can cause projects to jump ship.
- I have seen more cases where order of hook implementations was important.
- A lot of contrib and custom modules will keep their procedural hooks for a long time.

(My other solution attempted to address this problem, but was arguably overengineered.)

An alternative could be to have a dedicated event listener for the procedural hooks, with priority 0.
This way, OOP hooks can run before or after all procedural hooks.

With this, we still disrupt contrib modules that rely on hook_module_implements_alter() to run first.
But we no longer force modules to use procedural implementations if they want to run last.
The former group of modules / use cases will be smaller.

Retrospectively applying some credit from #2551893: Add events for matching entity hooks which was a very different direction but a lot of these 5-10 year old attempts led to the nice solution here.

@donquixote I think the problem with legacy hooks first, is that all of the existing modules that use hook_module_implements_alter() to run last, are currently using procedural hooks. If we convert core to use all OOP hooks for 11.1 (TBD but assuming we do), then a module that uses hook_module_implements_alter() to run after a core hook, will suddenly not run after it, but instead run before - which is potentially very disruptive and could break a lot of contrib and site-specific modules.

It's worth an issue to discuss, but for me the limitation here seems the right way around.

If a module really, really wants to run last even against unknown modules which might also possibly implement the same hook, they could do every part of the conversion to a service hook except omitting the attributes, and then add the attributes when they're more confident that no modules (or few enough not to worry about) are implementing procedural hooks any more.

I opened #3481773: [Discussion] Should Legacy procedural hooks run with a Priority 0 for the discussion about order of execution.

@catch

I think the problem with legacy hooks first, is that all of the existing modules that use hook_module_implements_alter() to run last, are currently using procedural hooks.

Actually I don't advertise for legacy hooks first.
Just for the possibility to have some new hooks run last.
So maybe we should set the priority of legacy hooks to -1, not 0, if that achieves the goal.
Then you can set yours to -2 or lower to have it run after the legacy hooks.

Actually, I think I made noise for nothing.

If I understand correctly now, procedural implementations are added as listeners with prio like -1, -2, -3 etc.
So you can always set a prio of "ridiculously negative" like -9999, to make sure it will run last, after the procedural hooks.
So, apologies for the distraction.

This makes sense, even though the discovery is different, they are all listeners, so they can all be ordered independently of each other, but I'd definitely forgotten this detail in between wherever it was in the 250 comments on this issue and the eventual commit, so good to confirm.

Congrats to everyone that worked hard to get this over the line. This is so exciting!!!

It's great to see this get committed. Could someone please verify the "Backwards-compatible Hook implementation for Drupal versions from 10.1 to 11.0" section of the change record? I tried to implement this in a contrib module I maintain and I'm encountering some trouble.

A few points of concern:

• When I implement the Hook class as a service as described in the CR, I get a fatal error in Drupal 10.3 when enabling the module: Fatal error: Cannot declare class PublicKeyCredentialSourceHooks, because the name is already in use in \/var\/www\/html\/web\/modules\/contrib\/public_key_credential_source\/src\/Hook\/PublicKeyCredentialSourceHooks.php on line 13.
• The LegacyHook example does not return the service function-- that's a mistake, right? Because unless you return the service function, hooks that return a value won't be given a value to return.
• The example code will produce phpstan warnings using the GitLab CI template on Drupal 10.3 (e.g., Attribute class Drupal\Core\Hook\Attribute\LegacyHook does not exist). The recommended approach to fixing such warnings should be described.
• The example uses the node module, but since this is to be implemented by contrib, I think it would be better to use a generic contrib module like MyModule for the example.

Class already defined errors typically happen when the namespace is wrong, at least for plugins , check that you have that correct.

If a hook does return a value, then yes you need to pass it through. hook_user_cancel()) does not.

Yes, phpstan warnings will need to be ignored, it will also complain about the Hook attribute.

When I implement the Hook class as a service as described in the CR, I get a fatal error in Drupal 10.3 when enabling the module: Fatal error: Cannot declare class PublicKeyCredentialSourceHooks, because the name is already in use in \/var\/www\/html\/web\/modules\/contrib\/public_key_credential_source\/src\/Hook\/PublicKeyCredentialSourceHooks.php on line 13.

Was this a copy and paste issue or something else?

The LegacyHook example does not return the service function-- that's a mistake, right? Because unless you return the service function, hooks that return a value won't be given a value to return.

A lot of hooks don't have a return value, but it would probably be better if the example did return what it gets from the service method for the ones that do, since that's the sort of thing people could run into trouble with if they don't realise.

The example code will produce phpstan warnings using the GitLab CI template on Drupal 10.3 (e.g., Attribute class Drupal\Core\Hook\Attribute\LegacyHook does not exist). The recommended approach to fixing such warnings should be described.

Is that from PHPStan?

We might want to backport just the attribute class so that this doesn't come up maybe. Re-opening to discuss that last bit.

Thank you @berdir and @catch. For reference, here are the phpstan warnings for the backwards-compatible implementation of a single hook as described in the change record:

  Line   public_key_credential_source.module                                    
 ------ ----------------------------------------------------------------------- 
  18     Attribute class Drupal\Core\Hook\Attribute\LegacyHook does not exist.  
  20     Cannot call method theme() on mixed.                                   
 ------ ----------------------------------------------------------------------- 
 ------ ----------------------------------------------------------------- 
  Line   src/Hook/PublicKeyCredentialSourceHooks.php                      
 ------ ----------------------------------------------------------------- 
  15     Attribute class Drupal\Core\Hook\Attribute\Hook does not exist.  

"Cannot call theme() on mixed" can be fixed with a /** var**/ declaration I think, but it would be nice if the "Hook does not exist" and "LegacyHook does not exist" could be addressed somehow. In addition to backporting, another option would be to update the GitLab CI global phpstan.neon with ignores for these two specific lines.

Ok, I got my implementation working (I was missing the namespace statement-- I guess this is a "not thinking during copy-and-paste" failure).

I suggest making the following edits to the example code:

• Return the \Drupal::service() call.
• Add the use statement to the LegacyHook example.
• Add the namespace declaration to the NodeHooks class.

I know anyone can edit the CR (I already made some changes), but since this code is likely to be referenced a lot, I think the people who worked on this MR should make the decision.

The end result would look like this:

node.module

use Drupal\node\Hook\NodeHooks;

#[LegacyHook]
function node_user_cancel($edit, UserInterface $account, $method) {
  return \Drupal::service(NodeHooks::class)->userCancel($edit, $account, $method);
}

node.services.yml

services:
  Drupal\node\Hook\Nodehooks:
    class: Drupal\node\Hook\Nodehooks
    autowire: true

src/Hook/Nodehooks.php

namespace Drupal\node\Hook;

class NodeHooks {

  #[Hook('user_cancel')]
  public function userCancel($edit, UserInterface $account, $method) {
     // User cancellation processing.
  }

Just a quick note we've been iterating on the rector quite a bit it now handles returns.

Services are not needed for core.
Legacy hook is not needed for core.

We still need to handle the disallow list in rector.

The missing attribute is a good point for lower than 11.1 those two attributes likely need backport.

I'll look at those cr changes if someone doesn't update it first.

Might be worth clarifying you only need to return if the hook expects a return.

I've made a new release of Drupal Code Builder, which adds support for generating OO hooks. It can generate legacy hook support as well.

Great to see this get in. Thanks to everyone involved!

This somehow broke tests on a custom module. Cannot reproduce it locally, but CI output is full of such errors.

Notice: file_get_contents(): Read of 12288 bytes failed with errno=21 Is a directory in web/core/lib/Drupal/Component/Annotation/Doctrine/StaticReflectionParser.php on line 184

After some research I found a contributed module that faced same issue.
#3482165: phpunit (next minor) failing

@chi are your tests against the next minor version too?

@nicxvan yes.

Stack-trace looks like follows

(
    [0] => Array
        (
            [file] => /web/core/lib/Drupal/Component/Annotation/Doctrine/StaticReflectionParser.php
            [line] => 332
            [function] => parse
            [class] => Drupal\Component\Annotation\Doctrine\StaticReflectionParser
            [type] => ->
        )
    [1] => Array
        (
            [file] => /web/core/lib/Drupal/Core/Hook/HookCollectorPass.php
            [line] => 164
            [function] => getMethodAttributes
            [class] => Drupal\Component\Annotation\Doctrine\StaticReflectionParser
            [type] => ->
        )
    [2] => Array
        (
            [file] => /web/core/lib/Drupal/Core/Hook/HookCollectorPass.php
            [line] => 122
            [function] => collectModuleHookImplementations
            [class] => Drupal\Core\Hook\HookCollectorPass
            [type] => ->
        )
    [3] => Array
        (
            [file] => /web/core/lib/Drupal/Core/Hook/HookCollectorPass.php
            [line] => 74
            [function] => collectAllHookImplementations
            [class] => Drupal\Core\Hook\HookCollectorPass
            [type] => ::
        )

Same for me as in #275

@claudiu.cristea can you share where?

It happens here https://git.drupalcode.org/project/unpublished_file/-/jobs/3102786 with PHP 8.3/11.x-dev (1b55373, which is 1 commit after c05f9be). The error is slightly different (the number of bytes differ):

Notice: file_get_contents(): Read of 8392 bytes failed with errno=21 Is a directory in /builds/project/unpublished_file/web/core/lib/Drupal/Component/Annotation/Doctrine/StaticReflectionParser.php on line 184

I can't find the error stack

Tracking the issue here: #3482283: Symlinking a module breaks HookCollectorPass