Posted by ryan_courtnage on December 3, 2008 at 10:01pm
18 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | theme system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Deciphered |
| Status: | needs review |
| Issue tags: | DrupalWTF, needs backport to D7 |
Issue Summary
With the following module code, I'm able to serve up my-xyz-index.tpl.php from my module directory:
<?php
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:
<?php
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
#1
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.
#4
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.
#5
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
templatecould 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.
#6
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.
#7
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.
#8
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.
#9
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?
#10
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
function test_theme() {return array(
'test' => array(
'template' => 'test2',
),
);
}
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.
#11
The last submitted patch, template_overides_by_template_name-342350-10.patch, failed testing.
#12
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.
#13
'Needs Review'
#14
The last submitted patch, template_overides_by_template_name-342350-12.patch, failed testing.
#15
Ok, this time for sure. Issue identified and logic relocated to fix correctly.
#16
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.
#17
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.
#18
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_themeavailable in core, so maybe we can extend that.#19
Also, this is probably backportable.
#20
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.
#21
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.
#22
The last submitted patch, test_fail_demo-342350-21.patch, failed testing.
#23
The test is nice and simple, and fails as expected without the patch. I think this is RTBC for #20. :)
#24
+ // 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.
#25
I think so too.
#26
then maybe not.
#27
Doco and patch style changed.
#28
#29
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?
#30
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.
#31
@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. ;)
#32
@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.
#33
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.
#34
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. :)
#35
Could we open the followup before this is committed?
#36
Here you go: #1321670: Don't allow theme hook and base template file name to be different
#37
OK committed/pushed to 8.x, moving to 7.x
#38
- if (($pos = strpos($template, '.')) !== FALSE) {+ if (($pos = strpos($template, '.tpl')) !== FALSE) {
Won't this break alternative theme engines, such as Smarty and PHPTal?
#39
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.
#40
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.
#41
@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.