Problem/Motivation

In Drupal < 11.3, it was possible to define a theme preprocess function as THEMENAME_preprocess_HOOK() inside a module.

The reason to do that would be to to only run the preprocess if both the module is installed, and the theme is active (as current or base theme).

In Drupal >= 11.3, these are either fully ignored, or added in the wrong order relative to sub-theme preprocess functions.

To be specific:

  • If a regular (without "__") THEMENAME_preprocess_HOOK() is defined in a MODULENAME.module file in a module directory, it is not added.
  • If a preprocess for a "theme hook suggestion" (containing "__") THEMENAME_preprocess_HOOK__SUFFIX() is defined in a MODULENAME.module file, it is either not added, or added at the end of the list, after implementations by a sub-theme. (depends if that sub-hook is known at the time)

The same problem also occurs for other theme hooks like THEMENAME_something_ALTER() - to be verified in tests.

Steps to reproduce

There will be test cases in the MR.

Proposed resolution

First, we need A LOT more tests to cover this fragile system.

For a solution, we need to extend the function search space in ThemeHookCollectorPass to functions in module files.

To avoid parsing the same file more than once, we can cache plain function lists per file, instead of hook lists per file.

Remaining tasks

User interface changes

Introduced terminology

API changes

Restoring behavior of 11.2 could be seen as regression relative to 11.3.

Data model changes

Release notes snippet

Comments

donquixote created an issue.

nicxvan’s picture

nicxvan commented

I would welcome more tests, but this particular issue should be closed works as designed.

It was an explicit bc break decision when adding oop support: https://www.drupal.org/node/3551652

nicxvan’s picture

nicxvan commented

THEMENAME_preprocess_HOOK__SUFFIX getting added like that will go away once .theme files are removed and we can refactor https://git.drupalcode.org/project/drupal/-/blob/main/core/lib/Drupal/Co...

That also explains why they are at the end of the list now too and only if the suggestion is known.

donquixote’s picture

donquixote commented

Ok, I should have checked that change record.

But...

Edge cases that are no longer supported:

  • Themes cannot implement oop hooks on behalf of other themes.
  • Modules cannot implement hooks on behalf of themes.

Why do we break stuff in a minor? It has become a bad habit in core.

We have used this pattern for a long time, for a specific reason:
To only run that preprocess if both the module is installed and the theme is active.
Now this is failing in non-obvious ways, because in some cases the function is removed, in other cases the order changes.

https://www.drupal.org/project/drupal/issues/3544715#comment-16307074 (@nicxvan)

Refers to the fact that currently if you create gin_form_alter in a custom module when the theme executes it's hooks that would run.
This behavior is unintentional [...].

How did we determine that it is "unintentional"?

and incompatible with OOP hooks

It is not.
It would have just been more work to continue supporting.

https://www.drupal.org/project/drupal/issues/3544715#comment-16307591 (@berdir)

I think that information probably belongs somewhere but not in this CR. Because it's not targeted towards themes, but modules. If anything, we'd need to document the opposite here, that we do not support defining hooks for modules in themes, which technically was also possible now, it was just very unpredictable and only worked if the theme was already loaded and would always be loaded when the hook is invoked.

Not really unpredictable.
It has worked for us reliably for a long time.

Also, maybe we can clarify that we don't support *discovery* of such hooks anymore. But it's actually very easy now (but not explicitly supported and covered by BC, IMHO) to put stuff in the theme data parameters, just like #3502432: Make hook testing with kernel tests very simple is doing. You could easily add an arbitrary callback to a specific theme in a service provider. And I think there are interesting use cases or that, we've struggled a few times hardcoded form alters such as claro_form_node_form_alter() that we wanted to undo and currently the only option would be to define a subtheme if claro.

I don't get this part from berdir :)

(continuation next comment)

donquixote’s picture

donquixote commented

THEMENAME_preprocess_HOOK__SUFFIX getting added like that will go away once .theme files are removed and we can refactor https://git.drupalcode.org/project/drupal/-/blob/main/core/lib/Drupal/Co...

This is hopefully only in Drupal 12. So not relevant here.

Not really unpredictable.

Well maybe it was..

That also explains why they are at the end of the list now too and only if the suggestion is known.

Maybe that "only if the suggestion is known" was already an issue in the past.
I would have to test that.

donquixote’s picture

donquixote commented

It would have just been more work to continue supporting.

This is also on me for not doing more reviewing.

It was a reason why long ago I wanted to add more tests before doing any of the work on hooks.
But my dedication of time to core dev is far less than others, and not consistent.

donquixote’s picture

donquixote commented

Workaround

A workaround we found was:
- Move these functions to the theme
- Add a custom attribute #[RequiredModule('MODULENAME')] on each function.
- Use hook_theme_registry_alter() to remove those preprocess if the module is not installed.

This only works if you control both the module and the theme.

The other option would be:
- Rename the functions to MODULENAME_preprocess_*
- Add custom attribute #[RequiredTheme('THEMENAME')]
- Use hook_theme_registry_alter() to remove those preprocess if the theme is not active as a base theme or current theme.

In both cases this has to be a custom attribute, we cannot add it in core, because it has to work for older core versions.

For the second option, removal is not trivial because hook_theme_registry_alter() does not know what is the theme name being processed.

nicxvan’s picture

nicxvan commented

Rather than using custom attributes make them module hooks and use the theme manager to check the active theme.

And use hook theme to ensure they are picked up.