[meta] Standardize and clean up hook classes in core

In #3493951: Split File oop hook implementations into separate classes 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 hook classes wherever it is possible. There very few use cases when hooks are highly related to each other.

Cross posted from #3442009: OOP hooks using attributes and event dispatcher. 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 {

  }

}

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

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

This issue is very similar to #3376518: Allow omitting @var, @param and @return tags or their descriptions if the variable name and type is enough. Maybe we should take same approach once it is resolved.

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.

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, core and 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/or 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.

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

Here's what I did in Group: #3508876: Convert hooks to OOP hooks with attributes

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.

I believe you meant to post that at #3479141: Implement FormAlter attribute, right?

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.

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

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 with Ent in their name inside 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.

Just re-emphasizing #32 as Nic commented on the MR where I'm about to commit the hook conversion to Group:

I will not be prefixing my hook classes with my module name. I will simply be naming them EntityHooks, UserHooks, and so on. It feels really wrong to completely ignore namespaces when naming classes.

My reasoning now (I may have not always done this in the past) for adding the word Group to some classes and not to others is this: Does it have to do with the concept of Group and does it not live in a Group-specific subnamespace (like plugin dirs)? Then I add it. E.g.: The concept of permissions within a group is specific and not to be confused with the concept of permissions in general. So I name those classes GroupPermissionFoo. Implementing hooks has nothing to do with Group itself, it's implementing third-party API, so I choose not to add Group to the class names.

If I CMD+O in PhpStorm for "UserHooks", I see two results, but if I type "g userho" I already see what I want. Even "group Hooks" gives me everything I need if I were to search for Group's hook implementations. I see no reason to make class names extremely long if we already have all the info we need. Keep in mind some modules have really long names.

Then I type "gr us" or "gro us" or whatever :) PhpStorm is really good at autocompleting for you.

I think general dependency injection would be next to impossible to do with rector, but converting t() to $this->t() seems like something that could be fairly easily handled by inserting the use statements, and replacing the existing t() calls.