Handle module preprocess functions

This feels like a workaround/hack to handle the current system and then only modules. I'm not sure that's a good way forward.

getModuleImplementations() feels like a step back from the work that was done on ModulerHandler to encapsulate that logic and allow for different implementations (getImplementations() => invokeAllWith()).

And the template prefix currently ensures that there can only be one of those, but everyone could implement a template_preprocess_foo() hook and then it just picks the first, that will IMHO lead to confusion if people copy examples.

AFAIK there is an issue to reimplement hook_theme() using a plugin-like discovery. If theme implementations were plugins then the special template_preprocess_FOO() could become a preprocess method on that plugin and class inheritance could be used to solve some use cases that we've tried to improve for many, many years (primary example being template_preprocess_ENTITY_TYPE()).

I'd also prefer to see a solution that works for modules and themes, so that people don't need to learn two different mechanism for this. Preprocess *could* be a separate but parallel register and discovery process using #Preprocess (sure, we could also just have a Preprocess extends Hook class).

This feels like a workaround/hack to handle the current system and then only modules.

I mean we have to work within the current system, and modules and profiles are the only extensions that support OOP hooks. No hooks in themes or on behalf of themes are able to be OOP so this is no different. Setting up themes to allow OOP hooks is a whole different can of worms.

getModuleImplementations() feels like a step back from the work that was done on ModulerHandler to encapsulate that logic and allow for different implementations (getImplementations() => invokeAllWith()).

Except we don't invoke these, they are technically not hooks, we're collecting them.
It may make sense to get a different collector for them, but we can't invoke them since render is where they are called.

And the template prefix currently ensures that there can only be one of those, but everyone could implement a template_preprocess_foo() hook and then it just picks the first, that will IMHO lead to confusion if people copy examples.

I hadn't considered that, but it could be easily handled to gather all of them instead.

I'd also prefer to see a solution that works for modules and themes, so that people don't need to learn two different mechanism for this

Same comment, that is a separate issue, this is explicitly about preprocess.

Preprocess *could* be a separate but parallel register and discovery process using #Preprocess (sure, we could also just have a Preprocess extends Hook class).

Not sure how that is different than what we do here unless you mean a different HookCollectorPass.

We could subclass Hook for Preprocess, but that doesn't change the implementation here unless we add a different collector pass.

> I mean we have to work within the current system, and modules and profiles are the only extensions that support OOP hooks. No hooks in themes or on behalf of themes are able to be OOP so this is no different.

Depends on the scope we set for this. We don't necessarily have to make it work with the system that we have, we can also change the system. Like changing how hook_theme() works.

This is not preventing us from removing legacy hooks, because as you said, they are not actually hooks. So the question to me is whether we should force them into being pseudo-hooks just to fit them into the system we have, or if we should take a step back and think about how to actually improve this (If you only have a hammer, then every problem is a nail).

It *is* preventing us from removing .module files, yes. But the current direction won't help us remove theme .theme files, we need a solution for that eventually anyway IMHO.

> Setting up themes to allow OOP hooks is a whole different can of worms.

It is. But I think it is a can that we do want to open eventually, per above. As I said, I don't think this is a blocker for legacy hooks removal, unlike things like hook_module_implements_alter() and hook_hook_info(). So we don't have to push through a workaround.

> Except we don't invoke these, they are technically not hooks, we're collecting them.

We collect them so we can invoke them *eventually*, it just happens later on. As you said, they are not actually hooks. "Not sure how that is different than what we do here unless you mean a different HookCollectorPass.". This is in fact exactly what I was thinking about.

getModuleImplementations() has no restriction on preprocess. People *will* use it for other things if we don't. And an arbitrary limitation would be, well, pretty arbitrary. Adding methods to ModuleHandlerInterface also has its own BC issues.

> I hadn't considered that, but it could be easily handled to gather all of them instead.

That's not what I meant to suggest. template_preprocess_FOO() are even less hooks than regular preprocess functions. It's really just a very simple priority system. You can reliably assume that template_preprocess_FOO() is called first for FOO and then other preprocess functions can rely on what it prepared. that's really all there is to it. *If* we go with hooks, we could also just completely deprecate it in favor of the in-progress hook priority system and something like a special Hook attribute class that enforces a very high priority for it.

We can handle template, and also update this so module handler invokes the ones implemented by modules.

I'll respond more fully once we've worked out some kinks

