Updated: Comment #N

Problem/Motivation

When an array of suggestions is passed to theme(), the twig_debug output doesn't match up.

Proposed resolution

Display the suggestions that are passed in as an array.

Remaining tasks

Patch
Tests

User interface changes

Twig debug output will display the suggestions passed in as an array.

API changes

None

Files: 
CommentFileSizeAuthor
#93 interdiff.txt2.73 KBAntti J. Salminen
#93 twig_debug_output_does-2118743-93.patch18.96 KBAntti J. Salminen
#89 interdiff.txt1.35 KBlauriii
#89 twig_debug_output_does-2118743-89.patch18.89 KBlauriii
#87 interdiff.txt2.12 KBlauriii
#87 twig_debug_output_does-2118743-87.patch18.93 KBlauriii
#82 twig_debug_output_does-2118743-82.patch18.56 KBlauriii
#80 twig_debug_output_does-2118743-78.patch18.57 KBAntti J. Salminen
#77 interdiff.txt2.08 KBAntti J. Salminen
#77 twig_debug_output_does-2118743-77.patch18.56 KBAntti J. Salminen
#73 http___d8_dev_.png266.65 KBjoelpittet
#73 http___d8_dev_.png294.86 KBjoelpittet
#72 twig_debug_output_does-2118743-72.patch18.4 KBlauriii
#62 2118743-62.patch18.19 KBhussainweb
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,271 pass(es). View
#62 interdiff-59-62.txt813 byteshussainweb
#59 2118743-59.patch18.08 KBhussainweb
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,272 pass(es), 0 fail(s), and 1 exception(s). View
#56 interdiff.txt8.53 KBAntti J. Salminen
#56 2118743-56.patch17.82 KBAntti J. Salminen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,798 pass(es). View
#46 interdiff-2118743-45-46.txt6.31 KBAntti J. Salminen
#46 2118743-46.patch17.57 KBAntti J. Salminen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,066 pass(es). View
#45 interdiff.txt655 bytesrteijeiro
#45 2118743-45.patch15.23 KBrteijeiro
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,625 pass(es). View
#43 interdiff-2118743-27-42.txt14.03 KBAntti J. Salminen
#43 2118743-42.patch15.22 KBAntti J. Salminen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,133 pass(es). View
#43 2118743-42-testonly.patch11.57 KBAntti J. Salminen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,118 pass(es), 6 fail(s), and 0 exception(s). View
#41 2118743-40.patch15.35 KBAntti J. Salminen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,035 pass(es). View
#41 2118743-40-testonly.patch10.37 KBAntti J. Salminen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,008 pass(es), 8 fail(s), and 0 exception(s). View
#35 2118743-35.patch11.66 KBAntti J. Salminen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,004 pass(es), 3 fail(s), and 0 exception(s). View
#35 2118743-35-testonly.patch8.68 KBAntti J. Salminen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,001 pass(es), 2 fail(s), and 0 exception(s). View
#33 2118743-33.patch8.51 KBAntti J. Salminen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,586 pass(es), 1 fail(s), and 0 exception(s). View
#33 2118743-33_testonly.patch5.53 KBAntti J. Salminen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,597 pass(es). View
#27 2118743-27.patch2.97 KBjjcarrion
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,458 pass(es). View
#23 interdiff.txt479 bytesbotanic_spark
#23 2118743-23.patch2.93 KBbotanic_spark
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2118743-23.patch. Unable to apply patch. See the log in the details link for more information. View
#20 interdiff.txt826 bytesbotanic_spark
#20 2118743-20.patch3.58 KBbotanic_spark
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,757 pass(es). View
#16 interdiff.txt1.38 KBdawehner
#16 2118743-16.patch2.98 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,527 pass(es), 1 fail(s), and 0 exception(s). View
#13 theme-2118743-13.patch1.6 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,409 pass(es), 1 fail(s), and 0 exception(s). View
#13 interdiff-2118743-11-13.txt1.49 KBlauriii

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
1.66 KB
FAILED: [[SimpleTest]]: [MySQL] 59,405 pass(es), 4 fail(s), and 0 exception(s). View

Thank you for creating the issue.

Before

:

<div class="view-content">
      

<!-- THEME DEBUG -->
<!-- CALL: theme('views_view_unformatted') -->
<!-- BEGIN OUTPUT from 'core/modules/views/templates/views-view-unformatted.html.twig' -->
  <div class="views-row views-row-1 views-row-odd views-row-first views-row-last">
    

<!-- THEME DEBUG -->
<!-- CALL: theme('node') -->
<!-- FILE NAME SUGGESTIONS:
   * node--article.html.twig
   * node--1.html.twig
   * node--view--frontpage.html.twig
   * node--view--frontpage--page-1.html.twig
   x node.html.twig
-->
<!-- BEGIN OUTPUT from 'core/themes/bartik/templates/node.html.twig' -->
<article id="node-1" class="node node-article promoted view-mode-teaser contextual-region clearfix" data-history-node-id="1" data-edit-entity="node/1" role="article" about="/d8/node/1" typeof="schema:Article">

  <header>
    
          <h2>
        <a href="/d8/node/1">test</a>
      </h2>
        <div data-contextual-id="node:node:1:" class="contextual-render-processed contextual" role="form" style="top: 30px;"><button class="trigger focusable visually-hidden" type="button" aria-pressed="false">Open test configuration options</button><ul class="contextual-links" hidden=""><li class="quick-edit"><a href="" role="button" aria-pressed="false">Quick edit</a></li><li class="node-edit odd first"><a href="/d8/node/1/edit?destination=node">Edit</a></li><li class="node-delete even last"><a href="/d8/node/1/delete?destination=node">Delete</a></li></ul></div><span property="schema:name" content="test" class="rdf-meta"></span>

          <div class="meta submitted">
        

