[meta] Clean up hook classes in core

In 📌 Split File hooks into separate classes Active I split out hooks based on common dependencies, and kept those with no dependencies in the FileHooks class.

Certain hook groups should be in a class together.

I would force using single class hooks wherever it is possible. There very few use cases when hooks are highly related to each other.

Cross posted from 📌 OOP hooks using event dispatcher Needs review . The "implements" seems redundant now.

/**
 * Implements hook_help().
 */
#[Hook('help')]
Do you think that "implements" note is redundant for this kind of hooks?

It gets worse in single hook classes.

/**
 * Implements hook_help().
 */
#[Hook('help')]
final class Help {

  /**
   * Implements hook_help().
   */
  public function __invoke(): void {

  }

}

Thank you for commenting.

Re 6 I think there are a bunch, especially if you have a shared helper method.

For 7, I agree, but what would you suggest for comments then? A description of what it is doing?

@nicxvan we probably need to understand why "Implements hook_*" comment was initially added to Drupal coding standars. I guess it is needed to distinguish ordinary PHP functions from functions that implement Drupal hooks. Obviously with the Hook attribute is no longer needed. Also api.drupal.org might use this notation when listing hook implementatations.

For me it seems that in most cases docblock in hooks is not needed at all.

This issue is very similar to 📌 Allow omitting doxygen when type and variable name is enough Active . Maybe we should take same approach once it is resolved.

It's so the api project can find them and document implementations, there is an issue to replace that.

I do think that it is useful to split hooks into common groups and IMHO, it's pretty often that multiple hooks are related, for example modules might implement multiple entity CRUD or access hooks. Some hooks like tokens by design come in a pair.

In the original issue, I proposed to split by hook group, which translates fairly well into the first part of a given hook.

From the meta issue to convert core:

I think the main topic where I'd love to see some improvements is class grouping. I can see that it is capable for splitting it up into classes, but currently it's 1:1 by file. I don't know how complicated that is, especially since the code isn't in order, but I think it would be great if we could from the start adopt some consistent patterns that would make it much easier to see at a glance what kind of hooks a module implements and where they are, like plugins. This is very unlikely to happen later if we don't do it from the start I think. At first, I thought we could have a few specific rules, like FormHooks, and EntityHooks. But what if we just generalize that into prefixing it based on the first word of the hook name? This won't work so great for modules that consist of multiple parts, like hooks for content_moderation and content_translation would both go into ContentHooks. Not perfect, but also not completely wrong?

User module is currently mostly UserHooks, with 18 functions and 370 lines of code. With my suggestion, we'd have:

HelpHooks
ThemeHooks
JsHooks
EntityHooks
UserHooks
TemplateHooks
MailHooks
ElementHooks
ToolbarHooks
FormHooks
FilterHooks

(+ the already created UserTokensHooks and 2 views classes)

I think one of the biggest advantages of the plugin system is that the grouping/namespacing allows you to very quickly get an overview of what kind of plugins a module provides. Using such a grouping of Hooks would IMHO have a similar benefit. Doing single hook classes means we have a lot more classes, to the point that scanning them isn't quite as efficient anymore. We can still say that standalone hooks that we don't expect to have multiple like cron could be a class with a single __invoke() method.

I agree, it's also useful to have less code loading for hooks when invoked too.

It would be good to open a new coding standards issue for removing the Implements doc - I agree that looks redundant with the hook attribute.

Certain hook groups should be in a class together.

While it could be a good idea for simple websites an contrib modules, IMHO it's a bad one for complex sites with a lot of custom code.

If one class handle all the form hooks or entities actions hooks where you might have to use multiple different services, we'll reach soon or later some CouplingBetweenObjects and ExcessiveClassComplexity errors.

There are several coding standard issues about hooks

#983268: Use @implements Doxygen directive for hook implementations →
#3004139: Define form alter hook docblock standard →
#782016: Need doxygen standards for overriding a theme hook/template →
#1663218: Allow hook_update_N() doc block to contain details that are not displayed to the user →

Maybe a meta issue is needed to discuss the 'big picture' and the ones above can be children.

@macsim / #14
Yes, but in that case I would imagine it would be broken up across multiple custom modules. And you could break them up across multiple classes.

Nothing is intended to be enforced here beyond maybe core, this is mostly intended as guidelines.

@quietone / #15
Did you mean another meta specifically for those issues? Cause we could roll those into here.

@Berdir / #11 I mostly agree with you except for the groups names.
Since we already are working in a Hook directory we shouldn't append "Hooks" to the groups/classes name
DRY philosophy is appliable, those classes are not meant to end up in other classes use statements and won't engender collisions forcing to use aliases.

