Fix the order in which templates are selected for Node, User, and Term.

[sbgraphicdesign]

In D7 I have theme node template files like so:

   node--1234.tpl.php
   node--blog.tpl.php

The first one, node--1234 template is selected for the node with that NID value, overriding the node--blog template, even when that node is a Blog content type..

I'm moving the site from D7 to B, and it appears in Backdrop this behaviour is the other way round. The Devel module is reporting this:

<!-- FILE NAME SUGGESTIONS:
   * node--blog--full.tpl.php
   x node--blog.tpl.php
   * node--1234.tpl.php
   * node.tpl.php
-->

Surely a template that has a NID value is a more specific target, and should override any other template?

Is my site/theme behaving correctly? Perhaps I'm I misunderstanding the output from the Devel module. If so, what could cause my Node TPL file with the NID value in the name not get selected as I'm expecting?

---

Update:

This is related to this bug: Adding the theme_hook_suggestion node__blog__premium in mymodule_preprocess_node() (and adding a template file with that name) should select that template, yet it results in choosing node--blog.tpl.php if that template exists:

<!-- CALL: theme('node__blog__full') -->
<!-- FILE NAME SUGGESTIONS:
   * node--blog--full.tpl.php
   x node--blog.tpl.php
   * node--blog--premium.tpl.php
   * node--235100.tpl.php
   * node.tpl.php
-->

[avpaderno]

Theme suggestions are checked in FILO order, so the last suggestion is the first one to be checked.

This is documented in a comment in theme().

If the preprocess functions specified hook suggestions, and the suggestion exists in the theme registry, use it instead of the hook that theme() was called with. This allows the preprocess step to route to a more specific theme hook. For example, a function may call theme('node', ...), but a preprocess function can add 'node__post' as a suggestion, enabling a theme to have an alternate template file for post nodes. Suggestions are checked in the following order:

• The 'theme_hook_suggestion' variable is checked first. It overrides all others.
• The 'theme_hook_suggestions' variable is checked in FILO order, so the last suggestion added to the array takes precedence over suggestions added earlier.

The code following that comment is this.

$suggestions = array();
if (!empty($variables['theme_hook_suggestions'])) {
  $suggestions = $variables['theme_hook_suggestions'];
}
if (!empty($variables['theme_hook_suggestion'])) {
  $suggestions[] = $variables['theme_hook_suggestion'];
}
foreach (array_reverse($suggestions) as $suggestion) {
  if (isset($hooks[$suggestion])) {
    $info = $hooks[$suggestion];
    break;
  }
}

The Devel module shows the suggestions as found in the array, without passing first the array to array_reverse().

[sbgraphicdesign]

Thanks for responding. I'm not sure I'm clear on what you're suggesting though, as your post is too technical for me, I'm afraid.

Is my Backdrop theme behaving as it should be? If so, I'd argue that the logic is wrong, and any theme template file with a Node ID in the name should be the most specific template file, and therefore be the to be selected.

I'm not sure if I'm experiencing expected behaviour, or a bug.

[avpaderno]

I'd argue that the logic is wrong, and any theme template file with a Node ID in the name should be the most specific template file

The template files you shown are looked for in the following order.

• node.tpl.php
• node--1234.tpl.php
• node--blog.tpl.php
• node--blog--full.tpl.php

The one for the specific node is the second template file Backdrop looks for. node.tpl.php is always the first one, and that is expected.

[olafgrabienski]

any theme template file with a Node ID in the name should be the most specific template file

I agree, as a site architect, I'd expect exactly this behavior.

[klonos]

Although I know what they are, I haven't actually worked extensively with theme suggestions, but yes, I would also expect that a suggestion with a specific node ID would always "win" and take precedence over any other template.

[herbdool]

I'm putting the label back to bug since it's something that subverts most expectations. I haven't seen a reason yet for why it's better this way. Perhaps someone can explain why it's not a bug.

[herbdool]

@argiepiano has a good explanation what's going on here: https://backdrop.zulipchat.com/#narrow/stream/218635-Backdrop/topic/theme_hook_suggestions/near/450034424. From that comment:

This strange behavior is the result of NodeStorageController::view(). Unlike D7s Entity API, Backdrop actually hard-codes this:

      $build += array(
        '#theme' => 'node__' . $node->type . '__' . $view_mode,
        '#node' => $node,
        '#view_mode' => $view_mode,
        '#langcode' => $langcode,
        '#page' => $page,
      );