<!-- THEME DEBUG -->
<!-- CALL: theme('user') -->
<!-- BEGIN OUTPUT from 'core/modules/user/templates/user.html.twig' -->
<article data-edit-entity="user/1" class="profile" typeof="schema:Person" about="/d8/user/1">
  </article>

<!-- END OUTPUT from 'core/modules/user/templates/user.html.twig' -->


        <span rel="schema:author">Submitted by <a href="/d8/user/1" title="View user profile." class="username" lang="" about="/d8/user/1" typeof="schema:Person" property="schema:name" datatype="">admin</a> on Wed, 10/23/2013 - 15:27</span><span property="schema:dateCreated" content="2013-10-23T15:27:52+00:00" class="rdf-meta"></span>
      </div>
      </header>

  <div class="content clearfix">
            <div class="field field-node--body field-name-body field-type-text-with-summary field-label-hidden edit-processed edit-field" data-edit-id="node/1/body/und/teaser"><div class="field-items"><div class="field-item even" property="schema:text"><p>test</p></div></div></div>
  </div>

      <footer class="link-wrapper">
      <ul class="links inline"><li class="node-readmore odd first"><a href="/d8/node/1" rel="tag" title="test">Read more<span class="visually-hidden"> about test</span></a></li><li class="comment-add even last"><a href="/d8/node/1#comment-form" title="Add a new comment to this page.">Add new comment</a></li></ul>
    </footer>
  
</article>

<!-- END OUTPUT from 'core/themes/bartik/templates/node.html.twig' -->


  </div>

<!-- END OUTPUT from 'core/modules/views/templates/views-view-unformatted.html.twig' -->


    </div>

After

<div class="view-content">
      

<!-- THEME DEBUG -->
<!-- CALL: theme('views_view_unformatted') -->
<!-- FILE NAME SUGGESTIONS:
   * views-view-unformatted--frontpage--page-1.html.twig
   * views-view-unformatted--page-1.html.twig
   * views-view-unformatted--default.html.twig
   * views-view-unformatted--frontpage--page.html.twig
   * views-view-unformatted--page.html.twig
   * views-view-unformatted--frontpage.html.twig
   x views-view-unformatted.html.twig
   x views-view-unformatted.html.twig
-->
<!-- BEGIN OUTPUT from 'core/modules/views/templates/views-view-unformatted.html.twig' -->
  <div class="views-row views-row-1 views-row-odd views-row-first views-row-last">
    

<!-- THEME DEBUG -->
<!-- CALL: theme('node') -->
<!-- FILE NAME SUGGESTIONS:
   * node--article.html.twig
   * node--1.html.twig
   x node.html.twig
   * node--view--frontpage.html.twig
   * node--view--frontpage--page-1.html.twig
   x node.html.twig
-->
<!-- BEGIN OUTPUT from 'core/themes/bartik/templates/node.html.twig' -->
<article id="node-1" class="node node-article promoted view-mode-teaser contextual-region clearfix" data-history-node-id="1" data-edit-entity="node/1" role="article" about="/d8/node/1" typeof="schema:Article">

  <header>
    
          <h2>
        <a href="/d8/node/1">test</a>
      </h2>
        <div data-contextual-id="node:node:1:" class="contextual-render-processed contextual" role="form" style="top: 30px;"><button class="trigger focusable visually-hidden" type="button" aria-pressed="false">Open test configuration options</button><ul class="contextual-links" hidden=""><li class="quick-edit"><a href="" role="button" aria-pressed="false">Quick edit</a></li><li class="node-edit odd first"><a href="/d8/node/1/edit?destination=node">Edit</a></li><li class="node-delete even last"><a href="/d8/node/1/delete?destination=node">Delete</a></li></ul></div><span property="schema:name" content="test" class="rdf-meta"></span>

          <div class="meta submitted">
        

<!-- THEME DEBUG -->
<!-- CALL: theme('user') -->
<!-- BEGIN OUTPUT from 'core/modules/user/templates/user.html.twig' -->
<article data-edit-entity="user/1" class="profile" typeof="schema:Person" about="/d8/user/1">
  </article>

<!-- END OUTPUT from 'core/modules/user/templates/user.html.twig' -->


        <span rel="schema:author">Submitted by <a href="/d8/user/1" title="View user profile." class="username" lang="" about="/d8/user/1" typeof="schema:Person" property="schema:name" datatype="">admin</a> on Wed, 10/23/2013 - 15:27</span><span property="schema:dateCreated" content="2013-10-23T15:27:52+00:00" class="rdf-meta"></span>
      </div>
      </header>

  <div class="content clearfix">
            <div class="field field-node--body field-name-body field-type-text-with-summary field-label-hidden edit-processed edit-field" data-edit-id="node/1/body/und/teaser"><div class="field-items"><div class="field-item even" property="schema:text"><p>test</p></div></div></div>
  </div>

      <footer class="link-wrapper">
      <ul class="links inline"><li class="node-readmore odd first"><a href="/d8/node/1" rel="tag" title="test">Read more<span class="visually-hidden"> about test</span></a></li><li class="comment-add even last"><a href="/d8/node/1#comment-form" title="Add a new comment to this page.">Add new comment</a></li></ul>
    </footer>
  