Once the ordering issue gets in they will be in use statements, but they are in the hook namespace so I agree we could drop hook from the class name.

📌 Hook ordering across OOP, procedural and with extra types Active

Here's what I did in Group: 📌 Convert hooks to OOP hooks with attributes Active

I looked at which NAME.api.php file hooks were in and grouped them, unless I only used one hook from said file and it was unlikely I would use more hooks. Example being hook_help() from help.api.php. Those hooks I put into a CoreHooks class instead.

I ended up with:

• CoreHooks
• EntityHooks
• FieldHooks
• FormHooks
• QueryHooks
• ThemeHooks
• UserHooks

UserHooks contains the entity hooks for that entity type along with some user-specific hooks. I only put the generic entity hooks in EntityHooks.

I marked all classes as final and added typehinting for everything.

Ok, huge WTF, I injected comment.manager and getFieldUIPageTitle does not exist, did a quick search, it was removed 10 years ago.
#2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form →

Can we safely just delete these? These alters can't have done anything in years.

I believe you meant to post that at 📌 [PP-1] Determine how to implement Form Alters with attributes. Active , right?

thanks!

Just my $0.02:
I've started to implement hook classes in my own work. A decision that I made pretty quickly was to adhere to the single-responsibility principle. This has resulted in a shift in the way that I think about hooks, which I realize was dictated largely by the fact that we could only have a single implementation per module. Now we can have multiples. We're no longer bound to lumping everything a hook needs to do into a single function.

As a result, I'm more inclined to make hook classes that focus on what function it's supposed to perform, not on what part of Drupal it alters. Granted, sometimes those are the same thing. In the case of the former, then I've been naming the class according to its purpose. For the latter, then I do name it according to the hook it contains. I like this pattern. It means that it's a little easier to know just from the class name what that class is supposed to be doing. And it means that the hook is no longer what's important - its purpose is. I've been enjoying working with them! Unit tests for hook classes are so easy to write.

I've read through this issue and another and understand that there's some performance concerns surrounding having too many hooks. And I get that Core may want to have well-defined patterns in order to prevent chaos. I just wanted to share what I've been doing as someone developing contrib and custom modules.

Re #17 I slightly disagree. If, for example, we have NodeHooks for general hooks in the node module, calling the class Node just makes it harder to find for developers as there are already a handful of classes called Node and files called Node.php.

@dcam and @acbramley, I agree with both of you.

I think that organization makes sense longer term specifically for contrib and custom code. We also need something better defined so we can make progress in core. I'm going to update the issue summary, but I think we follow three steps for organizing / converting hooks.

1. Rough conversion, i.e. convert as close to exact as possible, (lift and shift) This is done for almost all hooks in core.
2. Rough organization, DI, t(), Split out by *.api.php, add DI, convert to $this->t()
3. Fine tuning - individual refactors and split out critical path performance hooks

I have attached a file with the core hooks and the recommended grouping.

For ease of searching I've also created a text file I have attached so you can search for the hook class the hook implementations should go in.

#24

I'm not sure I understand what you mean by general hooks, but I'm pretty sure these hooks could go in one of the other files suggested by #11 and #19.

If there was a NodeHooks file in a module I would expect to find hooks implementations related to nodes rather than "general hooks".

I don't think Node module would need to contain a NodeHooks file as Node.php already holds the entity logic ; not sure why the module would need to call it's own hooks rather than just add the code to the entity class.
By definition, hooks are a way for other modules to alter/add behaviors on something they don't own the logic for.

While in core and for most cases, EntityHooks should be fine, what you say is correct:
Several contrib / custom modules might end up using a NodeHooks file for a very specific logic related to nodes (I had something like that in my mind in #14, I do agree with #16 that, for some use cases, EntityHooks might need to be split into multiple more specific classes and we already have an example in #19 with UserHooks in Groups module).

So maybe we should keep appending Hooks to the classes name.
This will make autocomplete easier in the IDE when we want to automatically add a use statement.

We definitely want to keep the module name as part of it, finding the right EntityHooks file out of dozens in a project will be a massive pain.

I think general hooks were just ones that were hard to find a place for, the file I attached I think solves that issue though since we can just use where the hooks are defined as our guide.

Not sure what there is to review? But there appear to still be opened related issues so the META is still in progress right.

If this is a meta I think we can just make it an active plan.

We definitely want to keep the module name as part of it, finding the right EntityHooks file out of dozens in a project will be a massive pain.

I'm not sure about that. I tend to press the shortcut to find classes in PhpStorm and then type e.g.: "group Ent" and it would show me classes starting with Ent in the group namespace. I feel that adding your own module name to your classes is a bit redundant and leads to classes being harder to find. Namespaces are your friend here and I'm sure most IDEs support them in their search.