Depends on the scope we set for this. We don't necessarily have to make it work with the system that we have, we can also change the system. Like changing how hook_theme() works.

Yes, that is true, I think it is out of scope here and this approach is not incompatible with that goal.

This is not preventing us from removing legacy hooks, because as you said, they are not actually hooks.

Yes, that is not my goal with this issue, I want to make steps towards removing .module files.

But I think it is a can that we do want to open eventually, per above

Again, I agree. I don't think this is the issue for it, and I don't think this harms that effort.

We collect them so we can invoke them

That is the approach now.

getModuleImplementations()

Is gone, as I mentioned on slack, this wasn't really ready for a deeper review yet :) that was a bridge attempt.

It's really just a very simple priority system

Yes, also I agree with your idea on priority, but we want to make it automatic.

Please continue not crediting me, thanks.

Posted a review comment, didn't review everything yet.

Expanding on one thing from my review:

> I'm still not convinced that this is the best way to progress through these problems

The thing is, what hasn't changed with the new implementation, is that preprocess hooks aren't really hooks as you said. you're not supposed to be able to call $moduleHandler->invokeAll('preprocess_node', $variables) because doing that only works on modules. Now that we do register them as hooks, you kind of can call it like that.

The result of that is that we store module preprocess functions twice. Once in the container, for modules, and then also in the theme registry (for each theme). That's not a huge issue for performance. preprocess functions are mostly a theme thing. claro/olivero each have about the same if not more preprocess hooks as all core modules combined (excluding test modules and themes).

The main question for me is whether or not this is meant to be the "final form" of preprocess hooks. I think it shouldn't "only" help us remove .module files but also in general improve upon DX around preprocess and theming.

One aspect of that is the whole template prefix stuff:

It's really just a very simple priority system

Yes, also I agree with your idea on priority, but we want to make it automatic.

How automatic? I think the template prefix stuff is pretty arcane and I wouldn't be surprised if a lot of developers don't really understand it. It was weird before with special prefixed functions, now we essentially declare it as a hook for a non-existent pseudo module "template". That's at least as weird.

I also reviewed the hook order issue. The ability to just say one hook is first or least is neat. Combined with this, IMHO this would be easier to understand than the template thing:

#[Preprocess('comment', order: Order::First)]

It's explicit, it tells me, this is the first comment preprocess hook/thing. I don't need to know about the special meaning of "template". Yes, it's possible to forget this, but it's also possible to not know about the template prefix and just implement it as a regular preprocess hook. It still works, the only problem is that some other module could be called before that. If that's a problem for your module, you can either report it and patch it, or you can just configure yours to run after this, also pretty easy with the new system. We could also have an attribute class like InitialPreprocess or FirstPreprocess that does the order. Easier to remember then.

We would need to change how we discover and register module hooks, right now the order doesn't work as we do one-by-one hasImplementations() checks on the module list. But I also proposed that and it would be faster with an invokeAllWith() instead of many hasImplementations() checks.

Just commenting here as a placeholder, I think you might be missing a couple of finer points.

I'll expand on these later, I'm focusing on the ordering issue at the moment.

The way it's done here preserves how ordering, deduplication and several other things are done, if we split it out things are way more likely to break.

hasImplementations only finds the OOP implementations, it's not used for themes.

template prefix stuff is pretty arcane

Yes, I think the new attribute makes it way more accessible, you don't need to know how it works under the hood.

getFunctionForLegacyInvoke never returns true, it either returns FALSE or the function.

Storing the implementations in the invokes array GREATLY simplifies this while making it significantly more BC.

On ordering, these hooks will not be able to be ordered with the attribute parameter, invoke only invokes one implementation, ordering is done by the preprocess functions array and if you need to alter the order you should use hook_theme_registry_alter which is compatible with this update.

I might have missed or glossed over a few details, I'll come back to this once I've address the Ordering issue: 📌 Hook ordering across OOP, procedural and with extra types Active

> getFunctionForLegacyInvoke never returns true, it either returns FALSE or the function.

the logic uses it as a true/false value in an if condition. sure, it doesn't return true, but we rely on it to evaluate to true.

Storing the implementations in the invokes array GREATLY simplifies this while making it significantly more BC.

On ordering, these hooks will not be able to be ordered with the attribute parameter, invoke only invokes one implementation, ordering is done by the preprocess functions array and if you need to alter the order you should use hook_theme_registry_alter which is compatible with this update.

