- Issue created by @catch
- πΊπΈUnited States nicxvan
Changed through file so far, just want to run one test to confirm nothing blows up.
- πΊπΈ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.
- πΊπΈUnited States nicxvan
The directory file issue might be a red herring, I'm going to revert.
- πΊπΈ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/3578324Kernel 1 without patch:
https://git.drupalcode.org/issue/drupal-3491275/-/jobs/3580330Tons 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.
- πΊπΈUnited States nicxvan
For credit purposes @mstrelan helped me figure out the last test failures on slack.
- π¨π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 - πΊπΈUnited States nicxvan
- π¨π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
- πΊπΈUnited States dww
Thanks! Resolved 2 of the open threads.
For the last one, yeah, https://www.drupal.org/node/3490771 β needs some help:
- 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. - 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.
- 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. π
- This is no longer true:
- πΊπΈ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.