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.