There's a feature to override preprocess functions but it is not useful in its current state.

As the preprocess functions are built, the existence of the key "override preprocess functions' prevents further building of the "preprocess functions" array as it recurses through adding to the registry.

See _theme_process_registry()

if (isset($cache[$hook]['preprocess functions']) &&
    is_array($cache[$hook]['preprocess functions']) &&
    empty($cache[$hook]['override preprocess functions'])) // <- Checks the cached version of this key.
{
  $info['preprocess functions'] = array_merge($cache[$hook]['preprocess functions'], $info['preprocess functions']); 
}

So, if a theme engine wanted to override a preprocess function, All it can do is override the functions that are set from themes as it is further down the chain. This is because it checks the version in $cache. For example, phptemplate_theme() can override all the preprocess functions set from garland_theme(), not the other way around. This seems backwards and not intentional. A theme cannot override the functions and template engines doing the overrides cripples the themers ability to add more preprocess functions.

What this patch does is allow *any* invocation of HOOK_theme() to override all preprocess functions that were set *before* it not after.

All that's needed is this. Overrides 'page' hook to use my custom preprocessors:


function garland_theme($cache, $type, $theme, $path) {
  // Attache Cached information.
  $result['page'] = $cache['page'];
  
  // Supply override.
  $result['page']['override preprocess functions'] = array(
    'template_preprocess_page',
    'garland_preprocess',
  );

  return $result;
}

What this will do is replace all cached preprocess functions with the override. Not many themers or engine authors may use this but this advanced feature makes it work with more flexibility. Besides, the current code seems broken.

CommentFileSizeAuthor
#11 theme_registry_override_4.patch1.88 KBdvessel
#3 theme_registry_override_3.patch1.54 KBdvessel
#2 theme_registry_override_2.patch2.3 KBdvessel
theme_registry_override_1.patch4.81 KBdvessel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

Will this work?

      if (isset($cache[$hook]['preprocess functions']) && is_array($cache[$hook]['preprocess functions']) && empty($cache[$hook]['override preprocess functions'])) {
        $info['preprocess functions'] = array_merge($cache[$hook]['preprocess functions'], $info['preprocess functions']);
      }
      // Override preprocess functions means overriding the past ones, not the future ones.
      if (!empty($info['override preprocess functions'])) {
        unset($info['override preprocess functions']);
      }
      $result[$hook]['preprocess functions'] = $info['preprocess functions'];
dvessel’s picture

dvessel CreditAttribution: dvessel commented
FileSize
theme_registry_override_2.patch2.3 KB

Here's another. The 'override preprocess functions' key works as a flag. Just like before but it doesn't check the cached version.

A few things were cleaned up too. The theme registry was littered with empty preprocess function arrays for functions that do not need them. (non templated functions). And I also added an array_unique() to safeguard it against duplicate preprocessors for any given hook. I think this is important since it can easily be overlooked placing more load.

I tested this in and out. It works like a charm.

dvessel’s picture

dvessel CreditAttribution: dvessel commented
FileSize
theme_registry_override_3.patch1.54 KB

Not sure what happened. Did a fresh checkout and the previous patch wasn't doing it's job.

A bit more simple. doesn't try to clean the empty arrays.

Here's another. Seriously, tested and definitely works.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

this is some confusing mojo.

dvessel’s picture

dvessel CreditAttribution: dvessel commented

mosh, yeah I know. I'll make it clear inside the docs. Implementing isn't too complicated, just the underlying theme registry is but most theme dev's don't need to know all the details on that.

merlinofchaos’s picture

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

Moshe: Yes, this is a little confusing. However:

1) This is an advanced power-users feature. This is only going to be used by really advanced users who want to be able to reduce the amount of code that's run on theme('page'), mostly. Otherwise, it will generally have no effect whatsoever, and 99.9% of the installations will never want nor need this feature.

It's analogous to creating your own theme_page() so that you didn't run everything that was in phptemplate_page() for whatever reason.

2) I apparently implemented it broken.

3) This fixes it.

Tested this out, and it does the job.

Dries’s picture

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

I agree with Moshe that this is hairy. The PHPdoc doesn't really help me understand this -- it gives me a vague hint at best.

(Are we _sure_ we need this ability? What preprocess functions are so expensive that we might want to alter them? Is this driven by a concrete use-case or are we just over-engineering this? I'd be interested to learn more about dvessel's use case ...)

dvessel’s picture

dvessel CreditAttribution: dvessel commented

Dries, due to the additive nature of variable functions, we can no longer override them. In Drupal 5, it was a simple process.

Some uses for this:

Add more style sheets before drupal_get_css() is called from template_preprocess_page(). Doing it after creates tons of preprocessed style sheets. Doubles on each page load and each page can change its list of style sheets creating even more. On top of that, if color.module is implemented into the theme, we get even more. drupal_get_css() ideally should be called once since it does so much.

By overriding the *order* of the preprocessor functions, it can be overcome.

In 5 it was solved with this:

function_themeName_page($content, $show_blocks = TRUE) {
  drupal_add_css('etc.', 'etc.');
  return phptemplate_page($content, $show_blocks = TRUE);
}

Similarly, altering $show_blocks can be intercepted in the same way. I've seen themers who needed it to be shown on all pages. Without this override in the registry, it will be out of our hands.

As merlinofchaos mentions, specific hooks like "page" can remove the default preprocessor. I can see this being useful to developers of theme engines. PHPTemplate makes a lot of assumptions on the output. Sidebars blocks and whatnot can be redone. Engines can bypass that completely and provide something very different.

This feature was already implemented but in a broken state. It doesn't affect most themers so why not?

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

It's worth noting that I made dvessel work pretty hard to convince me to put this feature in. =)

dvessel’s picture

dvessel CreditAttribution: dvessel commented

I hope this can go in. I'll roll another after this patch goes in. http://drupal.org/node/165343

dvessel’s picture

dvessel CreditAttribution: dvessel commented
Status: Needs work » Needs review
FileSize
theme_registry_override_4.patch1.88 KB

Dries, I can see plenty of uses for this. I hope you reconsider.

Another thing I had in mind is to line up a whole list of custom preprocessors instead of having functions calling other functions. With the new theme features patch that went in, I can supply a ui on the theme and have the registry build a custom list of preprocessors depending on what features are selected.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

It's also important to note that this feature already exists...but went in broken. So this IS a bug fix of an existing feature.

dvessel’s picture

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

Heck, it's such a trivial patch so marking RTBC.

Functionality is already built in. This just fixes it. :)

Gábor Hojtsy’s picture

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

OK, you added the docs which Dries requested and this is really an existing faeture, although possibly only used by a small minority. Committed.

dvessel’s picture

dvessel CreditAttribution: dvessel commented

Thank you. :)

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)
moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Given that we now have a drupal_alter() operation on the theme registry, can this "feature" now be dropped? I would think so. I ask here becuase the ones talking here are surely the only ones who know the answer.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

since themes can't use drupal_alter(), I would say no.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

but since this is such a exotic need, i think we could recommend that sites use a module for this. you characterized it as a high performance tweak earlier, so we aren't talking about vanilla drupal sites.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

Let's see what dvessel says there; it was his feature request.

dvessel’s picture

dvessel CreditAttribution: dvessel commented

Yeah, it's not necessary any longer. drupal_alter can do everything this feature did. But...

I would prefer to have this functionality directly inside themes since it is after all a theming feature and publishing a theme with this enabled would mean pointing to another module. Theming engines can use this too and it opens a lot of possibilities there also.