With the following module code, I'm able to serve up my-xyz-index.tpl.php from my module directory:

function my_abc_menucallback() {
  return theme('my_abc_index');
}

function my_abc_theme() {
  return array(
    'my_abc_index' => array(
      'template' => 'my-xyz-index'
    );
}

However, if I create a my-xyz-index.tpl.php in my theme's directory (and rebuild the registry), the new .tpl file will not be recognized (not added to the theme registry).

Turns out the template file name needs to exactly match the array key used in hook_theme (with the exception of dashes and underscores). In the case of my example, the template value would need to be my_abc_index and the file name my-abc-index.tpl.php. Failure to do so will result in an un-over-ridable template.

So the following works fine with a template override in the theme:

function my_abc_menucallback() {
  return theme('my_abc_index');
}

function my_abc_theme() {
  return array(
    'my_abc_index' => array(
      'template' => 'my-abc-index'
    ),
}

I'm assuming this is a code bug and not a documentation bug.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dvessel’s picture

Version: 6.6 » 7.x-dev

I've seen this confusion popup many times. I agree it should be fixed. It must be fixed for 7. Not sure we can touch this for 6.

pbuyle’s picture

This issue is one year old. This is understandable since the workaround is to use the array key as template file name. A simple fix would be to document this in the hook_theme documentation.

apaderno’s picture

It doesn't make sense to allow to define a template name, if it's accepted only when it is equal (or almost) to the array key used to define the theme function.
The array key template could be changed to use a boolean value: when the value is TRUE, the theme uses a template file (and template name is the same as the theme name); when it is FALSE, the theme doesn't use a template file.

What I reported is valid for Drupal 6; I don't know if the same applies for Drupal 7.

apaderno’s picture

This actually creates a problem when you want to change an existing theme that uses a template.

If you would want to change how nodes are themed (I am sorry, this is the only theme with a template file I could find in 2 minutes; I am not suggesting that it would be a good idea to do so), you could change the template file used, because this must be named node.tpl.php.
It is still possible to only change where the file is searched, thought.

effulgentsia’s picture

Issue tags: +DrupalWTF

I marked #519698: Document the pitfalls of deviating from naming convention for hook_theme()'s 'function' and 'template' keys a duplicate.

In principle, we could fix drupal_find_theme_functions() and drupal_find_theme_templates() to find overrides of the actual 'function' and 'template' values rather than of the hook name. But:

1) Would this be an API change that is no longer in-scope for D7?
2) What if 2 theme hooks have the same value for 'template' (only differing in module path)? Should a discovered template file in the theme folder override both?

Alternatively, we could document hook_theme() to just inform people that if the module deviates from naming convention in setting 'template', then that's a "local" decision only, and the override template in the theme folder needs to be named based on the theme hook, not on the template name used by the module.

effulgentsia’s picture

Title: template naming can break theme overrides » Overriding template files that aren't named the same as the theme hook name is confusing

Changing issue title since overrides aren't broken, just made confusing to the theme developer, because the expectation is to override using the same template name as what the module uses, but what's currently needed is for the theme to use the template name that matches the theme hook name.

ralf.strobel’s picture

Version: 7.x-dev » 7.4

I just wanted to say that this still isn't fixed in Drupal 7.4 and it just took me two hours to figure this behavior out myself.

Can't you at least update the documentation then?

Deciphered’s picture

Title: Overriding template files that aren't named the same as the theme hook name is confusing » Override template files based on template filename as well as hook name
Version: 7.4 » 8.x-dev
Assigned: Unassigned » Deciphered
Status: Active » Needs review
FileSize
2.47 KB

Got bitten by this the other day, and based on my common naming structure of template files this one would be common with my modules.

Patch attached for Drupal 8, also applies to Drupal 7.4, adds the ability to override template files based on the hook_theme() template name as well as the existing ability to override based on the hook name and patterns.

 

Testing instructions

  1. Add a hook_theme() entry to a new or existing module using a 'template' named differently to the key of the hook_theme() item:
    function test_theme() {
      return array(
        'test' => array(
          'template' => 'test2',
        ),
      );
    }
    
  2. Create a template file named using the 'template' value in the module and in the current theme, each with different content:
    e.g. sites/all/modules/test/test2.tpl.php and sites/all/themes/custom_theme/templates/test2.tpl.php
  3. Invoke the theme in whatever fashion you wish (quick and dirty via hook_init(), or however you wish).

Without the patch the modules version of the tpl.php will be used, whereas with the patch the themes version of the tpl.php will be used.

 

Testing and reviews appreciated.

Cheers,
Deciphered.

Status: Needs review » Needs work

The last submitted patch, template_overides_by_template_name-342350-10.patch, failed testing.

Deciphered’s picture

Round #2, as I was unable to reproduce that issue with my current development environment I've made what I believe to be the correct fix, but blind. So here's hoping.

