Add postprocess phase for theme()

Here's a patch that does the equivalent that I propose above, except:

• Instead of using drupal_alter(), it mimics the equivalent functionality, but using the theme registry, adding 'alter functions' as another property of a theme hook. This results in virtually no performance overhead in having this functionality. See http://drupal.org/node/400292#comment-1936414 and that issue in general, which goes into more detail with benchmarks. In addition to the performance gain, using the registry allows for more flexibility. For example, devel_themer.module could create a single alter function and then modify the theme registry to use it for all theme hooks.
• This patch only enables this functionality for template-implemented theme hooks. I would like to see this functionality available for all theme hooks, whether template-implemented or function-implemented, but #400292: Allow preprocess functions for theme hooks implemented as functions would need to be resolved first for that to happen. If this patch gets committed to HEAD first, then I'll update the patch on the other issue to provide this functionality for all theme hooks. If the other patch gets committed to HEAD first, then I'll update this patch to have that functionality. But I wanted to structure this patch in a way that would not create a dependency on which issue got resolved first, and hopefully by doing so, it increases the odds that both will be accepted before code freeze.

#400292: Allow preprocess functions for theme hooks implemented as functions was committed, so this patch makes the alter functionality work for all theme hooks: function-implemented and template-implemented.

Just like in #400292: Allow preprocess functions for theme hooks implemented as functions, minimal performance overhead is incurred when not used (i.e., where no module implements the alter hook), which is important, since some theme hooks need to run fast, so any patch that adds serious overhead across the board to all theme hooks would be bad. Note comments 39-41 on that issue related to benchmarking the invocation of theme('username') 10,000 times.

HEAD (4 runs):
nothing: 0.00206422805786 seconds theme: 26.9105942249 seconds
nothing: 0.00207495689392 seconds theme: 26.9361908436 seconds
nothing: 0.00204992294312 seconds theme: 26.8832099438 seconds
nothing: 0.00204300880432 seconds theme: 26.774846077 seconds
26.88 +/- 0.08

This patch (4 runs):
nothing: 0.00210118293762 seconds theme: 27.0549638271 seconds
nothing: 0.00218486785889 seconds theme: 27.2283799648 seconds
nothing: 0.00208187103271 seconds theme: 26.9738938808 seconds
nothing: 0.00214695930481 seconds theme: 27.2108471394 seconds
27.12 +/- 0.12

However, the optimization patch in #523682: Small optimization to theme() more than compensates for this:
nothing: 0.00210309028625 seconds theme: 26.1672861576 seconds
nothing: 0.00209403038025 seconds theme: 26.318500042 seconds
nothing: 0.00215792655945 seconds theme: 26.1531839371 seconds
nothing: 0.0021800994873 seconds theme: 26.3963549137 seconds
26.26 +/- 0.12

If performance is the blocker on this, I can try optimizing further. However, before I spend time doing that, I'd like to know what folks think of this feature in general.

1) Performance aside, is it desired?
2) Any comments on the code?
3) Is the small performance penalty a deal breaker? If so, I'll wait till #523682: Small optimization to theme() is committed, and will then run more tests and try to optimize to where this feature becomes acceptable.

Thanks.

Thanks for your feedback, moshe. I highly respect your insights into the drupal theme system, and I'm happy to hear that D7 has made such great strides towards running almost everything through a render() of an array. With that in mind, I agree that the pseudo drupal_alter() isn't ideal.

