So, I'm not entirely sure I know how to express this bug so here's the example using views 2.

When you have a parent theme has a general template file like views-view-field.tpl.php and a child theme has a more specific template such as views-view-field--viewname--fieldname.tpl.php, the more specific template will not be detected.

Attached is a patch suggested by merlinofchaos as a solution.

CommentFileSizeAuthor
#20 pattern_match_test.zip23.94 KBneclimdul
#18 parent_theme_pattern_clobbering-e-HEAD.patch1.25 KBdvessel
#18 parent_theme_pattern_clobbering-e-D6.patch1.26 KBdvessel
#16 parent_theme_pattern_clobbering-d-HEAD.patch1.52 KBdvessel
#16 parent_theme_pattern_clobbering-d-D6.patch1.53 KBdvessel
#14 parent_theme_pattern_clobbering-c-D6.patch1.54 KBdvessel
#14 parent_theme_pattern_clobbering-c-HEAD.patch1.53 KBdvessel
#13 D6_parent_theme_pattern_clobbering.c.patch1.54 KBdvessel
#13 HEAD_parent_theme_pattern_clobbering.c.patch1.53 KBdvessel
#12 parent_theme_pattern_clobbering.c-head.patch1.75 KBdvessel
#12 parent_theme_pattern_clobbering.c-6.patch1.83 KBdvessel
#5 parent_theme_pattern_clobbering.b-head.patch861 bytesdvessel
#2 parent_theme_pattern_clobbering.b.patch876 bytesdvessel
parent_theme_pattern_clobbering.patch767 bytesneclimdul

Comments

dvessel’s picture

dvessel commented

Will dig into this soon...

dvessel’s picture

dvessel commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new parent_theme_pattern_clobbering.b.patch876 bytes

The patch works perfectly. Tested up to 3 level of subthemes. Tweaked the docs and fixes offset..

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Version: 6.x-dev » 7.x-dev

Please get this into 7 first.

damien tournoud’s picture

damien tournoud commented
Status: Reviewed & tested by the community » Needs work

What's the link with #279573: Themes can't use node-story.tpl.php without node.tpl.php? Are those duplicate?

Putting as code needs work, as this will need a test case before getting in 7.

dvessel’s picture

dvessel commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new parent_theme_pattern_clobbering.b-head.patch861 bytes

Damien, the template suggestions and this issue is unrelated. That issue depends on theming paths which depends on the base template.

merlinofchaos’s picture

merlinofchaos commented

FYI I helped neclimdul with this original patch, so if it matters I'm very much in favor of it.

In order to describe what happens, we'll still use Views as an example.

We have the theme 'views_view', and through the pattern system, 'views_view__foo' is called. During ordinary operations, views-view--foo.tpl.php will be found and utilized.

However, there is an obscure situation in which a base theme having 'views-view.tpl.php' will register that template and inadvertently destroy the 'pattern'. When the child theme goes to register 'views_view' as a theme, it won't see the pattern, and this it won't search for 'views-view--foo.tpl.php' through the normal mechanism. Thus the theme file is not found.

This patch corrects it; it should be applied to both 7.x and 6.x.

merlinofchaos’s picture

merlinofchaos commented

dvessel: I believe that there needs to be two of these. One in drupal_find_theme_templates and one in its sister function to find theme functions. THey should both do essentially the same thing.

dvessel’s picture

dvessel commented

merlinofchaos, I thought it needed the fix there too and I did test it. I really tried breaking it but works without any patching.

merlinofchaos’s picture

merlinofchaos commented

Do we have any theme functions that work the samew ay though? Having theme_views_view() in a base theme and theme_views_view__foo() in a child theme? I think it should run into the same limitation.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs review

Sounds like this still needs some review.

(also, ninja subscribe ;))

dvessel’s picture

dvessel commented

I thought I tested it throughly but apparently it can slip through when naming the function after the theme engine. Bah!!

dvessel’s picture

dvessel commented
StatusFileSize
new parent_theme_pattern_clobbering.c-6.patch1.83 KB
new parent_theme_pattern_clobbering.c-head.patch1.75 KB

And here's a patch. I also modified the comments. Makes more sense I think.

dvessel’s picture

