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.
Comments
Comment #1
dvessel CreditAttribution: dvessel commentedI'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.
Comment #4
pbuyle CreditAttribution: pbuyle commentedThis 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.
Comment #5
apadernoIt 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 isTRUE
, the theme uses a template file (and template name is the same as the theme name); when it isFALSE
, 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.
Comment #6
apadernoThis 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.
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #8
effulgentsia CreditAttribution: effulgentsia commentedChanging 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.
Comment #9
ralf.strobel CreditAttribution: ralf.strobel commentedI 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?
Comment #10
Deciphered CreditAttribution: Deciphered commentedGot 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
e.g. sites/all/modules/test/test2.tpl.php and sites/all/themes/custom_theme/templates/test2.tpl.php
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.
Comment #12
Deciphered CreditAttribution: Deciphered commentedRound #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.
Comment #13
Deciphered CreditAttribution: Deciphered commented'Needs Review'
Comment #15
Deciphered CreditAttribution: Deciphered commentedOk, this time for sure. Issue identified and logic relocated to fix correctly.
Comment #16
skwashd CreditAttribution: skwashd commentedOverall I think this looks good and it kills a WTF. One quick personal style suggestion:
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.
Comment #17
Deciphered CreditAttribution: Deciphered commentedThanks 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.
Comment #18
xjmThe 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.Comment #19
xjmAlso, this is probably backportable.
Comment #20
Deciphered CreditAttribution: Deciphered commentedAdded 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.
Comment #21
Deciphered CreditAttribution: Deciphered commentedBy 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.
Comment #23
xjmThe test is nice and simple, and fails as expected without the patch. I think this is RTBC for #20. :)
Comment #24
catchTypo in the comment.
Also "instead of restricting" sounds like it's describing the fact that this bug is fixed - not what actually happens here.
Comment #25
chx CreditAttribution: chx commentedI think so too.
Comment #26
chx CreditAttribution: chx commentedthen maybe not.
Comment #27
Deciphered CreditAttribution: Deciphered commentedDoco and patch style changed.
Comment #28
xjmComment #29
JacineI 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?
Comment #30
Deciphered CreditAttribution: Deciphered commentedJacine,
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.
Comment #31
Jacine@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. ;)
Comment #32
xjm@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.
Comment #33
catchI'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.
Comment #34
JacineSure, 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. :)
Comment #35
catchCould we open the followup before this is committed?
Comment #36
JacineHere you go: #1321670: Don't allow theme hook and base template file name to be different
Comment #37
catchOK committed/pushed to 8.x, moving to 7.x
Comment #38
webchickWon't this break alternative theme engines, such as Smarty and PHPTal?
Comment #39
catchHmm 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.
Comment #40
Deciphered CreditAttribution: Deciphered commentedWill 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.
Comment #41
Deciphered CreditAttribution: Deciphered commented@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.
Comment #42
dynamicdan CreditAttribution: dynamicdan commentedI just ran into this bug and it made me cry.
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.
Comment #43
Deciphered CreditAttribution: Deciphered commentedI'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.
Comment #44
dynamicdan CreditAttribution: dynamicdan commentedGo 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?
Comment #45
effulgentsia CreditAttribution: effulgentsia commentedRe #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.
Comment #46
webchickJust 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.
Comment #47
webchickAnd since there's a workaround in the OP, I don't think this is major.
Comment #48
effulgentsia CreditAttribution: effulgentsia commentedPer #43, it seems reasonable to ask David for whether this is a "won't fix" before spending time working on it.
Comment #49
dynamicdan CreditAttribution: dynamicdan commentedYes. 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.
Comment #50
David_Rothstein CreditAttribution: David_Rothstein commentedI 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:
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.
Comment #51
David_Rothstein CreditAttribution: David_Rothstein commentedOh, 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:
But then the patch makes drupal_find_theme_templates() do this:
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 likesites/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.
Comment #52
David_Rothstein CreditAttribution: David_Rothstein commented@dynamicdan:
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.
Comment #53
dynamicdan CreditAttribution: dynamicdan commented@David_Rothstein
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()).
Comment #54
David_Rothstein CreditAttribution: David_Rothstein commentedIf 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.
Comment #55
bbinkovitz CreditAttribution: bbinkovitz commentedClosed #1610124: theme() not available in theme_preprocess_page() as a duplicate of this issue.
Comment #58
xjmFeedback was given by @David_Rothstein in #50 on.