Twig debug output does not display all suggestions when an array of theme hooks is passed to #theme

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>

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?

3: theme-change-twig-debug-theme-call-to-theme-hooks-2118743-3.patch queued for re-testing.

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

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

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

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;

+++ 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.

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.

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.

I have made just the reroll

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

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?

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.

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.

Just triggering test runs, still needs work.

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 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.

Fixed a small nitpick.

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.

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?

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

@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.

@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.

@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?

@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.

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.

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).

Rerolled.

Fixing tests (hopefully).

I will take a look at reviewing this.

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).

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.

@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.

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?

#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...

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.

Rerolled the patch

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.

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?

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

• +++ 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.

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

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

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

Simple reroll

This probably helps a bit

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.

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

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.

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);

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

+++ 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'?

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

@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.

Would love to see this move forward.

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.

@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.

This patch applies specifically to Drupal 8.3.0, to be used in a Composer file.

I'm working (off and on) to incorporate @Cottser's feedback incorporated into a new patch during the DC Baltimore sprints.

Posting my work in progress. Changes in this patch:

• Rewording of some comments for clarity/grammar
• Removed the $original_hook variable as it is not needed
• Added some tests to cover a more specific suggestion being used over a more general one
• Added tests for invalid single theme suggestions (one that derives to a valid suggestion and one that does not)

I still need to address #100.2.

The testbot is happy, but back to Needs Work until #100.2 is addressed. Also realized we're missing tests for when there is an array of options that aren't implemented but one of the derived options is.

