I just noticed this from this queue on cleaning up Garland.

What is the expected behavior when prefixing a base themes preprocessor with the name of the theme while its sub-theme is active?

I expected sub-themes to run all preprocessors from it's base theme regardless of it's naming but this is not the case. Naming it after the theme engine will have it run in the sub-theme but name it after the base theme and it'll only run when the base theme is active. Sub-theme's will ignore it.

See the docs on preprocessors. When I wrote it, I might have just assumed the wrong behavior. There's no mention in here about this behavior.

http://drupal.org/node/223430

So, what is this? A bug or the wrong idea?

Files: 
CommentFileSizeAuthor
#1 theme_base_theme_process.patch1.63 KBquicksketch

Comments

quicksketch’s picture

Status: Active » Needs review
FileSize
1.63 KB

This looks like a bug to me. The existing documentation and code seems to indicate that the base theme is supposed to be processed, but an errant IF statement is causing the base theme to be skipped in the processing. This patch corrects the mistake, and updates the PHPdoc to reflect all the values that are passed into the function.

quicksketch’s picture

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

The patch is against HEAD. Though since this is a bug, we should definitely backport it to D6.

dvessel’s picture

Status: Needs review » Reviewed & tested by the community

Assumed it was a bug also especially since theme functions prefixed with the base theme worked as I thought. Not exactly the same thing but to have them work in similar ways makes sense to me.

Nicely done! Tiny and it now behaves.

quicksketch’s picture

Some more testing on this patch. I created a "minelli2" theme that is a subtheme of minelli. Then creating a template.php file for both minelli and minelli2, the inheritance shows this behavior (last function in the list takes precedence):

theme_breadcrumb
phptemplate_breadcrumb
garland_breadcrumb
minelli_breadcrumb
minelli2_breadcrumb

Removing minelli2_breadcrumb causes it to fallback to minelli_breadcrumb. Removing minelli_breadcrumb causes it to fall back to garland_breadcrumb. This is all *very* cool.

As for preprocess functions, minelli and minelli2 only have their preprocess functions called if the theme also has a copy of the .tpl.php file. Inheritance works just the same (page.tpl.php in minelli takes precedence over garland's page.tpl.php, and minelli2 takes precedence over minelli).

Anyway, this is a fantastic fix. I wan't aware it would even handle multiple inheritance. Making it even more clear this was supposed to be the way it worked and this is simply a one-line error (you don't get multi-level inheritance by accident ;)

quicksketch’s picture

If I may quote IRC also:

merlinofchaos: quicksketch: Ok, now that I've looked up the code, yes, that patch makes sense. Note that dvessel's +1 should be enough to get it in.

JohnAlbin’s picture

Priority: Normal » Critical

I've reviewed the patch and it works great.

Without a fix for this bug, multi-level sub-themes are broken in core, so this is critical IMO.

For example (without this patch), if basetheme1 has a HOOK.tpl.php and defines a preprocess function that it know will need to be run by a subtheme, it must call it phptemplate_preprocess_HOOK(); otherwise, a basetheme1_preprocess_HOOK() function won't get called by a subtheme. Now, subtheme2 (a child of basetheme1) overrides that HOOK.tpl.php, but it cannot re-define that function, so it must define subtheme2_preprocess_HOOK(). And, if subtheme3 (a child of subtheme2) doesn't override that HOOK, the expected behaviour would be that its parent HOOK.tpl.php and preprocess function (subtheme2_preprocess_HOOK) would be used, but instead we get the parent's HOOK.tpl.php and the grandparent's phptemplate_preprocess_HOOK() running.

Crell and I have been creating a workaround for this in the Zen theme, but it sure would be nice to get this fixed in 6.3.

quicksketch’s picture

Priority: Critical » Normal

Drupal 6 can everything 5 can do theming-wise. Let's not get overly excited. I'd love to see this in 6.3 also.

JohnAlbin’s picture

Yes, but multi-level sub-themes was supposed to be a feature of Drupal 6. And its clearly broken in the example in #5 above. If subtheme2 overrides HOOK.tpl.php than any subtheme of subtheme2 requires that subtheme2_preprocess_HOOK() get called. And it doesn't get called; that's serious breakage.

I do think this is critical. However, I won't start a "priority"-changing war. ;-)

dropcube’s picture

I have applied the patch and everything seems to be working as expected. I tested with 2nd-level and 3rd-level sub-themes and it works.

