The concept of preprocess functions is a powerful step towards the mythical ideal of separating logic from presentation. However, currently, you can only have preprocess functions for theme hooks implemented as templates, and not for theme hooks implemented as functions. This introduces an unnecessary coupling between what a module should be concerned about (preprocessing data sent to a theme implementation) and what a theme should be concerned about (whether a particular hook is implemented as a function or as a template).

Has the idea of allowing preprocess functions to run on theme hooks implemented as functions already been discussed and shot down? If so, please point me to that discussion if it's online. Otherwise, please add comments if you like this idea or if you have concerns about it, and I'll be happy to create a patch for it.

Files: 
CommentFileSizeAuthor
#40 theme.php.txt605 bytescatch
#38 theme-processors_for_function_implementations-400292-38.patch17.56 KBeffulgentsia
Unable to apply patch theme-processors_for_function_implementations-400292-38.patch
[ View ]
#32 theme-processors_for_function_implementations-400292-32.patch16.82 KBeffulgentsia
Unable to apply patch theme-processors_for_function_implementations-400292-32.patch
[ View ]
#28 theme-processors_for_function_implementations-400292-28.patch14 KBeffulgentsia
Passed: 12203 passes, 0 fails, 0 exceptions
[ View ]
#26 theme-processors_for_function_implementations-400292-26.patch16.29 KBeffulgentsia
Passed: 12233 passes, 0 fails, 0 exceptions
[ View ]
#26 test400292.zip2.8 KBeffulgentsia
#24 theme-processors_for_function_implementations-400292-4.patch14 KBeffulgentsia
Unable to apply patch theme-processors_for_function_implementations-400292-4.patch
[ View ]
#19 theme-processors_for_function_implementations-400292-3.patch11.28 KBeffulgentsia
Unable to apply patch theme-processors_for_function_implementations-400292-3.patch
[ View ]
#9 theme-processors_for_function_implementations-400292-2.patch11.36 KBeffulgentsia
Unable to apply patch theme-processors_for_function_implementations-400292-2.patch
[ View ]
#9 test400292.zip2.75 KBeffulgentsia
#6 theme-processors_for_function_implementations-400292-1.patch10.92 KBeffulgentsia
Unable to apply patch theme-processors_for_function_implementations-400292-1.patch
[ View ]
#5 theme-processors_for_function_implementations-400292-0.patch10.75 KBeffulgentsia
Unable to apply patch theme-processors_for_function_implementations-400292-0_0.patch
[ View ]

Comments

derhasi’s picture

I would support this, because this might ease the way modules can adjust the output of some elements without the need to completely overwrite the whole theming function.

Are there any plans for D7 allready?

danielb’s picture

Yes I face this problem on plenty of modules I write.

My module needs to step in and take over a theme function, just to change one little thing. In some cases I've even been able to pass it off to the original theme function (by storing theme data in a custom place during theme_registry_alter) allow themers to do their business as usual. What I've done here is effectively 'preprocess' a theme function.... but then I need to add a true preprocess for the potential template anyway.

It would be great if I could hook in before the theme function and return my own output, or preprocess the incoming data, and/or allow it to continue to the theme function/template.

effulgentsia’s picture

Assigned:Unassigned» effulgentsia

Great! I'm glad there's both interest and that no one has shot the idea down. I'll work on a patch, and will post it here when ready. With any luck, it'll get accepted into D7.

effulgentsia’s picture

Apologies for not getting to this yet. I've been busy with other things. I still plan on submitting a patch for this soon, to try to get it accepted before the D7 code freeze.

effulgentsia’s picture

Assigned:effulgentsia» Unassigned
Status:Active» Needs review
StatusFileSize
new10.75 KB
Unable to apply patch theme-processors_for_function_implementations-400292-0_0.patch
[ View ]

Here's the patch. This does the following:

1) Allows *_preprocess_HOOK and *_process_HOOK functions to get called on function-implemented theme hooks.

2) Does not, by default, setup the non-hook-specific processor functions (*_preprocess and *_process) to get called on function-implemented theme hooks. The reason for this is so that this patch does not negatively impact the performance of drupal. Many function-implemented theme hooks get called many times in a page request and need to be fast. I do not want to introduce a change that would result in adding overhead to these theme invocations by having to call the non-hook-specific *_preprocess functions. With this patch, no overhead is added by default. Overhead is only added if a module chooses to implement a specific *_preprocess_HOOK function. If a module wants non-hook-specific processor functions used for a function-implemented theme hook, it can alter the theme registry to achieve that, in which case, it would be responsible for considering performance implications of doing so.

