Convert all procedural hook implementations to Hook classes

This looks amazing, well done.

A couple of thoughts/questions:

1. With things like

-/**
- * Implements hook_rebuild().
- */
-function block_rebuild() {

This shouldn't be considered API, but I would bet that someone somewhere is calling a procedural hook like this directly. Should we leave the existing code in place and apply the #[LegacyHook] attribute to it?

2. In turn this made me consider BC. If you were hook_module_implements_alter'ing to remove or reorder core hooks before, now we have broken this I think? But if we have the #[LegacyHook] attribute could we add metadata to point to the new OO hook name - and then we can provide a BC layer based on this information? This does sound like it could be quite difficult and perhaps not performant, however.

I think the problem with #75.1 is that we don't really know which procedural hooks someone might be calling directly; we could search contrib but we have no idea about custom code. The BC policy says that hook implementations are not part of the BC promise so perhaps we just hard break by removing all these, unless there are a small number that are widely used - but again I wouldn't know how to find that out.

The "gigantic mess" has put me off #3484754: Ordering of hooks needs some attention :D but I will try to look soon!

I think it is OK for us to remove the procedural hook implementations - we do that for individual hook implementations every so often, and it's very clearly documented that you shouldn't call them directly. Also it will be an easy thing for a module to adapt to. My concern with leaving all the procedural hook implementations in is that we'll be doing hook discovery on those files until Drupal 12 then, which anecdotally has put our memory requirements through the roof. Core is big enough that fully converting core might mitigate some of this, got another issue open to hopefully mitigate it further but it's just an idea with no code still.

If you were hook_module_implements_alter'ing to remove or reorder core hooks before, now we have broken this I think?

I think this will actually be OK after #3484754: Ordering of hooks needs some attention.

hook_module_implements_alter() works on an array keyed by the module name, so as long as a module only implements one hook, the alter will continue to work the same way.

Once modules start having two implementations of the same hook, then it won't be possible to use h_m_i_a() to put an implementation between the two implementation, but that will be a result of refactoring of hook implementations (in 11.2 or later, or contrib) rather than the conversion itself. At that point we'll need a different ordering mechanism which will eventually replace h_m_i_a().

I spot checked the first handful of converted modules and the conversion looks really great - all comments and formatting preserved where it needs to be. Overall I agree committing this in one go will be less disruptive than module-by-module, even if a few more MRs have to be re-rolled at least it's one hit instead of over and over again. Tagging with beta target (in the sense of good to do during the beta).

RTBC - I did not look at every single line of code, but performed isolated spot checks and the conversion seems fully automatic to me.

I am also +1 to do this in one full sweep, so that we can get off .module files ASAP.

This MR also clearly shows that hooks are as easy to implement as before the conversion, so that’s a huge plus.

I also reviewed the snippets of patches applied after the automatic conversion manually.

They all also look good to me.

The change record makes sense and even in D7 it was the right way to use module handler to invoke a single hook in another module.

Still RTBC

Thanks again for keeping on top of this.

Discussed this with @catch and @nod_ and we agree it would be good to get this into beta1. As it's such a big change and removes a lot of procedural code it would be good to shake out any issues as early as possible.

I have some nits, but they can wait for followups:

• Return types have a leading space which doesn't follow standards
• Not all methods have return types, this is an ideal time to add them

I also realised that we can infer the hook name from the method name to avoid repetition:

  #[Hook('help')]
  public function help($route_name, RouteMatchInterface $route_match) {

could just be

  #[Hook]
  public function help($route_name, RouteMatchInterface $route_match) {

Even for e.g.

  #[Hook('page_attachments')]
  public function pageAttachments(array &$page) {

we can swap camelCase for snake_case automatically. But again this easily can wait for a followup.

Committed and pushed 8aeb2ca5992 to 11.x and 059e256c285 to 11.1.x. Thanks!

Tagging as a release highlight and also for the release notes as it's such a big change.

👏 great advancement!

Adding credits from #146.

Explicit is better than implicit. But we have a core committer expressing his opinion and if that's the direction core wants to take, I wouldn't say no. nicxvan quotes the IS but it has the serious problem of being written by me and as such it's poorly worded -- I speak much better PHP than English. In this case I wanted to make sure we are not ending up with another solution where the only way to write a hook is a magic string. But I think an optional empty hook attribute with the method name converted to snake case as the default hook is okay. It just shouldn't be mandatory. And yeah, it's not usable for either angle of multiples but that's also ok -- it's a sensible default after all, nothing more.. It'll cover a lot of cases and when it doesn't it's very easy to do more. Change $hook to optional in Hook, put a comment on it "defaults to snake case converted method name" and in HookCollectorPass add said conversion:

  protected function addFromAttribute(Hook $hook, $class, $module) {
    if ($hook->module) {
      $module = $hook->module;
    }
    $hookValue = $hook->hook ?: strtolower(preg_replace('/(?<=.)[A-Z]/', '_$0',  $hook->method));
    $this->moduleImplements[$hookValue][$module] = '';
    $this->implementations[$hookValue][$module][$class][] = $hook->method;
  }

So simple ;) but really, this is just 1 line change besides factoring out $hook->hook into $hookValue. Your test string should be something like FooENtityInsert to check against a) first capital letter doesn't cause an underscore -- this is why the regex has a dot b) two capital letters next to each other do cause an underscore -- this is why the regex uses a positive lookbehind instead of a plain dot, this way the previous character doesn't get consumed.

This could be a second rector rule -- one that is much, much simpler than the first and way, way more aligned with what rector does.

Edit: on further thinking there's significant challenges here with numbers. Like, do we put an underscore before a block of numbers, between them...? Camelcasing is a lossy operation for numbers. Careful here. Perhaps throw an UnexpectedValueException when this conversion is being done but there are numbers to force the developer to be explicit?

That's what I am suggesting, it should be a whole lot easier since the rule would only remove superfluous strings from Hook attributes. It will not change anything behavior wise, it's free, no test failures, no nothing. It's just less strings. Not to mention how easy it is to write the rule. It doesn't matter whether it's extra types or not. It could look at fieldWidgetCompleteTestFieldWidgetMultipleSingleValueFormAlter, convert it to field_widget_complete_test_field_widget_multiple_single_value_form_alter check the first arg to Hook and see it's the same, so it deletes it. If it is not the same, the rule does nothing.

It would also skip ckeditor5PluginInfoAlter for reasons outlined above, it's the only eligible Hook implementation which is followed by a number.

Lets discuss that in a new issue? That said, -1 from me on that. It will make it harder to search/grep for implementations and understand for a human what on earth fieldWidgetCompleteTestFieldWidgetMultipleSingleValueFormAlter is exactly. I'd rather do the opposite and have the full Hook attribute and possibly use a shorter method name.

Certainly. I said "if core wants to go that way" and outlined how it could look, what would it take in core and in rector. I am not calling any shots.

It will make it harder to search/grep for implementations

Yes as someone who constantly greps looking for code and who doesn't use an IDE, it would be nice to retain this ability.

drush gen hook appears to have been forgotten for this refactor. Still no OOP option for drush gen hook !

screenshot of drush gen hook when using Drupal 10.3.6 and also occurs with Drupal 11 and Drush Commandline Tool 13.3.3.0

https://github.com/drush-ops/drush/issues/6175

It's actually higher than drush, it's the Chi-teck drupal-code-generator
https://github.com/Chi-teck/drupal-code-generator/issues/194