Problem/Motivation

Layout discovery doesn't load layout suggestions unless the layout definition is prefixed with layout__ and the template with layout--.

This break any existing site using the (deprecated) Layouts contrib module, where the prefixing wasn't necessary.

Additionally site-builders and developers get confused as twig debug comments suggest wrong [as in: not-prefixed] template suggestions names, i.e. (from: #2905197: Template override is not working):

layouts definitions file: naurodvolleys.layouts.yml

naurodvolleys_1x1:
  label: "naurodvolleys 1x1"
  category: "naurodvolleys"
  template: "templates/content-layout/naurodvolleys_1x1"
  regions:
    main:
      label: "Main"

Template suggestions:

Target suggestion: naurodvolleys-1x1--node-result-1x1.html.twig

Suggestion is not loaded.

For further investigation you can read #2862683: 'base hook' key prevents template suggestions from working comments from #25 on going.

Proposed resolution

The first task should be to understand if prefixing layouts with layout__/layout-- as core does is the right way.
- If it is then we need to update the documentation pages How to register layouts and How to upgrade from Layout Plugin, together with updating the twig debug comment suggestions.
- if it is NOT, then we need to fix the issue in order for layout suggestions to be loaded correctly.

@tim.plunkett on #9 confirmed this is actually a bug, themes and modules layout templates suggestions needs to be loaded in accordance with their definitions.

Remaining tasks

  1. Define what is the right way to name layout bundles and their templates, if the layout__/layout-- prefixes are mandatory. (#9
  2. Update the issue summary with the right proposed resolution when task 1) is complete #11
  3. Code a solution
  4. Update the documentation if necessary

User interface changes

This is a pure code change, so no UI changes expected.

API changes

There may be Layout API changes, depending what the result of task 1) will be.

CommentFileSizeAuthor
#41 interdiff_39-41.txt729 bytesravi.shankar
#41 2911996-41.patch7.89 KBravi.shankar
#39 drupal-layout_discovery-2911996-39-d95.patch7.78 KBvladimiraus
#37 drupal-layout_discovery-2911996-37-d95.patch7.66 KBvladimiraus
#25 drupal-layout_discovery-2911996-25.patch7.67 KBvladimiraus
#22 drupal-layout_discovery-2911996-22.patch7.65 KBvladimiraus
#20 drupal-layout_discovery-2911996-20.patch15.95 KBvladimiraus
#15 drupal_layout_discovery-2911996-15.patch3.64 KBvladimiraus
#11 interdiff-9-11.txt1.51 KBgambry
#11 2911996-layout-11.patch5.14 KBgambry
#9 2911996-layout-9.patch6.65 KBtim.plunkett
#9 2911996-layout-9-interdiff.txt1.75 KBtim.plunkett
#5 2911996-layout-5.patch4.91 KBtim.plunkett

Comments

gambry created an issue. See original summary.

gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Related issues: +#2862683: 'base hook' key prevents template suggestions from working, +#2905197: Template override is not working

Linking related issue #2905197: Template override is not working and #2862683: 'base hook' key prevents template suggestions from working

gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Issue summary: View changes

Updating IS with mentioning the investigation bit done from #2862683: 'base hook' key prevents template suggestions from working #25 and following comments.

gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Issue summary: View changes
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Active » Needs review
StatusFileSize
new 2911996-layout-5.patch4.91 KB

Let's prove that it works first.

Status: Needs review » Needs work

The last submitted patch, 5: 2911996-layout-5.patch, failed testing. View results

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

template_preprocess_layout() is no longer firing for these layouts, as suspected.

gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Issue tags: +Vienna2017

@tim.plunkett so this is actually a real bug, as layout are not supposed to have the prefixes layout__/layout-- ?

If so I will update the IS and start looking at a fix.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new 2911996-layout-9-interdiff.txt1.75 KB
new 2911996-layout-9.patch6.65 KB

It is not documented anywhere that layout templates are required to have the layout-- prefix, and IMO this is a bug deep in the theme system.

Consider these two tests. They are completely identical tests, but one is a Kernel test and one is a Functional test. The Kernel test passes, the Functional test fails.
Somewhere in between these two lies the bug.

Status: Needs review » Needs work

The last submitted patch, 9: 2911996-layout-9.patch, failed testing. View results

gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new 2911996-layout-11.patch5.14 KB
new interdiff-9-11.txt1.51 KB

Updating the issue summary as we now know this is a bug.

@tim.plunkett and I have discussed the result of test failing on #9. It looks like when layout templates overrides exist (for example in the stable theme on that test) then template_preprocess_layout doesn't exist in the "preprocess functions" array. This may or may not be related to this bug.
Attaching a test to prove this.

I'll have a look and open another issue if the two bugs are not related.