3) To support danielb's request in comment #2, a preprocess or process function can alter the special variable, 'theme_function', which would override which function is then used as the actual implementation once all the preprocess and process functions are done adjusting the arguments. This is similar to how a template preprocess function can alter the variable 'template_file' in order to force a different template file to be used instead of the one registered for the theme hook. If the 'theme_function' wants to invoke the original registered function as part of its implementation, it can find out what that function is by calling theme_get_registry().

I believe this patch adds a very useful feature to the theming system without introducing either functional incompatibility with existing code or a performance impact unless a module or theme chooses to use the feature for a specific hook.

effulgentsia’s picture

StatusFileSize
new10.92 KB
Unable to apply patch theme-processors_for_function_implementations-400292-1.patch
[ View ]

Apologies. Here's an updated patch. The earlier one was erroneously adding non-hook-specific processors in one of the code sections. That's fixed in this patch.

merlinofchaos’s picture

No, no no no no no no.

When preprocess functions were added, they were specifically not added for functions.

Theme functions exist the way they do because some theming portions are called many times per page. Adding preprocess functions for these systems adds considerable weight. It is not a big deal when you have half a dozen calls per page, but some can be called thousands of times per page in some situations.

I therefore -1 any attempt to do this. The most important goal of theme functions is speed and efficiency.

moshe weitzman’s picture

Status:Needs review» Closed (works as designed)

sad, but true.

if there are select functions that frequently need preprocess, then lets convert them to templates.

if you really want to reopen this issue, please do so with benchmarks which suggest that this does not hurt performance.

effulgentsia’s picture

Status:Closed (works as designed)» Needs review
StatusFileSize
new2.75 KB
new11.36 KB
Unable to apply patch theme-processors_for_function_implementations-400292-2.patch
[ View ]

Please read comment #5 as to why I believe the performance overhead is minimal. I'm attaching a new patch in this comment that optimizes a bit more, and my benchmarks are based on that. I'm also attaching a zip file of a tester module that I used for these benchmarks.

My benchmarks were performed on a late 2008 unibody macbook (core 2 duo 2.0 GHz, 2GB RAM) running MAMP (PHP 5.2.6 with APC).

I did a minimal install of Drupal 7 Head, enabled the "menu" module, the "devel" module, and the "test400292" module (attached). I added the "devel" block to the left sidebar, edited the "management" menu to have all menu items within it expanded, and added a "Google" menu item with a path of "http://www.google.com" to the "management" menu. Prior to each sub-test, "empty cache" in order to refresh the theme registry, and then run the test several times, ignoring the first run after a freshly emptied cache.

Test results

Test 1 (navigate to path /test400292/1) -- this measures the time it takes to call menu_tree_output() on the management menu, a menu with 40 items in it, which results to 40 calls to theme('menu_item') and 40 calls to theme('menu_item_link').

  • With unpatched theme.inc: 4.83ms
  • With patched theme.inc: 4.86ms
  • With patched theme.inc and the test400292_preprocess_menu_item_link() function uncommented: 6.00ms
  • With unpatched theme.inc, test400292_preprocess_menu_item_link() function uncommented, and the menu_item_link entry in test400292_theme() uncommented: 10.8ms

Test 2 (navigate to path /test400292/2) -- this measures the time it takes to call a trivial theming function 100 times.

  • With unpatched theme.inc: 1.15ms
  • With patched theme.inc: 1.18ms
  • With patched theme.inc and the test400292_preprocess_test400292() function uncommented: 2.76ms
  • With unpatched theme.inc, test400292_preprocess_test400292() function uncommented, and the 'template' entry within the 'test400292' entry in test400292_theme() uncommented: 14.8ms

