Mark core modules as converted to OOP hooks

Created on 3 December 2024, 15 days ago

Problem/Motivation

Follow-up from ✨ Explore ways to mark hook conversion status per module or file Active . Let's mark all the modules.

Would be great to do a before/after comparison of drush install to see if/how much memory and potentially time we save.

Steps to reproduce

Proposed resolution

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 Kingdom catch

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

Merge Requests

Comments & Activities

  • Issue created by @catch
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    Working on this.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    Changed through file so far, just want to run one test to confirm nothing blows up.

  • Merge request !10442Resolve #3491275 "Mark core modules" β†’ (Open) created by nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • Pipeline finished with Failed
    15 days ago
    Total: 125s
    #357769
  • Pipeline finished with Failed
    15 days ago
    Total: 476s
    #357779
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • Pipeline finished with Failed
    15 days ago
    Total: 441s
    #357843
  • Pipeline finished with Failed
    15 days ago
    Total: 582s
    #357864
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I added the patch from: πŸ› Include .module files during HookCollection always even if not scanned Active to confirm it fixes most failures.

  • Pipeline finished with Failed
    15 days ago
    Total: 458s
    #357920
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • Pipeline finished with Failed
    15 days ago
    Total: 1045s
    #358035
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • Pipeline finished with Failed
    15 days ago
    Total: 484s
    #358102
  • Pipeline finished with Failed
    15 days ago
    Total: 119s
    #358125
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • Pipeline finished with Failed
    15 days ago
    Total: 167s
    #358169
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • Pipeline finished with Canceled
    15 days ago
    Total: 1457s
    #358182
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I could use some help looking into failures.

  • Pipeline finished with Failed
    15 days ago
    #358201
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    The directory file issue might be a red herring, I'm going to revert.

  • Pipeline finished with Failed
    15 days ago
    Total: 683s
    #358372
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    Reverting that has many new failures so I don't think it's a red herring:

    Kernel 1 with patch:
    https://git.drupalcode.org/issue/drupal-3491275/-/jobs/3578324

    Kernel 1 without patch:
    https://git.drupalcode.org/issue/drupal-3491275/-/jobs/3580330

    Tons of stuff breaks, but I checked the $dir output and it should work so I must be excluding a hook that is not meant to be excluded, it seems related to config or entities.

  • Pipeline finished with Failed
    14 days ago
    Total: 67s
    #359325
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • Pipeline finished with Failed
    14 days ago
    Total: 77s
    #359326
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • Pipeline finished with Failed
    14 days ago
    Total: 150s
    #359341
  • Pipeline finished with Failed
    14 days ago
    Total: 124s
    #359346
  • Pipeline finished with Failed
    14 days ago
    Total: 504s
    #359348
  • Pipeline finished with Failed
    14 days ago
    Total: 610s
    #359378
  • Pipeline finished with Failed
    14 days ago
    Total: 484s
    #359396
  • Pipeline finished with Failed
    14 days ago
    Total: 935s
    #359563
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • Pipeline finished with Failed
    14 days ago
    Total: 1094s
    #359583
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    Repeated failure after rebase.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • Pipeline finished with Success
    14 days ago
    Total: 834s
    #359788
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    For credit purposes @mstrelan helped me figure out the last test failures on slack.

  • πŸ‡ΊπŸ‡ΈUnited States dww

    Saving credit per #27

  • πŸ‡¬πŸ‡§United Kingdom catch
  • Pipeline finished with Success
    10 days ago
    Total: 1659s
    #362845
  • Pipeline finished with Canceled
    9 days ago
    Total: 232s
    #363539
  • Pipeline finished with Success
    9 days ago
    Total: 425s
    #363545
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    I did some testing around the benefits of this.

    While the speed improvements on the parsing aspect are not huge, they do considerably reduce the number of possibly-legacy-hook-functions we have to collect and store:

    $ ddev drush cr | wc -l
    
    HEAD: 324
    MR without StopProceduralHookScan:219
    MR: 122
    

    While I'm still not a fan of StopProceduralHookScan, this shows that this saves us 100 entries that we load on every request and have to carry around. I think it's an edge case and contrib should probably not bother with it but the feature is in core, it's implemented, so I guess we can just as well use it.

    Out of those remaining 122 functions, 70 are for locale and 24 for update module. And we discussed in slack that we should implement the reverse of checkForProceduralOnlyHooks() to ignore all those functions that we know aren't proper hooks and are invoked differently (install, uninstall, hook_info, module_implements_alter, update functions), that should save us another 10-20 functions, at which point we'll be down to very, very few functions in this list for core.

    About the name, to be honest, I don't care too much personally, I think no name will be perfect and more importantly, the ship has pretty much sailed on that. 11.1.0 will release this week and then this is an API and we can't rename it. It's a temporary thing until Drupal 13. What I think we should do and is more important is file a follow-up for rector to add this with a comment that explains it to converted modules, to be defined if it should default to true or false. Unless we do that, nobody will remember to add that, no matter the name.

    To summarize, I can RTBC this if we have follow-ups for:
    * locale and update module, either one or two separate issues
    * reverse checkForProceduralOnlyHooks() to not add to the BC layer, might do that later if nobody beats me to it.
    * Add this to rector as a suggestion

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    @dww didn't follow-up here about my comment about the name of this flag, I hope he isn't too unhappy if we stick with this.

    With the follow-ups and my remarks in #31, I think this is now ready.

  • πŸ‡ΊπŸ‡ΈUnited States dww

    Opened a bunch of mostly trivial nits / suggestions. A few points of substance.

    I nearly reviewed every line. I'll confess I started to glaze over and didn't completely verify that in the cases where we're moving functions around in .install or .module files that the new code is identical to the old.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    Good catch on the media library comment.

    I applied all of the other changes too though I'm not sure I would say they should be addressed here, there is an issue quietone created to review the hook conversions and issues like this πŸ“Œ Add Dependency Injection Active and πŸ“Œ Cleanup hook implementations in the Workspaces module Active

  • Pipeline finished with Success
    7 days ago
    Total: 459s
    #366030
  • πŸ‡ΊπŸ‡ΈUnited States dww

    Thanks! Resolved 2 of the open threads.

    For the last one, yeah, https://www.drupal.org/node/3490771 β†’ needs some help:

    1. This is no longer true:

      Warning: If you later add procedural hooks and need them to be scanned, you must delete this container parameter.
      Setting hooks_converted to false will still prevent scanning, you must DELETE the parameter to re-enable scanning.

    2. We should add something about the special non-hook hooks like hook_install() and friends. I don't actually know the gory details enough to fix it myself, but would be happy to review for clarity once done.
    3. The instructions for both "No procedural hooks converted to OOP hooks" and "The module still has procedural hooks" are identical, duplicate, and therefore confusing. Both cases are the same, and from the perspective of a developer reading the CR, the difference is irrelevant. The bottom line is there are still procedural hooks, regardless of if some of them have been moved or not. I think we should merge these two sections.

    Once those edits are done, this is RTBC to me.

    Meanwhile, saving credit for @berdir and myself for detailed reviews, and the aforementioned epic Slack thread. πŸ˜‚

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    1. should not change until this issue lands.
    2. I will add that now.
    3. The behavior is the same if you have converted all hooks or just some hooks, not sure how to lay that out.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I'll update the CR again to remove the warning once this is merged.

  • πŸ‡ΊπŸ‡ΈUnited States dww

    The CR looks much better now, thanks! Back to RTBC.

Production build 0.71.5 2024