dvessel commented
StatusFileSize
new HEAD_parent_theme_pattern_clobbering.c.patch1.53 KB
new D6_parent_theme_pattern_clobbering.c.patch1.54 KB

And here's a patch. I also modified the comments. Makes more sense I think.

bleh. Come on testbot.

dvessel’s picture

dvessel commented
StatusFileSize
new parent_theme_pattern_clobbering-c-HEAD.patch1.53 KB
new parent_theme_pattern_clobbering-c-D6.patch1.54 KB

And here's a patch. I also modified the comments. Makes more sense I think.

merlinofchaos’s picture

merlinofchaos commented

Ok, that's more like what I thought. Does the $info['pattern'] in the first part of the patch generate notices? (I'm not sure if it's possible for $info['pattern'] to not be set at that point. If it's registering a new hook that it found from a pattern, $info['pattern'] is always set but I don't think the newly created pattern should actually inherit; that could cause multiple registrations of the same pattern?

dvessel’s picture

dvessel commented
StatusFileSize
new parent_theme_pattern_clobbering-d-D6.patch1.53 KB
new parent_theme_pattern_clobbering-d-HEAD.patch1.52 KB

Definitely no notices since that first part is within a "if (!empty($info['pattern'])) {".

http://api.drupal.org/api/function/drupal_find_theme_functions/6

I placed that there since a function named after the pattern could be setup in a base theme in addition to a sub-theme. Without it, only the base theme would catch onto the pattern while the sub-theme would not.

Updated patch fixes a stray word in the comments.

dvessel’s picture

dvessel commented

Merlinofchaos, oh.. I misunderstood. Never mind what I just said. I think your right about the first part and it should be removed.

dvessel’s picture

dvessel commented
StatusFileSize
new parent_theme_pattern_clobbering-e-D6.patch1.26 KB
new parent_theme_pattern_clobbering-e-HEAD.patch1.25 KB

Merlinofchaos, I looked through the registry and that first $info['pattern'] did cause newly registered patterned theming hooks to have the same pattern attached. It didn't look harmful since a pattern of "foo__" would register "foo__bar" but "foo__bar" I don't think can further register more patterns on top of that. But I guess it's better to be safe and just remove it.

Everything else looks good to me.

This isn't directly related but I made a few notes to help me grok how patterns fit into the way overrides work.

sub-theme > base theme

hook pattern > base hook
foo--bar.tpl.php vs. foo.tpl.php

function > template
themeName_foo() vs. foo.tpl.php

-----

sub-theme + base hook < base theme + hook pattern

sub-theme + base hook + function < base theme + pattern hook + template

-----

So, patterns have the most weight followed by sub-themes and then theme functions as opposed to templates for a given hook.

merlinofchaos’s picture

merlinofchaos commented
Status: Needs review » Reviewed & tested by the community

Ironically I think we've come full circle back to my original patch that I gave to neclimdul.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new pattern_match_test.zip23.94 KB

Tested on 7. Confirmed to work.

From a discussion with merlin, I created a module with:

/**
 * Implementation of hook_theme().
 */
function test_theme() {
  return array(
    'foo' => array(
      'pattern' => 'foo__',
      'arguments' => array(),
    ),
  );
}

Then create 2 themes with theme functions parent_foo() { print "parent"; } and subtheme_foo__a() { print "subtheme"; }

Before the patch, it prints "parent" and after if prints "subtheme" as is the expected behavior. But, it didn't work for me with templates. Attached is the themes and modules I used to test it in case I made a mistake.

dvessel’s picture

dvessel commented
Status: Needs work » Reviewed & tested by the community

neclimdul, it's because the template must use hyphens. Your test files were using underscores.

neclimdul’s picture

neclimdul
He/Him
commented

dope. lets do this thing then!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Version: 7.x-dev » 6.x-dev

http://testing.drupal.org/pifr/file/1/parent_theme_pattern_clobbering-e-... gives the patch in #18 a clean pass. We don't (yet) have a way to test theme logic in SimpleTest, but neclimdul's .zip in #20 looks like a really awesome start for when we do.

Committed to HEAD. Thanks! Marking back to 6.x.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

SimpleTest gurus, see #343898: Let SimpleTest test the theme system.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Committed to 6.x, thanks.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Committed to 6.x, thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

momper’s picture

momper commented

is this fixed in core now? 6.9?