Problem/Motivation

layout_discovery registers theme implementations for all layouts, modules and themes alike.
It includes a 'base hook' that allows template_preprocess_layout() to run for all layouts.

When a module registers a theme function that points directly to a theme's template, twig_theme() discovers the theme's template and uses that to overwrite the theme definition, overwriting the 'base hook', preventing the preprocess from running.

Proposed resolution

Use NestedArray::mergeDeep() instead of += to merge the results into the final set, the recursive merge will let previously-found keys to persist into the final set.

Remaining tasks

N/A

User interface changes

N/A

API changes

There are rare cases where this change will cause preprocess functions to run that previously weren't running before.
However these were supposed to run, and the documentation implies that they run.
There should be no adverse effects from this.

Data model changes

N/A

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett’s picture

- if ($this->theme->getEngine()) {
+ if (FALSE && $this->theme->getEngine()) {

Obviously this change will break many many things, but it fixes this one instance.

tim.plunkett’s picture

This fixes the one case, curious to see what else breaks.

The last submitted patch, 2: 2861840-layout-2-PASS.patch, failed testing.

The last submitted patch, 2: 2861840-layout-2-FAIL.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: 2861840-layout-4.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.74 KB
1023 bytes

We need the deep merge to handle things like 'base hook' and 'incomplete preprocess functions', but can't let it end up with both 'function' and 'template'.

aspilicious’s picture

Looking fine, will test this tomorrow.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

I tested this on our production site and it works perfectly.
Code and test looks good as well.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -565,10 +566,20 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
+        // Before merging the new result with the original information, ensure
+        // that the final result does not end up with both a template and a
+        // function defined.

I think the documentation here could be improved. Could we state more clearly why we want to avoid both being defined in the theme registry?

We should also create some test coverage outside the layout_discovery module since this is a bug in the theme system.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Agreed on both, thanks @lauriii!

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.14 KB
3.73 KB

Working on the test, posting the comment change.
Also while rewriting the comment I realized we only care about the case of 'template' needing to override 'function'.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Issue summary: View changes
FileSize
2.16 KB
5.89 KB

The layout-specific test is largely overkill with this new generic test, as it is testing the same thing.
However, I think it's worth keeping since it proves (past the existing unit tests) that themes can provide layouts.

tim.plunkett’s picture

Title: Layouts cannot safely be provided by themes » Preprocess functions are not merged when a module registers a theme hook for a theme-provided template
Component: layout.module » theme system
aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Seems like both points are covered.
Second try for RTBC.

lauriii’s picture

The bug fix itself looks good now. Thank you also for clarifying the documentation, it looks good for me now. I was manually testing this and added test coverage to ensure that multiple preprocess functions are added and ordered correctly since that was one of the first things I wanted to test manually and was still lacking test coverage.

I looked back to the history of this line and it looks like this bug exists in Drupal 7 as well and could be backported there later.

The minor BC break being caused by the change needs sign-off from another maintainer.

The last submitted patch, 17: preprocess_functions-2861840-17-test-only.patch, failed testing.

joelpittet’s picture

The proposed solution and test look great. I'll +1 this from another theme system maintainer's point of view.

This one hunk looked a bit suspect could you explain why this is needed?

+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -565,10 +566,18 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
+        // If a theme implementation definition provides both 'template' and
+        // 'function', the 'function' will be used. In this case, if the new
+        // result provides a 'template' value, any existing 'function' value
+        // must be removed for the override to be called.
+        if (isset($result[$hook]['template'])) {
+          unset($cache[$hook]['function']);
+        }

Not sure I understand why this was needed. Could you add this to the Proposed solution in the IS?