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?

aspilicious’s picture

DS implemented a workaround but other layout modules are probably being hit by the same bug. Joel, you should ask tim for more information if you need some.

markcarver’s picture

but other layout modules are probably being hit by the same bug

Yes, I can confirm that this is happening in other layout modules #2872583: Empty "layout" and "settings" in _bootstrap_layouts_preprocess_layout().

markcarver’s picture

Related core issue (or possibly a dup of this issue?).

tim.plunkett’s picture

#19
The docs say that template will be used if both are present, but the code checks function first. If they are merged together, that split needs to be manually done here.

himerus’s picture

+1 on this so far after some initial testing.

I'd been 'fighting' for a while why I couldn't get omega_preprocess_layout() to be fired.
I've been working on an integration (currently template based and not custom class) of layout_discovery into the 5.x branch of Omega, and getting some final logic in preprocess_layout is the last bit I need to get all features for responsive region management in order to assign some final classes based on populated regions.

Will be running this patch locally for testing, and looking more over the DS solution so that I don't need to require Omega users to patch Drupal for this functionality until this is in core.

Definitely have some additional investigation/discovery to do around this issue and #2862683: 'base hook' key prevents template suggestions from working.