All theme functions should take a single argument to make preprocess sane and meaningful

Coming from a themer... a big ++1

http://drupal.org/node/572620 marked duplicate. Here is what I wrote there:

In a recent patch we added preprocess functions for theme functions.

Unfortunately, theme functions received a fixed list of variables. All a preprocess function can do is modify them. This has 2 problems: 1) new variables cannot be added, and 2) any changes to existing variables destroys data. Both of these turn this into a very hard to use DX WTF. For an example of this, see the discussion on http://drupal.org/node/565792

My proposed solution is simple: All theme functions receive exactly one argument, and that argument is the $variables array that preprocess functions get fed. It does mean that there is an API shift on existing theme functions, but I think this is an important one:

1) Theme functions are now used MUCH less than templates and
2) It allows theme functions to make proper use of preprocess where it makes sense.

+1,030,458,203,949,205

a) theme() is one of the most slowing down functions in Drupal core.

b) This change would finally bring total consistency to theme functions and theme templates.

c) It just makes sense.

I am a themer that is in support of this.

subscribe

+1 for that, so we bring consistency to themes/templates and "named parameters" -> forget about the ordering.. !

makes sense to me

This is critical for proper RDF support, an API clean-up (since we introduced preprocess functions for theme functions not using templates), and generally solves a DX/themer WTF.

It would be really nice if people could avoid useless reviews that basically say nothing more than "+1". Explain why you support a change, in what specific ways this would make your life easier, but most importantly why such a change is justified after an 18-month code thaw, and after a 1-week extension to code freeze. So far I only see a couple replies that attempt to do so, and I'm not sure I'm convinced.

Since performance improvements are still a go during code slush, if you can show this dramatic API change is noticeably faster, then maybe. But if it's not, then it might make more sense to roll back the preprocess theme functions patch, although I'd really hate to do so.

Btw, I asked merlinofchaos about the justification for trying to push this after freeze, and he pointed out that it's a logical follow-up patch to the one that made theme functions able to be preprocessed. Since this change didn't go in until a couple of weeks ago, and since the theme_username() patch didn't really test the limits of this until just now, it could fall under the things allowed during "slush." It's still a pretty massive change, though. I'd check w/ Dries and see what he thinks.

We definitely have an argument that the preprocess functions patch wasn't fully thought through, in terms of how it would affect DX. Perhaps this is my fault, as I was so busy on other stuff that as the only person with real objections to that patch, I didn't feel like standing up and being the guy blocking the patch, and so I put fairly minimal effort into it.

That said, the DX WTF here is mighty, I think, so either a rollback should happen, or this. The problem is, 'this' can only happen if there is a reasonable likelihood of it making it in. The actual patch is likely to be tedious to create.

I think this is a great idea and am also tagging it with "security improvements", because prior to the preprocess for functions patch, we had a situation where the rule was "preprocess functions for templates are responsible for sanitizing variables", but "arguments to theme functions are sometimes sanitized by the calling code and sometimes not, in which case the theme function is responsible for sanitizing it". Which also meant that themers overriding theme functions were responsible for security but without clear knowledge of what needs sanitization and what doesn't without inspecting the code. This problem became more evident as we were working on the theme_username() refactoring for RDF. The combination of preprocess for functions and passing $variables to the theme function opens the door to never needing a template file or theme function having to do its own sanitization, but instead can receive properly sanitized variables.

Later this week, I can help work towards a patch on this.

Reasons this would be a performance improvement:

call_user_func_array() is slow, just by itself it can take up 1-2% of PHP execution time which we'd simply remove here.All of theme() internally takes anything from 3-5% of page execution time depending on the page - so if we're able to simplify the internals more due to this change, could be nice.

All attempts to optimize theme() by removing call_user_func_array() without an API change have fallen down on having to try to re-implement it using count($args) or on the overhead of passing extra NULL arguments to functions. See #329012: Remove call_user_func_array() from ModuleHandler::invoke() + invokeAll() #471326: call_user_func_array is slow #523682: Small optimization to theme() for background possibly others). So while 1-3% isn't enough reason to change the API by itself, since we have other compelling reasons to do so, it'd be a nice bonus.

