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.
Comments
Comment #1
derhasi CreditAttribution: derhasi commentedI 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?
Comment #2
danielb CreditAttribution: danielb commentedYes 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.
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedGreat! 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.
Comment #4
effulgentsia CreditAttribution: effulgentsia commentedApologies 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.
Comment #5
effulgentsia CreditAttribution: effulgentsia commentedHere'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.
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedApologies. 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.
Comment #7
merlinofchaos CreditAttribution: merlinofchaos commentedNo, 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.
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedsad, 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.
Comment #9
effulgentsia CreditAttribution: effulgentsia commentedPlease 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').
Test 2 (navigate to path /test400292/2) -- this measures the time it takes to call a trivial theming function 100 times.
Interpretation of test results
Summary
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commentedThats the spirit! This is pretty compelling evidence. Thanks for keeping this issue alive. I'll see if we can get some eyeballs on it.
Comment #11
mattyoung CreditAttribution: mattyoung commentedsubscribe
Comment #12
danielb CreditAttribution: danielb commentedYeah, 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.
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedIMO, this code is RTBC. I'm, gonna ask for some themer feedback before I move this.
Comment #14
webchickI'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! :)
Comment #15
merlinofchaos CreditAttribution: merlinofchaos commentedOk, 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?
Comment #16
JacineWould 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.
Comment #17
moshe weitzman CreditAttribution: moshe weitzman commented@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().
fixing up theme functions is a separate patch.
@merlinofchaos - thanks for being open about this. more iterations is a good idea.
Comment #18
JacineWow. 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 :)
Comment #19
effulgentsia CreditAttribution: effulgentsia commentedAttached 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):
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):
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.
Comment #20
moshe weitzman CreditAttribution: moshe weitzman commentedWell, I'm pretty satisfied with this. Thanks for the re-benchmarking.
Comment #21
Dries CreditAttribution: Dries commentedI'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. ;-)
Comment #22
Dries CreditAttribution: Dries commentedI'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.
Comment #23
mattyoung CreditAttribution: mattyoung commentedHow about allowing preprocess function suggest alternate function or template like the template version currently work if this does not add any overhead.
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedThank 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:
@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?
Comment #25
yched CreditAttribution: yched commentedAs 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.
Comment #26
effulgentsia CreditAttribution: effulgentsia commentedNormally 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):
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.
Comment #27
moshe weitzman CreditAttribution: moshe weitzman commented@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.
Comment #28
effulgentsia CreditAttribution: effulgentsia commentedFair 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.
Comment #29
quicksketch- 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:
- 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:
- $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?Comment #30
sunNice, 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.
Trailing white-space here.
Can you please re-roll with diff -upN ?
(and possibly elsewhere) Comments need to wrap at 80 chars.
Use a proper PHPDoc directive here:
@see theme()
I don't understand this part from the comments. What sets this flag, where, and when?
s/reason/reasons/
Please use a regular PHPDoc list here:
- Foo...
- Bar...
s/exist/exists/
13 days to code freeze. Better review yourself.
Comment #31
quicksketchTo clarify my last comment,
$hooks[$hook]['_no processors']
should have the space replaced with an underscore.Comment #32
effulgentsia CreditAttribution: effulgentsia commentedWith sun's and quicksketch's feedback, except:
Comment #33
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #34
effulgentsia CreditAttribution: effulgentsia commentedSince 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.
Comment #35
moshe weitzman CreditAttribution: moshe weitzman commentedThanks for persisting here. We are RTBC again.
Comment #36
Dries CreditAttribution: Dries commentedI'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.
Comment #37
merlinofchaos CreditAttribution: merlinofchaos commentedMy 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.
Comment #38
effulgentsia CreditAttribution: effulgentsia commentedSmall 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).
Comment #39
catch10,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.
Comment #40
catchComment #41
effulgentsia CreditAttribution: effulgentsia commentedThanks, 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
Comment #42
webchickWow, 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!
Comment #43
effulgentsia CreditAttribution: effulgentsia commentedThanks, 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:
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.)
Comment #44
effulgentsia CreditAttribution: effulgentsia commentedThis 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.
Comment #45
effulgentsia CreditAttribution: effulgentsia commentedApologies. Changing the status in comment #43 was not intentional.
Comment #46
moshe weitzman CreditAttribution: moshe weitzman commented@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.
Comment #47
effulgentsia CreditAttribution: effulgentsia commentedhttp://drupal.org/update/theme/6/7#variables-processors-for-all-theme-hooks