When using DS (8.x-2.5) and selecting a panel layout (onecol) for the display layout the twig template was not overridable.

Changing the function ds_theme_suggestions_alter(&$suggestions, $variables, $base_theme_hook) to use $base_theme_hook as a prefix instead of $layout_id for ALL suggestions (obviously) finally fixed the problem for me.

So:

$suggestions[] = $base_theme_hook . '__' . $variables['content']['#entity_type'];
...

instead of

$suggestions[] = $layout_id . '__' . $variables['content']['#entity_type'];
...

Can a maintainer please verify whether the use of $layout_id = $variables['content']['#ds_configuration']['layout']['id'] is correct as opposed to my working $base_theme_hook solution?
- DS layouts worked but they are also called e.g. ds-1col etc. and the base_theme_hook-variable was the same.
- Panels defines them as e.g. onecol etc. but the base_theme_hook is panels-onecol.

After the change I could define the templates "panels-onecol--node-mynodetype.twig.html" etc. I believe the Registry/ThemeManager doesn't load any templates that don't begin with a module name (or something similar).

One more thing: The display layout suggestions didn't work for me either, I guess those are totally off, e.g. ds-1colo--article-article.html.twig etc.

I won't add a patch, as I want feedback first.

Not sure if this is a related issue: https://www.drupal.org/node/2785103

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Deraynger created an issue. See original summary.

Anonymous’s picture

Issue summary: View changes
Anonymous’s picture

A comment would be nice from a maintainer. I'll just upload the patch I use internally and then it'll be reviewed then or once it breaks something else. All points above are still relevant.

Anonymous’s picture

Status: Active » Needs review
John Pitcairn’s picture

Priority: Normal » Major

Some maintainer feedback would be good. I am unable to override the ds-1col template for a specific node or entity type.

Anonymous’s picture

John, I don't think that is the same issue, as I didn't experience problems with DS, although some maintainer feedback would be cool.

Try to name your template ds-1col--node-mynodetype.twig.html (maybe enable twig template suggestions in development.services.yml and look at the suggestions in the HTML output source), the suggestions in the display layout are totally wrong last I looked, e.g. ds-1colo--article-article.html.twig etc.

John Pitcairn’s picture

Priority: Major » Normal

Thanks, I have it working now. What confused me is that in D8 you can select a DS layout for the default view mode, but for an overridden twig template to be applied, you need to be using a non-default view mode (and get the template name right, ignoring the suggestions in the UI).

aspilicious’s picture

Status: Needs review » Postponed (maintainer needs more info)

Can you reupload the patch. For some reason the testbot isn't kicking in.

Anonymous’s picture

Status: Postponed (maintainer needs more info) » Needs review
FileSize
1.78 KB

@aspilicious Re-uploaded the patch.

aspilicious’s picture

Tests are passing but I'm still afraid about this change...
You're now changing the suggestions but that means they conflict wit the suggestions at "_ds_field_ui_table_layouts_preview"
Could you also try to fix that for you?

Anonymous’s picture

@aspilicious The suggestions are in either case totally wrong, as I wrote in my initial comment. For DS I didn't find a problem with my change, but I did initally want feedback from a maintainer, because I don't know why $layout_id was used instead of $base_theme_hook, which is why I also waited 2 weeks for feedback before posting my patch. The DS suggestions is IMO a seperate issue.

Here the relevant parts:

- DS layouts worked but they are also called e.g. ds-1col etc. and the base_theme_hook-variable was the same.
- Panels defines them as e.g. onecol etc. but the base_theme_hook is panels-onecol.

After the change I could define the templates "panels-onecol--node-mynodetype.twig.html" etc.

One more thing: The display layout suggestions didn't work for me either, I guess those are totally off, e.g. ds-1colo--article-article.html.twig etc.

I won't add a patch, as I want feedback first.

aspilicious’s picture

It is related, if there is a bug we need to fix it at once.
The suggestions in the UI should match the suggestions in code which should match actual working suggestions.

Anonymous’s picture

@aspilicious

I finally got around to it. I guess the suggestions aren't that off anymore since I wrote this issue, but this issue is still open, so here's the patch with the _ds_field_ui_table_layouts_preview so it shows the correct suggestions for panels too.

rodrigoaguilera’s picture

Version: 8.x-2.x-dev » 8.x-3.x-dev
FileSize
3.94 KB

I'm running into this problem for DS 3 and this approach enables to have suggestions again. I did not had this problem in DS 2.

I don't know if it is the policy of this module to fix first for the more recent version and then backport but I made a patch for DS 3.

I kept the layout id as I think is relevant for the template since the same template won't work for different layouts.

