Problem/Motivation

The bc layer for procedural hooks is slow.

Provide a way for modules to mark they have converted

Provide a way for modules that have not fully converted to mark a file as having no procedural hooks after a certain point.

Steps to reproduce

N/A

Proposed resolution

Complete skip APPROACH 2 seems to work

Add sibling file called module_name.skip.yml
Check for file existence in hook collector pass so we can skip procedural.
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

Attribute for partial conversion works

Add attribute called #[StopProceduralHookScan]
Check for attribute in line below and break inner foreach if it is found.
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

Complete skip APPROACH 1 (Does not work) 3490355-explore-ways-to

kept for reference.
Add flag in info.yml call skip_procedural_hooks_check
Check for flag in line below and pass to collectModuleHookImplementations so we can skip procedural.
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

Remaining tasks

Review

User interface changes

N/A

Introduced terminology

skip
StopProceduralHookScan

API changes

New attribute for stopping scan after procedural hooks.
New file for short circuiting all procedural hook scans.

Data model changes

N/A unless we figure out approach 1.

Release notes snippet

Issue fork drupal-3490355

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

nicxvan created an issue. See original summary.

nicxvan’s picture

nicxvan commented
Title: Explore ways to mark hook conversion » Explore ways to mark hook conversion status per module or file
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
commented

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.

nicxvan opened merge request !10402

nicxvan’s picture

nicxvan commented

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.

nicxvan’s picture

nicxvan commented

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

nicxvan’s picture

nicxvan commented

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

nicxvan’s picture

nicxvan commented

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.

nicxvan’s picture

nicxvan commented

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.

nicxvan’s picture

nicxvan commented

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.

nicxvan’s picture

nicxvan commented
Issue summary: View changes

nicxvan opened merge request !10405

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
Status: Active » Needs review
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Title: Explore ways to mark hook conversion status per module or file » Add procedural hook short circuit per module or file
Issue tags: +Performance
godotislate’s picture

godotislate
he/him
commented

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

nicxvan’s picture

nicxvan commented

Approach 2, !10405 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.

catch’s picture

catch
he/him
Primary language English
commented

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.

nicxvan’s picture

nicxvan commented

Do you mean moduleName.hooks_converted.yml?

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

nicxvan’s picture

nicxvan commented

Ok I refactored that bit, now we only check once per module per collector pass. We avoid including the .module file entirely and skip all checks and cache an empty procedural count. As I type that out, do we need to set a procedural cache at all?

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

godotislate’s picture

godotislate
he/him
commented

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");

nicxvan’s picture

nicxvan commented

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 opened merge request !10423

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

nicxvan’s picture

nicxvan commented

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.

catch’s picture

catch
he/him
Primary language English
commented

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.

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work
nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review
godotislate’s picture

godotislate
he/him
commented

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

godotislate’s picture

godotislate
he/him
commented
Status: Needs review » Reviewed & tested by the community

lgtm

  • catch committed 756c93fb on 11.x 
    Issue #3490355 by nicxvan, godotislate, catch: Add procedural hook short...

  • catch committed f03b175f on 11.1.x 
    Issue #3490355 by nicxvan, godotislate, catch: Add procedural hook short...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 11.1.x-dev
Status: Reviewed & tested by the community » Fixed

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 #3491275: Mark fully converted core modules as converted to OOP hooks 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

ptmkenny’s picture

ptmkenny commented

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?

nicxvan’s picture

nicxvan commented

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

nicxvan’s picture

nicxvan commented

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

#3491386: Using hooks_converted container parameter changes $dir during hook collection breaking collection of oop hooks.

dww’s picture

dww
we/he/they
commented

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

nicxvan’s picture

nicxvan commented

What about no_procedural_hooks

dww’s picture

dww
we/he/they
commented

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

nicxvan’s picture

nicxvan commented

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

Status: Fixed » Closed (fixed)

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

nicxvan closed merge request !10402

nicxvan closed merge request !10405