Which means that the #theme takes precedence over the theme_hook_suggestions suggestions. Since the . '__' . $view_mode part doesn't exist in my template example above (only the node type was provided), the theming layer "prefers" using node__' . $node->type over any of the suggestions provided in the preprocess function.

This is the issue where it was changed: https://github.com/backdrop/backdrop-issues/issues/315 but there doesn't seem to be any explanation on why it was done this way. So seems like indeed it is a breaking change that might not be necessary.

The suggested change (from @argiepiano):

One way to fix this would be to remove 'node__' . $node->type . '__' . $view_mode, and move it to a "suggestion" , in template_preprocess_node(), instead of hard-coding it. So, NodeStorageController::view() would be changed to

      $build += array(
        '#theme' => 'node',
        '#node' => $node,
        '#view_mode' => $view_mode,
        '#langcode' => $langcode,
        '#page' => $page,
      );

and template_preprocess_node() would look like:

  $variables['theme_hook_suggestions'][] = 'node__' . $bundle;
  $variables['theme_hook_suggestions'][] = 'node__' . $bundle . '__' . $view_mode;
  $variables['theme_hook_suggestions'][] = 'node__' . $node->nid;

[avpaderno]

The only comment about that is in theme() and says:

• The 'theme_hook_suggestion' variable is checked first. It overrides all others.
• The 'theme_hook_suggestions' variable is checked in FILO order, so the last suggestion added to the array takes precedence over suggestions added earlier.

It is shown right before the following code.

$suggestions = array();
if (!empty($variables['theme_hook_suggestions'])) {
  $suggestions = $variables['theme_hook_suggestions'];
}
if (!empty($variables['theme_hook_suggestion'])) {
  $suggestions[] = $variables['theme_hook_suggestion'];
}
foreach (array_reverse($suggestions) as $suggestion) {
  if (isset($hooks[$suggestion])) {
    $info = $hooks[$suggestion];
    break;
  }
}

[bobchristenson]

I'm not sure about the code logic of why things are happening how they are...but @herbdool is exactly right. Any custom template suggestions (no matter what they are) should take precedence over stock/core template suggestions. That's how Drupal 7 always worked, and it's the expected behavior. I posted an issue about custom templates, specifically: https://github.com/backdrop/backdrop-issues/issues/6649

But, in terms of core template suggestions, the node_id suggestions should obviously be used over the content type. It's only logical because nid is more specific. Again this is how it always worked in D7 and it really should work that way here too.

[herbdool]

I've got a PR now - ready for testing: https://github.com/backdrop/backdrop/pull/4825. Would need to try it it out locally. Can create a copy of node.tpl.php and put it into an active theme and rename it node-1.tpl.php. It should now take precedence if on /node/1. And same with adding theme suggestions in a custom module.

[bobchristenson]

Just put your changes in place and I can confirm that it created the behavior I expected. My custom preprocess_node template declarations now work and my node--[nid] templates are working as well.

[herbdool]

I think the original change was to replicate some behaviour found in https://git.drupalcode.org/project/entity_view_mode/-/blob/7.x-1.x/entity_view_mode.module?ref_type=heads#L360 since it was incorporating the entity_view_mode functionality into core.

[herbdool]

I created an alternative PR which adapts the code from entity_view_mode to add theme suggestions for all entities: https://github.com/backdrop/backdrop/pull/4825.

[argiepiano]

@herbdool thank you for providing this PR. I've done some quick testing with the 4 core entities (node, file, term and user).

This is what I'm seeing:

Node

The PR fixes the issue 👍🏽

File

File entity templates don't need the PR to work as expected. The one with the FID is highest in the list and is the one that takes precedence over all of the rest.

The only strange thing about this one is that the "base" template file in core/modules/file/templates is file.tpl.php. However, the base template suggestion is file-entity.tpl.php. So, if you copy file.tpl.php into your theme's template folder, it won't be picked up. You have to rename it to file-entity.tpl.php. This could be fixed in a separate issue. However, it has to be done carefully to avoid breaking existing sites.

The way to fix this might be (untested):

• If FileStorageController::view(), replace '#theme' => 'file_entity', with '#theme' => 'file',
• Rename template_preprocess_file_entity() into template_preprocess_file(),
• Be sure to call the implementations template_preprocess_file_entity() from within template_preprocess_file() to avoid breaking existing sites
• Add the suggestion file_entity in template_preprocess_file()

Taxonomy terms

The PR does not fix the problem here 👎🏽

With the PR, this is what it looks like:

