"hook_process()? What's that?"

That's what you get when template_process() was added to the theme system. This function, combined with the fact that modules may implement preprocess hooks, means that some unsuspecting modules will now be doing awful, awful things to the template variables.

template_process() is implied to exist at http://drupal.org/node/254940#variables-processor, but there is no indication that module maintainers need to check their function names for the word "process".

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Wow. Good catch!

Looking at http://api.drupal.org/api/function/theme/7, it looks like for modules there is hook_process() and hook_preprocess(). Neither of these is documented in Drupal 7, and both should be.

I've just added these to the critical catch all issue for hooks in D7: #675046: Make sure all hooks in D7 have documentation

jhodgdon’s picture

Someone needs to look through theme() and make sure that the doc at the top of it is really correct, as well as adding doc for hook_process() and hook_preprocess() if those are actually invoked. The code is not actually clear to me...

JohnAlbin’s picture

Agreed. modules/system/theme.api.php needs to add hook_preprocess, hook_preprocess_HOOK, hook_process, hook_process_HOOK docs.

And it also needs to be added to the module upgrade docs.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

I'm taking this on...

jhodgdon’s picture

Title: hook_process() is undocumented » hook_process() and associated hooks are undocumented
Assigned: jhodgdon » Unassigned
Status: Active » Needs review

Here's a patch. It fixes up the theme() documentation, and adds the four needed hook docs.

jhodgdon’s picture

FileSize
16.06 KB

It might help if I attached the patch.

jhodgdon’s picture

Note: If accepted, we also need to add something to the module update docs. JohnAlbin: I'm not sure what we need to add though? What of this is new to D7, just these process/preprocess hooks?

aspilicious’s picture

+++ includes/theme.inc	23 Apr 2010 21:05:42 -0000
@@ -621,130 +621,88 @@
+ * - template_preprocess_HOOK(&$variables): Should be implemented by ¶

trailing white space

Powered by Dreditor.

aspilicious’s picture

FileSize
16.06 KB

fixed the trailing white space

aspilicious’s picture

Status: Needs review » Needs work
+++ modules/system/theme.api.php	23 Apr 2010 21:05:42 -0000
@@ -93,3 +93,104 @@
+ if (!user_access('access contextual links')) {
+    return;
+  }

EDIT: I removed a confusing line

I think something is wrong with the intendation

+++ modules/system/theme.api.php	23 Apr 2010 21:05:42 -0000
@@ -93,3 +93,104 @@
+  else {
+    $key = $hooks[$hook]['render element'];
+  }
+  if (isset($variables[$key])) {
+    $element = $variables[$key];
+  }

Maybe a newline between the else ad the if statement?

No other remarks

90 critical left. Go review some!

jhodgdon’s picture

Status: Needs review » Needs work
FileSize
16.04 KB

I don't see a problem with the indentation in the first part there. That second line you referenced is inside an if() {}

???

Added the newline in your second spot.

jhodgdon’s picture

Status: Needs work » Needs review
aspilicious’s picture

+-if (!user_access('access contextual links')) {
+----return;
+--}

I replaced spaces with '-'.
As you can see there is one space before the keyword 'if' and two before '}'.
You always need two spaces.

BTW: the last sentence in my first code block sneeked into my review, wasn't supppsed to happen

You see the indentation problem better in the patch (when you see more of the function)

 if (!user_access('access contextual links')) {
    return;
  }

  if (!isset($hooks)) {
    $hooks = theme_get_registry();
  }
aspilicious’s picture

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
16.04 KB

Ah, missed that. Here's a fixed patch.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

RTBC

Bot can turn it to red if he wants ;)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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

effulgentsia’s picture

#15 introduced some inaccuracies by using the phrase "all theme hooks" to refer to the execution of the non-theme-hook-specific preprocess() and process() functions, as per below:

+++ includes/theme.inc	28 Apr 2010 14:26:14 -0000
@@ -621,130 +621,88 @@
+ * - template_preprocess(&$variables, $hook): Creates a default set of variables
+ *   for all theme hooks.
...
+ * - ENGINE_engine_preprocess(&$variables, $hook): Allows the theme engine to
+ *   set necessary variables for all theme hooks.
...
+ * - THEME_preprocess(&$variables, $hook): Allows the theme to set necessary
+ *   variables for all theme hooks.
...
+ * - template_process(&$variables, $hook): Creates a default set of variables
+ *   for all theme hooks.
...
+ * - ENGINE_engine_process(&$variables, $hook): Allows the theme engine to set
+ *   necessary variables for all theme hooks.
...
+++ modules/system/theme.api.php	28 Apr 2010 14:26:14 -0000
@@ -95,6 +95,108 @@
+ * This hook allows modules to preprocess theme variables for theme templates.
+ * It is called for all invocations of theme(), to allow modules to add to
+ * or override variables for all theme hooks.
...
+function hook_preprocess(&$variables, $hook) {
...
+ * This hook allows modules to process theme variables for theme templates.
+ * It is called for all invocations of theme(), to allow modules to add to
+ * or override variables for all theme hooks.
...
+function hook_process(&$variables, $hook) {

As stated elsewhere in the PHPDoc of theme(), due to performance constraints, these only run for theme hooks implemented as templates.

We're working on a fix for this in #1333122: Fix documentation in theme() and template_preprocess(), so leaving this issue as closed. Just cross-linking here for posterity.