Posted by djnz on October 19, 2007 at 1:09pm
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | theme system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
drupal_find_theme_templates does not work for files named e.g. template.tpl because it assumes something like template.extra.ext.
The attached patch sorts it out.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| theme.inc__2.patch | 1.13 KB | Ignored: Check issue status. | None | None |
Comments
#1
Coding style corrected
#2
Well, you took the existing
substr($template, 0, strpos($template, '.'));and wrapped it inside anif (strpos($template, '.') !== FALSE). How does this solve the problem? Who reviewed this?#3
It solves the problem perfectly. substr($template, 0, strpos($template, '.')); will return an empty string if there is no '.' in $template, so if (strpos($template, '.') !== FALSE) will prevent that code trying to strip a non-existant '.foo'
#4
Based on http://www.php.net/manual/en/function.strpos.php this patch is correct.
#5
Now why would a template not have an extension?
#6
#7
djnz's patch appears correct to me. Allow me to 'splain.
PHPTemplate uses a tiered extension of .tpl.php -- however, the method we use to list files chops off the .php extension. This means we're left with a filename of 'foo.tpl' which won't match anything. To match, we have to chop off that .tpl.
djnz's patch is fixing the case where there is no extra extension at all. In his case, it will be 'foo.smarty'.
I believe this is right.
#8
Addendum: We probably should not run strpos() multiple times, so we should use something like:
<?phpif (($pos == strpos(...)) !== FALSE) {
...
}
?>
Though I guess strpos is probably fast enough, given our filenames won't be more than 32 characters long, likely, so maybe that optimization doesn't matter.
#9
Thanks for the clarification merlinofchaos.
#10
Thanks for the clarification. Now I understand and see that the documentation definitely needs improvement here. This is how it looks like, and how it confuses me.
+ // Chop off the extension if there is one. We do it this way because+ // $template will have one extension chopped off, but there might be more
+ // than one, such as with .tpl.php
+ if (strpos($template, '.') !== FALSE) {
+ $template = substr($template, 0, strpos($template, '.'));
+ }
It says $template will have one extension chopped off, but it actually means it is already chopped off at this stage. So this is what confused us. What about?
+ // Chop off the remaining extensions if there are any. $template already+ // has the rightmost extension removed, but there might still be more,
+ // such as with .tpl.php, which still has .tpl in $template at this point.
+ if (($pos = strpos($template, '.')) !== FALSE) {
+ $template = substr($template, 0, $pos);
+ }
Also included merlinofchaos' suggestion here (not tested). I think this comment covers what we discussed here way better. Opinions?
#11
Is it important to know that one has been removed already? How about simply
<?php+ // Remove any remaining file extensions.
+ if (($pos = strpos($template, '.')) !== FALSE) {
+ $template = substr($template, 0, $pos);
+ }
?>
#12
Hmm. Technically the 'will have' is correct English, but 'have' is one of those words with too many meanings. The funny thing is, every time I try to rewrite it in my head I turn back to that phrase. May be a personal overuse.
Earnie: I think it's good to note that the last extension is chopped off already. Gabor's version of the comment is good by me and makes plenty of sense.
#13
Honestly my problem with 'will have' is that I don't know whether it 'will have' before the code block is run (ie. it is not yet in that form) is 'will have' before the code block is run (ie. it is already in that format). Technically when one reads the code, the code 'will' run, so this might look like ok, but I think is confusing. At least for a non-native English speaker like me.
#14
It is through this process that we iterate towards perfection...
Amended patch attached, with Gábor's amended comment and merlin's temporary variable efficiency improvement.
#15
looks good. tink this is ready to go.
#16
Thanks guys, committed.
#17
Automatically closed -- issue fixed for two weeks with no activity.