Interpretation of test results

  • The patch has a minimal impact on performance of theme functions when no module implements a preprocess hook for it. So, the act of committing this patch in and of itself would have virtually no performance penalty. It's true there is some impact between the first two numbers of each test (1-3%), and the reason for this is an extra call to isset() within theme.inc's theme() function. But let's put this in perspective: in the first test, it takes 15ms to get the menu data prior to calling menu_tree_output() and another 5ms to call menu_tree_output(). Is an extra 0.03ms added to this process significant? Even in the case of the second test, where we're adding 0.03ms to the time it takes to call the fastest possible theme function 100 times, in what real-world use-case is this enough of a problem to discount including a useful feature?
  • Of course, the purpose of this patch is for it to be used, at least sometimes. The difference between the 2nd and 3rd set of numbers for each test illustrates the cost of a module actually implementing a preprocess hook for a theme hook where speed is important. In Test 1, the example use-case is a desire for a module to set the "target" attribute to "_blank" for menu item links that point to external links. Doing this adds a little over 1ms to the rendering of a menu with 40 items in it. In my opinion, it should be up to the module author to decide if this is a worthwhile trade-off. In Test 2, the difference is much larger as a percentage, because the module is implementing a preprocess hook for what is otherwise an extremely fast theme hook. Again, I think it should be up to module authors to consider the performance implications of adding a preprocess step to an otherwise fast function, and in effect, doubling the time it takes to execute that theme hook.
  • I'd like to point out that module authors already have the ability to add preprocess functions to a function-implemented theme hook. They can do so by converting the theme hook to be template-implemented. That's what the 4th set of numbers in each test represent. However, this has an even greater performance cost than allowing module authors to use preprocess functions on theme hooks without needing to convert them to be template-implemented.
  • Finally, while the above demonstrates when a module wants to implement a preprocess function, it's the same for when themes want to implement a preprocess function. Right now, if a theme wants to implement a preprocess function for a function-implemented theme hook, it can do so simply by creating a tpl file for the theme hook which automatically converts the hook into a template-implemented one. This patch would allow a theme to implement a preprocess function without converting a function-implemented theme hook into a template-implemented one, and this results in better performance.

Summary

  • Trivial performance impact for committing this patch.
  • Module and theme authors need to be judicious with using preprocess functions for theme hooks that need to be fast, since executing a preprocess function obviously takes some time.
  • Where a module or theme author really wants a preprocess function for a theme hook where speed matters, they already have this ability, but need to convert the theme hook to be template-implemented. This patch allows module and theme authors to have preprocess functions where they need it while only suffering the added cost of the preprocess function used, and not needing to also suffer the cost of rendering a template file.
moshe weitzman’s picture

Thats the spirit! This is pretty compelling evidence. Thanks for keeping this issue alive. I'll see if we can get some eyeballs on it.

mattyoung’s picture

subscribe

danielb’s picture

Yeah, the thing is: I've been faced with this problem at least 3 times, and it didn't stop me from going ahead and hacking together a faux preprocess using hook_theme_registry_alter, and other developers have obviously switched to templating just so they can preprocess. Actually putting a mechanism in place for this has got to be a positive thing - otherwise developers like me have to go underground with our elaborate workarounds - and probably just cause inconsistencies for themers.
I definitly don't think the prerequisite to allowing a preprocess should be the use of a template file - where is the reasoning to that? The prerequisite of preprocessing a theme should be when it says so in the registry.

moshe weitzman’s picture

IMO, this code is RTBC. I'm, gonna ask for some themer feedback before I move this.

webchick’s picture

Issue tags:+Needs themer review

I'd also like for Earl to take another look post-benchmarks in #9. He is one of the two theme system maintainers, after all. It'd be nice to get his blessing on this approach.

I don't think you'll hear too many complaints from themers on this though; it's frightfully confusing that it doesn't work this way, from what I've seen.

But let's find out! :)

merlinofchaos’s picture

Ok, I hate to be a stick in the mud, but I don't feel 100 calls are enough to show anything. The noise from normal CPU usage will overwhelm that. Can we do more like 100,000 calls and compare the percentage differences at that point?

Jacine’s picture

Would this allow, say a module like Skinr, to access functions like theme_breadcrumb() and theme_menu_local_tasks()?

Sorry if it's a dumb question. Thanks.

moshe weitzman’s picture

@jacine - yes it would. except, we have to go into theme functions and add places for you to plug in attributes. for example, you can do much with theme_breadcrumb().

<?php
function theme_breadcrumb($breadcrumb) {
  if (!empty(
$breadcrumb)) {
    return
'<div class="breadcrumb">' . implode(' » ', $breadcrumb) . '</div>';
  }
}
?>