However, though I don't have a burning use-case, I have a feeling that the lack of a post-render step creates a hole in the system begging to be filled. Here's a patch that addresses that in a really minimalist way. It allows a preprocess/process function to add functions to a $variables['postrender'] array (I'm open to that name being changed). Consider, for example, devel_themer.module. Right now, it has to reroute all theme hooks to a devel_themer_catch_function(), which then needs to call devel_themer_theme_twin(), a near clone of theme(). Now in this case, that might not be able to go away, because part of what you're trying to do there is get meta information like which tpl file actually got found. But suppose you didn't need that, and all you wanted was the ability to log how long a theme invocation took and to add some prefix and suffix to the theme output. This patch would allow for that without needing devel_themer_theme_twin().

I understand your concern, however, that if we add this, it would need to be documented, and would add more perceived complexity to an already complex system, especially for new developers.

@moshe - Yes. I'm honored that you trust me with that responsibility. I sent you a contact message so you have my email address so we can coordinate details.

I'm working on an updated patch and comment. In the meantime, setting the status to "needs work".

As I wrote in comment #7, I'm convinced that we don't want to have a drupal_alter() (or pseudo drupal_alter()) here. However, I would like something here, but I took Moshe's desire for minimizing perceived API complexity seriously. For that reason, instead of patch #7, I'm proposing this one.

In this patch, we're just adding a 'postprocess' phase, which acts exactly like the 'preprocess' and 'process' phases, except that it runs after the theme function / template file generates the output from the arguments / variables. Because it runs after $output is generated, the first argument to the postprocess functions is $output instead of $variables. Other than that, it's conceptually identical.

The purpose of the postprocess phase is to allow core/modules/engines/themes to either adjust $output and/or do some logging. To follow up on Moshe's point, if this does get accepted, we should make sure to include in the documentation that this should not be used where #theme_wrappers can accomplish the desired result, but rather that this is for special situations. One example of a special situation is the devel_themer.module, which if this patch is accepted, can remove the clunky use of an intercept function and clone of the theme() function.

Another example of a cool feature this patch would allow is suppose you wanted to add comments to your html output, so that you know which template file is responsible for each piece of the page. Perhaps you don't want all the bells and whistles of devel_themer, but just want this commenting capability. Well, with this patch, you could create a tiny module to do this:

function templatecommenter_postprocess(&$output, $hook, $variables) {
  if ($hook !== 'page') {
    $output = 
      '<!-- Start of output by ' . $variables['template_file_used'] . ' -->' . 
      $output . 
      '<!-- End of output by ' . $variables['template_file_used'] . ' -->';
  }
}

I made sure to write this patch so that it has no added performance overhead at all. Using the theme.php test script from http://drupal.org/node/523682#comment-1969914, I got the following results:

Unpatched theme.inc:
nothing__________theme(username)_____theme(placeholder)____theme(table)
0.208 +/- 0.001___97.86 +/- 0.81______16.37 +/- 0.06________163.33 +/- 0.49

Patched theme.inc:
nothing__________theme(username)_____theme(placeholder)____theme(table)
0.205 +/- 0.005___97.89 +/- 0.29______16.20 +/- 0.08________164.16 +/- 0.56

Differences are within the noise level.

So, the main concern to consider is does this add too much complexity to the API? I think not, because we're already asking people to understand preprocess and process functions, and the concept of phases: first all the preprocess functions run, then all the process functions run, then the theme function or template runs. All we're adding here is "and then after that, the postprocess functions run". At a deeper level of understanding, it's helpful to understand what you would do in each of these phases. I think one explanation is that preprocess functions are for adjusting the "primary variables", and process functions are for adjusting "derivative variables" (for example, converting $classes from an array to a string), and that this split is so that you don't lock in a derivative variable before all of the primary variables have been locked in. Well, I think if you understand things at that level, then it's not much of a leap to say, "and then postprocess functions are for adjusting the final output".

What do you think?

Thanks, sun! I also dislike 'process' and would have preferred 'latepreprocess', but it's probably too late to make that change. I'd be fine with changing this patch to use 'postrender' instead of 'postprocess' if that makes it more clear and approachable. What are other people's thoughts on this? Is 'postrender' more clear? Given the names of 'preprocess' and 'process', would 'postrender' be less approachable than 'postprocess'? @sun, please let me know if you'd like me to make the change now, or wait for other people to chime in.

I added comment #28 to #493030: RDF #1: core RDF module. @moshe and @pwolanin, please note the last bullet point, and decide if that's a compelling enough justification for the postprocess phase.

I'm now a -1 for this. Leaving this as "postponed" rather than "by design" in case it still turns out to be necessary due to a failure to cleanup other code, but I'm hoping that other code will be cleaned up to the point that this can be set to "by design".

My main reason for being against it now is that I have more confidence in the D7 render() system's #theme_wrappers than when I submitted this patch (I'm encouraged to see #283723: Make menu_tree_output() return renderable output at RTBC status), I have more confidence that we'll manage to clean-up theme hooks that embed too much logic into the theme function (the work on theme_username() was a good start, and #572618: All theme functions should take a single argument to make preprocess sane and meaningful will enable cleanup of remaining theme functions that need logic separated from presentation), and I've been made more aware of themers being passionate about wanting control over markup and being unhappy if a module were to use a postprocess phase to take that control away from them.

I can see 3 broad types of use cases for where a postprocess phase would be useful, and in each case, there exists a better solution.

1. To be able to wrap the result of a theme function with additional markup. This is better solved with #theme_wrappers. So, wherever it seems like this is necessary, we should instead make sure we're using the render() system appropriately.

2. For a module to be able to fix the result of a theme function that made logic decisions differently than what's needed by the module. This is better solved by cleaning up theme functions to rely on preprocess functions to make any decisions that a module may need to override. In any cases where this isn't done, a module can still manipulate the theme registry to route to a different theme function. Although this is ugly, this might not be a bad thing, since the situation of a module needing to override the result of a theme function *is* ugly, and requiring the implementation of that to be ugly provides more motivation for the underlying situation to be solved by fixing the theme function.

3. For development, to be able to do things similar to, but different than what devel_themer.module does (for example, comment #11's code block and comment #26). Moshe and I agreed that I will take over maintenance of devel_themer.module, so I'll try to update it to expose an API that other modules can use to accomplish this type of stuff. No need to add a too-easy-to-abuse postprocess phase in core just to handle development-oriented use-cases.

Changing this to "by design" for reasons covered in #29. The render() system is being used almost everywhere now, so #theme_wrappers are available. #572618: All theme functions should take a single argument to make preprocess sane and meaningful has landed. Unless someone is able to pull it off, I don't think we'll have #600658: Refactor core module theme functions (templates) to separate data logic from markup decisions, enabling modules to alter the former everywhere, but wherever some other issue depends on having that proper separation for a particular theme hook, that issue can fix that theme hook. And for D8, I'll help work on making sure the correct data/presentation separation is implemented in every theme hook. So, IMO, a postprocess phase is unrealistic for D7 and will have no use case for D8.