Backdrop gives preference to taxonomy-term--tags.tpl.php over taxonomy-term--1.tpl.php.

The issue with Terms is the same we had for nodes before this PR. In the view method, we still have '#theme' => 'taxonomy_term__' . $term->vocabulary . '__' . $view_mode,

This needs to be replaced by: '#theme' => 'taxonomy_term,`

Doing so, fixes the issue. I suggest we take care of this in this PR.

User

User entity templates did not include any suggestions with uid before. The PR adds those, but the user--1.tpl.php is not picked at all 👎🏽

Again, like file, this is a bit strange, since the suggestions are mixed - some are user-profile.tpl.php but the ones with the uid are user--1.tpl.php. The solution may be similar to what I suggested above for file, perhaps on a separate issue.

---

I would also suggest that we test with entities that use EntityPlus's controllers (e.g. Paragraphs, Registration).

[herbdool]

@argiepiano which PR did you test? Seems like it was the alternative PR. Perhaps we should focus on the less radical one for this bug fix and I could put the alternative PR on a follow-up issue. To help ensure the bug is fixed in a patch release. And can focus on a more ambitious fix in a minor release.

[argiepiano]

I tested the second one, more radical: https://github.com/backdrop/backdrop/pull/4826

Perhaps we should focus on the less radical one for this bug fix and I could put the alternative PR on a follow-up issue. To help ensure the bug is fixed in a patch release. And can focus on a more ambitious fix in a minor release.

Sounds good. I'll do so probably tomorrow evening.

[herbdool]

@argiepiano I've now moved the second PR to the new issue: https://github.com/backdrop/backdrop-issues/issues/6651

[argiepiano]

Tested and reviewed - WFM. Since I suggested this PR on Zulip, perhaps someone else should review as well, and mark this as RTBC?

[argiepiano]

I added a minor comment to the PR...

[herbdool]

@argiepiano whichever core committer looks at it will review the code too, so I think it's okay if you update the rtbc label first.

[argiepiano]

I see you added quite a bit of code to the PR, supporting naming suggestions for Taxonomy Term, User and Comment entities. I think it'd be a good idea to re-title this issue and the PR to reflect that. I'll remove the WFM label for now, until I test the new PR.

[argiepiano]

Also, since the PR now addresses all those other entities, should there be some automated tests? I see that only the node tests have been modified.

[argiepiano]

@herbdool I've reviewed and tested, and left comments in the comment code. Changing to NW. Everything except comment is working well.

[argiepiano]

Thanks @herbdool - a couple of comments re: the new tests. Setting to NW.

[argiepiano]

LGTM!

[quicksketch]

Hi folks, thanks for the work and PR. I agree with both the analysis and the fix here. Given that this is restoring the D7 API (and probably earlier versions of Backdrop prior to implementing Entity API), and it's almost impossible that you'd want a per-type template to override a specific node template, I think this is indeed a bug fix to a regression, rather than an API change. So putting into the next bugfix release makes sense to me.

Great work getting test coverage in, and expanding this to all the core entity types. I really appreciate the thoughtful reviews and feedback in this issue.

I've merged https://github.com/backdrop/backdrop/pull/4825 into 1.x and 1.28.x. Great job folks!

[quicksketch]

https://github.com/backdrop/backdrop/commit/a84b87db2f205e19b188ea7daa905cd57491f7e2 by @herbdool, @argiepiano, @sbgraphicdesign, @avpaderno, @bobchristenson, @klonos, and @olafgrabienski.

Thank you everyone!

[quicksketch]

I made a change record at https://docs.backdropcms.org/change-records/theme-suggestion-order-for-nodes-users-and-terms-updated

And I updated the theme debug mode documentation at https://docs.backdropcms.org/documentation/theme-debug-mode

[jenlampton]

It looks like this reverted the previous changes for node, user, and term, but leaves the previous code in place for comments.

See https://github.com/backdrop/backdrop/blob/1.x/core/modules/comment/comment.entity.inc#L508

 '#theme' => 'comment__node_' . $node->type . '__' . $view_mode,

Were comments tested, and it was determined that comments did not have the same issue, or should we create a follow-up to address comments?

edit: I did't see any mentions of comment in this issue, so I expect that was a simple oversight. I've opened a placeholder https://github.com/backdrop/backdrop-issues/issues/6714 to add to the next milestone.

[herbdool]

@jenlampton I had changes for comment (and file) but reverted because they were a bit more than just a bug fix. See https://github.com/backdrop/backdrop/pull/4825#discussion_r1674837228