Yes, I'm not suggesting to remove that.

What I'm suggesting is something like this:

    $this->moduleHandler->invokeAllWith('preprocess_'  $hook, function (callable $callback, string $module) use ($hook, &$cache, &$info) {
              $function = $module . '_preprocess_' . $hook;
              $info['preprocess functions'][] = $function;
              $cache['preprocess invokes'][$function] = ['module' => $prefix, 'hook' => 'preprocess_' . $hook];
    });

(untested)

Same result, you have a list of preprocess "functions" to call. But they do respect order and this is more efficient than calling hasImplementations() 100x.

You do this for $type == 'module', else you follow the existing logic.

> hasImplementations only finds the OOP implementations, it's not used for themes.

it also find legacy hooks in modules, otherwise that would be a BC break because we parse them. legacyInvoke() is only for special things.
But getFunctionForLegacyInvoke() *does* find theme functions if the string you pass in is a theme, which is exactly what you are doing. Because it's just a function, it doesn't validate $extension. That's what I mean with ModuleHandler doing theme stuff.

the logic uses it as a true/false value in an if condition. sure, it doesn't return true, but we rely on it to evaluate to true.

Oh yeah that is pretty obvious now that you put it that way, I was thinking too literally.

For the remaining bit that might be interesting!

Ok I took a look at this again, I think this should be a follow up, that would require a larger refactor of how these collections happen with processExtension as well.

This is really a minimal, backwards compatible way to allow Preprocess functions to be OOP in modules, not a refactor of Registry.

I pushed draft as a separate MR to use the invokeAll() idea and split processing between themes and modules. I didn't run any tests, it will fail

I feel quite strongly that this is the better solution (still with some concerns on the general direction). But, there are some problems with this that are more and less easy to solve:

1. The biggest problem is something that I wasn't aware of until I talked with @nicxvan about it on slack. We kind of shot ourselves in the foot with the BC layer optimization. preprocess isn't converted yet, but the optimization is active for it too, so we don't find those legacy preprocess hooks for modules that claim that they are fully converted. Suddenly they aren't actually fully converted anymore. I only have one idea for this at the moment. We rename the property and restart. There should be very, very few modules that already used it outside of core.

2. The template stuff. In initial version, I have the problem that some template functions actually exist outside of modules, so the BC layer doesn't find them, so I still need a function exists check for them. But that also finds those in modules, so I have to prevent them from being added twice. IMHO the cleanest solution for that is what I proposed in #29, that we drop the template thing completely and just use Order::FIRST regular preprocess hooks. We still have to find a solution for the functions outside of modules. We could put them back in system module as quickfix, support a Hooks namespace and discovery in components or the mentioned explicitly set callbacks on theme template definitions.

This isn't complete, I didn't remove the static legacy method on ModuleHandler, there's plenty of things that could be optimized (that we recollect the exactly same global preprocess hooks for all theme templates again and again, the leftovers of theme functions without preprocess, ...).

1 and 2 are the reason I didn't go this route unfortunately.

If killing that is on the table, and I'm on board with that we could explore this.

Checking once and prepending would be the way to go.

I think this round handling template like I did would be the least disruptive, we can use the legacy check for template functions no problem.

One other issue that my version solves that this approach would not is the include files section.

If a hook_theme function uses the file let those functions will never be picked up by hookcollectorpass, which I think lands us back to where we started with my original approach.

I think what you're doing here looks closer to what we want long term, but we have to deprecate procedural preprocess and the file key in hook theme or replace that entirely.

I still think optimizing the Registry is out of scope for this issue. Yes, it's not efficient to check for every $module_preprocess function there is for every theme HOOK there is but then again Registry has been doing so since Drupal 6 so why should this issue take the burden? The efficient way would be call this outside of processExtension to avoid O ( N * M ) and that is the level of optimization that is quite likely out of scope. It totes should do one OOP check and then preg_grep get_defined_functions() to pick the lot of them up in a single go, yes. (Potentially extending this to check for all preprocess functions in a single grep.) But not here, AFAIK.

Of course, I might be totally wrong.