Also, as more things get converted to fields, we'll see even more theme() calls. If we can make theme() itself a bit cheaper, that'd help with some of the PHP overhead we're introducing everywhere else. And I think if we use $function, it'll be possible to see what's being called within theme() and how long each one is taking, currently xdebug just shows call_user_func_array() - which would help a lot with #438570: Field API conversion leads to lots of expensive function calls from a PX (profiler experience :p) point of view.

Introducing a new tag for feature freeze: API clean-up.

Ok, confirmed with Dries that any of these sort of "API clean-up" patches are on-topic for code slush. So I guess go nuts. :P

But let's make sure to get some before/after benchmarks on this. We definitely do not want to make Drupal slower.

Dragging and dropping a tpl.php file into your theme and hacking it to bits is several orders of magnitude less taxing on themers than learning enough PHP to be dangerous, figuring out what a template.php file is, learning about the theme function override system, getting your function naming conventions right, etc.

AFAIK in D6 we selectively converted only the tpl.php files that we figured would be most commonly overridden, but Earl would need to confirm that.

I like the proposal because it seems to map with #373201: Enhance drupal_render() themeing. Return renderable array on tracker page.

$node->content['my_extras'] = array(
  '#theme' => 'image',
  '#path' => 'http://foo.com/test.jpg',
  '#title' => t('A title'),
  '#attributes' => array('class' => 'external-image'),
  '#getsize' => FALSE,
  '#weight' => 5,
);

During the template conversion, we went for "Everything, minus stuff that's used a lot, minus stuff that's too complex to be made into a template (like theme('table'))". So it's possible there are some smallish, rarely used items that are templates. I would not, however, consider converting them back at this point, unless there is a really tangible benefit to doing so.