Setting Needs Review just to trigger testbot. This issue still needs work.

Status: Needs review » Needs work

The last submitted patch, 11: 2911996-layout-11.patch, failed testing. View results

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Component: layout.module » layout_discovery.module
vladimiraus’s picture

vladimiraus
Primary language English
Location Brisbane 🇦🇺
commented
StatusFileSize
new drupal_layout_discovery-2911996-15.patch3.64 KB

Not a complete solution but will allow override templates.
IDs were renamed from `layout_x` to `layout__x`.
Previous layouts with originals IDs are still there but labelled deprecated.
Probably needs hook_update to check if we are using old layouts.

vladimiraus’s picture

vladimiraus
Primary language English
Location Brisbane 🇦🇺
commented
Status: Needs work » Needs review

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Needs work

The latest patch (#15) doesn't include the tests added in #11 anymore. It should, let's add those back in.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: -Vienna2017 +Needs tests

Also note that #2938132: Ship layouts that make sense with Layout Builder's concept of sections landed and we have more layouts.

vladimiraus’s picture

vladimiraus
Primary language English
Location Brisbane 🇦🇺
commented
Assigned: Unassigned » vladimiraus
Status: Needs work » Needs review
StatusFileSize
new drupal-layout_discovery-2911996-20.patch15.95 KB

This patch include:
* deprecation of all templates with `layout_x` IDs in layout_builder and layout_builder modules.
* introduction of replacement templates with `layout__x` IDs.
* tests from #11

Status: Needs review » Needs work

The last submitted patch, 20: drupal-layout_discovery-2911996-20.patch, failed testing. View results

vladimiraus’s picture

vladimiraus
Primary language English
Location Brisbane 🇦🇺
commented
StatusFileSize
new drupal-layout_discovery-2911996-22.patch7.65 KB

Removing accidental commits.

This patch includes:
* deprecation of all templates with `layout_x` IDs in layout_builder and layout_builder modules.
* introduction of replacement templates with `layout__x` IDs.
* tests from #11

vladimiraus’s picture

vladimiraus
Primary language English
Location Brisbane 🇦🇺
commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 22: drupal-layout_discovery-2911996-22.patch, failed testing. View results

vladimiraus’s picture

vladimiraus
Primary language English
Location Brisbane 🇦🇺
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-layout_discovery-2911996-25.patch7.67 KB

Fixing tests.

This patch includes:
* deprecation of all templates with `layout_x` IDs in layout_builder and layout_builder modules.
* introduction of replacement templates with `layout__x` IDs.
* tests from #11

Status: Needs review » Needs work

The last submitted patch, 25: drupal-layout_discovery-2911996-25.patch, failed testing. View results

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

adam3145’s picture

adam3145 commented

Where is this at as I am still having this issue.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

golddragon007’s picture

golddragon007 commented

Well, this is not the only issue, I have two template files with the same name in two different directories and it picks the wrong one...
i,e:
*.layouts.yml:
banner:
label: Banner
category: Custom
template: templates/layouts/banner
[...]

And I have two files, one templates/layouts/banner.html.twig and one templates/components/banner.html.twig, and it picks the templates/components/banner.html.twig instead the one that is explicitly specified... (within the same theme)

vladimiraus’s picture

vladimiraus
Primary language English
Location Brisbane 🇦🇺
commented
Assigned: vladimiraus » Unassigned
vladimiraus’s picture

vladimiraus
Primary language English
Location Brisbane 🇦🇺
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-layout_discovery-2911996-37-d95.patch7.66 KB

Making sure it works for latest branch.

Status: Needs review » Needs work

The last submitted patch, 37: drupal-layout_discovery-2911996-37-d95.patch, failed testing. View results

vladimiraus’s picture

vladimiraus
Primary language English
Location Brisbane 🇦🇺
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-layout_discovery-2911996-39-d95.patch7.78 KB

Fixing tests.

Status: Needs review » Needs work

The last submitted patch, 39: drupal-layout_discovery-2911996-39-d95.patch, failed testing. View results

ravi.shankar’s picture

ravi.shankar commented
Status: Needs work » Needs review
StatusFileSize
new 2911996-41.patch7.89 KB
new interdiff_39-41.txt729 bytes

Fixing failed test of patch #39.

Status: Needs review » Needs work

The last submitted patch, 41: 2911996-41.patch, failed testing. View results

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

In #9 I found a bug elsewhere in the system. I don't think doubling the number of layout plugins is a reasonable compromise...
Can we get back to trying to fix the underlying bug instead of treating the symptom?

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Related issues: +#2752443: Incorrect order and duplicate theme hook suggestions

Might this be caused by #2752443: Incorrect order and duplicate theme hook suggestions?

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.