[PP-1] Convert Core hook implementations to Class or method based implementations

I imagine this will end up being a meta with conversions being split up in some logical format, perhaps by module or by hook?

Yes that is in the proposed resolution this really was premature, but I was just planning ahead a bit.

Grouping this is challenging. Both per hook and per module are going to be pretty big in some cases.

Per hook sounds nice because you can then review multiple similar thing. But some are going to be huge, like form alter, so we'd need to split that further. And conflicts between different issues are going to be very frequent when functions that sit next to each other are being removed.

Per module on the other hand has lower risk of conflicts (at least between issues of this meta, it will cause havoc anyway with other issues) and we can sensibly group hooks together in a module. I think we will likely also need to do many non-trivial hooks one-by-one or in much smaller groups, because IMHO, the benefit of just moving hooks themself into classes without refactoring them is very limited. hooks often hide interdependencies between modules that we'll need to untangle.

To pick one example, we have node_comment_(delete|insert|update), which calls node_reindex_node_search(), which contains a module exists check for search module and then calls a search service. so what we IMHO want to do is have a conditional SearchReindexer service in node module that we only register if search module is enabled and inject the dependency. And then deprecate node_reindex_node_search(). But that's also called in Node::postSave(), so we have to touch that too.

And in 📌 Replace hook_cron() with a more modern approach Needs work , I found that many existing cron implementations are related to existing (often old issues) to refactor API's, so just moving the hook will improve little and conflict with those issues.

I guess this will keep us busy for years ;)

Ok we've made a lot of progress with the rector rule, one issue is the new phpstan rule saying that new functions need to have a return type defined. We do not have a way to programmatically determine that for old hooks.

For an automated conversion I think these files should be exempt from the rule and follow ups to add the return types.

Otherwise we'll be mired in a lot of make work verifying.

You can see the automated conversion for user: https://git.drupalcode.org/project/drupal/-/merge_requests/9886

You can see the phpstan complaints here: https://git.drupalcode.org/issue/drupal-3481582/-/jobs/3093899

To @berdir's comments I think we break up per module into their own issues, then if a module has a particularly nasty implementation we can break those up further.

In general though it should be as simple as dumping it over.

Another issue to consider is that we don't need to keep the legacy hook stuff for core. when we convert core we can delete the .module files

That's only there as a BC layer for Contrib to support drupal 10 and 11.

Left a few quick comments on https://git.drupalcode.org/project/drupal/-/merge_requests/9886.

IMHO, we need to consider cost/benefit of this. Moving the code around will be a lot of work and it will break dozens of patches and issues that will require larger rewrites and updates. Rector rules will in my opinion never be good enough that they they will improve the code, as of now, they even lose a lot of formatting and the result is worse than what we have now (e.g. the token hooks). Looking at the UserTokens class in that MR shows that it will require a ton of injected dependencies that will then be created if any of those hooks is called.

Thank you, I'm also doing some manual updates just to see what else is missing, e.g. I deleted the legacy hooks and updated baseline, I want to see what tests say.

@berdir, also I did not write the rector rule @ghostofdrupal past did, I've been testing it and moving it to a test place in drupal-rector here: 📌 Create Rector rule for converting procedural hook implementations to OOP Active . Just want to make sure he gets the credit for the rector work too.

https://git.drupalcode.org/project/drupal/-/merge_requests/9887 has a version where it only converts documented hooks.

I need to still update the baseline.

A couple of edge case naming questions for rector:

module foobar_field , hook config_insert
module foobar, hook field_config_insert

$x = ' * Implements hook_ENTITY_TYPE_insert';
$functionName = 'foobar_field_config_insert';

How should this be handled?

Is there baseline rule we can add for now to just exclude the hook namespace?

There is already a fair amount to just remove the rules for the functions that were converted.

Probably worth keeping 📌 [PP-1] Explore converting Install hooks to OOP Active on top too cause it would be nice to obliterated have to convert once.

Notes for running against User:

Pull in the drupal-rector version
Run ddev rector
Run ddev composer phpcbf

Review for empty files
Delete empty files
Delete references to empty files in baseline

Fix indentation in all hook files
First function needs to be indented
First closing brace and all following needs indenting

Run baseline (takes forever on a laptop)