Status: Needs review » Needs work

The last submitted patch, 14: ds_display-layout-overrides-not-working_2802429-14.patch, failed testing.

aspilicious’s picture

I'm extremely confused about all these theme suggestions stuff...
Latest patch is different than the one in #13.

There has been some changes concerning theme suggestions so I asked for more input.
Hopefully I'll get it soon.

rodrigoaguilera’s picture

For me I tested it manually very easily by installing a standard profile, enabling twig debugging and trying to use one of the overrides that are suggested.
I feel the reason is somehow the suggestions have to start with the base theme hook or they are ignored.

The latest patch is different because for 8.3.x the base theme hook is different from what it used to be.

If your policy is to change it first on DS 2 please change the branch of the issue.

I'm attaching the a new patch hoping to fix the tests.

rodrigoaguilera’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

I'm reasonably sure this change would be needed to work with Layout Plugin in 8.2.x, and I'm 100% sure it's needed for 8.3.x

The 'layout__' is what allows the 'base hook' portion to work, which is needed for preprocess and suggestions to work as expected.

See #2559825: Preprocess functions from base hooks not triggered for theme hooks not using the __ pattern

aspilicious’s picture

Priority: Normal » Major
Status: Needs review » Needs work

We should add a test for this.

swentel’s picture

Status: Needs work » Needs review
FileSize
2.42 KB
1.53 KB

So this is becoming confusing, I would have expected my test to fail to 8.x-3.x, but it doesn't.

However, the same test with #2860490: Invalid argument supplied for foreach() in ds_preprocess_ds_layout() line 366 of ds.module reverted fails. So I think we don't have problem anymore now in 8.x-3.x branch ?

swentel’s picture

Ok, I'm stupid, let me test something different :)

The last submitted patch, 21: 2802429-21-fail.patch, failed testing.

swentel’s picture

FileSize
6.32 KB

Ok, so when using a theme suggestion and then using layout--template.html.twig everything is fine .. hmm .. tricky upgrade path.
Patch using code from #17 + a test.

swentel’s picture

Actually, the test is lying, digging

swentel’s picture

FileSize
6.75 KB

ok, again, combination of #17 and a test.
But it will fail as the wrapper is not passed in .. very weird.

Status: Needs review » Needs work

The last submitted patch, 26: 2802429-26.patch, failed testing.

csedax90’s picture

even with the latest dev (as today), after updating all my templates overwritten me appear as <>

Is there anything that I can verify to better understand what's the problem?

tim.plunkett’s picture

The layout__ prefix should NOT be needed, the 'base hook' => 'layout' added by LayoutPluginManager should be sufficient.
If there are more bugs around that, it's once again a problem in the Theme System itself :(

But it's worth using that as a workaround now, seems like there are still other problems here to resolve in the meantime.

swentel’s picture

Some debugging:

  • without layout prefix/suggestion: both template_preprocess_layout() and ds_preprocess_ds_layout() are called, but the template is not switched
  • with layout prefix/suggestion: only template_preprocess_layout() is called, but not ds_preprocess_ds_layout(), template is switched.
swentel’s picture

So if I do this in core, it works fine for say ds-1col--node.html.twig with current DS (so no prefix of layout or renames in template) ...