This patch is massive and the errors are all coming up where $variables has replaced something in the function call. The variables being replaced aren't extracting. So like for instance the variable $tips is undeclared in filter.pages.inc where the function call
function theme_filter_tips($tips, $long = FALSE) {
has been replaced by:
function theme_filter_tips($variables) {

that is just one example.

One thing I was able to fix was in theme_username where you can set elseif to check if the $object->attributes, etc. are actually there and not empty. Of course they may just be empty due to the same thing with $variables. So instead the theme_username function could look something like the following:

function theme_username($variables) {
  $object = $variables['object'];
  if (isset($object->link_path)) {
    // We have a link path, so we should generate a link using l().
    // Additional classes may be added as array elements like
    // $object->link_options['attributes']['class'][] = 'myclass';
    $output = l($object->name . $object->extra, $object->link_path, $object->link_options);
  }
  elseif (isset($object->attributes)) {
    // Modules may have added important attributes so they must be included
    // in the output. Additional classes may be added as array elements like
    // $object->attributes['class'][] = 'myclass';
	  $output = '<span' . drupal_attributes($object->attributes) . '>' . $object->name . $object->extra . '</span>';
  }
  elseif (isset($object->extra)) {
      $output = '<span>' . $object->name . $object->extra . '</span>';
  }
  elseif (isset($object->name)) {
      $output = '<span>' . $object->name . '</span>';
  }
  else {
      $output = '<span></span>';
  }
  return $output;
}

Forgot to say, I'm not intimately familiar with this, but have done a bit of work with the realname module and am really interested in getting this in core, so whatever I can do to help this process let me know.

It looks like a bug cause if you apply patch then you get the following:

Notice: Undefined property: stdClass::$attributes in theme_username() (line 1975 of C:\drive\xampplite\htdocs\drupal7\includes\theme.inc).
Recoverable fatal error: Argument 1 passed to drupal_attributes() must be an array, null given, called in C:\drive\xampplite\htdocs\drupal7\includes\theme.inc on line 1975 and defined in drupal_attributes() (line 2306 of C:\drive\xampplite\htdocs\drupal7\includes\common.inc).

So I will report the same to the thread you linked to.

Why make a special exception for functions that take a single array parameter? Here's a patch that doesn't do that. Still needs quite a bit of work, but setting it to "needs review" to see what testbot thinks.

Testbot is slow tonight ;-)
@31 -- i think we are still at the point where we expect this patch to blow up drupal. Actually we're still at getting installation to work -- though i am interested in what testbot has to say about #32.

ok testbot, how about now?

HI FIVE! WE GOT INSTALL.

I'm done with this for tonight. Nick, if you want to sweep the failed tests, go for it. Otherwise, I'll do so tomorrow.

Sorry. Status change was not intentional.

Cleaned up test failures (all due to incorrect hook_theme() entries for 'arguments'). If testbot passes all tests, status should still be set back to "needs work".

theme() is a special case, so i am good with this patch.

But in general, the movement to using associative arrays instead of function params is to be avoided. function params are a much better way of documenting the inputs of your function, they are such a good way, that many tools build upon them such as IDE argument tips and so on. We have tons of opaque (from an IDE point of view) arguments in Drupal like $form_state and the $options array in url(). Lets not go crazy here.

By request of #40

So, now we're on to the performance question. This patch modifies theme_username() and its preprocess functions and theme_table(). I'm also attaching a theme.php script that you can place into drupal root and run, which benchmarks theme('username'), theme('placeholder'), and theme('table'). It executes each one in 10 sets of 1000 each (10,000 total) and reports the time it takes each invocation in microseconds (i.e., 100 microseconds means 10,000 invocations took 1 second). The relative number comparisons are what's important, but for anyone wanting to double-check my numbers, mine were calculated on a late 2008 unibody Macbook (Core 2 Duo, 2.0GHz) running PHP 5.2.6.

____________________________HEAD__________________patch43____________
theme('placeholder')___________16.82 +/- 0.08___________18.74 +/- 0.16______
theme('table')________________164.11 +/- 0.38__________166.08 +/- 0.59_____
theme('username')____________174.22 +/- 1.11__________147.49 +/- 0.40_____

So, performance so far is marginally worse for placeholder and table, and most likely all other theme hooks. Performance is better for username, because I was able to refactor its preprocess function to use $variables instead of sticking variables onto $object, and it's much faster to stick variables into an associative array than an object.

Note, performance of theme('table') was 10us worse with patch 40 because extract() is slow. So we'll definitely want to replace all instances of extract() with assignments as I did in patch 43 for theme_table().

It's sad to see performance marginally worse rather than better. Even though we got rid of the call_user_func_array(), we added the looping over $info['arguments'] in order to convert a numerically indexed $args into a string indexed $variables. We should be able to get better performance by changing calling code to pass in a string indexed $variables into theme() (as suggested in comment #23). In other words, replace calling code from:

theme('table', $header, $rows, $attributes, $caption, $colgroups, $sticky);

To:

theme('table', array('header' => $header, 'rows' => $rows, 'attributes' => $attributes, 'caption' => $caption, 'colgroups' => $colgroups, 'sticky' => $sticky));

So, do we want to do that as part of this issue in order to not introduce a performance slow-down relative to current HEAD? Or, do we want to leave that for a separate issue?

Note, the original motivation for this issue wasn't to improve performance. It was to support less ugly preprocess functions for things like theme_username(). However, webchick indicated in comment #15 that a performance slow-down resulting from this could be a blocker for it.

We definitely do not want core to be made any slower, no. In fact, I'm more inclined to roll back the preprocess for theme functions patch if the choice is between a further slowdown in D7 and a new DX improvement, no matter how much I like it. :( We need to be fixing performance problems at this stage, not adding more of them.

Let's discuss it more, though... maybe there's a way to make this shortfall up some other way, or a way to optimize what's being done here.

Grepping for "function theme_" and subtracting

- #type handlers
- #theme_wrappers
- form #theme functions
- field formatters
- internal Theme API functions

which all use a single argument already, I get ~78 theme functions to tackle (see attached text file). Actually, I expected much more. That should be doable - if not even in one fell swoop, then definitely using git/bzr.

I'm confident that we can get better performance by changing code that invokes theme() to pass in a string-indexed $variables as the 2nd argument (comment #43). Sounds like this needs to be part of this issue rather than dealt with separately. I'm fine with that. Assuming it results in desired performance gains, does anyone object to this being done? It's an API change that in my opinion is an improvement, but if anyone thinks it's a bad idea, now's your chance to say so.

Unfortunately, I'm not available to work on this for about a week. If someone else wants to run with it, please do so. Otherwise, I'll continue with it when I'm back.

subscribing

.

Thanks for the re-roll. I'm starting on #46.

#43/#46 implemented. Still needs cleanup, benchmarking, and optimizations, but I want to get past regression issues first. CNR for testbot.

damn typo. how about now, testbot?

All temporary uses of extract() cleaned up and some minor bug fixes. CNR for testbot. If all tests pass, next step is benchmarking.

Please see comment #43 regarding these simple benchmark tests. Attached are 2 benchmarking scripts. "theme_head.txt" should be renamed to have a .php extension, placed in the root folder of an installation of HEAD, and then run. "theme_patched.txt" should be renamed to have a .php extension, placed in the root folder of an installation of HEAD with patch 56 applied, and then run.

With that done, I got the following results:

____________________________HEAD__________________patch56____________
theme('username')____________168.45 +/- 0.39__________138.25 +/- 0.46_____
theme('placeholder')___________16.49 +/- 0.05___________14.17 +/- 0.05______
theme('table')________________171.99 +/- 0.48__________169.10 +/- 0.52_____

theme('username') is significantly improved with the patch, because it's more efficient, as well as more elegant to add extra variables to a $variables array rather than an object. The inelegance of adding variables to an object was what prompted this patch.

Both theme('placeholder') (a 1 argument theme hook) and theme('table') (a 6 argument theme hook) are marginally improved by the patch (approximately 2 microseconds). This improvement is a larger percentage for an otherwise fast function like theme_placeholder(), and a smaller percentage for an otherwise slower function like theme_table().

I think it's reasonable to expect that every theme function is improved by 2 microseconds with this patch. However, I haven't gone through to test each one, so it's possible that for an odd reason, some are made slower. In order to not go through the tedium of benchmarking every single theme function, what would be an appropriate set of benchmarks to run?

Again, the goal of this patch is not to improve performance. It's to bring sanity to preprocess functions for function-implemented theme hooks (see description of this issue and the first 10 comments). Compare the code before and after of template_preprocess_username() and theme_username() for an example of the motivation for this patch. However, #44 made it clear that performance degradation will not be tolerated. I believe this patch meets the criterion of not degrading performance, though more benchmarks are needed to verify that. The fact that we get a small performance boost is a bonus.

The PHPDoc for each theme function will need to be updated, but other than that, code review and help with benchmarks would be most welcome!

The theme functions which tend to get called a lot on each page are theme_links() theme_username() and the various field ones functions (although those use templates). Personally I think microbenchmarks are fine here.

Out of interest I did a webgrind of vanilla head install front page with no content. It should theme() itself being about 10% faster (no call_user_func_array() any more for a start), but overall time fractionally slower. Although a single page requests isn't enough to go on when the numbers are that close, so it's more likely a no-op overall.

As long as theme_links(), theme_username(), theme_item_list() and friends aren't slower I think we're very much OK, since any regression in other functions ought to be cancelled out.

It might be worth benchmarking something relatively theme heavy like a node with lots of comments, 30 nodes with taxonomy terms, or the user permissions page - if they come out at no change then at least that validates there hasn't been a regression.

webgrind screenshots attached - all numbers are percentages.

Re-rolled.

Spoke to webchick in irc about what further performance data we need to support this - she suggested the 'node with a lot of comments' example - effugentsia, easiest way to do this is to use devel to generate a node with 300 comments, set comment settings to 300 per page, then profile / benchmark before and after. Previous profiling shows theme() and drupal_render() to be some of the most expensive functions on those pages, so if there's no obvious regression, or hopefully a small improvement, that should be plenty.

Sorry. Been busy with non-d7 work for the last week. I'm back to this now, however, and will try to do my part to have this land by API freeze (ideally, with a few days to spare to give other patches in the queue a chance to react accordingly, since this one has broad impact).

This one's just a re-roll. I'll supply benchmarks once this passes regressions.

Benchmark results for viewing node with 300 comments (as per #61). Benchmark setup as follows:

Fresh install of head. Set permissions of anonymous role to include "Access comments", "Post comments", and "Post comments without approval". Set "Article" content type to show 300 comments. Enable "Devel generate" module. Use it to generate 1 node with 300 comments. Using Apache Benchmark:

1) ab -c1 -n500 http://localhost:8888/d7/node/1. Output attached in ab-theme-wip-572618-head.txt. Main result (in ms): 902 +/- 13.2.

2) Apply patch 63 and repeat "ab" command. Output attached in ab-theme-wip-572618-63.txt. Main result (in ms): 893 +/- 13.6.

