Many times, sub-themes just need to tweak a variable in a tpl file and they don’t need to modify the tpl file's contents.

For example, if a sub-theme wants to modify the $submitted text, it doesn't need to modify the html/php in the base theme’s node.tpl.php.

Unfortunately, the auto-discovery phase of the theme registry building process first looks for tpl files in a theme and then for the corresponding preprocess functions. So if there isn't a tpl file, the theme registry building process won't look for a preprocess function.

No patch yet, but the way I envision this working is similar to the work-around that Zen 6.x-1.x has (#249532: Allow subthemes to have preprocess hooks without tpl files.). Grab the existing theme registry and loop through the known hooks (those that have tpl files), while looking for possible preprocess functions in the theme.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Hm, this might qualify as a D6 bug indeed.

dvessel’s picture

This limitation is documented. A fix would be nice though.

Read the middle of this page in the list of notes:
http://drupal.org/node/223430

JohnAlbin’s picture

Since the theme guide is amazingly accurate and based on the actual implementation (warts and all) and not based on the design, all the documented "limitations" fall into the "maybe this a bug" category. :-D

Let's see what the patch looks like and then decide whether this should be backported to D6.

merlinofchaos’s picture

I am in favor of fixing this in D6 if the fix can be done without breaking the existing API, because it is a limitation that was not intended and causes quite a bit of acrobatics to get around.

cburschka’s picture

Please, please, please put this in. In D7 certainly, and it would be nice in D6 as well, so we don't need to wait a year for the awesomeness.

#277063: The next generation of DHTML Menu

cburschka’s picture

Status: Active » Needs review

Here is a patch for D6.

I'd submit the D7 patch first, but I've tested this one.

dixon_’s picture

Status: Needs review » Active

@Arancaytar: The patch is missing... ;)

cburschka’s picture

Status: Active » Needs review
FileSize
1.3 KB

And in this case I can't even blame d.o. While rolling it, I got caught up in something and actually forgot. :P

cburschka’s picture

The patch is for Drupal 6, but succeeds on HEAD with offset. Still testing if it works too.

cburschka’s picture

I've ported my new DHTML menu module to D7 for testing purposes, and it works. Unfortunately my server is incapable of running simpletest reliably.

cburschka’s picture

I still don't know whether this patch will be allowed into Drupal 6 or will have to wait for Drupal 7...

chx’s picture

Project: Drupal core » Documentation
Version: 7.x-dev »
Component: theme system » Customization and Theming Guide
Status: Needs review » Active

This is a documentation issue not a code one. Functions should have no preprocess for performance concerns. http://www.lullabot.com/articles/overriding-theme-functions-in-modules for a way .

merlinofchaos’s picture

Project: Documentation » Drupal core
Version: » 7.x-dev
Component: Customization and Theming Guide » theme system
Status: Active » Needs review

This has nothing to do with theme functions, it has to do with theme templates, chx.

chx’s picture

