Problem/Motivation

hook_theme is not listed on Hook as something that can't have multiple implementations. Despite this the same module implementing it twice throws an error.

Steps to reproduce

  • Create a custom module my_module
  • Add a Hooks/MyFunctionality1.php file and define a #[Hook('theme')] method
  • Add a Hooks/MyFunctionality2.php file and define a #[Hook('theme')] method
  • Flush the cache and try to load a page

Proposed resolution

There are two calls to hook_theme:

  1. Registry::build uses $this->moduleHandler->invokeAllWith to iterate over modules which have a hook theme implementation and then call processExtension for each such module. This needs to be separated into two loops: one that collects modules like this in invokeAllWith and then calls processExtension .
  2. then in processExtension itself $result = $this->moduleHandler->invoke($name, 'theme', $args); should be changed to invokeAllWith too. 
        $list = [];
        $this->moduleHandler->invokeAllWith('theme', function (callable $callback, string $module) use (&$list) {
          $list[$module] = TRUE;
        });
        foreach (array_keys($list) as $module) {
          $this->processExtension($cache, $module, 'module', $module, $this->moduleList->getPath($module));
        }

      $result = [];
      $this->moduleHandler->invokeAllWith('theme', function (callable $callback, string $module) use ($name) {
        if ($module === $name) {
          $result = NestedArray::mergeDeep($callback($name, 'theme', $args), $result);
        }
      });

Remaining tasks

Add the changes, test them.

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Comments

macsim created an issue. See original summary.

cilefen’s picture

cilefen commented

See #3442009: OOP hooks using attributes and event dispatcher, the issue that added this exception, for a long discussion on the matter.

nicxvan’s picture

nicxvan commented
Component: base system » extension system

Any hooks that go through invoke can only be implemented once.

Some are true restrictions because a second implementation doesn't make sense.

Invoke runs for a single module and doesn't merge results.

We'd have to look at how to merge those results.

I'm not sure we want to do that, but it's worth a discussion at least!

nicxvan’s picture

nicxvan commented
Version: 11.3.x-dev » 11.x-dev
ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented

I found this behaviour surprising as well, coming from using the Hux module which does support multiple implementations of the same hook per module. I think for most hooks (including preprocess), being able to split up code into multiple implementations so they can be organized in a more logical manner for the project feels better and more intuitive from a developer's perspective.

ghost of drupal past’s picture

ghost of drupal past
he/him
commented
Title: OOP: Module $module should not implement $hook more than once » Allow multiple theme hook implementations per module
Category: Support request » Feature request

Sigh.

Of course multiple hook implementations are supported.

The change record documents this: "a module can implement almost all hooks multiple times as documented on the Hook attribute itself."

Let me repeat: almost all. Hook says: "One module can implement a particular hook multiple times in multiple classes, although see below for some exceptions... The following hooks may not have multiple implementations by a single module: hook_library_info_build, hook_mail, hook_help, hook_node_update_index". Yes. We missed hook_theme because at first we thought it'll need to remain procedural but that turned out not to be the case and although it was removed from the "needs to remain procedural list" it was forgotten to be added here. This does not warrant the sort of comment we got in #5.

I can see two things which need to be changed to support multiple hook_theme implementations in the same module:

  1. $this->moduleHandler->invokeAllWith('theme' ...$this->processExtension in Registry::build should use %container.modules% instead to call processExtension once per module. (modulehandler currently provides a getModuleList but I would rather not see it proliferate further, it shouldn't be there). Otherwise processExtension will be called once per implementation and as the documentation $type correctly stated further calls to processExtension clobber what's there.
  2. then in processExtension itself $result = $this->moduleHandler->invoke($name, 'theme', $args); should be changed to invokeAll

Warning: it's possible there are issues with this that doesn't occur to me right now and this issue might need to be closed unsuccessfully and then probably a separate CR as a followup to the original needs to be filed explaining why. We faced a number of issues with the theme registry already and if there's a dragon in there then for the sake of this minor enhancement I doubt rewriting processExtension is worth it.

mail is being phased out for Symfony mailer AFAIK

help and library_build_info could use separate issues to explore them. hook_help can't use invokeAll because if two implementations return a render array for the same route then all hell breaks lose. But invokeAllWith and a callback that ensures that only one implementation answers to a specific route allowing the big per route name switches to be split into multiple methods looks possible at first. Anyways, separate issue. As for node_update_index, to me it seems very likely the Hook documentation is wrong and it's already working for every module except comment, see NodeSearch::prepareResults.

nicxvan’s picture

nicxvan commented

Yes, what chx said is correct, it's a very small subset.

Please provide examples of which hooks you have multiple implementations in hux that do not work with the core hook attribute and we can review.

Edit: while hux won't throw an exception on these hooks it just silently eats the values of the multiple implementations except the last if the hook goes through invoke.

https://git.drupalcode.org/project/hux/-/blob/1.8.x/src/HuxModuleHandler...

ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented

I don't mean to come off as ungrateful for all the work that was likely involved getting hooks to this point and I definitely appreciate how much of a benefit this is to have in core. I was mostly speaking from the newly introduced OOP theme hooks and primarily preprocess hooks, where attempting to implement more than one of the same preprocess in a theme (e.g. preprocess_node) results in the fatal error in the issue summary. That could have been my mistaken assumptions about the limitations.

nicxvan’s picture

nicxvan commented

Does hux support preprocess hooks?

I don't see how it can.
I'd still like to see which hooks HUX allows multiple implementations the core doesn't we can compare the approach.

ghost of drupal past’s picture

ghost of drupal past
he/him
commented

The issue is about #[Hook('theme')]. Implementing that multiple times by the same module (and probably theme, too) is something I can see working and I already outlined the path to there. But, let me repeat: there might be dragons. Theme registry has them.

#[Hook('preprocess_node')] is a tough nut. If someone wants to try to crack it, they can ping me on Slack (@chx) I will explain what the problems are and they are welcome to try. Or, of course, I am more than happy to give a quote to implement it -- I have some educated guesses about how spl object hashing the hook attribute objects could replace the current pseudo function keys but nothing more. It's not going to be an easy task that's for sure. I would recommend amending the relevant CR https://www.drupal.org/node/3496491 to say this is not supported.

ghost of drupal past’s picture

ghost of drupal past
he/him
commented
Issue summary: View changes

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

nicxvan’s picture

nicxvan commented
Title: Allow multiple theme hook implementations per module » Allow multiple hook_theme implementations per module
Component: extension system » theme system

I think even though this is about hooks it's part of the theme system.

This doesn't require any change to how invoke works, but requires changes to where is invoked.

Chx outlines at a high level how to approach, but there are dragons in Registry, so be prepared.

macsim’s picture

macsim commented

The proposed approach calls processExtension() once per implementation rather than once per module. This raises several concerns:

  1. Same theme hook defined in two classes of the same module - if two classes both return a definition for the same hook key (e.g. both define 'my_template' => ['variables' => [...]] but with different variables), what is the expected behavior: deep merge of the definitions, last-wins, or an explicit error?
  2. Preprocess duplication - processExtension() calls collectModulePreprocess() on each invocation. Calling it twice for the same module risks registering preprocess functions twice for the same hook.

These edge cases suggest the fix may need to happen at a different level — collecting all implementations for a module first, merging their results, and only then passing the merged result to processExtension().