Interpretation:

• 9ms improvement. Assuming normal distribution, the 95% confidence interval of the mean = 1.96 * 13.6 / sqrt(500) = 1.2ms, so the 9ms improvement is statistically significant.
• The 9ms improvement is what we would expect just from the improvement to theme('username'). See #57. 301 invocations of theme('username') (the node plus 300 comments) * 30 microseconds improvement per invocation = 9ms.
• Also from #57, I would expect to see a 2 microsecond improvement for every theme() invocation besides theme('username'). For this page, there are 1266 of those, so I would expect to see an additional 2.5ms improvement that I'm not seeing. I would like to find out why the theoretical 2.5ms improvement isn't manifesting.

Although I'd like to find out what's preventing an additional 2.5ms improvement for this test, I think my time will be better spent first updating the PHPDoc for each theme function. I'll do that first and will post that so that full code review can begin. I'd like this to move quickly into a code review stage and not be hung up by performance concerns, so if you have performance concerns that aren't addressed in this comment, please post them.

With PHPDoc changes. Many theme functions don't have full @param documentation in HEAD. This patch adds it to a couple of them, but mostly leaves those alone, to be handled in a separate issue. This patch just updates the ones that do have that documentation to be correct, given the change from a parameter list to a single $variables parameter.

This patch NEEDS code review!! As an aid for anyone daunted by reviewing such a large patch, here's a summary of the changes:

1. theme() implementation changed and those changes need thorough review.
2. template_preprocess_username(), template_process_username(), and theme_username() changed and need semi-thorough review.
3. The function signature of all theme_*() functions changed from a parameter list to a single $variables parameter. This needs cursory review.
4. Every line of code that invokes theme() changed from passing a parameter list to passing $hook and $variables. This needs cursory review.

Please post feedback, or even a comment saying you reviewed it and have no feedback. Let's try to get this to RTBC quickly. Thanks!

I think I tracked down the mystery from #64 as to why I wasn't seeing any performance gain beyond what was due to theme('username'). It was a glitch with my computer. It was apparently running too long since last restart, so I tried the benchmarks again after a reboot. Also, I had APC turned off, and because of that was seeing variance due to the extra hard drive IO (my hard drive is slow). So these new benchmarks are with APC turned on, removing much of the disk IO.

In each file, I'm supplying 2 runs of 500 each. For head, the first run had a couple outliers, so I ran it again to get tighter numbers. For patch 65, the first run produced less outlier effect, but still the 2nd run was tighter.

Summary
HEAD: 644.4 +/- 1.6
Patch 65: 627.7 +/- 2.1
Improvement: 16.7ms

Interpretation
Following the lines of #57, I micro-benchmarked the individual theme functions. I continued to see a 30 microsecond improvement to theme('username'), a 2.5 microsecond improvement for function-implemented theme hooks, and a new result of a 10 microsecond improvement to template-implemented theme hooks (which I hadn't micro-benchmarked before). For a node with 300 comments, theme('username') is called ~300 times, theme(FUNCTION_IMPLEMENTED_HOOK_EXCLUDING_USERNAME) is called ~600 times ('markup' and 'links' each at ~300), and theme(TEMPLATE_IMPLEMENTED_HOOK) is called ~600 times ('comment' and 'user_picture' each at ~300). This accounts for a 300 * 0.030 + 600 * 0.0025 + 600 * 0.010 = 16.5ms improvement, which is pretty much what the result shows, indicating there's no significant gain or loss of performance other than the changes to theme().

The improvement represents only 2.5% of the total page request time. However, according to my profiler, the time spent in theme() (just in lexical scope of theme(), so not counting what theme() calls) is 70-80ms. Pulling out the 9ms improvement from theme('username'), which improves template_preprocess_username() rather than theme(), we're still left with a 7-8ms improvement to theme(), or ~10%. Not bad.