Instructions from #10 still relevant.

Deciphered’s picture

Status: Needs work » Needs review
FileSize
2.5 KB

'Needs Review'

Status: Needs review » Needs work

The last submitted patch, template_overides_by_template_name-342350-12.patch, failed testing.

Deciphered’s picture

Status: Needs work » Needs review
FileSize
1.7 KB

Ok, this time for sure. Issue identified and logic relocated to fix correctly.

skwashd’s picture

Status: Needs review » Needs work

Overall I think this looks good and it kills a WTF. One quick personal style suggestion:

+++ b/includes/theme.incundefined
@@ -1214,6 +1214,17 @@ function drupal_find_theme_templates($cache, $extension, $path) {
+      if (isset($info['template']) && in_array($template, array($info['template'], str_replace($info['theme path'] . '/', '', $info['template'])))) {

I think it would be cleaner and clearer if the array used in the in_array call was defined on the line above.

I'm not going to knock it back on this, but if it is changed I'll RTBC it.

-12 days to next Drupal core point release.

Deciphered’s picture

Status: Needs work » Needs review
FileSize
2.14 KB

Thanks Dave,

Change made.

Used the advanced patch workflow this time, so hoping that doesn't mess with the testbot.... but knowing my luck...
Here's hoping.

Cheers,
Deciphered.

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

The patch in #17 looks good to me; thanks for your work.

I think we need an automated test for this -- one that fails without the patch in #17, and passes with the patch applied. There is already a test_theme available in core, so maybe we can extend that.

xjm’s picture

Issue tags: +Needs backport to D7

Also, this is probably backportable.

Deciphered’s picture

Status: Needs work » Needs review
FileSize
5.11 KB

Added tests.

As an aside, I did want to test the Theme Registry directly, but was unable to get the Theme Registry values for the assigned default theme, it kept returning the registry for the 'Seven' theme.

Deciphered’s picture

By request of xjm, attached patch contains only the test, not the fix itself, to demonstrate the failed test prior to the fix being in place.

Refer to this only for that case, the patch to be review/used is that at #20.

Status: Needs review » Needs work

The last submitted patch, test_fail_demo-342350-21.patch, failed testing.

xjm’s picture

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

The test is nice and simple, and fails as expected without the patch. I think this is RTBC for #20. :)

catch’s picture

Status: Reviewed & tested by the community » Needs work
+    // Match templates by designated template file name instead of restricting
+    // to templates named after the theme funciton hook alone.

Typo in the comment.

Also "instead of restricting" sounds like it's describing the fact that this bug is fixed - not what actually happens here.

chx’s picture

Status: Needs work » Reviewed & tested by the community

I think so too.

chx’s picture

Status: Reviewed & tested by the community » Needs work

then maybe not.

Deciphered’s picture

Status: Needs work » Needs review
FileSize
4.24 KB

Doco and patch style changed.

xjm’s picture

Status: Needs review » Reviewed & tested by the community
Jacine’s picture

I thought this actually made sense for consistency. Not sure why you'd want to name a template differently from a hook, other than to implement a theme hook suggestion.

Anyway, with this patch, which "name" would you use for the preprocess function when they are different? The template file or theme hook?

Deciphered’s picture

Jacine,

It doesn't change the preprocess functionality, which makes sense to be the hook name as it is preprocessing the theme('') call in code.

There are many reasons to have a different naming convention for the template than the hook, I personally use the naming convention of: [module].[functionality].tpl.php

However, given that that functionality is already available, it shouldn't need to be justified here. This patch is needed so when someone exercises their right to name a template file differently to the hook name that it doesn't inconvenience users who should be able to just copy that file and modify the contents as an override.

Cheers,
Deciphered.

Jacine’s picture

@Deciphered I didn't change the status and I'm not gonna try and hold this up. My question was perfectly valid, as I thought the current implementation was done that way on purpose. The theme system is complicated enough IMO, and naming them differently is just yet another inconsistency. Finding the files properly seems like a completely separate issue. As a themer, I'll try to run preprocess on your template name, assuming it's the hook because that's how it's done everywhere else and scream WTF for a bit before I have to look at your hook_theme() only to find that it's different, at which point I will curse your module.

Anyway, that's just my 2 cents, you guys do what you want. ;)

xjm’s picture

@Jacine -- Maybe a followup issue is in order? (Or maybe there is one already?) I do think this issue is a bug, but the point about TX is a relevant question.

Edit: themer experience, not Texas.

catch’s picture

I've experienced the bug before but was similarly annoyed with whoever was naming their templates funny names. Seems definitely worth a followup to see whether the feature can be justified but while it's there we should fix it.

Jacine’s picture

Sure, a follow up is fine. I can (and will) still curse any module that actually uses a different hook for their template in the meantime. :)

catch’s picture

Could we open the followup before this is committed?

Jacine’s picture

catch’s picture

Version: 8.x-dev » 7.x-dev

OK committed/pushed to 8.x, moving to 7.x

webchick’s picture

Status: Reviewed & tested by the community » Needs review
-    if (($pos = strpos($template, '.')) !== FALSE) {
+    if (($pos = strpos($template, '.tpl')) !== FALSE) {

Won't this break alternative theme engines, such as Smarty and PHPTal?

catch’s picture

Hmm good question. drupal_find_theme_templates() is only called by phptemplate_theme(), so if they're just copying what phptemplate does then it might, but if they do their own thing it won't.

I'll leave this in 8.x for now, but if it's won't fix for 7.x for this reason, then I'd rather pursue #1321670: Don't allow theme hook and base template file name to be different and just roll this back.

Deciphered’s picture

Will investigate shortly. I've never used Smarty or PHPTal, but I certainly don't want to prevent anyone else from doing so.

P.S., that particular section was essential in the case that someone used a fullstop in the template name, but I would assume there's a better approach and will pursue that approach.

Deciphered’s picture

@webhick/catch,

I've just looked through all available Theme engines on D.o, and there is only one that has a 7.x release, which is a dev release at that: ETS - Easy Template System.

So, I'm not sure how I would be able to test this issue with another Theme engine, and as Catch says, the function is only called by phptemplate.engine anyway.

I'm more than happy to make sure it works for other Theme engines, I just don't know where to start as I've never used another Theme engine, nor do I know of any Drupal developer/themer that has.

Cheers,
Deciphered.

dynamicdan’s picture

Priority: Normal » Major

I just ran into this bug and it made me cry.

function esp_theme($existing, $type, $theme, $path) {

  $items['toolbar'] = array(
    'render element' => 'toolbar',
    // some bug... template file must match hook name
    'template' => 'esp',
    'path' => drupal_get_path('theme', 'esp'),
  );

  return $items;
}

The bit that says 'some bug' is actually some bug. Please fix. I was losing life points trying to workout why my template file wasn't being found.
Upped to major because I'm down to 50 life points.

Deciphered’s picture

I'm still happy to do more on this patch if need be, but I have not received any word from the higher ups for a while, it has been committed to Drupal 8, but it needs Webchick to sign off on it for Drupal 7.

dynamicdan’s picture

Go forth I say and Drupal!

@Webchick, wherefore art thou input? If it works in D8 (regarding template systems/engines) won't it also work in D7?

effulgentsia’s picture

Re #42, are you putting esp_theme() into the template.php file of a theme named 'esp'? If so, the code you have should work. I just tried it locally in D7, and it worked fine for me (after clearing cache, of course).

What this issue is about is whether when you do this, if subthemes of 'esp' should also automatically have their esp.tpl.php be recognized as a toolbar template. Or if a module's hook_theme() does this, whether all themes should inherit that override.

I'm against this being backported to D7. It changes the meaning of the 'template' key from "this is the template my module/theme uses for implementing theme hook X" to something that affects what happens in other themes: a side effect we should not be forcing onto production websites. Also, #7.2 is still not addressed; well it is in that the code in #27 answers yes, but I don't think that's desirable.

webchick’s picture

Just two quick notes:

1) I am no longer the release manager for Drupal 7, so David Rothstein is who'd actually have to sign off on this.
2) Probably the worst way to get ahold of a core maintainer is via a comment in a random issue in the issue queue. :) Work through the community process to get it to RTBC, then it gets escalated to committer attention.