fixing up theme functions is a separate patch.

@merlinofchaos - thanks for being open about this. more iterations is a good idea.

Jacine’s picture

Wow. This is something I've been really been hoping for, so +1.

I'll watch this, and help fix up theme functions (separately) if this is going to happen. Thank you Moshe :)

effulgentsia’s picture

StatusFileSize
new11.28 KB
Unable to apply patch theme-processors_for_function_implementations-400292-3.patch
[ View ]

Attached is a new patch that is a minor change from the patch in comment #9. It's rerolled for latest theme.inc and I removed an unnecessary "$function = $info['function']" from the prior patch, resulting in slightly improved performance numbers:

From test400292.module attached in comment #9, the first line of test400292_test1_page() and test400292_test2_page() sets the number of iterations to average over. The below benchmarks were with $n = 30000.

Test 1 (each number represents time it takes to call menu_tree_output() averaged over 30,000 iterations; for first two sub-tests, four separate runs of 30,000 iterations were made, and the bold number is the average of these four):

  • With unpatched theme.inc: 4.809, 4.799, 4.805, 4.796 = 4.802ms
  • With patched theme.inc: 4.818, 4.823, 4.815, 4.819 = 4.819 (+0.35%)
  • With patched theme.inc and the test400292_preprocess_menu_item_link() function uncommented: 5.99 (+25%)

Test 2 (each number represents time it takes to call theme('test400292', '.') 100 times averaged over 30,000 iterations; for first two sub-tests, four separate runs of 30,000 iterations were made, and the bold number is the average of these four):

  • With unpatched theme.inc: 1.128, 1.125, 1.120, 1.128 = 1.125ms
  • With patched theme.inc: 1.150, 1.146, 1.140, 1.143 = 1.145 (+1.78%)
  • With patched theme.inc and the test400292_preprocess_test400292() function uncommented: 2.89 (+157%)

I did not re-run the sub-tests for converting to a template-implemention, because with my installation of APC, I get weird effects when invoking template-implemented theme hooks over that many iterations, and benchmarks without using APC are meaningless since anyone who cares about performance uses APC or other opcode cache. However, if you compare the numbers in this comment with the ones in comment #9, I think there's evidence that the numbers from the original test are at least roughly correct for interpreting that final sub-test.

Interpreting the results from the two tests is straightforward: in Test 1, the difference between the first two sub-tests is the difference that results from 81 invocations of theme() (40 calls to theme('menu_item_link'), 40 calls to theme('menu_item') and 1 call to theme('menu_tree')). So, 0.017ms / 81 = .21 microseconds per theme invocation, and this results primarily from the extra call to isset() needed in the patched theme.inc. Test 2 is a difference of 0.020 ms divided over 100 invocations of theme(), so essentially the same result of 0.2 microseconds per invocation. In Test 1, the difference between sub-tests 2 and 3 is the difference that results from the use of a preprocess function for 40 invocations (only menu_item_link gets a preprocess function in that test) = 1.17ms / 40 = 29 microseconds per preprocess invocation. In test 2, we have an extra 1.74ms / 100 invocations = 17 microseconds per preprocess invocation. The 2 preprocess functions have different implementations, so no surprise that the one that does more takes more time.

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

Well, I'm pretty satisfied with this. Thanks for the re-benchmarking.

Dries’s picture

I'd like to review this still.

In general, I think this is a good thing because it provides consistency and predictability. I consider this to be a significant DX improvement. I still want to review this patch in more detail, because the alter stuff confiscates some of that. Only looked at it for a couple minutes.

I glanced over the patch and what seems to be missing is some high-level documentation. Is there an .api.php file for theme stuff? It feels like we're missing some help text somewhere that explains the big picture. I also think it would be great to have an example in core/documentation.

I think this might be worth updating CHANGELOG.txt.

I'm pretty sure this wouldn't pass a @sun review. ;-)

Dries’s picture

I've reviewed the code and from what I can see this looks good. In addition to what I said above, I'd still to get merlinofchaos' take on this.

PS: I'm always surprised designers can actually make sense of this, because it always requires me to read very carefully. It's pretty dense and technical -- it always feels like too complex to me.

mattyoung’s picture

How about allowing preprocess function suggest alternate function or template like the template version currently work if this does not add any overhead.