There was only one real functional test fail (+ a random test fail but it's a tricky one). I was able to fix it, but for now only because not all template_preprocess_HOOK implementations have been converted. One thing that changes with my current approach is the order of preprocess functions. Right now it's:

template_preprocess
template_preprocess_HOOK
PREFIX_preprocess
PREFIX_preprocess_HOOK.

So, specifically, contextual_preprocess comes after template_preprocess_HOOK and for views, we actually rely on that to set the contextual links. That changes in the current implementation, because we add template_preprocess but then call all preprocess and then all preprocess_HOOK implementations.

The quickfix was to move the template_ BC check above the first invokeAllWith(), but that only works as long as views isn't converted. The problem is that my ORDER idea doesn't work, because that only works within a single hook, and the group stuff only works on alter hooks. we don't support invokeAllWith(['preprocess', 'preprocess_$HOOK']) and even then static groups wouldn't work.

The pseudo module template, as currently implemented would still work with this, we just have to single out template, check that first and then ignore it in the second run. A bit weird, but not really weirder than having a template pseudo module in the first place I think. So one option would be to stick with the template module approach until we can deprecate that by adding the ability to register initial/template preprocess in a different way, e.g. directly in hook_theme() as proposed in 📌 Convert theme hooks (defined by hook_theme()) to be objects Needs work .

Optimizations: Sure, some things are definitely out of scope here. The problem for me is that the first MR makes the discovery of those preprocess callbacks considerably more expensive, we switch from a single function_exists() check to calling hasImplementations() + that legacy method. Many, many times. And as discussed, 99% of those hasImplementations() calls return FALSE, so we need the second function too. And looking at \Drupal\Core\Extension\ModuleHandler::getHookListeners(), we currently don't even statically cache misses. That means we have tens of thousands of extra, completely unnecessary calls going into hasImplementations(), then into getHookListeners(), then into the event dispatcher, then an array_keys() and an array intersect and an array cast. All those things are fast on their own, but we do it a lot. It's "just" for the one-time theme registry build, but that's a blocking operation on e.g. the first request(s) after a cache clear. So if it's out of scope then maybe we'd need to do that first, in a separate issue.

Even if we don't go with my implementation, we can at least optimize the first MR to not do those extra checks for themes. Right now it even does it in a second loop that specifically only runs on themes, I commented on the MR. noticed that while working on my changes. And the first block IMHO still makes sense to split between modules and themes, so we don't have to go through the module handler for themes as well.

Then the decision between my and your implementation comes down to whether or not we're willing to accept #36.1. #36.2 is off the table anyway unfortunately.

As mentioned before, for me, it all comes down to much performance regressions and technical dept (like new methods, even if internal, the template pseudo-module, which requires changes such as the one in \Drupal\Core\Extension\ModuleHandler::getHookListeners() as well as things like #TemplatePreprocess that we might replace again with a different solution) we're willing to accept to push this through. Even with my approach, I still have concerns. I understand that we're trying to get rid of .module files, but I still don't have a full picture of how the preferred final state should look like (for example in regards to themes) and right now I'm not convinced that the gains outweigh the cost here.

All those things are fast on their own, but we do it a lot. It's "just" for the one-time theme registry build, but that's a blocking operation on e.g. the first request(s) after a cache clear. So if it's out of scope then maybe we'd need to do that first, in a separate issue.

Honestly I think it's a follow up unless others have the same concern, as you mentioned all of the underlying bits are fast, hasImplementations is fast.

This is true for the theme loop too, but we do cache the legacy check.

Speaking of which, we need to do these in the foreach and with a legacy check because theme arrays can add files to the list for a check and some template functions exist outside of modules.

If we kill the stop procedural scan work like I did for your MR hasImplementations will still miss the majority of it's calls since most possible preprocess functions do not exist, and we still need the legacy check to handle preprocess outside of modules AND files included by the theme hook (these will never be scanned by HookCollectorPass)

I think I mentioned this above, what you have here is closer to what I think this will look like far future, but we have a number of really nasty deprecations in between. Namely the file array key in hook_theme.

A couple of further points, personally I've never used template_preprocess_HOOK, but as you found it IS used so I think we need to preserve the order of the functions which my approach does, I think yours could too, but we'd be jumping through many hoops to ensure that.

The approach I took adds the ability to have preprocess hooks as OOP implementations of hooks, a two helper preprocess attributes for DX and, crucially, changes as little else as possible.

I really appreciate the amount of time you've spent reviewing this, and the questions you raise are great, I think most just need to be answered in follow ups.

I've had a chance to look at the alternative approach and I am not sure this is the right approach and I think it might be a premature optimization.

If we do want to optimize the first approach a bit we could kill the procedural short circuit so hasImplementations will return true, then the legacy check is only for the theme.

If we really do want to try to optimize this then we need to do it the level above in build(), cause we're already doing a double loop over modules for every hook_theme returned. That definitely should be a follow up, this is already a big enough change.

Another thing we could do is store a reverse map of the preprocess functions to check before doing anything and if it's the list then we know we've already checked it and added it to the preprocess functions and preprocess invokes arrays.

We could also store misses in hasImplementations, but I'm not convinced that is worth it.

I put a few hours into optimizing Registry with invokeAllWith as far as it is possible to do that and I got reasonably close to having a working branch. However, I hit a backwards compatibility brick wall which equally affects my attempt, berdir's attempt and any and all attempts using any ModuleHandler method which deal with by-the-hook implementations. The problem is this part of Registry, code from the original

                    if (isset($info['template']) && function_exists($name . '_preprocess')) {
                        $cache[$hook]['preprocess functions'][] = $name . '_preprocess';
                    }
                    if (function_exists($name . '_preprocess_' . $hook)) {
                        $cache[$hook]['preprocess functions'][] = $name . '_preprocess_' . $hook;

The order of preprocess functions for two modules and just one hook is:

1. module1_preprocess
2. module1_preprocess_hook
3. module2_preprocess
4. module2_preprocess_hook

any module handler attempts however lead to

1. module1_preprocess
2. module2_preprocess
3. module1_preprocess_hook
4. module2_preprocess_hook

Can this be fixed? Yes it can be but I think it makes the entire endeavor pointless speed wise and the code is ... well, look:

      $module_list = array_flip(array_keys($this->moduleHandler->getModuleList()));
      usort($info['preprocess_functions'], function ($a, $b) use ($module_list, $cache) {
        $a_module = $cache['preprocess invokes'][$a]['module'];
        $b_module = $cache['preprocess invokes'][$b]['module'];
        // For the same module, foo_preprocess should come before
        // foo_preprocess_bar. For different modules, keep module order.
        return $a_module === $b_module ? strlen($a) <=> strlen($b) : $module_list[$a_module] <=> $module_list[$b_module];
      });

nicxvan → changed the visibility of the branch deprecatehmiav6 to hidden.

I am going to move over the combined approach, note that this combined approach bypasses the procedural scanning skip.

Once this is up I will create yet another branch that adds the skip back in and updates modules with preprocess functions.

Adding the related and postponed conversion issues so I can track them for the full MR.

I chatted with Catch on slack about the skip procedural scan, since most of core is using that and those preprocess hooks will now be skipped. See this CR: https://www.drupal.org/node/3490771 → and the related issues.

We have three options:

1. Undo the skip work and bypass it in this issue for clean up in a follow up. Release CR saying that the former one is no longer valid. https://git.drupalcode.org/project/drupal/-/merge_requests/10825 uses this approach.
2. Keep the skip, but add a legacy check for preprocess that ignores the skip. Downside is we have to check for legacy each time. https://git.drupalcode.org/project/drupal/-/merge_requests/10681 uses this approach.
3. Keep the skip, and move / update modules so that preprocess functions are not in the skip section for modules. Release CR saying that if you use the skip you need to account for preprocess functions now. Needs a new MR.

Catch mentioned that approach 3 is preferred, in order to make this reviewable I will create a new MR based off of 10825 that reverts the undo skip part and moves all preprocess functions that need to be moved. I will also create an interdiff MR between the two so it can be reviewed.

nicxvan → changed the visibility of the branch 3495943-handle-module-preprocess-invokeall to hidden.

nicxvan → changed the visibility of the branch 3495943-handle-module-preprocess to hidden.

Actually handling the skip bit wasn't too bad. Any modules that used the hooks_converted parameter and had preprocess I toggled to false.
There are only three modules with preprocess using the #[StopProceduralHookScan] attribute so I moved the attributes where necessary.

I've attached the audit.

Here is the combined MR: https://git.drupalcode.org/project/drupal/-/merge_requests/10826

Here is the Interdiff: https://git.drupalcode.org/issue/drupal-3495943/-/merge_requests/1/diffs Note the interdiff tests will not run. Use 10826 for tests.

nicxvan → changed the visibility of the branch 3495943-preprocess-combined-approach to hidden.

fwiw I think it would be fine to ignore the per-file skip - the more we're able to convert, the less it's needed anyway and it's likely only core will use it.

I do think we will want the per-module skip though, especially if we're supporting the bc layer until Drupal 13, so it'd be better to update what it means asap if that's what we need to do.

Absolute worst case we could 'rename' the per-module skip - ignore the current flag and add a new one.

> Absolute worst case we could 'rename' the per-module skip - ignore the current flag and add a new one.

That was my suggestion, yes. Wouldn't be a huge difference for core. If we need to switch most flags again as an intermediate step we can also go remove it and add it back with a new name once converted. And if there are indeed a handful of contrib that used it then we wouldn't break them.

The in-file-skip attribute is currently quite useful for a handful of modules that have lots of functions like, such as locale. but unless I'm missing something, it's also really just necessary at the moment because we still have some gaps in hook support, such as requirements, info and module_implements_alter. once those 3 and this land, we don't need a partial skip anymore, do we?

The current mr already contains all the changes necessary for all preprocess hooks to be picked up. I toggled about a dozen modules to false, moved a couple in per file and postponed the remaining two issues to mark.

It's not too bad.

@berdir you are correct on the reason why per file is needed.

Here is an interdiff showing the updated modules.

Once this is in I'll open follow ups to convert the preprocess and then we can toggle them back.

> 16 modules had ben marked as fully converted, I changed the parameter to false

Yes, but we can only toggle core modules, not contrib that might have already picked this up.

As of today, 4 modules have added the flag: https://git.drupalcode.org/search?group_id=2&repository_ref=&scope=blobs.... That is exactly 4 more than I expected would do so ;) Based on a quick check, none of them would break because of this. But it takes another 5 or so month until 11.2 comes out and more will add it and then possibly break.

Technically, it *is* a BC break. Whether or not we care about that is for a core/framework/release maintainer to decide. IMHO we should do one of the following two options:

1. We play it safe and rename the parameter. Update the change record and state that modules should probably just wait with implementing that until 11.2 or if they care enough, add the parameter for 11.1 and then also add the one for 11.2 once they updated preprocess (if they have that). We lose the optimization on a handful of modules.

2. We keep the name and just toggle it for core as you did where necessary. We risk breaking a few modules. In this case, we should IMHO update the change record _now_ to state that if your module has preprocess hooks, then you should either not use it or you will need to touch it again and fix it for 11.2 (or whenever this issue lands. The LegacyHook approach should work fine on 11.1 because it doesn't parse preprocess functions anyway, LegacyHook or not, and they don't go through the ModuleHandler. there.

Referencing some template_preprocess related issues that we might want to revist/re-evaluate afte this lands:

📌 Replace template_preprocess() and possibly other things with Twig global variables/functions Active
📌 Move template_preprocess, _template_preprocess_default_variables into services Needs work

Didn't review the new MR yet.

Let me add a note referencing this issue to the CR at a minimum.

Safest thing would be to rename them both and we can find and replace in this MR.

We still toggle the ones I did so they are picked up with the new name and convert the preprocess in a follow up.

I'll hold off, until we get directing.

Maybe we call it skip_procedural_scan and #[ProceduralScanStop]?

@catch mentioned on slack we should rename since this is likely the last time, the only other hooks besides this are:

• hook_uninstall
• hook_post_update_NAME
• hook_install
• hook_install_tasks
• hook_install_tasks_alter
• hook_schema
• hook_update_N
• hook_update_last_removed

I see no way to ever invoke those with moduleHandler so we should be good with the rename, just need to decide on names.

One note too is that we still want to toggle the flags on the ones I did so we can move the preprocess conversion to a follow up beyond the ones we did here for confirmation.

skip_procedural_hook_scan maybe?

I like skip_procedural_hook_scan

Ok I renamed StopProceduralHookScan.php to ProceduralHookScanStop and hooks_converted to skip_procedural_hook_scan.

I added a change record.

The #[LegacyHook] attribute will allow contributed modules to support versions older than 11.2 when this comes out.

Since Registry only used function_exists prior to this patch we don't have to worry about LegacyHook preventing 11.1 - 11.2 picking up the functions, Registry will collect them as it always did.

Renaming skip_procedural_hook_scan and StopProceduralHookScan ensures that they will be picked up in 11.2 before conversion.

smustgrave → closed merge request !10681

smustgrave → closed merge request !10762

smustgrave → closed merge request !10823

smustgrave → closed merge request !10825

berdir → changed the visibility of the branch 3495943-preprocess-combined-approach-with-skip to hidden.

nicxvan → changed the visibility of the branch 3495943-preprocess-combined-approach-with-skip to active.

Brought back the most up to date branch and applied feedback.

> In a followup, refactor ModuleHandler and ThemeHandler so they have a common base (they used to be the same thing after all) and provide OOP capability for hooks by themes. This likely needs

This belongs more in the meta issue than this, but keep in mind that there are good reasons why themes can't and must not be able to implement any hook. Currently, we only invoke the special theme-hooks (like form alter) for the active theme and its parents. And I think we shouldn't change that. And if we don't, then it's important that themes are not able to implement hooks that may be cached without considering the theme. Because it results in a completely unpredictable situation then. Think of a theme that implements hook_entity_type_alter(), the change will either be there or not depending on whether the cache clear happens in the frontend or the backend theme. The ability that themes can implement any hook was removed on purpose and we need to keep it like this.

Currently, we only invoke the special theme-hooks (like form alter) for the active theme and its parents. And I think we shouldn't change that.

Yes, the solution mentioned would have to account for this of course, we never intended to drop it.

This is a challenging issue. So far I think I've been the only one to really review this, and my reviews and push-back already resulted in several improvements (IMHO), but there are still open things that I'm personally am not comfortable with. I'm tagging this as needs framework manager review to make some decisions. It's already a long issue with lengthy discussions, so I'm trying to summarize this from my perspective.

One one side, we have good arguments to get this in as soon as possible:

a) This is *the* blocker to getting rid of .module files, preprocess functions are the only thing left that must either be in .module files or .inc files (we also want to deprecate that once core is converted).

b) The awkward situation with the optimization flags that we have to break to be able to optimize the implementation here. Earlier approaches relied on more expensive loops and function-with-legacy checks that required new public methods on ModuleHandler. @catch agreed to renaming it. But that means we need to get it in asap so that only few modules already use the current flag, which will stop working (it won't break, we just lose the optimization for them).

However, there are IMHO a considerable number of workarounds/problems as well as performance/architectural issues with the current implementation. @nicxvan and I don't really agree on whether or not they are problems and their severity, so we need input.

1. To deal with the special template_ prefixed preprocess callbacks, this introduces the concept of a special "template" module, template callbacks are converted to hooks registered in the name of that module. It's unlikely but not impossible that this will cause some super weird conflicts, such as a real-world template module that someone has in a custom project.

2. It increases the size of the theme registry and I guess also the time to process it due to how it stores the callback. Current tests place it at around 17% larger for umami. An extra map that maps functions-that-might-no-longer-be-functions to ModuleHandler::invoke() arguments. Right now it does that for all, I think it should do that only when a function actually no longer exists and belongs to a module. One reason for this is that it attempts to provide BC to hook_theme_registry_alter() hooks that mess with preprocess functions. I guess the question is whether or not we actually need to provide BC for that. I think that is very rarely needed, never had that use case myself. unlike regular hooks, what preprocess does isn't actually final and you can usually just undo it in your own preprocess.

3. Related, it conflates modules and theme callbacks, so theme callback are routed through ModuleHandler::invoke() and it's legacy invoke system. This is something that we could just not do, @nicxvan and I just fundamentally disagree on whether or not this is a good idea :) Supporting OOP preprocess (and hooks) in themes is a a later step with several blockers, that much we agree on.

4. There is an additional lookup step where we use all defined functions and preg_match() to find preprocess functions for theme suggestions that have no corresponding template. (e.g. mymodule_preprocess_node__article() when the current theme has no node--article.html.twig template). This does not work yet with OOP preprocess as they are no longer functions. That means we'll tell modules to convert preprocess functions but some of them just aren't going to work. And it's fairly complicated and dynamic (depending on the current theme) which ones are going to work and which not.

There's some more minor things, but I think those are the primary issues that I see.

I have a number of ideas on how to address this, which are more or less complex to resolve. We don't really agree on what's follow-up/blocker. One challenge is the very specific sort order of preprocess functions (template_preprocess, template_preprocess_FOO, hook_preprocess(), hook-preprocess_FOO()).
* Getting rid of template_preprocess (actually surprisingly easy: 📌 Move template_preprocess, _template_preprocess_default_variables into services Needs work )
* Register template_preprocess_* callbacks in hook_theme(), either as callbacks (not very pretty, but easy to do) or converting the whole thing to classes with methods)
* at that point, hook_preprocess() at least for modules could be a regular hook that we call after the registered callback.
* For point 4, we do have all discovered hooks already in the event listener, it should be possible to return everything that's preprocess prefixed, we just need to expose it somehow and decide on follow-up or part of the initial version.

Definitely later:
Since we'll need a solution for 4 anyway, we could turn the whole preprocess discovery on its head and *only* do that. Instead of discovering them one-by-one for each template, we loop over all known and already registered preprocess things and add them to each definition as we see fit. Or even go further and support multiple hooks in invokeAllWith(), like we already do with alter(), just call that. Not sure how much slower that would be. When we no longer have to deal with template_ stuff being in the list and support this whole thing also for themes, this becomes feasible.

The feasibility of both the short-term and long-term ideas depends on the question of whether or not the preprocess function list/registry is something we consider an API and provide BC for.

Thoughts? In short, how much technical dept are we willing to accept to unblock .module removal?

I haven't reviewed the MR in depth yet and this doesn't respond to everything in the comment, because I'd probably need to do that review to have a useful opinion on the rest.

One reason for this is that it attempts to provide BC to hook_theme_registry_alter() hooks that mess with preprocess functions. I guess the question is whether or not we actually need to provide BC for that. I think that is very rarely needed, never had that use case myself. unlike regular hooks, what preprocess does isn't actually final and you can usually just undo it in your own preprocess.
[...]

The feasibility of both the short-term and long-term ideas depends on the question of whether or not the preprocess function list/registry is something we consider an API and provide BC for.

I also can't remember ever having or wanting to do this, or even thinking about it, whereas I've used hook_module_implements_alter() dozens of times probably. Also, even if modules are using it, we can and do break 'data structures' in minor releases. Is there a way to stop things from running with the new system though - I guess you could use #[Remove]?

To deal with the special template_ prefixed preprocess callbacks, this introduces the concept of a special "template" module, template callbacks are converted to hooks registered in the name of that module. It's unlikely but not impossible that this will cause some super weird conflicts, such as a real-world template module that someone has in a custom project.

Couldn't this already be a problem if there's a 'template' module?

Or even go further and support multiple hooks in invokeAllWith(), like we already do with alter(), just call that. Not sure how much slower that would be.

IME performance problems with preprocess are the amount of stuff we do in the preprocess hooks themselves, like the monster that is template_preprocess_node(). #649530: Creating the cool graph hangs until PHP times out → was one idea to reduce this, hasn't gone anywhere though. Not sure hook invocation itself is going to make much of a dent.

http://codcontrib.hank.vps-private.net/search?text=_theme_registry_alter there are some implementations in contrib... some funky stuff there.

Just to add to @berdir's great summary.

I think a) and b) are enough to justify this change.
I think there is also c) it significantly reduces the times we need to lap around looking for module_preprocess functions because we now do it once in build, whereas before we checked on every module and every prefix. This is a great change.