</article>

<!-- END OUTPUT from 'core/themes/bartik/templates/node.html.twig' -->


  </div>

<!-- END OUTPUT from 'core/modules/views/templates/views-view-unformatted.html.twig' -->


    </div>

The last submitted patch, theme-2118743-1.patch, failed testing.

oetseli’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.46 KB
FAILED: [[SimpleTest]]: [MySQL] 59,112 pass(es), 4 fail(s), and 1 exception(s). View

I have another suggestion and patch that changes the way Twig debug prints out the theme call itself.

Instead of doing this:

<!-- CALL: theme('views_view_unformatted') -->
<!-- BEGIN OUTPUT from 'core/modules/views/templates/views-view-unformatted.html.twig' -->

It does this:

<!-- THEME HOOKS:
   * views_view_unformatted__twig_article_test__page_1
   * views_view_unformatted__page_1
   * views_view_unformatted__twig_article_test__page
   * views_view_unformatted__page
   * views_view_unformatted__twig_article_test
   x views_view_unformatted
-->
<!-- BEGIN OUTPUT from 'core/modules/views/templates/views-view-unformatted.html.twig' -->

Views does this a lot. That there are no theme_hook_suggestions, but rather a lot of hooks in theme()-function's $hook array. What do you think?

Status: Needs review » Needs work

oetseli’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Cottser’s picture

Status: Needs work » Postponed
Cottser’s picture

Status: Postponed » Needs work

Re-activating because that issue got shot down.

dawehner’s picture

@Cottser
Do you have a suggestion which of the two aproaches would you prefer?

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,854 pass(es), 2 fail(s), and 0 exception(s). View

I like #1 more so I rerolled it and most likely fixed tests.

Status: Needs review » Needs work

The last submitted patch, 11: theme-2118743-11.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.49 KB
1.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,409 pass(es), 1 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 13: theme-2118743-13.patch, failed testing.

Cottser’s picture

Issue tags: +Needs tests

Thanks for working on this @lauriii!

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,527 pass(es), 1 fail(s), and 0 exception(s). View
1.38 KB

I wanted to ensure that we have some test coverage in views, you never know.

Status: Needs review » Needs work

The last submitted patch, 16: 2118743-16.patch, failed testing.

The last submitted patch, 11: theme-2118743-11.patch, failed testing.

botanic_spark’s picture

Assigned: Unassigned » botanic_spark
Status: Needs work » Needs review
FileSize
3.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,757 pass(es). View
826 bytes

It seams that TwigDebugTest is not passing because debug info is in wrong order. This happens because of the array_reverse in

 if ($hook != $theme_hooks) {
    $suggestions = array_merge($suggestions, (array) $theme_hooks);
    $suggestions = array_reverse($suggestions);
  }

My solution is to reverse it again in the twig.engine but just in case when #theme element is passed.

  // Reverse suggestions array before merging with derived suggestions.
  $variables['theme_hook_suggestions'] = array_reverse($variables['theme_hook_suggestions']);
  $variables['theme_hook_suggestions'] = array_merge($derived_suggestions, $variables['theme_hook_suggestions']);
  $variables['theme_hook_suggestions'][] = $base_hook;
dawehner’s picture