Setting this to needs review for process before creating all of the follow up issues, if you are reviewing this please review 📌 Test issue please ignore: Rector on user Active as well since that is a first test case.

If this process looks good I can create the follow up issues and start the rector runs.

Overall per-module (but including all test modules for a module) seems like the least painful scope here.

Looking at the UserTokens class in that MR shows that it will require a ton of injected dependencies that will then be created if any of those hooks is called.

This is true but conversely some of the test hook module hook implementations have no dependencies at all and can maybe be left entirely as-is after conversion. Also not bothered about the odd \Drupal call in a test hook implementation either.

So I think the idea of:

1. Run the rector rule for a per-module issue.
2. Review the results
3. Split anything tricky one or more side-issues, and get those side-issues committed.
4. Commit the remaining automated conversions from the main issue

Could be a good plan. Will try to get a second opinion from one or more other committers in case I've missed something.

Also I think we might want to consider going through the entire process with user module before opening the sub-issues, then if we need to adjust everything we don't have 50+ issues to adjust it on.

I think it is broadly fine to skip dependency injection in the initial automatic conversions (but tricky hooks we do manually in spin-offs we can do dependency injection in too), and then have another pass to sort those out later. That second pass will then have to split/group hooks depending on dependencies and maybe other criteria - we definitely don't want to be instantiating services that aren't used because of this change. But let's see what it actually looks like for user module and finalise things after that?

I apparently have quite a few opinions and thoughts about this issue, so sorry in advance for the long comment. A lot of this is also driven by me wearing my contrib-maintainer hat. Where there will be a lot less help to getting things ready and committable, so the work we do on rector isn't just for core.

Seems fine to skip DI and other refactorings for a first pass. I think we have to be realistic that in many cases, that additional work will then have a much lower priority and will possibly not happen or at least not for a very long time. But that's not different to now, we've had issues open to drop some of these hooks for years and it didn't get in.

Unsure about coding standards. I'm actually quite surprised that the user MR passes phpcs. There's no class docblock, there's a @file docblock that shouldn't be there and no empty line to uses, array formatting generally is way too much on a single line and more. The code is clearly way less readable than the current hooks, especially in info hooks like hook_token_info() with a lot of complex arrays. I think we need to touch that up by hand before we commit them so it's as good as it is now.