1. We do add the template pseudo module, but that is one line in HookCollectorPass and one line in ModuleHandler. As @catch noted, this would likely break currently if someone named their module template, we also significantly improve the DX here by adding #[TemplatePreprocess]`
2. The theme registry is larger, but 17% isn't too bad especially considering it is only a lookup for invoke and as @berdir notes, there are already issues that will reduce that further. The bc check for custom callbacks added theme_registry_alter is unfortunate, but there is an explicit test in core for this, and as @alexpott points out contrib does use this.
3. As @berdir notes, I fundamentally disagree that using invoke like this is a problem, it can be cleaned up when we add the ability for themes to be OOP, invoke was doing far weirder things before 11.1 so this isn't too far astray.
4. That still works, it uses the prefixes not the actual functions unless I am missing something.

Ok number 4 has been addressed.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

If this needs rebase I will, but this still needs review.

@berdir created this 📌 Explicitly register template_preprocess callbacks in hook_theme() Postponed on top of 📌 Move template_preprocess, _template_preprocess_default_variables into services Needs work as an alternate to how we are handling template_preprocess and template_preprocess_HOOK in this issue.

It does include a BC break if we do it that way, but we could include it in this issue if that is preferred and add a deprecation for template_preprocess_HOOK calls in a follow up pretty easily.