Prevent hooks that do not go through module handler from being added to the container.

Created a merge request. Haven't tested yet at all whether this works correctly, other than the reduced number of procedural hooks.

In my install profile, I counted 1040 registered procedural hooks. That is without #3491275: Mark fully converted core modules as converted to OOP hooks so it should go down around 200 then with that.

With this change, it's now 887, so that's 153 less functions stored in the container that need to be checked.

While testing, I noticed a strange thing with the regular expressions, it didn't catch some of the update functions. It turns out that the magic regular expression attributes update functions in submodules that share a prefix with another module are somehow attributed to the wrong module/hook.

if (in_array($function, ['redirect_404_cron', 'redirect_404_update_8101'])) {
  var_dump($matches);
}

results in:

array(7) {
  [0]=>
  string(17) "redirect_404_cron"
  ["function"]=>
  string(17) "redirect_404_cron"
  [1]=>
  string(17) "redirect_404_cron"
  ["module"]=>
  string(12) "redirect_404"
  [2]=>
  string(12) "redirect_404"
  ["hook"]=>
  string(4) "cron"
  [3]=>
  string(4) "cron"
}
array(7) {
  [0]=>
  string(24) "redirect_404_update_8101"
  ["function"]=>
  string(24) "redirect_404_update_8101"
  [1]=>
  string(24) "redirect_404_update_8101"
  ["module"]=>
  string(8) "redirect"
  [2]=>
  string(8) "redirect"
  ["hook"]=>
  string(15) "404_update_8101"
  [3]=>
  string(15) "404_update_8101"
}

So while the cron hook is attributed correctly to redirect_404 and not redirect, redirect_404_update_8101 is not.

I only realized while writing this one what the problem is. It's that $module_reg already attempts to filter out all update functions but it does not account for the prefix/length thing, so it will simply match the above on the non-existing hook 404_update_8101. I pushed an update that adjusts the original regex, but that definitely needs to be reviewed by someone smarter than me, because that regex is well above my paygrade.

While doing that, I also noticed that it also excludes preprocess functions, which means that a handful of weird hooks in contrib modules that start with preprocess will be broken since their legacy hook implementations won't be found anymore. Might be OK, might not be. Should be a separate issue if so: https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22invok...

This is passing now.

I had to allow hook_hook_info() and hook_module_implements_alter(), they can't be new OOP hooks, but they need to run through the legacy processing to properly register and work. That means there are a few additional functions to process compared to what I wrote in #4.

So with this change is #[StopProceduralScan] still needed?

I think most importantly, it also properly excludes update functions from being picked up, which is currently not working reliably when you have submodules with update functions.

Yes, it doesn't replace the need for StopProceduralScan, but it lessens the memory overhead when modules don't use it, and most will not because it's not trivial to use and quite tedious.

FWIW, StopProceduralScan is also mostly a memory optimization. The parsing is done, the only thing we save on is running the regex on the remaining functions in the file and then also not adding them to the list of possibly legacy hooks.

> Nominally this looks good, but I'm concerned that this could prevent something from being added that we would want added and that would be almost impossible to detect.

HEAD already attempts to automatically block all functions that contain "preprocess" (being removed in the preprocess issue, but it's still there now) and update functions (but inconsistently), so I don't think this introduces anything new.

IMHO this is safer and more reliable than the attribute to stop processing in the middle of a file, as it only excludes functions that explicitly match patterns that will never be hooks.

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.

Rebased, I think this is still useful and has a bigger impact than manually adding a few StopProcessing attributes.

Having to put a manual stop sign in several files in your module seems a bit tedious and counter-intuitive. Nothing seems to happen if you don't, so I can imagine many developers simply not knowing about this memory optimization.

I like that you can flag your module as "I've converted all my hooks" in one place (services file), though. We can scan for said flag when procedural hooks are gone and then give a little nudge saying you can now delete the container param.

So from what little understanding I have from trying to implement hooks in Group and having a cursory read of HookCollectorPass, I tend to agree with Berdir that we can do better here and significantly reduce the fallout if someone were to forget to add the stop signs. Then we should evaluate as a whole whether we cannot do away with the stop signs altogether.

That would fix the concern raised in #4, but it would still analyze all functions starting with the module name and "guess" that it could be a hook and add it to the container, no?

I'm asking because the initial issue title and summary were about having to put a manual stop sign in your module file and now it seems to have been repurposed entirely, not addressing the initial concern.

> That would fix the concern raised in #4, but it would still analyze all functions starting with the module name and "guess" that it could be a hook and add it to the container, no?

Only for the BC layer for legacy hook functions. We can't avoid this. We don't know what's a hook and what isn't. But this will completely go away in D13 (together with support for .module files)

We also have an attribute already to stop scanning in a certain file, so we can manually optimize this. But it's tedious and confusing to understand.

The purpose of this issue is to avoid adding functions where we *know for certain* that they are not hooks or to be more specific, not hooks that are supported by the new system: update functions, hook_install and so on. It's an automatic optimization, mostly for stuff that's in .install files. It's a very narrow scope and the goal is not to change how the BC system works.

To add, if you add the flag in your services.yml then this won't do anything because we will skip the BC scan completely for that module, so that's even better. But it will take years until the majority of contrib will be converted like that and this optimization reduces the current maybe-legacy-hook list by about 20% in my example, with IMHO zero drawbacks.

Okay, thanks for the clarification. It seems you have no objections going forward like this, so consider mine retracted.

Neither my nor your change to the update exclusion in the regex worked. https://3v4l.org/Nh9IQ didn't print the first $matches, that's why it shows nothing. It finds the update function.

I added explicit tests that assert the implementations map. we'll have to update that test 1-2 times to account for the updated storage, but I think it's worth having that.

This also has the benefit that it shouldn't conflict with #3502302: HookCollectorPass fails to register hooks for the correct module in some contexts. There's some overlap with that, but I think it also doesn't sufficiently address this.

Couple of small nits on the comment. I was going to try to simplify it a bit but failed to get anywhere.

Accepted that, makes sense. I had just 'requirements' instead of hook_requirements as I was referring to the list of strings in the other method, but either way is fine for me.

Comment #4 has some numbers, although that's outdated as requirements are now again in the list.

Committed/pushed to 11.x, thanks!