Explore inverting checkForProceduralOnlyHooks to prevent them from being added as listeners.

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 📌 Mark core modules as converted to OOP hooks Active 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?

That was one of the driving forces behind this question, if I remember what this does is prevent hooks not handled by module installer from being added as listeners. It's a memory optimization, it won't prevent other functions from being picked up other than these hooks:

• 'install',
• 'schema',
• 'uninstall',
• 'update_last_removed',
• 'install_tasks',
• 'install_tasks_alter',

So #[StopProceduralScan] is about telling HookCollectorPass to stop. This is telling it these particular ones shouldn't be added to the map.

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.

I added 📌 Fix hook_install_tasks and hook_install_tasks_alter reference in HookCollectorPass Active to take care of the install tasks.

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.

At least with the prevent scanning work there is someone intervening adding those to review.

I wonder if others thing the possibility of excluding something is worth the memory benefit we receive with this approach.

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

Double check this: 🐛 Update hook is incorrectly being registered as a normal hook Active

nicxvan → changed the visibility of the branch 11.x to hidden.

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.

This needs to test at least one of these do not show up in the container.

Can we also add the missing hooks I found in here 📌 Exclude hook_update_dependencies from OOP hooks Active ?

https://www.drupal.org/project/drupal/issues/3524872 📌 Exclude hook_update_dependencies from OOP hooks Active
I'll get some feedback on the regex.

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.