+++ b/core/themes/engines/twig/twig.engine
@@ -81,6 +81,8 @@ function twig_render_template($template_file, $variables) {
+      // Reverse suggestions array before merging with derived suggestions.
+      $variables['theme_hook_suggestions'] = array_reverse($variables['theme_hook_suggestions']);

This is really not obvious ... instead of explaning WHAT you do here, try to explain WHY you do it, because this is information not encoded in the code itself.

botanic_spark’s picture

Status: Needs review » Needs work

I spoke with @lauriii alot about this during last few days.
In theme.inc there is this part

if ($hook != $theme_hooks) {
    $suggestions = array_merge($suggestions, (array) $theme_hooks);
    $suggestions = array_reverse($suggestions);
}

For some reason array_reverse is done here, but this is only for case when suggestion is passed directly to theme function. For example
'#theme' => 'node__foo__bar',

But the thing is that in all other cases, the suggestions array is not reversed and this is braking the tests.

In twig.engine this array is reversed again in order to display debug info properly, but order of suggestions is different for case when
'#theme' => 'node__foo__bar', is passed.

So my "dirty" fix was to reverse it again in twig.engine but just for that specific case. This made tests to pass.

Both @lauriii and me agreed that this is not the solution, but it made clear what is the problem, and we need to avoid this double or even triple reversing.

I will keep working on this, and post some update when i have more info.

Ofcourse, every suggestion is welcome.

botanic_spark’s picture

Status: Needs work » Needs review
FileSize
2.93 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2118743-23.patch. Unable to apply patch. See the log in the details link for more information. View
479 bytes

To avoid multiple array inversion i just removed

   if ($hook != $theme_hooks) {
     $suggestions = array_merge($suggestions, (array) $theme_hooks);
-    $suggestions = array_reverse($suggestions);
   }

from theme.inc

This was already tried in #11 but for some reason tests were not passing.

I have tried this correction on my local testbot instance and it's passing.

lauriii queued 23: 2118743-23.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 23: 2118743-23.patch, failed testing.

lauriii’s picture

Issue tags: +Needs reroll
jjcarrion’s picture

Issue tags: -Needs reroll
FileSize
2.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,458 pass(es). View

I have made just the reroll

jjcarrion’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

We do have tests, not part of twig tests itself but at least for the views problem.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -232,6 +234,9 @@ protected function theme($hook, $variables = array()) {
    +    if ($hook != $theme_hooks) {
    +      $suggestions = array_merge($suggestions, (array) $theme_hooks);
    +    }
    

    It seems strange and slightly dangerous that we're altering this code to improve debug output.

  2. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -232,6 +234,9 @@ protected function theme($hook, $variables = array()) {
         if (isset($info['base hook'])) {
           $suggestions[] = $hook;
         }
    

    Is this even needed now? And if we do remove this - can be we remove the array_unique in twig.engine?

botanic_spark’s picture

Assigned: botanic_spark » Unassigned
Antti J. Salminen’s picture

Assigned: Unassigned » Antti J. Salminen
Antti J. Salminen’s picture

FileSize
5.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,597 pass(es). View
8.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,586 pass(es), 1 fail(s), and 0 exception(s). View

Adding a test that should expose a problem in the current implementation: When theme is passed an array of hooks that contains a base hook (as is often the case), using hook_theme_suggestions_HOOK doesn't work. This is because the base hook from the array will always be found before the suggestion. Working on a fix...

#30:

1. ThemeManager might be a more natural place for the debug output as the selection of the hook isn't really the theme engine's concern. Would need a more general equivalent for twig_debug to move it there.

2. I think we can get rid of it. The current patch relies on it but after fixing the issue that the test exposes, the cases where it's needed should also go away.

Status: Needs review » Needs work

The last submitted patch, 33: 2118743-33.patch, failed testing.

Antti J. Salminen’s picture

FileSize
8.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,001 pass(es), 2 fail(s), and 0 exception(s). View
11.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,004 pass(es), 3 fail(s), and 0 exception(s). View

Adding a couple of tests for the debug markup. I noticed the current committed version has duplicate suggestions in the debug output when there are specific suggestions in the hook passed to theme(). Also added a basic test for the array of hooks.

Antti J. Salminen’s picture

Status: Needs work » Needs review

Just triggering test runs, still needs work.

Status: Needs review » Needs work

The last submitted patch, 35: 2118743-35.patch, failed testing.

The last submitted patch, 35: 2118743-35-testonly.patch, failed testing.

Cottser’s picture

Just want to say thanks for working on this @Antti J. Salminen!

Cottser’s picture

Priority: Normal » Major
Issue tags: +DX (Developer Experience), +TX (Themer Experience), +needs backport to D7

Tagging and bumping.

Antti J. Salminen’s picture

Status: Needs work » Needs review
FileSize
10.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,008 pass(es), 8 fail(s), and 0 exception(s). View
15.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,035 pass(es). View

Haven't been able to work very fast on this unfortunately but here's some progress. I believe this would be what the approach suggested in the first patch looks like with everything in place.

What do you think about the assumption this makes that for arrays, base hooks should only be the last element? Without it those base hooks override direct suggestions but not other suggestions. It would mean it's impossible to represent the complete suggestions order with just a flat list.

The last submitted patch, 41: 2118743-40-testonly.patch, failed testing.

Antti J. Salminen’s picture

FileSize
11.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,118 pass(es), 6 fail(s), and 0 exception(s). View
15.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,133 pass(es). View
14.03 KB

The tests-only patch had some issues that should now be fixed. Additionally made some extremely small adjustments. Also adding an interdiff to the patch in #27. I'm thinking the suggestion selection in theme() could be reworked a bit from this.

The last submitted patch, 43: 2118743-42-testonly.patch, failed testing.

rteijeiro’s picture

FileSize
15.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,625 pass(es). View
655 bytes

Fixed a small nitpick.

Antti J. Salminen’s picture

Assigned: Antti J. Salminen » Unassigned
FileSize
17.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,066 pass(es). View
6.31 KB

Reworked the generation of the derived suggestions, fixed an issue in the patch and improved a test to detect it. If the tests don't come up with any problems this could use some looking at by others I think.

akalata’s picture

Since we're refactoring theme() function in #2348381: [META-0 theme functions left] Convert/refactor core theme functions, is this issue going to be applicable in D8?

Antti J. Salminen’s picture

#47: To me it looked like #2348381 only concerns theme hook implementations and in that case there would be no overlap.

akalata’s picture

@Antti, the goal of #2348381 is to replace theme() functions with template files. If we're doing that, then this issue (fixing theme() function debugging) will be useless.

lauriii’s picture

@akalata theme() functions will be still in there and there will be contribs using them so I wouldn't say refactoring it is useless. Even though they are highly deprecated.

Antti J. Salminen’s picture

@Akalata: The debug output that is being fixed is presented as a list of templates and can be implemented with templates. Are you saying that issue plans to also replace the current suggestions?

akalata’s picture

@laurii @Antti ah, I see. My initial read of the issue was that template suggestions coming from *.html.twig files were working, and that ones coming from theme() functions were not.

Cottser’s picture

Title: Twig debug output does not display all suggestions when an array of suggestions is passed to theme() » Twig debug output does not display all suggestions when an array of suggestions is passed to #theme

This might be a better title then :)

akalata’s picture

Debugging output BEFORE

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'block' -->
<!-- FILE NAME SUGGESTIONS:
   * block--seven-content.html.twig
   * block--system-main-block.html.twig
   * block--system.html.twig
   x block.html.twig
-->
<!-- BEGIN OUTPUT from 'core/themes/classy/templates/block/block.html.twig' -->
<div id="block-seven-content" class="block block-system">
  
    
      

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'container' -->
<!-- BEGIN OUTPUT from 'core/themes/classy/templates/form/container.html.twig' -->
<div class="views-element-container">

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'views_view' -->
<!-- BEGIN OUTPUT from 'core/modules/views/templates/views-view.html.twig' -->
<div class="view view-content view-id-content view-display-id-page_1 view-dom-id-b58a3131633d33a899a05166e6b98a4496c36bcb5f0ff59679d4e4d60835dff1">
  
    
        <div class="view-filters">
      

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'form' -->
<!-- BEGIN OUTPUT from 'core/themes/classy/templates/form/form.html.twig' -->
<form class="views-exposed-form" action="/admin/content" method="get" id="views-exposed-form-content-page-1" accept-charset="UTF-8">

Debugging output AFTER

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'block' -->
<!-- FILE NAME SUGGESTIONS:
   * block--seven-content.html.twig
   * block--system-main-block.html.twig
   * block--system.html.twig
   x block.html.twig
-->
<!-- BEGIN OUTPUT from 'core/themes/classy/templates/block/block.html.twig' -->
<div id="block-seven-content" class="block block-system">
  
    
      

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'container' -->
<!-- BEGIN OUTPUT from 'core/themes/classy/templates/form/container.html.twig' -->
<div class="views-element-container">

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'views_view' -->
<!-- FILE NAME SUGGESTIONS:
   * views-view--content--page-1.html.twig
   * views-view--page-1.html.twig
   * views-view--default.html.twig
   * views-view--content--page.html.twig
   * views-view--page.html.twig
   * views-view--content.html.twig
   x views-view.html.twig
-->
<!-- BEGIN OUTPUT from 'core/modules/views/templates/views-view.html.twig' -->
<div class="view view-content view-id-content view-display-id-page_1 view-dom-id-c20f87f4675e558b08d1230ece112410fad4c96840483893615041aa36ca8dcd">
  
    
        <div class="view-filters">
      

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'form' -->
<!-- BEGIN OUTPUT from 'core/themes/classy/templates/form/form.html.twig' -->
<form class="views-exposed-form" action="/admin/content" method="get" id="views-exposed-form-content-page-1" accept-charset="UTF-8">

I don't know testing well enough to review the added tests, sorry.

Cottser’s picture

Status: Needs review » Needs work

Been meaning to review this for a while so here's an initial pass, I'm not sure on some of the logic changes in ThemeManager just yet, need to do some manual testing/debugging on those.

Thanks for keeping on this @Antti J. Salminen and thank you @akalata for testing!

  1. +++ b/core/modules/system/src/Tests/Theme/ThemeSuggestionsAlterTest.php
    @@ -157,6 +157,28 @@ public function testSuggestionsAlterInclude() {
    +   * Tests hook_theme_suggestions_HOOK().
    +   */
    +  public function testArraySuggestions() {
    

    Could probably use a better summary line here, saying something about arrays.

  2. +++ b/core/modules/system/tests/modules/theme_test/src/ThemeTestController.php
    @@ -133,6 +140,18 @@ function suggestionAlterInclude() {
    +    return array(
    +      '#theme' => array(
    +        'theme_test_array_suggestions__not_implemented',
    +        'theme_test_array_suggestions__not_implemented_2',
    +        'theme_test_array_suggestions'),
    +    );
    

    Minor: Needs some small adjustments to meet https://www.drupal.org/coding-standards#array.

  3. +++ b/core/modules/views/src/Tests/ViewsThemeIntegrationTest.php
    @@ -81,4 +81,29 @@ public function testThemedViewPage() {
    +    $build = [
    +      '#type' => 'view',
    +      '#name' => 'test_page_display',
    +      '#display_id' => 'default',
    +      '#arguments' => array(),
    +    ];
    

    Minor: Go all the way with short array syntax, change the last array() :)

  4. +++ b/core/modules/views/src/Tests/ViewsThemeIntegrationTest.php
    @@ -81,4 +81,29 @@ public function testThemedViewPage() {
    +    $this->assertTrue(strpos($output, '* views-view--test-page-display--default.html.twig') !== FALSE);
    +    $this->assertTrue(strpos($output, '* views-view--default.html.twig') !== FALSE);
    +    $this->assertTrue(strpos($output, '* views-view--test-page-display.html.twig') !== FALSE);
    +    $this->assertTrue(strpos($output, 'x views-view.html.twig') !== FALSE);
    

    I know it's uglier to test but IMO we should really be testing the order of the suggestions because that is very important.

  5. +++ b/core/themes/engines/twig/twig.engine
    @@ -68,31 +68,20 @@ function twig_render_template($template_file, array $variables) {
    -      // Only add the original theme hook if it wasn't a directly called
    -      // suggestion.
    -      if (strpos($variables['theme_hook_original'], '__') === FALSE) {
    ...
    +      // Only add the original theme hook if it wasn't a directly called
    +      // suggestion.
    

    Seems like this comment doesn't apply anymore so let's remove it. Correct me if I'm wrong :)

  6. +++ b/core/themes/engines/twig/twig.engine
    @@ -68,31 +68,20 @@ function twig_render_template($template_file, array $variables) {
    +      if ( $pos !== FALSE) {
    

    Minor: Coding standards issue, extra space inside parens.

Antti J. Salminen’s picture

Status: Needs work » Needs review
FileSize
17.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,798 pass(es). View
8.53 KB

Thanks for the nice review Cottser and also thanks for testing Akalata! I've tried to fix the issues in this patch. Also reformatted the tests and fixed some other small things I noticed.

Hoping to get feedback on the logic changes, especially the assumption that only the last direct array suggestion can be a base hook. :) It seems reasonable to me to force there to be a linear list for the suggestion priorities. It matches how it's used in the places I've found and also the original intent of the feature (going from specific to generic, alternative to just providing a pattern).

Cottser’s picture

@Antti J. Salminen thanks for keeping on this! I definitely missed your patch in all the craziness of DrupalCon :) changes look very good!

Can you expand a bit more on this please? Perhaps with some code examples?

the assumption that only the last direct array suggestion can be a base hook

Cottser’s picture

Bump. Patch still applies :)

hussainweb’s picture

FileSize
18.08 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,272 pass(es), 0 fail(s), and 1 exception(s). View

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 59: 2118743-59.patch, failed testing.

The last submitted patch, 59: 2118743-59.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
813 bytes
18.19 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,271 pass(es). View

Fixing tests (hopefully).

akalata’s picture

I will take a look at reviewing this.

akalata’s picture

Another round of manual testing of template suggestion debug info:

Content admin page as a good set of debug messages to look at for *.html.twig suggestions: I compared the beginning of the system_main content block down to just inside the beginning of the exposed filters form. Existing correct template suggestions have not changed, and new template suggestions are showing up for views_view and views_exposed_form hooks.

I was trying to find debug info for the few remaining core theme functions, though I realized that those might not be showing up because they aren't Twig-based? If this is the case, the "THEME DEBUG" and "THEME HOOK" might be misleading (since I expected theme hook information to also show up here). If we can't extend the debug info to theme_ functions, maybe we need to rename the labels? Or is this an RTFM moment since services.yml clearly states that it's just for Twig functions?

I'm not sure if the request for elaboration in #57 happened elsewhere, but I think the logic is sound (and certainly how I've expected template suggestions to work). I also tested to be sure that the order of suggestions follows the order described in the documentation (and updated documentation where it differed from actual behavior in HEAD).

Antti J. Salminen’s picture

Sorry, I pretty much just forgot about expanding on what I meant with the difference.

Here's an example:

  • Take ['mandarin__spain', 'tangerine', 'mandarin__brazil'] as an example of directly passed array of theme hooks
  • If tangerine is implemented, hook_theme_suggestions_tangerine() will be called for it (instead of mandarin), and it can add for example 'tangerine__florida'
  • In the old version this will result in tangerine__florida being chosen if it's implemented, in the patch version it won't because base hooks are assumed to be in the last element

I haven't seen any actual use with base hooks in the middle like this and it's definitely pointless without multiple different base hooks involved. To me it seems like it could be just a side-effect of the current implementation.

@Akalata: Thanks for the review. Your point about theme functions not having debug information is good and it is caused by the printing of the debug information being part of the Twig engine currently. I had the thought that this functionality isn't really specific to the used theme engine and could be moved to a better place myself earlier as well.

akalata’s picture

@Antti can you think of any case where multiple base hooks would be an optimal solution? Or where the base hook isn't the last hook in the list of suggestions?

While technically possible to do with the code, I don't think it's something that people should be doing - so I'm personally not worried about it, but interested in other perspectives.

akalata’s picture

also re: #65, adding suggestions for theme_ function implementations would be cool, but would be out of scope for this issue (but would be great to have!). Could we think about changing the label as an intermediate step?

Antti J. Salminen’s picture

#66: I can't think of those examples and I tend to agree with you, but I've seen one example where the base hook isn't the last. The first test in #2111079: Add @code sample and test coverage per hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter() does that. Maybe @Cottser could comment on if that's based on a real-world use case?

#67: I agree changing the label could be a good idea, not sure what the replacement would be...

Cottser’s picture

@Antti J. Salminen I think I just did that test wrong, when you pass an array of hooks it will stop at the first one it finds so that should be reversed in the test.

Cottser’s picture

Or in other words more specific should come first.

flocondetoile’s picture

Status: Needs review » Needs work

Patch #62 has worked fine on Drupal 8.0.1. But applying patch after upgraded to 8.0.2 add the generic suggestion (block.html.twig, node.html.twig, etc.) in first position and so is always matched.

lauriii’s picture

Status: Needs work » Needs review
FileSize
18.4 KB

Rerolled the patch

joelpittet’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
294.86 KB
266.65 KB

Unless anybody has an issue with this solution it seems to fix things up and provides tests.

Screenshots.

Before

After

@akalata could you open up a follow-up to change the label and discuss what it should say so we can fix this separate. I also think it may alleviate confusion but since theme functions are deprecated or being deprecated it may be only good for D7.

alexpott’s picture

I'm not sure about this change - doesn't it mean we're doing a lot working stuff out in the ThemeManager that we will only use if twig debug is on?

dawehner’s picture

@alexpott What is the problem with that? Its not like dramatically more work and it actually fixes a bug

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  • +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -161,33 +162,33 @@ public function render($hook, array $variables) {
    +    if (!$theme_registry->has($hook)) {
    

    I think this can just be !$hook_found now... which helps with working out how all this hangs together.

  • +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -161,33 +162,33 @@ public function render($hook, array $variables) {
    +    // Get all the more generic fallbacks and check for them if there's no
    +    // implementation yet. If there's still no implementation, log an error and
    +    // return an empty string.
    

    This comment needs work. I suggest something along the lines of: Get the generic fallbacks. If a valid hook implementation has not been found, check each fallback until one is.

    The If there's still no implementation, log an error and return an empty string. is redundant and wrong - we return FALSE. I was going to suggest moving to above the relevant if but doing that made me realise this.

Antti J. Salminen’s picture

@alexpott: I made the suggested changes, had to do a few more to let $hook_found be always defined. Does this look better?

Antti J. Salminen’s picture

Status: Needs work » Needs review

Forgot to set needs review to see if it passes tests.

Status: Needs review » Needs work

The last submitted patch, 77: twig_debug_output_does-2118743-77.patch, failed testing.

Antti J. Salminen’s picture

Status: Needs work » Needs review
FileSize
18.57 KB

Oops, helps to have the right branch. This should apply cleanly I hope...

Status: Needs review » Needs work

The last submitted patch, 80: twig_debug_output_does-2118743-78.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
18.56 KB

Simple reroll

Status: Needs review » Needs work

The last submitted patch, 82: twig_debug_output_does-2118743-82.patch, failed testing.

lauriii’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Needs review

This probably helps a bit

Status: Needs review » Needs work

The last submitted patch, 82: twig_debug_output_does-2118743-82.patch, failed testing.

The last submitted patch, 82: twig_debug_output_does-2118743-82.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
18.93 KB
2.12 KB

This bit of code is very complex and for that reason hard to understand. I tried to add few lines of docs to make it more clear what happens there and for what reason. Let's see whats test bots response for this.

There was also problem where we were setting log messages in unnecessary cases.

Status: Needs review » Needs work

The last submitted patch, 87: twig_debug_output_does-2118743-87.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
18.89 KB
1.35 KB
Cottser’s picture

  1. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -145,48 +145,52 @@ public function render($hook, array $variables) {
    +    // All theme hooks are converted into an array. Save info to be used later
    +    // whether conversion has happened.
    

    I think the wording might be a bit confusing here. Maybe either no comment at all (or consider a slightly more descriptive var name like original_hook_is_array) or something like "All theme hooks are converted into an array, so we create $hook_is_array so that it can be checked later."

  2. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -145,48 +145,52 @@ public function render($hook, array $variables) {
    +    // Check for hook implementation. If an array of hook candidates were
    +    // passed, use the first one that has an implementation.
    +    foreach ($theme_hooks as $candidate) {
    

    Can we check $hook_is_array here before doing the foreach? Otherwise I think we do unnecessary processing when $hook is a string. What do you think?

greggmarshall’s picture

#89 solved an issue I was having with templates for certain entity references inside the paragraphs module, thanks.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Antti J. Salminen’s picture

How about this for avoiding the is_array(). Also tried to clarify the comment accompanying the rather weird behavior related to the watchdog messages vs. arrays of hooks.

Cottser’s picture

Version: 8.2.x-dev » 8.1.x-dev

I think this fix could be included in 8.1.x.

clemens.tolboom’s picture

Please add this to 8.1.x :-)

I needed this info for 8.0.5 so added kint($hook) in Drupal/Core/Theme/ThemeManager.php:136 render method like

  public function render($hook, array $variables) {
    kint($hook);
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice, we moved logic from the template engine more towards the theme layer. This is great, given that the theme engine should not have to implement this complex logic.
We also extended the test coverage for the new capability to see views template suggestions ...

The changes to the ThemeManager look sane as well.

+++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
@@ -145,48 +145,52 @@ public function render($hook, array $variables) {
+    while ($pos = strrpos($hook_pattern, '__')) {
+      // Save all derived hooks to be used later as a theme suggestion.
+      $derived_hooks[] = $hook_pattern;
+      $hook_pattern = substr($hook_pattern, 0, $pos);
+      if (!$hook_found && $hook_found = $theme_registry->has($hook_pattern)) {
+        $hook = $hook_pattern;
       }

out of scope of this issue: On the longrun I'm wondering whether we could move the logic to retrieve theme registry entries to the theme registry and let the theme manager deal with the rest of the problem space

Cottser’s picture

Version: 8.1.x-dev » 8.2.x-dev

Since this changes so much I don't think we should commit it to 8.1.x at this time (too risky), but we can get it into 8.2.x I think :)

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
@@ -145,48 +145,51 @@ public function render($hook, array $variables) {
+    if (!$hook_found) {
+      // Do not create a watchdog warning for arrays of hooks because their
+      // every element is allowed to be unimplemented.

I don't understand 'their every element is allowed to be unimplemented' - is this 'every element of the array'?

Antti J. Salminen’s picture

@catch: Yeah, that's what the comment is trying to say.

Cottser’s picture

@lauriii and I pair reviewed this patch for a while a couple days ago and I spent several hours today going over everything and trying some real world examples to see if I could break it.

Summary of what I'm asking for after this review, detailed thoughts below the horizontal rule:

  1. Write a change record because we are adding more output to $variables['theme_hook_suggestions'].
  2. Add unit tests and another variation on the web test around at least the parts of Drupal\Core\Theme\ThemeManager::render() that we are changing here - deriving theme suggestions especially.
  3. Four code and docs changes.

  1. This could likely use a change record because we are changing the value of $variables['theme_hook_suggestions'].
  2. I may have found a slight logic change with the patch (I think point #3 below may fix it but I've been staring at it too long today). I think in a followup we should revisit the logic around this to make sure it makes sense but probably in another issue. The logic is a bit complex and it would be best to focus this on fixing twig_debug for these use cases and not change the behaviour if we can help it. I would feel much better if we had some unit tests around some of the logic we are changing inside render() to make sure we are not changing behaviours.

    For the "pattern" used to derive more generic suggestions, I think there is a behaviour change and perhaps a bit of an assumption that the last item in the array of theme hooks is "the original hook" - there is a comment that says:

    +    // Remove the last item of the array which is the original hook and use it
    +    // as a pattern.

    Before: First theme hook in the array that is implemented, OR if none are implemented, the last theme hook.

    For this last part I'm not sure if it's a bug or by design. I'm talking specifically about the $hook = $candidate; after foreach ($hook as $candidate) {. When nothing is found $hook will be the last candidate that was checked.

    After: Always the last theme hook in the array regardless of implementations (via array_pop).

    Below are a couple example render arrays you can use to test this behaviour with and without the patch (the behaviour in terms of which templates are picked up seems to stay the same).

    $kittens = [
      '#theme' => [
        'item_list__stuff__things__kittens',
        'item_list__stuff__things',
        'item_list',
      ],
      '#items' => [1, 2, 3],
    ];
    
    $kittens = [
      '#theme' => [
        'item_list__stuff__things__kittens',
      ],
      '#items' => [1, 2, 3],
    ];
    

    With and without this patch, using the first render array a template of item-list--stuff.html.twig won't get picked up. I don't think any of the current web tests cover that behaviour either, so we should add that too IMO.

  3. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -145,48 +145,51 @@ public function render($hook, array $variables) {
    +    $hook_pattern = array_pop($theme_hooks);
    

    Based on the logic from twig.engine we are porting in here I think it might make more sense to use $original_hook rather than array_pop($theme_hooks);.

  4. The comment @catch pointed out in #98 could use some rewording. I don't think it's necessarily helpful to get into detail here, I would suggest something really simple like:

    Do not log warnings when an array of theme hooks is passed in.

  5. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -145,48 +145,51 @@ public function render($hook, array $variables) {
    +      if (!is_array($hook)) {
    +        \Drupal::logger('theme')->warning('Theme hook %hook not found.', array('%hook' => $candidate));
           }
    

    I think it would be less confusing to use $hook here rather than $candidate because we know $hook is a string.

  6. +++ b/core/themes/engines/twig/twig.engine
    @@ -82,31 +82,18 @@ function twig_render_template($template_file, array $variables) {
    +      $pos = strpos($variables['theme_hook_original'], '__');
    ...
    +      if ($pos !== FALSE) {
    +        $suggestions[] = substr($variables['theme_hook_original'], 0, $pos);
    +      }
    +      else {
             $suggestions[] = $variables['theme_hook_original'];
           }
    

    This logic could use a comment to say that it's appending the base hook or something along those lines.

Antti J. Salminen’s picture

@Cottser:

Thanks for taking the time to thoroughly review!

Regarding some of the items:

2.
If the hook is implemented the currently committed version doesn't use the pattern so it doesn't really matter. The only case where this would make a difference as far as I know is the case that I mentioned in #41, #56 and tried to describe in #65.

The reason was that If alternate base hooks (with or without a suggestion) are permitted in elements besides the last one, the list of possible templates to implement won't be just a single list of template names. It means that more suggestions are possibly inserted into the list by hook_theme_suggestions_HOOK() in the case that they have an implementation in the registry.

At least this is what I believe the implications are, it's been a year since I really looked at this in depth. My assumption was that this is not something that was ever intended as a real use case, I've found nothing implying so in code or ancient issue discussions and it certainly makes the way the hooks are selected more complicated.

3.
As outlined above, If this was changed to $original_hook, the list of hooks that twig_debug outputs would change depending on what you have implemented and would not show the full list of possible template/theme function names.

4.
Don't you think the explanation is the part that is actually useful about the comment? Without it, it just states the exact same thing you can see from the lines it's commenting on. I can't really tell what @catch's stance on this was from his comment.

The reason for the behavior isn't obvious to me from the code and I added the explanation because I was confused enough that I didn't properly address it in a version of this patch.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kclarkson’s picture

Would love to see this move forward.

Antti J. Salminen’s picture

I looked at the current functionality and the patch to carefully look at the possible concerns in detail and I noticed there is one issue that needs addressing related to what @Cottser wrote previously: If there is a hook derived from a base hook in the array that does not follow the naming pattern of suggestions (name doesn't start with the base hook and have the suggestion(s) separated with '__'), the displayed base hook for the Twig debug output will not be correct.

Other than that I believe what I wrote before is correct provided that the assumption I have mentioned previously, namely that all the hooks in the passed theme hook array *must* have the same base hook holds true. I hope that I properly addressed the other concerns in @Cottser's review there in that previous comment. I think coming up with a fix for the remaining problem and adding some tests that expose the assumptions/behavior for added clarity and coverage as he mentioned is a good way forward. I've also searched all of contrib for usages where the assumption regarding the base hook would be a problem, I'll report my findings once that's finished.

lauriii’s picture

Issue tags: +Triaged D8 major

@alexpott, @Cottser, @joelpittet, @xjm, and I discussed this and agreed that this is a major bug because of the widespread TX issues this causes. This mostly effects theming Views, which is a common use case. There are no known workarounds for this.

This is also a regression from Drupal 7 where views used to provide this information in the Views UI, and now it is not available anywhere.