diff --git a/core/lib/Drupal/Core/Layout/LayoutPluginManager.php b/core/lib/Drupal/Core/Layout/LayoutPluginManager.php
index e11f105..d694d3c 100644
--- a/core/lib/Drupal/Core/Layout/LayoutPluginManager.php
+++ b/core/lib/Drupal/Core/Layout/LayoutPluginManager.php
@@ -150,7 +150,7 @@ public function getThemeImplementations() {
       if ($template = $definition->getTemplate()) {
         $hooks[$definition->getThemeHook()] = [
           'render element' => 'content',
-          'base hook' => 'layout',
+          //'base hook' => 'layout',
           'template' => $template,
           'path' => $definition->getTemplatePath(),
         ];
swentel’s picture

So this seems to be core bug. For testing, can anyone just comment out the 'base hook' line in LayoutPluginManager::getThemeImplementations()

I've tested with module and theme provided layouts, and some variations (e.g. just copy over ds-1col, or add ds-1col--node suggestion in theme) and everything works fine .. at least for DS layouts ..

swentel’s picture

Added patch over at https://www.drupal.org/node/1970534#comment-11995931 just for fun to see what breaks in core when that line is gone ..

swentel’s picture

Status: Needs work » Needs review

Ok, I'll be honest, I'm completely stuck on this one. the suggestion works with layout plugin and 8.x-2.x so something is either different in layout_discover or in theme registry that breaks it.

swentel’s picture

Status: Needs review » Needs work

Woops, still needs work.

swentel’s picture

Status: Needs work » Needs review
FileSize
4.55 KB

For reference, the patch I'm using locally to try and figure out to get this working with core (again, move out 'base hook' from layout plugin manager and the suggestion is found ..)

+++ b/ds.module
@@ -94,6 +94,11 @@ function ds_theme_registry_alter(&$theme_registry) {
+      if ($theme_registry[$theme_hook]['type'] == 'theme_engine') {
+        unset($theme_registry[$theme_hook]['base hook']);
+      }

You can ignore that however

Status: Needs review » Needs work

The last submitted patch, 36: 2802429.patch, failed testing.

swentel’s picture

We might have a core patch available over at #2862683: 'base hook' key prevents template suggestions from working

With that patch, it means we can also remove following code in ds:

diff --git a/ds.module b/ds.module
index eab805f..fede342 100644
--- a/ds.module
+++ b/ds.module
@@ -87,12 +87,6 @@ function ds_theme_registry_alter(&$theme_registry) {
   // layout is using the Display Suite layout class.
   foreach ($theme_registry as $theme_hook => $info) {
     if (in_array($theme_hook, $layout_theme_hooks) || (!empty($info['base hook']) && in_array($info['base hook'], $layout_theme_hooks))) {
-
-      // @todo Remove once https://www.drupal.org/node/2861840 is resolved.
-      if (!in_array('template_preprocess_layout', $theme_registry[$theme_hook]['preprocess functions'])) {
-        $theme_registry[$theme_hook]['preprocess functions'][] = 'template_preprocess_layout';
-      }
-
       $theme_registry[$theme_hook]['preprocess functions'][] = 'ds_preprocess_ds_layout';
     }
   }
swentel’s picture

Status: Needs work » Needs review
FileSize
7.17 KB

Let's see what this gives, insane workaround which made the suggestion work on my machine.

swentel’s picture

FileSize
7.36 KB

some cleanups in the docs

swentel’s picture

FileSize
6.79 KB

Added a check to be sure the hook is registered, although it would be weird, but I want to be sure at this point.

Status: Needs review » Needs work

The last submitted patch, 41: 2802429-41.patch, failed testing.

swentel’s picture

Ok, so interestingly enough, it seems that there's still a problem with themes that provide templates.

This was the latest change which made the patch fail:

+      // Ignored if not registered (which would be weird).
+      if (!isset($theme_registry[$hook])) {
+        continue;
+      }
swentel’s picture

Status: Needs work » Needs review
FileSize
7.49 KB

Actually, I forgot to include a file in the latest patch, trying again.

  • swentel committed a722b26 on 8.x-3.x
    Issue #2802429 by swentel, Deraynger, rodrigoaguilera, John Pitcairn:...
swentel’s picture

Status: Needs review » Fixed

Committed and pushed, beta1 will be up soon

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

gambry’s picture

As issue #2862683: 'base hook' key prevents template suggestions from working mentioned on #38 has been postponed, does this commit still require it?

I'm assuming yes as in my scenario I'm still affected by the problem (layout suggestion template file is not loaded, loading always the base layout template file).

Can we add a bit of context or try to understand why #2862683 has been postponed?
This is a quite annoying bug as website working on 8.2 are forced to update to 8.3 to get security patches and when they do any module layout-related stops working.

gambry’s picture

The reason why my suggestion wasn't loaded is because the workaround introduced by #39 process all LayoutDefinition[], but adds to the theme registry only suggestion of layouts implementing class: '\Drupal\ds\Plugin\DsLayout'.
Just adding the property to the layouts yaml definition and the suggestion will be found.

But I'm assuming this invalidate all the idea of creating a layout and being able to reuse it i.e. in DS and Panels, right?

And from the DS point of view having a generalised (on ds_theme_registry_alter() on ds.module):

is_a($info->getClass(), \Drupal\Core\Layout\LayoutDefault::class, TRUE)

instead of

is_a($info->getClass(), DsLayout::class, TRUE)

Is may be too much.

gngn’s picture

thanx @gambry in #49.

Adding class: '\Drupal\ds\Plugin\DsLayout' to my layouts.yml indeed did the trick.
And since I'm only using it for ds (and not with panels) everything is fine to me.

The proposed change to
is_a($info->getClass(), \Drupal\Core\Layout\LayoutDefault::class, TRUE)
also worked, but I prefer to change my modules yml instead of patching ds...

MXT’s picture

Here's a follow up and a patch based on gambry previous comment: #2887778: Layout template suggestions does not always work