Modules still can't ship with template preprocess chunks

Talking with chx, I needed some additional explanation on exactly what this patch does. Let's take an example template_preprocess_ function and look at how it changes:

We currently have: template_preprocess_page(), which can over-ridden by themes and theme engines via:

phptemplate_preprocess_page()
mytheme_preprocess_page()

This change allows modules to add and change variables the variables during preprocess. Such as:
template_MODULE_NAME_preprocess_page

The flexibility this gives us is tremendous (though it blurs the line between modules and themes further). jjeff has been looking for a way to make jQuery update module swap out various versions of jQuery (see http://drupal.org/node/173941), now this is possible via:

function template_jquery_update_preprocess_page(&$variables) {
  $variables['scripts'] = '<script type="text/javascript">alert("Hello");</script>';
}

This patch has some "interesting" (but correct) side-effects. Modules seem to have a higher priority than theme engines, but less than themes in setting variables. Here's the new order for variable declaration:

template_preprocess_page() (default)
phptemplate_preprocess_page() (theme engine)
template_MODULE_NAME_preprocess_page() (modules)
mytheme_preprocess_page() (theme)

So variables set in phptemplate_preprocess_page (as in the Garland theme) can be over-ridden by a module, but if Garland were to use garland_preprocess_page instead, the variables could not be over-ridden.

Looks great though. I've tested it with several theme functions to confirm this is the behavior. I'd love to see it in Drupal 6.

merlinofchaos had some questions about this segment of code, fearing it would put theme engine processing before the default processing.

+          // theme engines get an extra set that come before the normally named preprocess.
+          array_unshift($prefixes, $prefix .'_engine');

But checking it a second time (each individually), I'm confirming that the order I stated above is correct.

template_preprocess_page() (default)
phptemplate_preprocess_page() (theme engine)
template_MODULE_NAME_preprocess_page() (modules)
mytheme_preprocess_page() (theme)

Capitalizing 't' at the start of comment sentence. :)

subscribe

The ordering is wrong. I tested with "template_aggregator_page" and it comes in last.

I'm not sure I like this new ability as a "themer". I like knowing where all my variables are being assembled. Simply enabling a new module could turn into more confustion. But yeah, it sounds super useful and I have hacked on the registry myself to get devel a complete theming log. So, I can go either way with this but expect some confusion.

And back to chx's point. In Drupal 5, modules never touched variable functions since there was a clear separation between the theme engine and theme(). Not that it wasn't possible but it wasn't done so I don't see this as a critical bug. Module authors should simply stay away from naming preprocess functions not meant for it.

The devel issue I mentioned is here:
http://drupal.org/node/167337

It started as an issue similar to what chx pointed out. It used phptemplate_preprocess to do it's tricks but we did find an odd workaround. Mosh mentioned some sort of hook so the registry could be altered right before it's saved. Maybe another option to consider in D7..

I'm not sure I like the template_ prefix. For consistency with the rest of Drupal, and the other preprocess hooks, we should use MODULE_preprocess_page() not template_MODULE_preprocess_page().

Also, the documentation is sloppy. We write THEME and MODULENAME, instead of THEME and MODULE.

I also don't like this one:

+          // Theme engines get an extra set that come before the normally named preprocess.
+          array_unshift($prefixes, $prefix .'_engine');

This patch is not quite right.

1) module-based template preprocess should happen prior to theme engines, shouldn't they?
2) If there is no theme engine, modules won't get put in because they happen under 'theme_engine'; they need to happen if $type == 'module'

I think what we want is this:

$prefixes = array();

// theme engines get an extra set that come before the normally named preprocess.
if ($type == 'theme_engine') {
  $prefixes[] = $prefix .'_engine';
}

$prefixes[] = $prefix;

// Also let modules intervene.
if ($type == 'module') {
  foreach (module_implements('preprocess_'. $hook) as $module) {
    $prefixes[] = $module;
  }
}

I'm not completely sure about that module_implements; I guess we actually don't want to use that this way, since that's actually basically already doing our function_exists for us, so maybe just module_list() is fine.

Here, try this patch instead, it is more robust.

I went ahead and left the array_shift in. It's a little more difficult to read, but it prevents us from having to have an if on either side of the $prefixes = array() line.

It should indeed happen before theme engines right after the default preprocessors since themes may use the engine name.

I change merlin's patch so it's more readable. Removed the array_unshift. The docs for theme() was changed a bit too so it reflects the order the preprocessors are run.