@@ -596,7 +596,27 @@ function theme() {
   }
   if (isset($info['function'])) {
     // The theme call is a function.
-    $output = call_user_func_array($info['function'], $args);

Huh what? This has nothing to with theme functions???? I am confused.

merlinofchaos’s picture

Status: Needs review » Needs work

chx: Arancaytar's patch is wrong, but that's no reason to bury the whole issue. The actual issue is about themes having preprocess functions but using an ancestor's template.

Arancaytar: Your patch is wrong. It shouldn't be intercepting theme functions. chx is right on that part; theme functions need to have the speediest route possible.

merlinofchaos’s picture

IMO the proper way to fix this bug is have an additional phase in phptemplate_theme() that searches for preprocess functions that match items in the $existing array that are not in the array that it's returning.

eMPee584’s picture

Wow this ridiculously sucks *g
I was just trying to override preprocess_forum_icon without having to modify the (incredibly complex ;) template file because my theme did not yet need any and i wouldn't want THAT one to be the first.. Well now indeed it is!
after putting debug statements into the theme.inc and noticing it looks for _preprocess hooks for EVERY enabled theme (of course it does) but not for themename_preprocess_$hook i can hardly see how not doing it for the theme aswell is a performance issue. The patch imho would be to simply add $prefixes[] = $theme; in the if ($type == 'module') block.. this would of course require to have the correspondent _theme_process_registry call actually provide the $theme as 4th parameter (this itself probably qualifies as bug, invoking $module->name as $theme parameter is clearly bogus)..
anyways, at least in the documentation (#223430: Preprocess functions) putting it in as the last of several notes is as good as not documenting it at all.

Pedro Lozano’s picture

Subscribiing. Because I also had this problem when trying to make my first theme in 6.x. Intuitively I thought that I cound preprocess vars for a template in template.php without having to copy the template to the theme when I'm not modifiying it.

merlinofchaos’s picture

Version: 7.x-dev » 6.x-dev
Status: Needs work » Needs review
FileSize
1.36 KB

This is a patch to 6.x to fix this issue. I think this is an important bug and I am personally responsible for letting this sit far too long; I've known how to fix it but I was unable to adequately explain the process.

This patch adds an extra phase in registry processing and specifically looks for preprocess functions for themes that do not register template overrides. I've tested this on a 3 level subtheme and it works quite well.

merlinofchaos’s picture

How to test:

If you don't have an easy to access subtheme already set up, here is a simple one you can do:

sites/all/themes/marland/marland.info:

name = Marland
description = Site specific Garland
base theme = garland
core = 6.x
stylesheets[all][] = marland.css

sites/all/themes/marland/template.php:

function marland_preprocess_page(&$vars) {
  $vars['title'] = "If you see this, the test passed.";
}

If you like, copy themes/garland/logo.png to sites/all/themes/marland so you get a logo.

Then set your theme to Marland. The title of all pages should be modified with this test, even though you have no page.tpl.php.

If you make a similar test mod to an existing theme, be sure that:

1) You choose a preprocess that has no associated tpl.php (or theme function) for that theme and
2) You clear the cache.

quicksketch’s picture

Subscribing.

dvessel’s picture

Status: Needs review » Needs work

Naming the preprocessor after the sub-theme will still omit it. I tested with minnelli with "minnelli_preprocess_block" and it was ignored. Using "phptemplate_preprocess_block" did work though.

merlinofchaos’s picture

dvessel: That's weird, the test I gave uses a subtheme name, and it worked for me in several different attempts. Are you sure you cleared cache?

dvessel’s picture

Yeah, I did and just tested again with the example you provided. Same situation here.

I am using the latest Drupal 6 from CVS.

merlinofchaos’s picture

Status: Needs work » Needs review
FileSize
1.54 KB

Ok, the first patch required the presence of hook_theme in the template.php which I hadn't realized. Moving the clause outside of that function_exists() (and providing a default array to $result) fixes that.

dvessel’s picture

Status: Needs review » Needs work

Okay, this is fixed but there is another issue when the base theme uses the name of the theme, the preprocessor will register twice.

I tested with Garland by renaming "phptemplate_preprocess_page" to "garland_preprocess_page" resulted in this:

array
  'template' => string 'page' (length=4)
  'path' => string 'themes/garland' (length=14)
  'type' => string 'base_theme_engine' (length=17)
  'theme path' => string 'themes/garland' (length=14)
  'arguments' => 
    array
      'content' => null
      'show_blocks' => boolean true
      'show_messages' => boolean true
  'theme paths' => 
    array
      0 => string 'modules/system' (length=14)
      1 => string 'themes/garland' (length=14)
  'preprocess functions' => 
    array
      0 => string 'template_preprocess' (length=19)
      1 => string 'template_preprocess_page' (length=24)
      2 => string 'garland_preprocess_page' (length=23) // first run,
      3 => string 'garland_preprocess_page' (length=23) // repeat!! Could cause problems on second run!
      4 => string 'marland_preprocess_page' (length=23)
dvessel’s picture

Status: Needs work » Needs review
FileSize
1.41 KB

The only change here is the addition of an array_unique. Worked in my testing.

merlinofchaos’s picture

Hmm. Does garland get called for both base_theme and theme? How does that even get into the array twice?

I suppose array_unique is a good enough safety to catch any issues.

dvessel’s picture

Assigned: JohnAlbin » Unassigned

It looks like it's already added before running into your new block of code when looped in the "theme" phase. It's within $cache when it loops through the "theme_engine" or "base_theme_engine" phase.

