Mark core modules as converted to OOP hooks

Working on this.

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

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

I could use some help looking into failures.

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

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.

Repeated failure after rebase.

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

dww → credited mstrelan → .

Saving credit per #27

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

📌 Mark Locale and Update as converted Active
📌 Explore inverting checkForProceduralOnlyHooks to prevent them from being added as listeners. Active
✨ Explore adding rule to mark contrib as having been converted Active

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

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.

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

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

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.

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

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

Found a module typo in the MR.

Now we're starting to convert things over (ie. moving things out of modules like 📌 Move helpers in field_ui_test.module and delete it Active I have concerns about the scope of this issue.

Should re repurpose it to mark the modules as converted that are actually converted and handle the ones that can't be in their own issue?

I think we got to the bottom of the memory leak and now we have stopped the bleeding we can do this in a much more controlled fashion right?

I’m open to splitting this further if y’all prefer. It’s big now. 😅

Resolved the thread about the module name typo. Good spot.

Thanks,
-Derek

nicxvan → changed the visibility of the branch 3491275-mark-core-modules to hidden.

Both branches need review now that they are split up: 10612 is green and simple. 10613 is more complex. It will likely fail because it will need the test changes in 10612.

MR !10612 includes the following:
It is all the simple changes that only include the services.yml
It includes the HookCollectorPass change to read the value.
It includes two tests that need updating

MR !10613 includes the following:
All other changes including moving / converting the hooks that need to be moved.

nicxvan → changed the visibility of the branch 3491275-more-complex-mark to hidden.

I have reviewed MR 10612 and appreciate the updated issue summary. Had a question on the MR but I think it's a non-issue. All looks good to me and pipeline is green.

Committed to 11.x, tagging needs followup for the work to get other modules to the point where they can flip this parameter on.

Thanks for the fast turnaround @nicxvan @mstrelan

larowlan → closed merge request !10612

Thanks! I added the other issue as a child and the other follow ups are already referenced.

Automatically closed - issue fixed for 2 weeks with no activity.

Removing tag based on #55