Anyway, I'm pleased to see a small performance improvement resulting from this patch, and I am not seeing any situation resulting in performance degradation, though if someone wants to see benchmarks of other pages, please say which ones you'd like to see.

2.5% of the total request time and 10% within theme() itself sounds like a decent improvement to me, especially considering that isn't the main focus of this patch. Between the microbencharks, profiling, and ab that should be plenty of data.

Very, very quick code review - I only scanned through about 50% of the patch, only style issues since 99% of this is moving code around with no substantive changes otherwise.

+++ install.php	6 Oct 2009 22:35:31 -0000
+++ install.php	6 Oct 2009 22:35:31 -0000
@@ -263,7 +263,7 @@ function install_begin_request(&$install

Please remove this hunk and any others which are whitespace fixes only - it makes the patch overly large, prone to breakage, and harder to review.

+++ includes/form.inc	6 Oct 2009 22:35:32 -0000
@@ -1549,10 +1549,14 @@ function form_options_flatten($array, $r
+ *   An associative array containing:
+ *
+ *   - element
+ *     An associative array containing the properties of the element.
+ *     Properties used: #title, #value, #options, #description, #extra, #multiple,

Exceeds 80 chars.

+++ includes/pager.inc	6 Oct 2009 22:35:32 -0000
@@ -192,20 +192,32 @@ function pager_get_query_parameters() {
+ *   An associative array containing:
+ *
+ *   - tags
+ *     An array of labels for the controls in the pager.
+ *
+ *   - element
+ *     An optional integer to distinguish between multiple pagers on one page.

Here I'm not sure what the standard is, but it seems like we probably don't need the empty lines?

+++ includes/pager.inc	6 Oct 2009 22:35:32 -0000
@@ -344,20 +366,32 @@ function theme_pager_first($text, $eleme
+function theme_pager_previous($variables) {
+  $text = $variables['text'];
+  $element = $variables['element'];
+  $interval = $variables['interval'];
+  $parameters = $variables['parameters'];

Since theme('pager_first') gets passed the same array, wouldn't it be better to use the array rather than assigning variables like this? If the answer is "yes, but not in this patch." That's completely fine with me.

This review is powered by Dreditor.

Thanks, catch. I have my editor set to strip trailing whitespace when saving a file, which is how those hunks got in there. I tried to manually remove all of those hunks, but I might have missed a couple. I also fixed the wrap to 80 issue you found.

Before I change all the PHPDocs, is there agreement to remove blank newlines between keys within $variables? Any other requests regarding the PHPDoc style?

For theme_pager_previous(), 'interval' isn't expected by theme_pager_first(). Probably wouldn't hurt to pass $variables through anyway, and I'll make that change if someone else requests it, or if you change your mind and decide you feel strongly about it. Otherwise, maybe better to leave for a separate issue?

I guess my attempt at manually removing hunks resulted in a problem. Hopefully, this fixes it.

Looks like the example at http://drupal.org/node/1354 has no blank line between list items in PHPdoc, so I'd probably go with that, but would be good to get a +1 from someone before changing the 273kb patch.

I don't feel strongly about how we deal with $variables - and certainly no in this initial conversion, we need to keep this out of re-roll limbo as soon as it's ready.

+ * An associative array containing:
+ *
+ * - tags
+ * An array of labels for the controls in the pager.
+ *
+ * - element
+ * An optional integer to distinguish between multiple pagers on one page.

Here I'm not sure what the standard is, but it seems like we probably don't need the empty lines?

...

Before I change all the PHPDocs, is there agreement to remove blank newlines between keys within $variables? Any other requests regarding the PHPDoc style?

Yes, we have a standard, but not all of core follows it properly. Following the standard is actually required to make the API parser output it properly. See #596250: Garbled output of cache.inc doxygen for a current patch that needs to fix it elsewhere. All code should use:

 * @param $variables
 *   An associative array containing:
 *   - tags: An array of labels for the controls in the pager.
 *   - element: An optional integer to distinguish between multiple pagers on
 *     one page.
 *   Any further description - still belonging to the same param.
 * This no longer belongs to the param, and, there should be a newline after all
 * params, but not between them.

Which means:

- The list is aligned with the paragraph, no newline before or after the list.
- No newlines between list items.
- Each list item starts with the key, followed by a colon, followed by a space, followed by the key description. The key description starts capitalized.
- If a list item exceeds 80 chars, it needs to wrap, and the following lines need to be aligned with the key, not the list item bullet/hyphen.
- If the param description continues, it uses the same alignment as the initial paragraph.
- Within a single param description, NO blank lines are supported. For example, you can compare the docs of http://api.drupal.org/api/function/drupal_add_css/7 with the actual doxygen to see what would happen.

The same rules apply to doxygen where no @param is involved. I just used that for clarity.

Once for all, I finally took the time to document the proper and *required* syntax for lists'n'stuff in Doxygen: http://drupal.org/node/1354

(same as above, but now you have an official reference)

There is not enough praise in this world to shower on this patch. Named arguments for theme(): OH YES FINALLY! I wanted this for... uh oh... five years?

Implemented #72. Unless I'm missing a subtlety, http://drupal.org/node/1354 seems to contradict itself (2nd vs 6th code blocks) as to whether there should be an empty line between the last @param and the @return. This patch has the empty line. Please let me know if I need to remove them.

Also, please look at the PHPDoc for theme_username(). This is our only current example where a preprocess function is used for a function-implemented theme hook. Only the 'account' key within $variables is assumed to be passed by the calling code. The other keys are added by the preprocess function. Do we need a standard for distinguishing these within the PHPDoc?

#550942: Blank line between @param and @return

Cool. That means the patch is good, at least with respect to that. I updated http://drupal.org/node/1354 (diff)

I looked through this again, nothing obvious sticks out, performance results are great, so going to be bold and mark this RTBC. I only ask that we try to either commit this within the next day or so to give RTBC patches ample time to get re-rolled, or right at the end of code slush when those other big RTBC patches are already in. Either way should be manageable, but either end seems preferable to in the middle somewhere.

Holy cow! I really wonder how you rolled that patch... looks like original hand-made stuff? Crazy.

It would have been good to change all the theme functions separately and do the further changes to theme() in a follow-up patch, but yeah.

This one should really go in next, so let's make sure it's on the top of the hit list.

Thanks for your support, sun! See #43 and #44 for why this was all done in 1 patch instead of 2. Here's a re-roll due to forum.module changes landing recently.

There is on hunk that fails in this patch. Could we get a re-roll?

Fixed failures due to recent changes to update.report.inc, user.admin.inc, and node.admin.inc.

Alrighty! Committed to CVS HEAD. Thanks all.

This will require upgrade instructions in the handbook so marking this as 'needs work' until the documentation has been updated. Please mark this 'fixed' after the documentation has been updated.

Tagging for docs.

Yay! I think this moves the whole preprocess/theme thing from being half-baked to about 90% baked. How much of the remaining 10% can we do before API freeze? If interested, please help out on #600658: Refactor core module theme functions (templates) to separate data logic from markup decisions, enabling modules to alter the former.

Unfortunately, this patch introduced a major problem: #600974: theme() fails for theme functions taking one argument in renderable arrays

I believe that problem predates this patch. From 1.531 of theme.inc's theme() function:

// If a theme function that does not expect a renderable array is called
// with a renderable array as the only argument (via drupal_render), then
// we take the arguments from the properties of the renderable array. If
// missing, use hook_theme() defaults.
if (isset($args[0]) && is_array($args[0]) && isset($args[0]['#theme']) && count($info['arguments']) > 1) {
  $new_args = array();
  foreach ($info['arguments'] as $name => $default) {
    $new_args[] = isset($args[0]["#$name"]) ? $args[0]["#$name"] : $default;
  }
  $args = $new_args;
}

I'm thinking about it, and will post any ideas for how to address #600974: theme() fails for theme functions taking one argument in renderable arrays on that issue. If anyone subscribed to this issue participated in the above code block and has thoughts to share about it, please post on that issue.

http://drupal.org/update/theme/6/7#function-argument-variables

#600974: theme() fails for theme functions taking one argument in renderable arrays is a separate issue that's been fixed and will soon be documented :)