I want to note that this applies to base themes too when they have no .tpl files, not just sub-themes from it's parent. This could get confusing when using path_to_theme() as it will always point to where the template is located. It's not as bad as splitting off of suggestions from its base template but I'm sure this will cause some head scratching. I'll update the docs when this gets in.

It would also be great if we could redo path_to_theme or simply rename it. themed_path() pointing to the template? And maybe path_to_theme() pointing to the theme. Dunno..

merlinofchaos’s picture

Redoing path_to_theme() is probably only doable in Drupal 7, but I agree that it should be done.

dvessel’s picture

Status: Needs review » Reviewed & tested by the community

Since you think array_unique is okay, marking RTBC. Everything worked while testing.

emmajane’s picture

+1 to finding a solution for this original issue. I haven't tested the patch but I agree with the logic of why it's needed. This is an issue that will affect themers who want to take advantage of base themes (which, quite frankly, will be everyone once they discover how AWESOME it is!). The assumption I propagate in the documentation I write is that subthemes inherit everything from their base (parent) theme and that you only need to add things to the subtheme if you're actually altering their output. Forcing people to copy an unaltered file into the sub-theme's directory is counter-intuitive (for lack of a better word).

webchick’s picture

Ok, this sounds like something we definitely need to fix, however:

- Merlin's patch doesn't apply for me.
- Dvessel's applies, but puts things in weird places. The $result = array(); appears in the middle of two function declarations, and the other stuff is all out-dented and weird.

Is it possible to re-roll this for 7.x?

webchick’s picture

Version: 6.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs work

And actually let's fix this in the proper order. :)

webchick’s picture

Version: 7.x-dev » 6.x-dev
Status: Needs work » Reviewed & tested by the community
FileSize
1.72 KB
1.72 KB

OK we figured out the conflict (silly concatenation rules).

I ran through the test in #20 (other than making 6.x into 7.x) and confirmed this patch fixes the bug.

Committed to D7. Moving back for consideration in D6. Here's the 6.x and 7.x version of the patch I committed.

webchick’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Hm. Actually the 6.x version needs to be ported for 6.x's concatenation rules come to think of it. Should be a quick re-roll.

JohnAlbin’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.71 KB

Re-rolled Angie’s patch with the proper fubar-6.x string concat rules.

keith.smith’s picture

Status: Needs review » Needs work

Minor, but there's a comment in there that also needs to end with a period.

webchick’s picture

Gah. How did I miss that? I am clearly slipping. :P

I guess we'll need to catch that in a monster "Add periods to all comments that don't have them" D7 patch.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
888 bytes
1.71 KB

Re-rolled 6.x with period.

And the 7.x patch is my entry for Trivial Patch of the Month!

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, moving to RTBC, will review even more later.

Gábor Hojtsy’s picture

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

Committed to 6.x, thanks. The final trivial patch against Drupal 7 still stands.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

LOL.

Committed to HEAD. :)

dvessel’s picture

As soon as 6.7 is released, the docs need to be updated. http://drupal.org/node/223430

I'm just making this note here to remind myself or anyone else if I forget.

Status: Fixed » Closed (fixed)

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

kramssov’s picture

With patched D6.8 phptemplate_preprocess_node still gets ignored for me if node.tpl.php is not within the theme directory.

JohnAlbin’s picture

@kramssov: Don’t use the phptemplate_ prefix for your preprocess functions (or theme functions). It causes all sorts of issues. Use mytheme_preprocess_node instead.

liquidcms’s picture

so, what's the outcome of all this? and more importantly.. is there a Views example anywhere?

I was expecting that if i wanted to override a var used in a view that i no longer need to create a .tpl file for this; but as far as i can tell this still doesn't work.

should this not work:

function myzensubtheme_preprocess_views_view_field__myviewname__myfieldname(&$variables) { ... code to mod var ... }

nvanhove’s picture

You can use this code:

function wim_preprocess_views_view(&$variables) {
	if(isset($variables['view']->name)) {
		$function = "wim_preprocess_views_view__" .$variables['view']->name;
    if(function_exists($function))
      $function($variables);
	}
}

function wim_preprocess_views_view__homepage_a_blogs() {
	// view 'homepage_a_blogs'
}