effulgentsia’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new14 KB
Unable to apply patch theme-processors_for_function_implementations-400292-4.patch
[ View ]

Thank you, everyone, for all the wonderful feedback.

@mattyoung: prior patch already included mechanism for preprocess function to suggest alternate theme function to use. However, this patch now also supports a way to specify an array of suggestions, for greater symmetry with how template-implemented hooks work.

This patch also includes:

  • An entry in changelog.txt
  • Better phpdoc comments for theme()
  • Minor optimization to call the preprocess functions without using the slow call_user_func_array(). Doesn't affect first two sub-tests of the benchmarks in comment #19, but provides a small boost for the third sub-tests, where preprocess functions are actually used.

@dries: I agree that a high-level description of the theming system is needed, but I think http://drupal.org/theme-guide/6 is pretty good. Separate issue, perhaps, to flush out http://drupal.org/node/550722? If an issue gets started for that, comments as to what is inadequate about http://drupal.org/theme-guide/6 would be helpful. Does my addition to the phpdoc comment for theme() and entry in changelog.txt adequately address the documentation needs of what's new as a result of this patch? Should I add an entry to http://drupal.org/update/theme/6/7 before or after this patch gets committed?

yched’s picture

As a side note, this possibly lets us get rid of the 'sanitize' step in Field API, which was introduced in CCK D6 to keep data sanitization out of the formatter's theme functions.

effulgentsia’s picture

Title:Allow preprocess functions for theme hooks implemented as functions» Allow preprocess functions for theme hooks implemented as functions and drupal_alter() on all theme hooks
StatusFileSize
new2.8 KB
new16.29 KB
Passed: 12233 passes, 0 fails, 0 exceptions
[ View ]

Normally I would add this as a separate issue with a separate patch once this one got accepted, but we're getting close to code freeze and I want to make the best use of merlinofchaos's limited time and attention, so I'm taking the risk of upsetting someone by expanding this issue to also include the desire to let modules alter the output of a theme function AFTER the rendering step.

Ideally, I'd want the last line of theme() to be drupal_alter('theme_' . $hook, $output, $variables, $hook) prior to returning $output. However, this would be slow. I'm attaching a patch that adds the equivalent functionality (in addition to the functionality from the prior patch), but in a more optimized way.

