Themes cannot have a preprocess function without a corresponding .tpl.php file

JohnAlbin - May 13, 2008 - 19:50
Project:Drupal
Version:7.x-dev
Component:theme system
Category:bug report
Priority:normal
Assigned:Unassigned
Status:closed
Description

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.

#1

Gábor Hojtsy - May 13, 2008 - 19:55

Hm, this might qualify as a D6 bug indeed.

#2

dvessel - May 13, 2008 - 20:26

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

#3

JohnAlbin - May 13, 2008 - 20:42

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.

#4

merlinofchaos - May 13, 2008 - 23:03

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.

#5

Arancaytar - July 1, 2008 - 06:40

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

#6

Arancaytar - July 1, 2008 - 19:58
Status:active» needs review

Here is a patch for D6.

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

#7

dixon_ - July 1, 2008 - 20:03
Status:needs review» active

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

#8

Arancaytar - July 1, 2008 - 20:58
Status:active» needs review

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

AttachmentSizeStatusTest resultOperations
drupal-theme-preprocess-258089-8.patch1.3 KBIdleUnable to apply patch drupal-theme-preprocess-258089-8.patchView details | Re-test

#9

Arancaytar - July 1, 2008 - 21:00

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

#10

Arancaytar - July 1, 2008 - 22:24

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.

#11

Arancaytar - July 4, 2008 - 16:34

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

#12

chx - July 7, 2008 - 14:50
Project:Drupal» Documentation
Version:7.x-dev» <none>
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 .

#13

merlinofchaos - July 7, 2008 - 20:07
Project:Documentation» Drupal
Version:<none>» 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.

#14

chx - July 8, 2008 - 04:35

<?php
@@ -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.

#15

merlinofchaos - July 8, 2008 - 06:02
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.

#16

merlinofchaos - August 26, 2008 - 23:33

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.

#17

eMPee584 - October 5, 2008 - 10:25

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.

#18

Pedro Lozano - October 5, 2008 - 17:41

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.

#19

merlinofchaos - October 22, 2008 - 22:27
Version:7.x-dev» 6.x-dev
Status:needs work» needs review

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.

AttachmentSizeStatusTest resultOperations
fix-preprocess-theme.patch1.36 KBIdlePassed on all environments.View details | Re-test

#20

merlinofchaos - October 22, 2008 - 22:39

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:

<?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.

#21

quicksketch - October 23, 2008 - 00:05

Subscribing.

#22

dvessel - October 23, 2008 - 18:17
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.

#23

merlinofchaos - October 23, 2008 - 18:33

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?

#24

dvessel - October 23, 2008 - 18:40

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

I am using the latest Drupal 6 from CVS.

#25

merlinofchaos - October 23, 2008 - 18:53
Status:needs work» needs review

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.

AttachmentSizeStatusTest resultOperations
fix-preprocess-theme.patch1.54 KBTest request sentNoneView details

#26

dvessel - October 23, 2008 - 19:11
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)

#27

dvessel - October 23, 2008 - 19:38
Status:needs work» needs review

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

AttachmentSizeStatusTest resultOperations
fix-preprocess-theme_1.patch1.41 KBIdlePassed on all environments.View details | Re-test

#28

merlinofchaos - October 23, 2008 - 19:55

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.

#29

dvessel - October 23, 2008 - 20:32
Assigned to:JohnAlbin» Anonymous

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..

#30

merlinofchaos - October 23, 2008 - 20:35

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

#31

dvessel - October 23, 2008 - 20:47
Status:needs review» reviewed & tested by the community

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

#32

emmajane - October 24, 2008 - 05:15

+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).

#33

webchick - October 24, 2008 - 05:23

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?

#34

webchick - October 24, 2008 - 06:06
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. :)

#35

webchick - October 24, 2008 - 07:08
Version:7.x-dev» 6.x-dev
Status:needs work» reviewed & tested by the community

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.

AttachmentSizeStatusTest resultOperations
drupal-fix-preprocess-theme-258089-35-7.x.patch1.72 KBIdleUnable to apply patch drupal-fix-preprocess-theme-258089-35-7.x.patchView details | Re-test
drupal-fix-preprocess-theme-258089-35-6.x.patch1.72 KBIdleUnable to apply patch drupal-fix-preprocess-theme-258089-35-6.x.patchView details | Re-test

#36

webchick - October 24, 2008 - 07:24
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.

#37

JohnAlbin - October 25, 2008 - 17:00
Status:patch (to be ported)» needs review

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

AttachmentSizeStatusTest resultOperations
drupal-fix-preprocess-theme-258089-37-6.x.patch1.71 KBIdleUnable to apply patch drupal-fix-preprocess-theme-258089-37-6.x.patchView details | Re-test

#38

keith.smith - October 25, 2008 - 17:17
Status:needs review» needs work

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

#39

webchick - October 25, 2008 - 17:33

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.

#40

JohnAlbin - October 25, 2008 - 18:01
Status:needs work» needs review

Re-rolled 6.x with period.

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

AttachmentSizeStatusTest resultOperations
drupal-fix-preprocess-theme-258089-40-6.x.patch1.71 KBIdleUnable to apply patch drupal-fix-preprocess-theme-258089-40-6.x.patchView details | Re-test
drupal-fix-preprocess-theme-258089-40-7.x.patch888 bytesIdleUnable to apply patch drupal-fix-preprocess-theme-258089-40-7.x.patchView details | Re-test

#41

Gábor Hojtsy - October 28, 2008 - 19:13
Status:needs review» reviewed & tested by the community

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

#42

Gábor Hojtsy - November 10, 2008 - 10:12
Version:6.x-dev» 7.x-dev

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

#43

webchick - November 10, 2008 - 15:40
Status:reviewed & tested by the community» fixed

LOL.

Committed to HEAD. :)

#44

dvessel - November 10, 2008 - 16:23

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.

#45

System Message - November 24, 2008 - 16:32
Status:fixed» closed

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

#46

kramssov - December 14, 2008 - 17:32

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

#47

JohnAlbin - December 29, 2008 - 14:00

@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.

 
 

Drupal is a registered trademark of Dries Buytaert.