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

Created on 10 December 2024, 8 days ago

Problem/Motivation

Steps to reproduce

Proposed resolution

Follow up from πŸ“Œ Mark core modules as converted to OOP hooks Active

@berdir wanted to explore a way to prevent these hooks from being added without using the #[StopProceduralScan] attrribute

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component

extension system

Created by

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @nicxvan
  • First commit to issue fork.
  • Merge request !10513skip known procedural hooks for BC layer β†’ (Open) created by berdir
  • Pipeline finished with Failed
    8 days ago
    Total: 704s
    #364895
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

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

  • Pipeline finished with Success
    8 days ago
    #364906
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    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.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
Production build 0.71.5 2024