+++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
@@ -210,9 +205,11 @@ public function render($hook, array $variables) {
+
+    // Save the original theme hook, so it can be supplied to theme variable
+    // preprocess callbacks.
     $variables += [
-      'theme_hook_original' => $original_hook,
+      'theme_hook_original' => $candidate,
     ];

Is the _original theme hook_ or the _candidate_ supposed to be saved? Either adjust the comment or revert $variables.

@clemens.tolboom, thank you for the review.

Is the _original theme hook_ or the _candidate_ supposed to be saved? Either adjust the comment or revert $variables.

They are the same. Earlier, we set a variable named $original_hook that was only used to set $variables['original_hook']. That seemed extraneous to me, but I suppose it did help with readability...

I believe that #2752443: Incorrect order and duplicate theme hook suggestions is the appropriate place to handle the logic side of this issue, namely the order of suggestions and whether derivative suggestions are used for both string and array #theme items. I *think* this addresses @Cottser's concerns in #100.2 so I'm removing the Needs followup tag.

The tests I considered missing in #110 should probably be handled in #2752443: Incorrect order and duplicate theme hook suggestions as well, so I'm removing the "Needs tests" tag. I believe we've got the tests we need to cover the fact the debug info doesn't appear. Plus some extras that would be better handled in the followup (see the interdiff).

Added a draft CR.

I just tried this and it works very nice and for twig debugging views is almost a must.

Thanks for your help trying to get this issue landed. I'm starting to be pretty confident about this, but there are a few things I think we should still try to solve here.

1. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -140,48 +140,43 @@ public function render($hook, array $variables) {
   +      if (is_string($hook)) {
   

   We will have to change this since the $hook is now always an array. We probably should also add test coverage for this since the tests are passing and it seems like this was broken.

2. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -210,9 +205,11 @@ public function render($hook, array $variables) {
   +      'theme_hook_original' => $candidate,
   

   I don't fully understand why is the original theme hook now determined. Probably we should find this value in another way than we do currently or at least document how it is determined.

@lauriii, thank you for the review!

re: #116:

We will have to change this since the $hook is now always an array.

I think there's still plenty of places we call render with a string instead of an array for $hook. And we don't force $hook to be an array in the code so I don't think this is correct.

Agreed, though, I couldn't find a test for the warning when an invalid single template string is passed.

I don't fully understand why is the original theme hook now determined.

That makes two of us... :)

Honestly, that was in the original code so I think the hope was to change as little as possible. There is also a lot of confusion on how theme suggestions should be ordered (least-to-most specific or the opposite) and how derived suggestions are to be included in the mix. Is there a source of truth for how this should be handled?

Needs work?

sites/drupal/d8/www % git apply 2118743-113-twig-debug-info.patch
error: core/modules/views/src/Tests/ViewsThemeIntegrationTest.php: No such file or directory

Rerolled. Tests moved to core/modules/views/tests/src/Functional/ViewsThemeIntegrationTest.php.

Yay!!! Template suggestions for Views! Fantastic work everyone.

Working great.

The patch, #119, fixed the problem for me as well. Thanks.

@Cottser, thank you for the review!

I've replaced all long-syntax array declarations with short-syntax array declarations, some of which were handled in other bug fixes and I think I may found another couple along the way. Anyhow,

$ git diff 8.4.x | grep "array("
-    if (is_array($hook)) {
$ 

So I think that should cover it...

Updated the inline comments to (hopefully) better reflect what's happening in the code.

Oops.

These are the list of theme hook suggestions for views module which has not included in suggestions. So it will not display while twig debugging.

views-view--foobar--page.html.twig
views-view--page.html.twig
views-view--foobar.html.twig
views-view.html.twig

views-view-unformatted--foobar--page.html.twig
views-view-unformatted--page.html.twig
views-view-unformatted--foobar.html.twig
views-view-unformatted.html.twig

views-view-fields--foobar--page.html.twig
views-view-fields--page.html.twig
views-view-fields--foobar.html.twig
views-view-fields.html.twig

Hi,

This patch will create possible template suggestions for views template.

In same way we can create suggestion for views display and views field templates.

@bapi_22, please don't hijack an existing issue. The patch in #124 is the correct one for this issue.

Yes markcarver,

Its great job. Its a complete suggestion for views as well as its child elements.

Thanks to everyone who's been working on this. Great to see it moving forward!

+++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
@@ -169,18 +166,13 @@ public function render($hook, array $variables) {
-      // Do not create a watchdog warning for arrays of hooks because their
-      // every element is allowed to be unimplemented.
-      // This is utilized with for example form arrays to provide a theme hook
-      // that could be used if theming a specific form.
-      if (!is_array($hook)) {
-        \Drupal::logger('theme')->warning('Theme hook %hook not found.', array('%hook' => $candidate));
+      // Only log missing theme hook implementations for single (string) hooks.
+      if (is_string($hook)) {
+        \Drupal::logger('theme')->warning('Theme hook %hook not found.', array('%hook' => $hook));

I don't understand why this got changed and I'd still like to see something closer to the older comment that actually tried to explain why this exception exists. I believe comments should primarily be used for answering why the code does what it does and only repeat what it does when that is non-obvious to a less-experienced reader (for example summarising larger block of code or some unfamiliar idiom). See also: http://wiki.c2.com/?CodeComments

If you don't think the explanation is needed here just removing the comment altogether would be better because the code is already very simple and clear. In general I think comments shouldn't be used too sparingly but the current version just doesn't add much.

We could almost just concatenate the two comments. The wording is still a little awkward.

Do not create a watchdog warning for arrays of hooks because their every element is allowed to be unimplemented. This is utilized with for example form arrays to provide a theme hook that could be used if theming a specific form. Only log missing theme hook implementations for single (string) hooks.

What if we changed it to:

Do not create a watchdog warning for arrays of hooks because every element is allowed to be unimplemented. For example, this is utilized to allow form arrays to provide a theme hook that could be used to theme a specific form. Therefore, we only log missing theme hook implementations for single (string) hooks.

How does that sound?

I went back and forth between which sounds better:

...because every hook in the array is allowed to be unimplemented

or

...because not every hook in the array is required to be implemented

I went with @Cottser's suggestion because he has core commit permissions and I don't. :) But I'm happy to switch it if folks feel the second is more clear than the first.

Also, pushing this back to 8.4.x in hopes of getting it in the next release. As per the allowed changes, I feel this falls under non-disruptive bugs where the disruption (very, very low) outweighs the benefit (we can see Views' theme suggestions!).

Yes, I like the version in #135 much better.

Out with the old, in with the new! :)

Zooming out for a second, maybe it's a bit weird that we support this, in the form example it seems like you'd want to at least have a fallback of form in case no specific suggestions are implemented but that also seems out of scope for this issue! :)

Maybe handled in #2752443: Incorrect order and duplicate theme hook suggestions? But, agreed, definitely outside the scope of this issue...

@Cottser, thanks, as always, for the review!

Nits have been picked.

I will say, one thing that still feels a bit weird is the logic for the THEME HOOK: part in Twig debug

I completely agree. Thank you for opening the followup for that.

I'm a bit concerned about this change:

-      'theme_hook_original' => $original_hook,
+      'theme_hook_original' => $candidate,

That changes the meaning of that variable, doesn't it? It was the incoming $hook variable before and now it's the last found $candidate.

In most cases it would be the first one in the array, but the way it's written if it doesn't find it, it will keep moving through candidates.

Oh weird, theme_hook_original really isn't that in some edge cases(it's mostly that, and sometimes last found hook)... thanks for double checking that @Cottser, we are good here then:) No harm, no foul.

Looks like an unrelated test failure, requeuing.

Test is back to green. Setting the status to where it was.

Adding to a queue of issues for the frontend framework managers. Thanks!

This is a massive quality of life and time saver for theming - Thank you!

This applies cleanly on 8.3.x

Minor IS updates.

Again, the test failures seem unrelated. Re-queuing.

Testbot came back green so I'm resetting to the status in #144.

Looks like the test failing is systemic to an issue with the testbot and not to do with with anything related to the patch.

I was unaware of that protocol. thank you.

I tested https://www.drupal.org/node/2118743#comment-12218334 patch with 8.4.0, and it works great!

Thanks everyone for your ongoing work on this issue!

Since this issue has been at RTBC awhile, @lauriii, @catch, @effulgentsia, and I discussed this issue today to figure out how to move it forward. There are a couple of reasons we have hesitated on it:

• The patch changes some low-level code that affects how basically all templates are loaded, and so it is risky and could have a lot of side effects. While the patch itself has good test coverage, not everything that might be affected by it does (or we don't know for sure that it does).
• Even though the patch isn't actually all that large, we need to consider the implications of each bit pretty carefully when reviewing it, so the review can take quite a bit of time.

Given these two things, we discussed whether it might be better to commit this issue very early in the development cycle for a minor release (for example, right away when we open 8.6.x in January, rather than now just two months before the alpha). Sometimes for things like this we postpone the issue and schedule it for a certain date, but we want to be ready to commit it other than that before we do so (in case there's other points that come up during review that need to be addressed).

Other than that, the main thing that's blocking this is just committers having enough time to give it the careful review it needs. In the meanwhile, though, there are a couple things that contributors can do to help give us confidence in the patch.

1. Are you using this patch on a production site? If so how is it working, and what side effects if any have you noticed?
2. Provide thorough manual testing with base themes that do theme system customizations (lauriii suggested Bootstrap as a good example).
3. Code review from core extension system contributors as well as contributed theme maintainers might also help.

I'm going to keep the issue at RTBC for now since there aren't any specific blockers currently. @lauriii might have additional feedback on the issue as well. Thanks everyone.

I can add a bit to 1. but isn't this patch something that affects only twig debug, or does it affect anything else?

In any case, I am running several productions sites, both 8.4.x and 8.3.x with #119 without any issue so far, and this patch has proven very useful for local envs.

1. Are you using this patch on a production site? If so how is it working, and what side effects if any have you noticed?
- We've been tracking the patch in production on about 20 sites (same install profile) since May 2017, without any noticeable side-effects.

The install profile uses paragraphs module with about 15 unique paragraph types on a custom, style guide driven theme.

This issue is of very little importance.

It only really focuses on the debug output concerning like 0.01% of code that actually implements the rarely used "theme hook array" (a.k.a. views).

This is a legacy "feature" that really should be converted to use the, now, proper hook: hook_theme_suggestions_HOOK().

If anything, I would say that the real "major bug" here lies within Views and its need to use the proper hooks now. Thus, I'm downgrading this issue to "Normal".

The related issue I'm attaching now is a real major bug in theme hook suggestions that affect which suggestion is actually used and should probably be fixed before this one. It may even semi-fix this one.

Provide thorough manual testing with base themes that do theme system customizations (lauriii suggested Bootstrap as a good example).

FTR, Bootstrap does little to nothing with the debug output of theme hook suggestions. That being said, given that this patch is altering the underlying suggestions array, I wouldn't be surprised if this patch actually improves the base theme in some way, regarding Views anyway.

Normal string theme hooks still seem to function the same, from what I can tell by reviewing the patch.

---

Initial quick review of latest patch:

1. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -140,48 +140,46 @@ public function render($hook, array $variables) {
   +    $theme_hooks = (array) $hook;
   

   I'm really not a fan of typecasting this. It just makes this more unnecessarily complicated.

   As I mentioned above, this "theme hook array" feature is rarely used. If anything, I think this feature should be removed entirely in the future.

   For BC reasons, we can keep this "array theme hook" detection (is_array) and just log a warning that this will be removed in the future and to use the proper hooks for theme hook suggestions.

   Changing the overall structure to "just deal with arrays" is what has gotten the theme system in the "Render arrays of doom" mess we're in now.

   Perpetuating this mentality in code is wrong.

   Especially in the theme system.

2. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -140,48 +140,46 @@ public function render($hook, array $variables) {
   +        $hook_found = $candidate;
   
   @@ -210,9 +208,11 @@ public function render($hook, array $variables) {
   -      'theme_hook_original' => $original_hook,
   +      'theme_hook_original' => $candidate,
   

   I don't like this unnecessary change.

   It may seem trivial, but given how complex this method/topic is, keeping semantic variable names is important for future work.

A little history:

In D6, #141730: Allow theming system to use wildcards introduced this "theme hook array" feature. It was, essentially, the first iteration of "theme hook suggestions" in core.

In D7, #653622: Make the '__' pattern for theme suggestions easier to use and #678714: Unify use of theme hook / template suggestions, fix clobbering problems, and improve suggestion discovery performance moved to a more "internal" way of providing theme hook suggestions via $variables['theme_hook_suggestions'] in the preprocess phase.

In D8, dedicated hooks were introduced for theme hook suggestions. These hooks removed the internal preprocess suggestions, but never removed the original "theme hook array" way of providing suggestions.

It's wildly redundant, unnecessary, should be deprecated and then ultimately removed.

@markcarver I agree with the logic but this is a further dialution of Drupal's usability for non-developers.

Basically theming shouldn't be so layered and complex. Out of the box a CMS should be able to create logic, clean markup which can be altered without too much fuss. By removing the debugging output it requires a further step to markup alterations.

The hook system isn't overally intuitive due to Drupal's endless nesting of elements which isn't always easy to discover or interpret. Worse it requires an understanding of the underlying Drupalisms. Often I find myself poking away to see what effects the markup at the right point in the render chain.

My biggest concern with only using suggestion hooks is that it seems like an impossibility to document it effectively to allow new Drupal users to make frontend alterations as you would expect in a modern CMS.

this is a further dialution of Drupal's usability for non-developers.

No, it's not. It's putting the blame where it belongs: with Views. If Views used D8's new theme suggestion hooks, then non-developers would be able to see the proper theme hook suggestions in the debug output. I think you are conflating what I have said above with an assumption that this is somehow shifting the focus from "fixing a problem" to "telling non-developers they have to code". That isn't true. What I am proposing would actually help everyone, even non-developers.

Basically theming shouldn't be so layered and complex. Out of the box a CMS should be able to create logic, clean markup which can be altered without too much fuss.

It's a nice sentiment and I generally agree. However, Drupal has over a decade of its own history. How a page is ultimately "themed" in Drupal has evolved over time (rather slowly I might add). There are years of technical debt and antiquated techniques still being used to this day. As I outlined above in #165, this issue is a direct symptom of using said outdated theming practice (from D6).

By removing the debugging output it requires a further step to markup alterations.

No one is "removing" the debugging output. What I have suggested will actually make the debugging output work properly, because it wouldn't have to deal with "arrays of theme hooks".

The hook system isn't overally intuitive due to Drupal's endless nesting of elements which isn't always easy to discover or interpret. Worse it requires an understanding of the underlying Drupalisms.

As someone who has spent their entire career in understanding the fundamentals of Drupal's theming layer, I couldn't agree more. However, you are again assuming that I am pushing the responsibility of fixing this onto non-developers. That is not the case. Views is in core. Developed by and maintained by core developers. They are the ones that should use these hooks, not non-developers.

My biggest concern with only using suggestion hooks is that it seems like an impossibility to document it effectively to allow new Drupal users to make frontend alterations as you would expect in a modern CMS.

The documentation would be the (proper) debug output. I still think you are confusing what I am saying here. I will try to break this down and explain in greater detail:

ViewExecutable::buildThemeFunctions is responsible for generating the necessary theme hooks.

As you can see, this is creating an array of theme hooks (not theme hook suggestions) that will be passed directly to #theme (typically, this is just a single string).

This technique is very old, outdated and harkens back to the days of D6. merlinofchaos, who also developed Views, created this "array of theme hooks" technique (#141730: Allow theming system to use wildcards) that was added to core (likely a direct correlation to Views). In the 11+ years that this "array of theme hooks" feature has been in core, almost nothing has really used it except Views.

Even though Views is now in core, this specific aspect of Views (providing an "array of theme hooks") really hasn't changed all that much. It is severe technical debt that was simply inherited.

Early on in D7's development (#653622: Make the '__' pattern for theme suggestions easier to use), it was realized that passing array() each time and duplicating the same prefix in hooks was entirely unnecessary and simply added more unnecessary technical debt:

But, if we're gonna use this in core, and if in core, we're settling on the '__' convention, let's make it easier to use. Instead of code having to call theme(array('links__comment', 'links'), ...), how about letting theme('links__comment', ...) work.

This is when the "array of theme hooks" technique should have been deprecated and Views' code adjusted to using proper preprocess alterations to the $variables['theme_hook_suggestions']. However, given that Views was still contrib and my own personal experience with trying to fix bugs in Views was always met with hostility and a superiority complex, I can see how that never happened.

Now that, in D8, there are dedicated hooks for providing and alter actual theme hook suggestions, Views' code should be fixed (moved) to utilize them.

This simple change would just mean that instead of providing an "array of theme hooks" to #theme, it would provide a single theme hook and then these "arrays of theme hooks" would then be provided as proper suggestions later when they're actually needed.

This would make everything "just work", even for non-developers.

@markcarver Thank you for the great suggestion. Let's see whether its possible to implement that at least for views: #2923634: [PP-1] Use hook_theme_suggestions in views

So what is the actual problem with the patch? This is an RTBC patch that fixes an issue with DX.

Can we just revert this patch after the underlaying issue with views is fixed? I just don't understand why this was changed from RTBC to needs work. What work needs to be done?

See my review in #164.

I'm sorry, its been a long week already. I completely missed the later half of your comment. I read the whole rest of the thread though. Sorry.

I'm sorry, its been a long week already.

I hear that, no worries :D

Can we just revert this patch after the underlaying issue with views is fixed?

Since the "array of theme hooks" feature has been in core since 6.x, this is still technically a bug and needs to be fixed. So no, I don't think it should be reverted after Views gets fixed. It should, however, be removed entirely once all deprecated code is removed in the future.

I just don't think that focusing on this particular issue is all that important right now considering that it's just a symptom due to outdated theming techniques.

The end goal of the OP (Views theme hook suggestions) can be fixed using the proper hooks first, without fiddling around with the internals of an already extremely complicated theme "system".

In my mind, the priority of importance is as follows:
#2923634: [PP-1] Use hook_theme_suggestions in views
#2752443: Incorrect order and duplicate theme hook suggestions
#2923506: Deprecate and discourage use of the "array of theme hooks" feature
... then this issue.

How the above issues are tackled will likely affect how this issue implements it's "fix" for displaying "suggestions" for an "array of theme hooks". Given that, I would almost say this issue should probably be postponed until then.

I agree with #174. The work done here is as huge as important, but this fix may create more problems than the ones is trying to fix if we don't do #2923634: [PP-1] Use hook_theme_suggestions in views, #2752443: Incorrect order and duplicate theme hook suggestions and #2923506: Deprecate and discourage use of the "array of theme hooks" feature before.

So let's postpone this in favour of those three.

I'm also updating the IS to reflect these thoughts.

Ignore these patch files (they include by mistake a /web directory)
Use #179, works like a charm

Until a better solution for Views suggestions is found, this patch is awesome!
I re-applied manually @mikeker patch #139
It is now working with Drupal 8.6.1