It sounds like there are still some concerns, per #45 though.

webchick’s picture

Priority: Major » Normal

And since there's a workaround in the OP, I don't think this is major.

effulgentsia’s picture

Assigned: Deciphered » David_Rothstein
Status: Needs review » Patch (to be ported)
Issue tags: +Needs committer feedback

Per #43, it seems reasonable to ask David for whether this is a "won't fix" before spending time working on it.

dynamicdan’s picture

Re #42, are you putting esp_theme() into the template.php file of a theme named 'esp'? If so, the code you have should work. I just tried it locally in D7, and it worked fine for me (after clearing cache, of course).

Yes. Code works but I shouldn't have to change the 'template' name in my opinion. I would have preferred to name my file toolbar.tpl.php in my themes/esp/templates/ (or similar) folder.

In regards to subthemes, shouldn't the 'subbiest' theme take priority anyway? Normal inheritance concept? My esp theme is a subtheme of zen btw. I would love to override the zen toolbar.tpl.php in any case/scenario if I have a file called toolbar.tpl.php in my theme directory.

David_Rothstein’s picture

Assigned: David_Rothstein » Unassigned
Status: Patch (to be ported) » Needs work

I looked over this issue and it sounds to me like the biggest risk of this patch would be the possibility of introducing an accidental name collision? (I.e., a module uses the 'template' key which so happens to conflict with another tpl.php file in the theme's directory.) If so, that sounds risky, but I wouldn't be totally opposed to committing it to Drupal 7 if there's consensus that this needs to change. However, right now there isn't consensus, and I think for an issue like this the consensus needs to include the theme system maintainers who are active in the issue (e.g., @effulgentsia), so right now I'd suggest trying to build consensus before writing more code :)

The other thing to point out is that in general we should be looking for low-impact fixes to Drupal 7 when possible (at this point in its life cycle), and the risk-vs-reward here doesn't seem very good. If we fix it this way, it allows a bit more flexibility for people who want to name their files differently (which seems like a pretty minor reward), but if we fix it by improving the documentation instead that's clearly simpler. Also, it's not really clear to me that this is even a functional bug... If you look at the hook_theme() docs for this parameter:

template: If specified, this theme implementation is a template, and this is the template file without an extension. Do not put .tpl.php on this file; that extension will be added automatically by the default rendering engine (which is PHPTemplate). If 'path', above, is specified, the template should also be in this path.

That last sentence sort of indirectly implies that this was always intended to be "local-only" (since the 'path', by definition, points to the implementing module). Obviously, of course, the documentation could be a lot more explicit about this.

David_Rothstein’s picture

Oh, also, I was trying out this patch to understand what it does, and I couldn't get it to work. I might have been doing it wrong, but I don't think I was. Rather, the site I was testing this on happened to use a sub-theme, and it seems to me like the patch doesn't work at all in this case (or more specifically, it only works for a "child", but not anything below that; i.e., if a module sets 'template' to an alternate filename, then a base theme can use that alternate filename and have it recognized, but a subtheme of that base theme, which is two levels removed from the original implementation, cannot).

I did a little debugging and it seems like that might be due to the fact that _theme_process_registry() does this:

      if (isset($info['template'])) {
        // Prepend the current theming path when none is set.
        if (!isset($info['path'])) {
          $result[$hook]['template'] = $path . '/' . $info['template'];
        }
      }

But then the patch makes drupal_find_theme_templates() do this:

  $template_candidates = array($info['template'], str_replace($info['theme path'] . '/', '', $info['template']));

So by the time you get to the subtheme, $info['template'] will be something like sites/all/modules/my_module/mytemplate.tpl.php (based on the original template location), but $info['theme path'] which it's trying to strip off will instead be something like sites/all/themes/my_base_theme, so it won't work.

If that's true, this patch needs work for that reason also; sounds like above there were a few other things that needed work too, but I'm not sure if that means this issue should be bounced back to Drupal 8 or remain at Drupal 7 for the time being.

David_Rothstein’s picture

@dynamicdan:

In regards to subthemes, shouldn't the 'subbiest' theme take priority anyway? Normal inheritance concept? My esp theme is a subtheme of zen btw. I would love to override the zen toolbar.tpl.php in any case/scenario if I have a file called toolbar.tpl.php in my theme directory.

It sounds like you're experiencing a different issue; as far as I know, that should already work fine. You don't need a hook_theme() at all, just put toolbar.tpl.php in your theme directory and clear caches, and it should take effect (provided the toolbar is actually being rendered by your theme rather than a separate admin theme, of course).

This issue is instead about situations where a module or theme doesn't want to use the default template name pattern, but rather use an alternate name.

dynamicdan’s picture

@David_Rothstein

This issue is instead about situations where a module or theme doesn't want to use the default template name pattern, but rather use an alternate name.

Actually toolbar.tpl.php didn't work. It was ignored by the toolbar module. I had to make sure that my 'template' attribute was the same as my theme name and therefore name the file esp.tpl.php (where esp is my theme name using the hook_theme()).

David_Rothstein’s picture

If you just add the toolbar.tpl.php file and stop there, it should all work fine (or at least it did when I tried it). It shouldn't be necessary to have any code for this in hook_theme() at all.

It's definitely possible that there's a bug where if you do have an entry in your hook_theme() for this, that causes things not to work, but again, it sounds like a separate issue.

bbinkovitz’s picture

  • catch committed d0fb1ad on 8.3.x
    Issue #342350 by Deciphered: Fixed Override template files based on...

  • catch committed d0fb1ad on 8.3.x
    Issue #342350 by Deciphered: Fixed Override template files based on...
xjm’s picture

Issue summary: View changes
Issue tags: -Needs committer feedback

Feedback was given by @David_Rothstein in #50 on.

  • catch committed d0fb1ad on 8.4.x
    Issue #342350 by Deciphered: Fixed Override template files based on...

  • catch committed d0fb1ad on 8.4.x
    Issue #342350 by Deciphered: Fixed Override template files based on...

  • catch committed d0fb1ad on 9.1.x
    Issue #342350 by Deciphered: Fixed Override template files based on...