Benchmark from running "Test 2" (same as comment #19, calling a fast theming function 100 times, averaged over 30,000 iterations):

  • With unpatched theme.inc: 1.115, 1.124, 1.126, 1.124 = 1.122ms
  • With patched theme.inc: 1.155, 1.152, 1.156, 1.155 = 1.155ms (+2.94%)
  • With patched theme.inc and test400292_preprocess_test400292() function uncommented: 2.68ms (+139%)
  • With patched theme.inc and test400292_theme_test400292_alter() function uncommented: 2.80ms (+150%)
  • With patched theme.inc and test400292_preprocess_test400292() and test400292_theme_test400292_alter() functions uncommented: 3.19ms (+184%)

Percentages are all relative to unpatched theme.inc.

Same conclusion as before: minor overhead for having this functionality. More significant overhead for using it, but the decision whether to use it and take the performance hit should be up to the module author.

I hope others find this useful, and that given the circumstances, rolling an extra feature into the same issue wasn't too out of line.

moshe weitzman’s picture

@effulgentsia - i don't think thats related enough to jam into this issue. i understand that the time is tight, but even so it is not a god match. also, i don't think a post theme step is needed. you should use an alter hook to add a #theme_wrappers item after the element that you care about. then your themed output will be siting in the $element['children'] property and you can fiddle from there. if you don't like that, then change #theme to your own theme function which calls the original theme function and then calls your own function. finally, #post_render can be useful for these situations.

i hope you will put your prior patch up again since we ned testbot to keep testing it in order for it to garner reviews and support.

effulgentsia’s picture

Title:Allow preprocess functions for theme hooks implemented as functions and drupal_alter() on all theme hooks» Allow preprocess functions for theme hooks implemented as functions
StatusFileSize
new14 KB
Passed: 12203 passes, 0 fails, 0 exceptions
[ View ]

Fair enough. Not every use-case of wanting to alter the output of a theme function is run via drupal_render(), so I'll still lobby for the pseudo drupal_alter() at the end of theme(), but I'll accept that that needs to be a separate issue, and so will try my luck with timing. Attached patch is identical to the one in comment #24, just renumbered to match the comment number.

quicksketch’s picture

Status:Needs review» Needs work

- The docs for "theme_function(s)" is 90% identical to "template_file(s)". Surely we could consolidate these two rather than copy/pasting the exact description twice.

- Lots of this sort of action going on (extra whitespace at end of lines):

-
+     

Any whitespace at the end of any line should be removed, even in PHPdoc new lines. i.e. this is also incorrect:

+ * blah blah.
+ * 
+ * 1. blah blah...

- Code comments are being wrapped at completely arbitrary points. These should always wrap at 80 characters (turn on the right-margin or gutter indicator in your text editor).

- Similar to module_invoke(), I think this would be a place where we should be using if (function_exists($function) || drupal_function_exists($function)) { for all function calls. See #528896: Small optimization to module_invoke_all().

- Make sure comments are full sentences:

// We don't want a poorly behaved process function changing $hook

- $template_phases should probably be renamed to just $phase since it now applies to both templates and functions.

- $hooks[$hook]['_no processors'] is the strangest looking array key I've seen in a while. Can we just make this "_no_processors", even though it's just a private key?

sun’s picture

Status:Needs work» Needs review

Nice, we all always asked for that.

Though, I'm with the others, some more benchmark data would be good. Hint: It seems like this patch could be easily ported, so we could even see live benchmarks of a moderately large site.

The code, however, needs a bit of work. The following are the most obvious things. Also, some comments are a bit too verbose, while there are other parts in the added code that don't have any remarks about what's happening at all.

+++ CHANGELOG.txt Sun Aug 16 12:56:02 PDT 2009
@@ -101,6 +101,12 @@
+    * Variable preprocessing of theme hooks prior to template rendering now goes
+      through two phases: a 'preprocess' phase and a new 'process' phase. See
...
+    * Theme hooks implemented as functions (rather than as templates) can now
+      also have preprocess (and process) functions. See
+++ includes/theme.inc Sun Aug 16 13:07:55 PDT 2009
@@ -365,49 +365,53 @@
+     
+      // Allow variable processors for all theming hooks, whether the hook is
+      // implemented as a template or as a function.
@@ -664,14 +667,43 @@
- *
+ *

Trailing white-space here.

+++ includes/theme.inc Sun Aug 16 13:07:55 PDT 2009
+++ includes/theme.inc Sun Aug 16 13:07:55 PDT 2009
@@ -365,49 +365,53 @@

Can you please re-roll with diff -upN ?

+++ includes/theme.inc Sun Aug 16 13:07:55 PDT 2009
@@ -365,49 +365,53 @@
+            // Add all modules so they can intervene with their own variable processors. This allows them
+            // to provide variable processors even if they are not the owner of the current hook.
...
+            // Theme engines get an extra set that come before the normally named variable processors.
...
+            // The theme engine registers on behalf of the theme using the theme's name.
...
+            // This applies when the theme manually registers their own variable processors.
...
+            // Only use non-hook-specific variable processors for theming hooks implemented as templates.

(and possibly elsewhere) Comments need to wrap at 80 chars.

+++ includes/theme.inc Sun Aug 16 13:07:55 PDT 2009
@@ -365,49 +365,53 @@
+            // See comment of theme() for why.
@@ -418,13 +422,15 @@
+          // See comment of theme() for why.

Use a proper PHPDoc directive here:

@see theme()

+++ includes/theme.inc Sun Aug 16 13:07:55 PDT 2009
@@ -365,49 +365,53 @@
+        // Check for the override flag and prevent the cached variable processors from being used.
+        // This allows themes or theme engines to remove variable processors set earlier in the registry build.
+        if (!empty($info['override ' . $phase_key])) {
+          // Flag not needed inside the registry.
+          unset($result[$hook]['override ' . $phase_key]);
+        }

I don't understand this part from the comments. What sets this flag, where, and when?

+++ includes/theme.inc Sun Aug 16 13:07:55 PDT 2009
@@ -664,14 +667,43 @@
+ * above list. There are two reason why the non-hook-specific preprocess

s/reason/reasons/

+++ includes/theme.inc Sun Aug 16 13:07:55 PDT 2009
@@ -664,14 +667,43 @@
+ * 1. Function-implemented theme hooks need to be fast, and calling the
...
+ * 2. Function-implemented theme hooks can only make use of variables

Please use a regular PHPDoc list here:

- Foo...

- Bar...

+++ includes/theme.inc Sun Aug 16 13:07:55 PDT 2009
@@ -664,14 +667,43 @@
+ *   files, one at a time, and use the first one that exists. If none exist,

s/exist/exists/

13 days to code freeze. Better review yourself.

quicksketch’s picture

Status:Needs review» Needs work

To clarify my last comment, $hooks[$hook]['_no processors'] should have the space replaced with an underscore.

effulgentsia’s picture

Status:Needs work» Needs review
StatusFileSize
new16.82 KB
Unable to apply patch theme-processors_for_function_implementations-400292-32.patch
[ View ]

With sun's and quicksketch's feedback, except:

  • Consolidating PHPDoc explanation of template_file, template_files, theme_function, theme_functions: Any ideas on how to shorten it without making it harder for the intended audience to understand? Is the intended audience advanced developers, beginner developers, or designers?
  • function_exists() optimization: I agree that it would be nice, but the issue is being debated on #523682: Small optimization to theme(). Perhaps we should let that issue proceed without forcing it in this patch.
  • sun's comment about the comment for the override flag being unclear: it's a pre-existing condition. This patch just moves where that comment is.
effulgentsia’s picture

I updated the patch on #523682: Small optimization to theme(). If that patch gets committed to HEAD first, I'll re-roll this one to include the same optimizations. If this patch gets committed to HEAD first, I'll re-roll that patch. I'd like to point out that the simple optimization reflected in that patch results in a larger performance gain than this patch results in a performance penalty. So, the net effect of committing both patches to HEAD will be a minor performance improvement over HEAD as it is currently.

effulgentsia’s picture

Since Moshe understandably rejected my attempt to include drupal_alter() functionality with this patch (comment #27), I started a separate issue for that: #553038: Add postprocess phase for theme(). For anyone interested, please comment on it. Thanks.

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

Thanks for persisting here. We are RTBC again.

Dries’s picture

I'd still like to get merlinofchaos' (fresh) take on this. Not a hard requirement but let's wait a bit for him to chime in.

merlinofchaos’s picture

My take is that the benchmarks are the key. We need to be sure that the benchmarks are valid and that the overhead added in the minimal case is worth the benefit. I'm not enough of a benchmarks expert to really speculate on the validity of them (maybe catch can?) but I will say that as long as core committers are satisfied with the benchmarks my objections are moot.

effulgentsia’s picture

StatusFileSize
new17.56 KB
Unable to apply patch theme-processors_for_function_implementations-400292-38.patch
[ View ]

Small change in this patch. It doesn't affect the functionality or performance of the minimal case. However, there's an active discussion on #497118: Remove the registry (for functions) about the registry, and while I don't fully understand the intricacies of what's being discussed, I did get to thinking that one potential problem with the prior patch is that it adds a lot of empty array entries in the theme registry that wasn't there before ('preprocess functions' and 'process functions' keys for each function-implemented theme hook). This patch cleans that up, resulting in the same size registry as current HEAD ('preprocess functions' and 'process functions' entries only when used).

catch’s picture

10,000 iterations on theme('username'), APC on, xdebug off. nothing means an empty loop just to make sure nothing weird is going on while running benchmarks.

HEAD:
nothing: 0.00215005874634 seconds
theme: 37.8369731903 seconds

Patch:

nothing: 0.00350117683411 seconds
theme: 38.6609568596 seconds

Since adding this has negligible impact (and we've got something like 10-15% improvements on the other issue which more than outweighs it), and it's adding a nice feature, and the feature it adds, allows you to do things faster and more cleanly than registering a template etc., happily leaving this RTBC. Note I've not done a code review, just worked up these quick benchmarks. Script attached in case anyone else needs to run it - just drop it into your drupal root, hit theme.php in your browser, note down the numbers, then try again with the patch applied: My only concern here is that modules abuse this to do something stupid, but that's quite possible whatever we do.

catch’s picture

StatusFileSize
new605 bytes
effulgentsia’s picture

Thanks, catch. I ran your theme.php, but 10 times for each test to gather statistics: Here's the results from my machine:

Applied HEAD, emptied cache (using devel module), refreshed theme.php 10 times
Run:___________nothing_____________________theme
1______________0.00215291976929 seconds____26.9176940918 seconds
2______________0.00214409828186 seconds____26.6899728775 seconds
3______________0.0020592212677 seconds_____26.6998519897 seconds
4______________0.00207591056824 seconds____26.6978750229 seconds
5______________0.00215888023376 seconds____26.9298269749 seconds
6______________0.00206995010376 seconds____26.6365301609 seconds
7______________0.00216102600098 seconds____26.7416698933 seconds
8______________0.00211310386658 seconds____26.744494915 seconds
9______________0.00209903717041 seconds____27.05220294 seconds
10_____________0.00216388702393 seconds____26.6263570786 seconds
avg +/- stdev____0.00212 +/- 0.00004_________26.77 +/- 0.14

Applied patch 38, emptied cache (using devel module), refreshed theme.php 10 times
Run:___________nothing_____________________theme
1______________0.00214099884033 seconds____28.0407369137 seconds
2______________0.00204586982727 seconds____26.8423268795 seconds
3______________0.00201511383057 seconds____26.6162879467 seconds
4______________0.00216507911682 seconds____26.6972930431 seconds
5______________0.00217700004578 seconds____26.9850349426 seconds
6______________0.00225687026978 seconds____26.8219690323 seconds
7______________0.0021059513092 seconds_____27.0208730698 seconds
8______________0.00218486785889 seconds____27.0993590355 seconds
9______________0.00211405754089 seconds____27.1034128666 seconds
10_____________0.00212597846985 seconds____26.5691199303 seconds
avg +/- stdev____0.00213 +/- 0.00007__________26.97 +/- 0.42

webchick’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs Documentation

Wow, thanks a lot for your thorough benchmarks, catch and effulgentsia!

Given that merlinofchaos's main concern was performance, and our resident performance expert thinks this is a negligible increase, AND given that this solves a major DrupalWTF for themers and would thus be a "killer feature" for D7, I went ahead and committed this.

Please edit the theme upgrade guide at http://drupal.org/update/theme/6/7 to reflect these changes. Thanks!

effulgentsia’s picture

Status:Needs work» Reviewed & tested by the community

Thanks, webchick!

I don't have access to edit that documentation page (probably because it's locked to admins only), and I don't meet the criteria of having "helped out for a little while", so I'll refrain from applying to be an admin. However, in case this helps someone who does have the appropriate privileges, feel free to use this as a starting point:

After #12 (HTML classes generated through a variable), add:

Title:
Variable process functions can now be used for all theming hooks

Body:
(issue) In Drupal 6, preprocess functions could only be used for theming hooks implemented as templates. In Drupal 7, preprocess and process functions can be used for plain theme functions as well. For example, your theme can convert all menu item links that start with "http:" or "https:" (as opposed to ones that refer to an internal drupal path) to open in a new browser tab with this code:

function THEMENAME_preprocess_menu_item_link(&$variables) {
  if (
    substr($variables['item']['href'], 0, 5) == 'http:' ||
    substr($variables['item']['href'], 0, 6) == 'https:'
  ) {
    $variables['item']['localized_options']['attributes']['target'] = '_blank';
  }
}

Keep in mind that doing this slows down the execution of the theme function by a little bit, so especially for theme functions that get called a lot, keep an eye on your website's performance when adding extra process functions.

The non-hook-specific process functions are reserved for templates only, and are not used for plain theme functions. (Todo: link to the Drupal 7 version of http://drupal.org/node/223430 when it's available, which can explain this distinction in more detail. For now, http://api.drupal.org/api/function/theme/7 includes the rationale for this.)

effulgentsia’s picture

This is redundant with comment #34, but now that this has been committed, for anyone interested in discussing, promoting, or demoting what I believe to be another nice feature to add to the theme system, please check out #553038: Add postprocess phase for theme(). Thanks.

effulgentsia’s picture

Status:Reviewed & tested by the community» Needs work

Apologies. Changing the status in comment #43 was not intentional.

moshe weitzman’s picture

@effulgentsia - i gave you the 'docs maintainer' role on drupal.org. please use your new powers wisely ... and update the theme guide for this issue.

effulgentsia’s picture

Status:Fixed» Closed (fixed)
Issue tags:-Needs Documentation, -Needs themer review

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