I tested by throwing in all the possible preprocessors + 2 node preprocessors.

[0] => template_preprocess
[1] => template_preprocess_page
[2] => node_preprocess
[3] => node_preprocess_page
[4] => phptemplate_engine_preprocess
[5] => phptemplate_engine_preprocess_page
[6] => phptemplate_preprocess
[7] => phptemplate_preprocess_page
[8] => garland_preprocess
[9] => garland_preprocess_page

Looks good, I approve.

Hm, indeed, much more readable. But what does this supposed to mean?

// The module list is used to intervene preprocessors not registered to itself.

What does "itself" refer to?

Itself means the current hook that it's working under. Here's another. :)

Hm, I am getting to understand the calling order now. So the ENGINE_preprocess* functions are allowed to be defined by themes to let subthemes defined their own THEME_preprocess* functions and still get the ENGINE_preprocess* (ie. the parent theme's preprocess) functions called? So this kind of overcomes the problem of parent theme preprocess functions not being called from an inherited theme, and parent theme authors need to think about this in advance? Basically meaning that every theme author who considers her theme to be a possible parent for subthemes should use ENGINE_preprocess* instead of THEME_preprocess*?

This sounds like quite a hack to me and obviously it does not work at all with multiple inheritance levels of themes.

Or do I misread something?

Gábor, sorry for not being clear. There's only so much we can explain in the php docs.

The base and sub-themes can use it's own name or the engine name. Even if the base theme used it's own name, all the sub-themes will inherit their preprocessors. The suggestion to use the engine name for the base theme was made because it's easier to use "phptemplate_preprocess_HOOK" when copying all the various snippets that would eventually get posted in the handbooks. I'm not sure if that's a wise choice. It's simply a guideline but nothing prevents themes from doing otherwise.

I talked with Merlin on this and his thought was to move away from "ENGINE_preprocess*" for themes. At the moment, that naming ability is there and we have to document to minimize fatal re-declaration errors.

theme "foo" : (top base theme)
  function phptemplate_page(&$vars) {...}
  
  theme "bar" : (base theme "foo")
    function bar_page(&$vars) {...}
    
    theme "baz" : (base theme "bar")
      function baz_page(&$vars) {...}

So, baz will run all it's parent themes preprocessors for theme('page'). If more than one of the themes used the theme engine, we'd have a problem.

Eh, bad example.. here it is again:

theme "foo" : (top base theme)
  function phptemplate_preprocess_page(&$vars) {...}
  
  theme "bar" : (base theme "foo")
    function bar_preprocess_page(&$vars) {...}
    
    theme "baz" : (base theme "bar")
      function baz_preprocess_page(&$vars) {...}

And theme('page') would run the above preprocessors when "baz" is the active theme.

So then I see no reason to support or in any way suggest themes to use function names which conflict. This caused us enough trouble in the past (eg we was unable to get the regions from a theme because we have not been able to include it). Although the region problem itself is solved with .info files, I would encourage a practice where themes would stay in their own namespace.

I see some code examples in forums might not work anymore, but given the extent of theming changes in Drupal 6 anyway, I think this one is a minor change.

Goba;

I've thought about this, but it's still a good thing to use the phptemplate_* notation (i.e, ENGINE_*) in snippets and in include files that might be included in different themes.

A prime example of this is that I have my own pager.inc that uses my (surprisingly unpopular) pager variant. I just drop it into the theme and 'include_once pager.inc' -- because it uses the phptemplate_* notation, it works great wherever I put it.

Now, if we remove the ability to use phptemplate_* notation, it has to be rewritten for every theme that utilizes it.

This is why I have been merely edging toward encouraging themes to always use their theme name. I think I agree with dvessel that the guideline should be that if you have 'base theme' in your .info file, you should *never* use ENGINE_* notation, but if you don't, it is okay. That minimizes the risk of errors, and still retains flexibility.

There is also the fact that we have trained all of our themers into phptemplate_* notation and it is not clear to me that about-facing on that right now will be beneficial.

BTW, I'm not sure this conversation really affects the current patch.

It does affect the patch for sure. The fact that we "trained" people to do something, which hit us on the face anyway, is not a good reason to support this further. The reusability argument sounds better, but this is not reflected in the patch. There is a growing number of functions we can implement and I think it is very important to have clear documentation about these.

I don't understand; this patch doesn't really affect the ENGINE_ stuff, it's adding MODULE_preprocess.

The ENGINE_ stuff is already there.

Also, regardless of the training argument, I still find the reusability argument (in snippets and in include files) to be both valid and valuable.

Also, changing that now would be an API change rather late. It would very seriously affect any theme that has already been ported to D6.

Merlin, I am not genuinely against "adding" ENGINE_preprocess, but I read this diff lots of times now, and I still do not see where it was possible to use ENGINE_preprocess with the old code (as compared to ENGINE_engine_preprocess):

-        $prefix = ($type == 'module' ? 'template' : $name);
-        // theme engines get an extra set that come before the normally named preprocess.
-        if ($type == 'theme_engine') {
-          if (function_exists($prefix .'_engine_preprocess')) {
-            $info['preprocess functions'][] = $prefix .'_engine_preprocess';
-          }
-          if (function_exists($prefix .'_engine_preprocess_'. $hook)) {
-            $info['preprocess functions'][] = $prefix .'_engine_preprocess_'. $hook;
-          }
-        }
-
-        // Let the theme engine register theme specific variable functions.
-        $prefixes = array($prefix);
-        if ($type == 'theme_engine') {
+        $prefixes = array();
+        if ($type == 'module') {
+          // Default preprocessor prefix.
+          $prefixes[] = 'template';
+          // Add all modules so they can intervene with their own preprocessors. This allows them
+          // to provide preprocess functions even if they are not the owner of the current hook.
+          $prefixes += module_list();
+        }
+        elseif ($type == 'theme_engine') {
+          // Theme engines get an extra set that come before the normally named preprocessors.
+          $prefixes[] = $name .'_engine';
+          // The theme engine also registers on behalf of the theme. The theme or engine name can be used.
+          $prefixes[] = $name;
           $prefixes[] = $theme;
         }
+        else {
+          // This applies when the theme manually registers their own preprocessors.
+          $prefixes[] = $name;
+        }

To me it seems like this line adds this possibility:

+          // The theme engine also registers on behalf of the theme. The theme or engine name can be used.
+          $prefixes[] = $name;

Although it might be that the first removed line adds that for theme engines, in which case I am mistaken. Frankly I was not an expert in the previous code and the new code really looks simpler, so this might be my misunderstanding.

I think your comment illustrates just how much better the patch is than the original code.

-        $prefix = ($type == 'module' ? 'template' : $name);
-        // theme engines get an extra set that come before the normally named preprocess.
-        if ($type == 'theme_engine') {
-          if (function_exists($prefix .'_engine_preprocess')) {
-            $info['preprocess functions'][] = $prefix .'_engine_preprocess';
-          }
-          if (function_exists($prefix .'_engine_preprocess_'. $hook)) {
-            $info['preprocess functions'][] = $prefix .'_engine_preprocess_'. $hook;
-          }
-        }
-
-        // Let the theme engine register theme specific variable functions.
-        $prefixes = array($prefix); // This line, right here. $prefix == the name of the engine if $type == 'theme_engine'.

Eh, this came up because I updated the php docs. It had little to do with the original issue.

Gábor, should I update the docs again? Don't want to hold up the issue for but the doc changes not related to this patch..

To answer this question..

Merlin, I am not genuinely against "adding" ENGINE_preprocess, but I read this diff lots of times now, and I still do not see where it was possible to use ENGINE_preprocess with the old code (as compared to ENGINE_engine_preprocess):

It happened right here:

$prefix = ($type == 'module' ? 'template' : $name);

$name held the name of the engine. _theme_process_registry() iterates over and over again from here:

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

Should I dare set it back RTBC?

updated for hunk offsets. Same as patch #21

Dvessel: I know that that the documentation improvements are not strictly related to this patch, but adding more preprocess functions just makes it *much* more important to document the options better. The explanation here is not too practical. We know from merlin's explanation, that we can use this to provide included and reused code for our themes and it helps people copying code from our repositories. Cleaning up why we have these options is important IMHO.

+ * - ENGINE_preprocess(&$variables)
+ * This is meant to be used by themes that utilize a theme engine. Theme
+ * authors must be careful when using this in sub-themes with PHPTemplate
+ * since it can cause a fatal redeclare errors due to multiple template.php
+ * files being used. A good practice is to use the engine name for the base
+ * theme and the theme name for the sub-themes to minimize name collisions.

Gábor, well as it stands now we still have ENGINE_preprocess* without this patch and the explanations in there are completely off.

I'd like to hear some suggestions on what would be a practical guideline. Once we know that, I'll make the changes.

I just reread your post.. So all I have to do is make it more clear. I thought you didn't like the guideline in general.

Patch forthcoming.

Here's the changed docs. Everything else is the same.

/**
 * Generate the themed output.
 *
 * All requests for theme hooks must go through this function. It examines
 * the request and routes it to the appropriate theme function. The theme
 * registry is checked to determine which implementation to use, which may
 * be a function or a template.
 *
 * If the implementation is a function, it is executed and its return value
 * passed along.
 *
 * If the implementation is a template, the arguments are converted to a
 * $variables array. This array is then modified by the module implementing
 * the hook, theme engine (if applicable) and the theme. The following
 * functions may be used to modify the $variables array. They are processed in
 * this order when available:
 *
 * - template_preprocess(&$variables)
 *   This sets a default set of variables for all template implementations.
 *
 * - template_preprocess_HOOK(&$variables)
 *   This is the first preprocessor called specific to the hook; it should be
 *   implemented by the module that registers it.
 *
 * - MODULE_preprocess(&$variables)
 *   This will be called for all templates; it should only be used if there
 *   is a real need. It's purpose is similar to template_preprocess().
 *
 * - MODULE_preprocess_HOOK(&$variables)
 *   This is for modules that want to alter or provide extra variables for
 *   theming hooks not registered to itself. For example, if a module named
 *   "foo" wanted to alter the $submitted variable for the hook "node" a
 *   preprocess function of foo_preprocess_node() can be created to intercept
 *   and alter the variable.
 *
 * - ENGINE_engine_preprocess(&$variables)
 *   This function should only be implemented by theme engines and exists
 *   so that it can set necessary variables for all hooks.
 *
 * - ENGINE_engine_preprocess_HOOK(&$variables)
 *   This is the same as the previous function, but it is called for a single
 *   theming hook.
 *
 * - ENGINE_preprocess(&$variables)
 *   This is meant to be used by themes that utilize a theme engine. It is
 *   provided so that the preprocessor is not locked into a specific theme.
 *   This makes it easy to share and transport code but theme authors must be
 *   careful to prevent fatal re-declaration errors when using sub-themes that
 *   have their own preprocessor named exactly the same as it's base theme. In
 *   the default theme engine (PHPTemplate), sub-themes will load their own
 *   template.php file in addition to the one used for it's parent theme. This
 *   increases the risk for these errors. A good practice is to use the engine
 *   name for the base theme and the theme name for the sub-themes to minimize
 *   this possibility.
 *
 * - ENGINE_preprocess_HOOK(&$variables)
 *   The same applies from the previous function, but it is called for a
 *   specific hook.
 *
 * - THEME_preprocess(&$variables)
 *   These functions are based upon the raw theme; they should primarily be
 *   used by themes that do not use an engine or by sub-themes. It serves the
 *   same purpose as ENGINE_preprocess().
 *
 * - THEME_preprocess_HOOK(&$variables)
 *   The same applies from the previous function, but it is called for a
 *   specific hook.
 *
 * There are two special variables that these hooks can set:
 *   'template_file' and 'template_files'. These will be merged together
 *   to form a list of 'suggested' alternate template files to use, in
 *   reverse order of priority. template_file will always be a higher
 *   priority than items in template_files. theme() will then look for these
 *   files, one at a time, and use the first one
 *   that exists.
 * @param $hook
 *   The name of the theme function to call. May be an array, in which
 *   case the first hook that actually has an implementation registered
 *   will be used. This can be used to choose 'fallback' theme implementations,
 *   so that if the specific theme hook isn't implemented anywhere, a more
 *   generic one will be used. This can allow themes to create specific theme
 *   implementations for named objects.
 * @param ...
 *   Additional arguments to pass along to the theme function.
 * @return
 *   An HTML string that generates the themed output.
 */

Wait.

Please, this patch is NOT where we should be arguing about documentation changes. This is silly. Let's pull the documentation changes from this patch and put them in their own patch, because important issue A is getting slowed up by a bikeshed issue.

Thanks for the documentation improvements. I stand behind my belief that when we complicate a system with more options and possibilities, improving documentation for the different possibilities should go hand in hand. Committed the patch with the improved docs.