Problem/Motivation

PreprocessManager::preprocess() calls getSuggestions() for every theme hook
rendered on a page, whether or not any Preprocess plugin is registered against
a suggestion of that hook. getSuggestions() does:

$suggestions = $this->moduleHandler->invokeAll('theme_suggestions_' . $hook, [$variables]);

That re-invokes every hook_theme_suggestions_HOOK() implementation a second
time. The problem is when it happens.

Core's ThemeManager::render() invokes those hooks before the preprocess
chain runs, passing the variables originally given to theme(). This module
invokes them from hook_preprocess(), and preprocess_theme_registry_alter()
deliberately moves preprocess_preprocess() to the end of every hook's
preprocess function list. The implementations therefore receive variables that
the entire preprocess chain has already rewritten.

Implementations are written against the pre-preprocess structure and are not
defensive about it, because under core's contract they never see anything
else. Core's own node_theme_suggestions_node() is one of them.

Concrete failure

Rendering a node whose view display contains a field group, where the fields
in that group are all empty:

Warning: Undefined array key "#node" in node_theme_suggestions_node()
         (line 494 of core/modules/node/node.module)
Warning: Undefined array key "#view_mode" (line 495)
Deprecated: strtr(): Passing null to parameter #1 ($string) is deprecated (line 495)
Error: Call to a member function bundle() on null (line 498)

All four are one page view; the first three are the cascade before the fatal.

The trigger is field_group, behaving legitimately.
field_group_build_entity_groups() binds $element as a reference to
$variables['elements'], and field_group_remove_empty_display_groups()
(field_group.module, line 1170 in 4.0.0) ends with:

// Reset an empty group.
if ($empty_group) {
  $element = [];
}

That function recurses over groups, but is first called on the top-level
element. A node with no visible non-empty children leaves $empty_group TRUE,
so the whole array is blanked, #node and #view_mode included.

Under core's ordering that is harmless: node_theme_suggestions_node() has
already run and field_group is finished with the array. It is fatal only
because this module runs the suggestion hooks again afterwards.

Worth noting: on the site where this was found, no Preprocess plugin
targets node at all. The only plugin in the codebase targets media. The
entire failing code path was work that no plugin had asked for. 22 of 113
published nodes of one bundle returned HTTP 500.

Steps to reproduce

1. Install Preprocess and Field Group.
2. Add a field group to a content type's default view display.
3. Create a node of that type, leaving every field in that group empty.
4. View the node.

Result: HTTP 500 with the backtrace above. Uninstalling Preprocess, or
removing the field group, makes the page render.

Reproduced on Drupal 10.6.17, Preprocess 2.2.3, Field Group 4.0.0,
PHP 8.3.20 (production) and PHP 8.4.14 (local).

Proposed resolution

Only resolve suggestions when a plugin is actually registered against one.

This is a strict narrowing rather than a behaviour change: if nothing targets
a suggestion of the hook, the resulting suggestion list is discarded unused,
so computing it is pure cost and pure risk. Ordering relative to
doPreprocess() is preserved, so when suggestions are computed they are
still computed from pre-plugin variables exactly as now.

Adds a hasSuggestionPreprocessors() helper and gates the getSuggestions()
call on it.

Remaining tasks

This does not fix the underlying contract violation, and deliberately so. A
site that does register a plugin against a suggestion (say node__article)
will still re-invoke the suggestion hooks late and can still hit this. Fixing
that properly means not re-invoking the hooks at all and instead reusing what
core already resolved, which is a larger change and likely belongs in its own
issue. The change proposed here removes the failure for every site that does
not target suggestions of an affected hook, which looks like the common case.

Test coverage for the gate would be worth adding.

User interface changes

API changes

Data model changes

Issue fork preprocess-3624397

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:

Comments

sean_fremouw created an issue. See original summary.

sean_fremouw’s picture

Status: Active » Needs review