JohnAlbin’s picture

Making the issue title a little more descriptive.

And, while I'm here... here's a diagram to help visualize how this bug breaks multi-level sub-themes. Because even I have a hard time understanding what I typed in my last two comments. :-D

+- basetheme1
|  + HOOK.tpl.php
|  + phptemplate_preprocess_HOOK()  [base themes in D6 know that
|      basetheme1_preprocess_HOOK() won't get called by sub-themes.]
|
+- subtheme2 (subtheme2.info has "base theme: basetheme1")
|  + HOOK.tpl.php
|  + subtheme2_preprocess_HOOK() [because it can't re-define phptemplate_preprocess_HOOK().]
|
+- subtheme3 (subtheme3.info has "base theme: subtheme2")
   + CSS-only child of subtheme2
   + when this theme is active and rendering HOOK, Drupal uses subtheme2's HOOK.tpl.php,
       but calls basetheme1's phptemplate_preproces_HOOK().  Here be dragons.
Moonshine’s picture

Title: Preprocessor naming prefix, its expected behavior. » Allow BASETHEME_ prefix in preprocessor function names

I've also applied this patch to 6.2 and it's working as expected for me as well...

+1 for getting this into 6.3 !

IMO this really is a critical issue. Not only because it "fixes" expected behaviour (and is very small), but it also provides a solution for the subtheme related performance issue I've been trying to sort out:

Let's say you have "basetheme1" that implements phptemplate_preprocess_page()

Then you create a subtheme named "subtheme1" that implements its own subtheme1_preprocess_page()

This will result in phptemplate_preprocess_page() being registered twice in the theme registry (and therefore run twice), in addition to subtheme1_preprocess_page().

In looking through theme.inc, it appears this is because function_exists() based checks look for ENGINE_preprocess* functions for each theme involved. Thus ENGINE_preprocess* calls can be duplicated (or worse) in the theme registry for subthemes that provide their own hooks.

While calling a preprocess_page hook twice might not be so bad, give it a shot with preprocess_node or preprocess_comment on a loaded page! 2x (or 3x) the calls can make a big difference. At least with this patch, base themes can name all their preprocess functions after themself (rather then their engine) and the stacking of ENGINE_preprocess* calls is avoided. Really after this patch, it makes me wonder if ENGINE_preprocess* calls have any place in a theme?!

quicksketch’s picture

Moonshine, yeah that was my point in the Garland cleanup patch. ENGINE_preprocess hooks *never* should have been in any theme, it was a shortcoming of the theme system that led us to break our own conventions (all functions should start with the name of their owning module/theme/engine).

So now phptemplate_ has been removed from Garland, but the result is that the theme is currently broken, because now that phptemplate_ has been renamed to garland_, Minelli is no longer getting any of the override variables.

Moonshine’s picture

Just a followup that this patch has been working well for me on 3 seperate D6 installs. I've also seen this issuse come up in #drupal-support several times now, so it would be really nice to see it applied for 6.3. :)

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Looks pretty simple. The double registration mentioned by Moonshine does not sound good though.

quicksketch’s picture

Status: Needs work » Needs review

I'm both disappointed and frustrated with this particular issue. It was a freaking typo. We should fix it.

While calling a preprocess_page hook twice might not be so bad, give it a shot with preprocess_node or preprocess_comment on a loaded page! 2x (or 3x) the calls can make a big difference.

Yes. This makes it so that phptemplate can define a phptempate_preprocess_node() function, and the theme (and basetheme) can also provide a themename_preprocess_node() function. While we're at it, I want to mention that EVERY MODULE can define a module_preprocess_node() function. Not only that, but if the function does not exist, the theme system will not even attempt to call it because the list of preprocess functions is cached. Citing performance or "doubling-up" of function calls as a reason for reworking is unnecessary.

Moonshine’s picture

Yipes...

I was trying to say that this patch would work to FIX the problem of duplicated/triplicated/etc calls to engine_preprocess_*() that currently happen.

As it stands, the function_exists() based checks look for engine_preprocess_* functions for each theme/subtheme involved. This creates the problem, as themes should really never need to use engine_preprocess_* calls themselves, but yet they must currently to allow for subthemes. That's where this patch comes in as another benefit.

Basically get patch in there, and themes no longer have to (or even should!) use engine_preprocess_*. Instead they should all follow the normal Drupal conventions themename_preprocess_*. Repetition of the engine calls will go away and subthemes will be able to endlessly override.

quicksketch’s picture

Thanks Moonshine. I'm just upset with the release of 6.3 and this not in it. Apologies for the outburst.

JohnAlbin’s picture

quicksketch, I don't know if you (or anyone else) needs a workaround for your basetheme/subthemes until Drupal 6.4 comes out, but Zen 6.x has one. This is the minimum code required to work around the core bug:

In the base theme’s template.php:

function BASETHEME_theme(&$existing, $type, $theme, $path) {
  // Each theme has two possible preprocess functions that can act on a hook.
  // This function applies to every hook.
  $functions[0] = $theme . '_preprocess';
  // Inspect the preprocess functions for every hook in the theme registry.
  // @TODO: When PHP 5 becomes required (Drupal 7.x), use the following faster
  // implementation: foreach ($existing AS $hook => &$value) {}
  foreach (array_keys($existing) AS $hook) {
    // This preprocess function only applies to this hook.
    $functions[1] = $theme . '_preprocess_' . $hook;
    foreach ($functions AS $key => $function) {
      // Add any functions that are not already in the registry.
      if (function_exists($function) && !in_array($function, $existing[$hook]['preprocess functions'])) {
        // We add the preprocess function to the end of the existing list.
        $existing[$hook]['preprocess functions'][] = $function;
      }
    }
  }
  // Since we modify the $existing cache directly, return nothing.
  return array();
}

In every sub-theme’s template.php

function ANYSUBTHEMES_theme(&$existing, $type, $theme, $path) {
  $hooks = BASETHEME_theme($existing, $type, $theme, $path);
  // Additional theme hooks here.
  return $hooks;
}

Of course, a four-word core patch is much simpler. :-p

Gábor Hojtsy’s picture

quicksketch: well, this is still not RTBC, and from the looks, you fine folks who worked on the theme system do not consider it RTBC, so how come would it be committed already?

JohnAlbin’s picture

Status: Needs review » Reviewed & tested by the community

dvessel (in #3), myself (in #6), dropcube (in #9), and Moonshine (in #11) have all reviewed the patch and said its RTBC. Neither Moonshine or myself (in #16 and #18) noticed the issue got bumped off of RTBC or we would have bumped it back.

Gábor, I don't see any comments that show that the patch doesn't work. All the “complaints” are about the BUG, not about the patch.

For good measure, I reviewed the un-patched core bug, then applied the patch (still applies but with fuzz after 2.5 months as RTBC) and marveled at how well this simple patch fixes so much awfulness.

(Quicksketch, I still think this is Critical, btw. Base themes do NOT work in Drupal 6.3. And Moonshine's been seeing lots of questions/complaints about it in #drupal-support.)

webchick’s picture

Priority: Normal » Critical

I agree.

merlinofchaos’s picture

Priority: Critical » Normal

Just to reiterate my comments from IRC that I failed to post personally, I completely agree with this patch.

JohnAlbin’s picture

/me sets the priority back to critical; then looks to see if there's a "fix cross-posting errors" issue in project*.

[edit: found it. #218066: Prevent cross posting from reverting metadata fields]

BioALIEN’s picture

Priority: Normal » Critical

I'm really disappointed not to find this in D6.3 was actually looking forward to it, but when I didn't see it mentioned in the changelog I had to come here and take a peek.

Chill35’s picture

Disappointed as well :-(

quicksketch’s picture

1 line of changed code.
3 months RTBC.
6 RTBC agreements by the most experienced themers in the Drupal community.
? confused Drupal users.

Still applies cleanly.

jbrauer’s picture

Another successful confirmation that the patch applies cleanly and has the desired effect.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

I've committed this to CVS HEAD and DRUPAL-6. Woop, woop.

quicksketch’s picture

Thanks Dries! Looks like this didn't make it into 6.4, so anyone looking for this will find it in 6.5.

Chill35’s picture

Thanks for the heads-up, quicksketch.

Anonymous’s picture

Status: Fixed » Closed (fixed)

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

hass’s picture

Status: Closed (fixed) » Postponed (maintainer needs more info)

Seems like this introduced the bug I found at #328122: Kill base theme's hook_preprocess_page function

Moonshine’s picture

Status: Postponed (maintainer needs more info) » Closed (fixed)

One thread is enough to discuss behavior that's by design.