Explore ways to mark hook conversion status per module or file

If we make this per-module, it will require the .info information - however, it will also mean we can skip scanning the directories and parsing the files at all.

I'm not sure if we already check .info information in a container rebuild, if not, that might not be an option.

Did also wonder about a tiny file next to the module.info.yml that indicates this - if it exists, conversion is done and stop there. Massive hack but again would be efficient.

Ok, so the Attribute is much easier to implement, there are no tests yet, but it looks like it's working.

I'm still a bit unsure why the skip_procedural isn't being picked up. On discovery and list it is in the array, but when scanProceduralHooks is called info isn't set yet, so it defaults to scan.

It's also not set in moduleData, any advice would be appreciated.

Oh I figured it out, this definitely needs cleanup and optimizing.

Added some tests, let's see how broken things are.

I think marking core modules should be a follow up, but leaving a few now to see how tests react, I know for sure ordering is broken, and the new tests.

The only thing failing is the skip all, the attribute works as expected.

When I debug and output the values for moduleInfo I see the test module 5 times. The first 4 times procedural is set to scan, the 5th it is set to skip, at this point though the hooks have already been scanned and cached.

It looks like the compiler pass happens before the info file is parsed.

The attribute is working. I'm going to try the sibling file for now in a separate branch.

There are two open MRs. Can you clarify which one is for review and hide the other one?

Approach 2, the one called sibling file.
I didn't hide the other one in case someone wanted to review that approach.

Also for completeness @ghost suggested renaming covered modules to modulename.converted.module
This would require updating extension discovery and would not handle .inc files from hook hook info.

I have not provided a poc for @ghost's suggestion.

The sibling file approach was my idea, but seeing it in the MR it doesn't look too bad. I wonder if 'hooks_converted.info.yml' would be more self documenting though?

Also - if this is set, I think we could also optimise ::collectModuleHookImplementations to only scan inside src/Hook in the first place maybe? It could just completely ignore all other files then.

Do you mean moduleName.hooks_converted.yml?

If so I can change that and look at optimizing that method.

Ok I refactored that bit, I think we have to leave:

if ($extension === 'inc') {
  $parts = explode('.', $fileinfo->getFilename());
  if (count($parts) === 3 && $parts[0] === $module) {
    $this->groupIncludes[$parts[1]][] = $filename;
  }
}

Alone or risk breaking hook_hook_info

I don't feel strongly about this, but I thought I'd mention it: what about using container parameters instead of the sibling file approach?

So for my_module, in my_module.services.yml:

parameters:
  my_module.hooks_converted: true

In HookCollectorPass, the $container object would need to be passed down to the relevant method, but the logic would look something like
$skip_procedural = $container->hasParameter(''$module.hooks_converted");

That is a great suggestion.

Let me do it on yet another branch, just in case something weird happens early compile pass again.

I'll close the sibling branch as long as this approach works.

nicxvan → changed the visibility of the branch 3490355-sibling-file-approach to hidden.

It's green, this has the added advantage of allowing you to mark a contrib module as converted if the maintainer has not converted it if you know there are no procedural hooks.

nicxvan → changed the visibility of the branch 3490355-explore-ways-to to hidden.

Looks great to me. We need a follow-up to add this to all core modules. Maybe one for all the ones that don't use hook_hook_info() / hook_module_implements_alter() and another issue for the ones that implement those two hooks.

Once we've done those follow-up issues it would be useful to get a drush site:install comparison before/after to see how much this actually helped with Umami or Standard in terms of memory usage.

One small comment that isn't critical, but would be nice. Otherwise RTBC.

lgtm

Service parameter is so much nicer than the extra file. We won't be able to tell the benefit from this until we convert core modules 📌 Mark core modules as converted to OOP hooks Active but feels like it should be a lot.

Committed/pushed to 11.x and cherry-picked to 11.1.x, thanks!

catch → closed merge request !10423

As a module developer, I read over the change record and have a few questions:

1. If a module does not have any files where procedural hooks can be defined (for example, the only php code is classes in the /src directory), is there any benefit to specifying the parameter module_name.hooks_converted: true?
2. If a module has converted all hooks that can be converted to OOP but still has hooks that cannot be converted to OOP yet (for example, template_preprocess hooks), then module_name.hooks_converted: true cannot be specified and the attribute #[StopProceduralHookScan] needs to be used for such hooks, correct?

Correct, I think you have to be careful too about includes if you manually include them.

The patch here is required if you're going to use this.

🐛 Include .module files during HookCollection always even if not scanned Active

Wow, this moved fast. 😅 nice work, y’all!

One complaint / concern. “Hooks_converted” is sort of vague and potentially confusing. Converted to what? Is it too late to propose something like only_object_hooks or something? The 3 of y’all that worked on this issue are totally familiar with what hook conversion this is about, but I wonder about the other 99.99% of Drupal developers.

Thanks again!
-Derek

What about no_procedural_hooks

Right, something like that. Although I don't love configuration knobs of any sort that start with negatives. E.g. no_procedural_hooks: true is kinda weird. "Check this box to turn off the thing" is a UI anti-pattern. But setting has_procedural_hooks: false after conversion is weird, too, since if the parameter isn't defined, defaulting to TRUE is also maybe unexpected. I dunno, naming is hard. 😅

It's complicated by the fact that we don't even check the value if it exists we skip procedural.