> (can be before or after 3 as long as it doesn't touch the same code)
I think that's not really true, because most hooks are currently in new $ModuleHooks file that both would add. My grouping idea below might help with that, but might still touch the same file and will likely also conflict on the removed code. But we can deal with that.

Focusing and completely finishing user module MR before we proceed seems like a good idea, but I'd like to see generated code for a few more modules, or possibly even all, for example to evaluate my grouping idea.

Seems a bit unclear to me what the tricky cases would be. If we don't do DI and just copy the code 1:1, pretty much everything should work. unless we find bugs within hook discovery or so? We'll only discovery technical issues like hidden dependencies when we do DI and refactor related methods, and we will likely do that later. So it will likely not be about technical issues but just things that we'd like to do better from the start?

What about generating the hook classes as final and internal, to make it very clear that they are not to be messed with and that we can change constructors, add/remove methods as we please.

I think the main topic where I'd love to see some improvements is class grouping. I can see that it is capable for splitting it up into classes, but currently it's 1:1 by file. I don't know how complicated that is, especially since the code isn't in order, but I think it would be great if we could from the start adopt some consistent patterns that would make it much easier to see at a glance what kind of hooks a module implements and where they are, like plugins. This is very unlikely to happen later if we don't do it from the start I think. At first, I thought we could have a few specific rules, like FormHooks, and EntityHooks. But what if we just generalize that into prefixing it based on the first word of the hook name? This won't work so great for modules that consist of multiple parts, like hooks for content_moderation and content_translation would both go into ContentHooks. Not perfect, but also not completely wrong?

User module is currently mostly UserHooks, with 18 functions and 370 lines of code. With my suggestion, we'd have:

HelpHooks
ThemeHooks
JsHooks
EntityHooks
UserHooks
TemplateHooks
MailHooks
ElementHooks
ToolbarHooks
FormHooks
FilterHooks

(+ the already created UserTokensHooks and 2 views classes)

Quite a bit and many just have one function, but the module also implements a pretty diverse set of hooks. and that list looks like a pretty neat overview of what the module integrates with? And we could apply a few rules, like Element/Template/Theme could all go into a single file. Custom modules are often full of form alters, that could still be split up further to group them by altered module.

One last thing. Any thoughts around grouping related functions? For example, user.module now still has user_form_system_regional_settings_submit(). Form alters frequently add validate and submit callbacks. Would it make sense that we move them into the FormHooks class too? Not automatically, that seems non-trivial, but as manual follow-ups?

Thank you for taking a look!

I agree with your end goal and the grouping.

However, I don't think it's critical at this stage, I think the first focus should be getting core hook implementations into oop so we can work on deprecating procedural implementations.

I think already the current grouping makes it nicer to work on.

Breaking them up into separate files and grouping is an easy fast follow that we can implement. No need to block this effort on that initiative.

Final and internal are probably good so we can refactor more later.

The other consideration is this a big change already so the closer we match the converted implementations with the current implementations the less likely we'll run into tests that need changes as well.

I also wanted to mention that there is already consideration in the rector role for contrib. In contrib it will create the legacy hook fallback and the services yml.

The rector rule is being worked on here: https://github.com/palantirnet/drupal-rector/pull/308/files

> Breaking them up into separate files and grouping is an easy fast follow that we can implement. No need to block this effort on that initiative.

From experience, I would be quite surprised if those follow-ups happens, consistently, all over core. It would also be break other issues that touch hook implementations twice. This effort will conflict with literally hundreds of other issues and merge requests. I understand that we want to push this to deprecate legacy hooks, but it's an impact we should be aware of.

I'm not suggesting to do that manually with all the merge requests, but explore if rector can handle that so we can automate it and have them in a sensible place from the start. If not then so be it.

Only somewhat related to the discussion above, but the discussion made me think of it:

I wonder if a better scope would be to do all hooks in all test modules across core with the rector rule in a single core issue. That would hopefully be a quick/review commit as long as the rector rule is good enough. The advantage of this is it would concentrate a lot of baseline moves/conflicts into one commit which might be less disruptive both for these issues and other ones. Also the MRs for non-test hooks/modules would be a lot smaller to review then.

There's then a dozen or so core modules with very few hook implementations (at least ban, basic_auth, big pipe, breakpoint, datetime, datetime_range, dynamic_page_cache, rest, mysql, sqlite, pgsql, serialization) that could maybe be grouped into one issue too. Most of those implement hook_help() and 0-1 other hooks.

Also makes me think we should split hook_help() into its own file given it's only invoked when help module is installed - save some opcache space that way. Would it be worth considering doing hook_help() across all modules in a single issue prior to the other hooks? That would then incorporate all the hook_help() only .module files too, meaning we'd cover most of the list above that way.

Then per-module for the bigger ones still.

For example, user.module now still has user_form_system_regional_settings_submit(). Form alters frequently add validate and submit callbacks. Would it make sense that we move them into the FormHooks class too? Not automatically, that seems non-trivial, but as manual follow-ups?

Yes I think we should - eventually we'll want to deprecate .module files, although that will be a further step after deprecating procedural hooks. However we're going to run into interesting issues if the form submit needs a service (e-mail, queue seem like likely ones) that the form alter does not, maybe service closures can help with that though.

@internal and final definitely sounds good - we can leave that on too.

So far, I like the suggestion to do all hooks in all test modules across core first. Doing that should uncover problems with more types of hooks than a single module could. And when any uncovered problems are fixed the conversions for the run time code will be improved. What to after that, I am not sure, but the process should give us information to decide what the next scope should be.

As Berdir pointed out some of the changes don't follow coding standards and/or make the code harder to read. It is possible to do any of the following?

• sort use statments
• wrap comments to 80 chars
• wrap long arrays
• wrap lines greater than say 120 characters
• do not add @file doc blocks

I'm going to try running the codesniffer in a different way and see if that cleans up some of the inconsistencies.

I can also try to run it against just test modules and see how that looks in a separate issue.

Breaking them up by file can probably be done, the rector rule is here: https://github.com/palantirnet/drupal-rector/pull/308 let me know if someone wants access.

From experience, I would be quite surprised if those follow-ups happens, consistently,

I can still see the appeal trying to get as much clean up in this round as possible, however as you mentioned this will be disruptive to a large number of issues, and these conversion issues will not likely rebase cleanly since it's an automated move of code. As I've been testing I've had to rerun the process each time kind of from scratch.

Is there a way to rerun baseline that ignores / removes missing files automatically cause that is the biggest manual issue at the moment.
Once I delete the missing files from baseline it runs an update as expected.

On the codesniffing, it seems ddev composer phpcbf is ok with all of the code style issues, running ./vendor/bin/phpcbf \
--standard="Drupal,DrupalPractice" -n \
--extensions="php,module,inc,install,test,profile,theme" manually against the files seems to catch the remaining issues which need to be manually fixed.

Again, unless I'm missing something, the more manual steps we add the more times we'll have to do this for each conflict though.

I pushed up the changes to fix all of the code style issues to the user issue 📌 Test issue please ignore: Rector on user Active EXCEPT help, I didn't want to spend all that time working on it while we're still discussing process.

Interestingly fixing many of the issues manually now causes the test to fail! https://git.drupalcode.org/issue/drupal-3481582/-/jobs/3110962

I wonder if there are just so many failures it gives up and just decides to pass, or there is is some error that causing it to fail, maybe the comment that was inside the use statements?

Most fails are "A comma should follow the last multiline array item", before the arrays weren't multiline, so a trailing comma wasn't required. Now it is.

Drupal core only actively uses a subset of the default Drupal standard. Partially that's because it can't be enforced yet as core doesn't consistently follow certain parts yet and partially because there is disagreement over certain parts (e.g. formatting and handling of long lines, specifically function definitions). And then.

> I can still see the appeal trying to get as much clean up in this round as possible, however as you mentioned this will be disruptive to a large number of issues, and these conversion issues will not likely rebase cleanly since it's an automated move of code. As I've been testing I've had to rerun the process each time kind of from scratch.

That's kind of my argument: It will be very disruptive anyway, so the goal should be to make it only really disruptive once, not multiple times.

I understand it's a lot of work and slow even with rector to move all that and update baseline, at least for grouping, for me it's either that we can automate that grouping logic or then we don't do it (initially and likely also not later)

@berdir thanks for that clarification.

Btw I figured out how to generate baseline from scratch I think if you start with the .phpstan-baseline.php
with just this code it works:

<?php declare(strict_types = 1);

$ignoreErrors = [];


return ['parameters' => ['ignoreErrors' => $ignoreErrors]];

A VERY naive run with some huge gaps for just test module conversion is being done here: 📌 Convert all test modules to use OOP Hook implementations. Active
I've highlighted missing directories, and this has been done with almost no introspections e.g. just deleting the .module files that are empty and committing the results for review.

I am updating the baseline too, I think this might be useful for review, I don't think this process will be good it was super painful, I think we just do module by module and maybe the system module we break up test and system since there are so many tests, but all test modules is too much across too many separate modules.

Reran PHPCBF again on the user issue 📌 Test issue please ignore: Rector on user Active and it's now passing.

I just reran the test module conversion with some updates.

We also have a new script for removing the empty .module and .inc files. It keeps any file that still has function or CONST and then keeps the intentionally empty .module file core/tests/fixtures/empty_file.php.module

Looking at how the conversions work, it's quite complex to decide how to break things up.

I really think that bit should be follow ups. The conversion is already going to be complex.

I think I'm also going to make an honest attempt at a couple of smaller modules fully to see if it shakes anything loose.

Announcements feed module 📌 Convert Announcements Feed to OOP Hooks Active is ready for review.

There was a request on slack to remove the Implements comments, but codesniff doesn't allow that.

The api module still requires it as well so I think we should leave it.

Other comments are welcome.

Check the IS for ddev instructions.

Note to add define( to the protected .module and .inc files

Updating Convertoop script.

We figured out how to fix the Attribute spacing! 🐛 Incorrect indent in fixed up Rector generated file Active I will update my script and move it to a gist.

I've moved the script to a gist and added patching for coder to fix the attribute issue, I'm going to retest the Announcements Feed conversion.

Announcements feed is ready for review, we figured out the array sorting!

If we get approval we can move forward with the individual modules.

There are a couple of test failures that could use another set of eyes.

I need to update the issue summary with the current status. A lot of work has been done.