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