theme() fails for theme functions taking one argument in renderable arrays

This patch is ugly, and quite likely a bad idea, but just curious what testbot thinks.

Weird. I'm able to install HEAD, so I'm not sure what testbot is complaining about, but respecting sun's wishes, and stopping on this. I have no experience yet writing tests, and with only 4 days till API freeze, I'll concentrate on areas I do have experience with, and leave this to others.

Here's a summary of the problem (can be a guide to someone who wants to write tests). Version 1.497 of theme.inc (2009/07/02) introduced the ability for theme() to be called with a single argument after the hook (i.e., theme(HOOK, $element)), and if that argument is a renderable array, but the registry information for the theme hook indicates that the theme implementation wants more than 1 argument, then theme() used the element's properties to fill in those arguments. However, if the implementation for the theme hook wants 1 argument, then the assumption is it wants the renderable array as that argument, and not some property of it.

This apparently worked fine until we started converting more things to use the render() system. #599706: Allow to alter local tasks/actions attempts such a conversion, but for a theme hook that expects a single argument that is not a renderable array, but a property of it. There are actually 59 theme hooks that take a single argument that is not named 'element', 'elements', or 'form' (in which case, it's obvious that a renderable array is wanted). Install HEAD, enable all core modules plus devel, and run this from devel's "execute php" page:

// initialize the theme system
$x = theme('placeholder', 'y');
// get the full registry
$hooks = theme_get_registry();
// get the hooks that take a single argument not named something that
// obviously indicates a renderable array is wanted
$interesting_hooks = array();
foreach ($hooks as $hook => $info) {
  if (count($info['arguments']) == 1) {
    $keys = array_keys($info['arguments']);
    if (!in_array($keys[0], array('form', 'element', 'elements'))) {
      $interesting_hooks[$hook] = $info;
    }
  }
}
// display them
dpm($interesting_hooks);

The difficulty is that some of these 59 expect that argument to be a renderable array and some don't. For all that don't (e.g., menu_local_action from #599706: Allow to alter local tasks/actions), we can't take advantage of the render() system. While not the end of the world, it's definitely a WTF.

Patch #3 introduces a half-baked (really, 1%-baked) idea for how to give theme() a hint as to what the theme hook wants. But this just solves one WTF by introducing another one.

Ideas??

I thought about this some more over lunch. Just offering my opinion here, and I accept that it might be controversial, but what I would love to see (for D8) is for ALL theme hooks to have an 'arguments' of a single renderable array (preprocess functions can be used to add more to $variables). This would allow every theme function to be invokable via the render() system. Furthermore, I think we should try to ONLY invoke theme functions via the render() system -- as in, have drupal_render() be the only function that ever calls theme(). We'd need to take performance into account, but to me, this should be the ideal that we only make exceptions to when there's a compelling reason. At least, that's my vision for D8.

If you agree that the above is a good goal for D8, then it seems to me like any theme function that doesn't take a renderable array as its one and only argument is basically using a deprecated way of doing things that's only still supported, because we don't have time this late in the D7 cycle to "upgrade" all of them. The ones that take more than one argument we don't have to worry about. But, the ones that take a single argument, if we want to make them work with render(), then we should change them to take a renderable array as the argument, and change any code that invokes theme() directly for that hook to match. If you agree with this, then let's mark this issue "by design", and apply the corresponding change to #599706: Allow to alter local tasks/actions and any other issue that's trying to use the render() system but running into this same problem.

Just FYI, I dug around in CVS annotate this evening, and this code originally originates from #373201: Enhance drupal_render() themeing. Return renderable array on tracker page., where we added support for expanding theme function arguments to #properties. It appears the restriction to > 1 argument happened in #13, though I don't see anything there that gives a particular clue as to why. :(

Thanks for finding that, webchick. The reason it's done for >1 arg theme functions only, is that if you do it for 1 argument, then you can't have theme functions that take the renderable array itself as that argument, and about two thirds of all the theme functions do. For theme() to know whether a given 1-argument theme function wants the renderable array itself or its 1 property, it would need to somehow be specified in hook_theme() or done by naming convention or some ugly hybrid system (see patch 3). That's why my recommendation is to not have 1 arg theme functions unless that 1 arg is the renderable array (bring the minority of theme functions into compliance with the majority). I'm not necessarily suggesting that we have to do this for all of them, but we do need to do it for those that we want to be invokable via drupal_render().

Bleh. That seems like an incredibly horrible limitation that's going to bite developers in the future just as it bit sun tonight. Having a theme function with one non-element array argument is a perfectly reasonable thing to do.

Can we not do some sort of a check to see if the argument coming in is 1. an array, and 2. contains some sort of property (are there any that are consistent across renderable elements? #type?) and act on that instead?

How about this? The test is biased towards assuming that an element is wanted, since prior to this patch, that was the assumption. Only when the element neither has a type AND has a property matching the argument name expected by the theme hook do we pass the property value instead of the element.

Same logic as #13, but more readable :)

Done. Hope you like it. Still a WTF, but at least a documented one.

I think the main WTF is that the $variables argument can be three things currently:

• An array of variables
• A renderable array that is passed as the first argument of the theme function / template
• A renderable array that is broken up in several arguments of the theme function / template

The following patch standardize that stuff a little bit more. The $variables argument is now always an array of variables. If the array only contains a 'elements' variable, and the theme definition expects other arguments, we expand this variable into one or several arguments.

For this patch to work, we need to standardize the theme functions that expect a renderable array to have an argument list of array('elements' => NULL).

Same patch, without the silly parse error. It should not pass either, because we need to update the theme functions.

Trying something. Will explain it and/or write better comments if bot likes it.

Found and fixed the test failure in update.report.inc, though I'm confused why it was working in HEAD. Also, moved the function added by this patch from common.inc to theme.inc. Seeing what testbot thinks now.

Ok, this one I think is ready for human review, once bot re-confirms. The patch includes PHPDoc for hook_theme() in system.api.php, which I think pretty much explains it all. Note that this patch introduces a new property for each hook declared in hook_theme(), so if that counts as an API change, we need to move quickly on this.

Here's a re-description of the problem and what this patch solves: drupal_render() calls theme() with a variable for the $hook parameter. That means drupal_render() doesn't know what the particular theme hook's arguments are. The proposal in #19 would require changing the arguments definition for a lot of theme hooks, which seems difficult to pull off given where we are in the D7 cycle. So, in HEAD, drupal_render() calls theme() passing it the renderable element instead of a $variables array. Everything except drupal_render() calls theme() and passes it a $variables array because it knows the hook it's calling and therefore what arguments are needed. But, given the nature of drupal_render(), theme() must know how to map a renderable element to the theme hook's arguments. In HEAD, it decides this based on whether the theme hook expects one argument or more than argument. Everyone agrees that this creates a huge WTF for developers (why exactly can't we have 1 argument theme hooks that need the element's property?) And in fact, in HEAD, we DO have over 50 1-argument theme hooks where the first argument is not a renderable element, and therefore all 50+ of these theme functions will break if used in the context of an element's #theme property.

However, in HEAD, we also have over 100 1-argument theme functions that need the element and not its properties. But, the vast majority of them name that argument either 'element', 'elements', or 'form', and not surpisingly, the 1-argument theme functions that don't take an element as its argument name that argument something other than one of these 3.

So, this patch, changes the default logic from assuming that ALL 1-arg theme hooks act on an element to assuming that only those for which the argument is named one of those 3 do. But, to handle the small set of theme hooks in core that don't follow that naming convention, this patch adds a property to a theme hook's registry information that can explicitly set the mode needed.

Note, the solution in #14 implemented an alternate logic (based on element #type). But that seemed to still have a WTF. In my opinion, this patch is better.

Nice explanation ... This is an ugly problem and has an appropriately ugly solution. I can think of no better approach than this.

With sun's nits except doc shortening.

With much doc cleanup and a little code re-org for setting 'render' default.

Nice! Much better commenting/documentation. My only nit:

+++ modules/system/system.api.php	18 Oct 2009 06:18:55 -0000
@@ -853,8 +853,24 @@ function hook_permission() {
+ *   It is recommended to define all theme functions with one argument named
+ *   'element', 'elements', or 'form' only. Otherwise, the 'render' property has
+ *   to be defined to pass the expected theme function arguments.

Is this actually our recommendation? If it is, ok, but I think our recommendation can be looser, which is: When you define a new theme hook, decide whether you want that hook to receive a renderable element, or a set of non-element arguments (you have to pick -- it's one or the other). If you want an element, then just have one argument in your 'arguments' and name it 'element', 'elements', or 'form', or else set the 'render' key to THEME_ELEMENT. If you want non-element arguments, then don't call the first argument 'element', 'elements', or 'form', or else set the 'render' key to THEME_PROPERTIES. And if this is too confusing, then just set the 'render' key to what you want and don't worry about how the default is figured out.

Ok, I don't think we necessarily want to write all that, but that's my understanding of our recommendation. If the rest of you decide to actually make the much stronger recommendation towards an element argument, then I think the patch is good as-is.

I'm on crack. Are you, too?

Oh one more thing. If explaining this convoluted way of determining the default mode is just too much of a problem, an alternative is we could just say that the default is always THEME_ELEMENT, and if you want THEME_PROPERTIES, then you have to set the 'render' key to that (which we could rename and make into a boolean). But, this would mean adding that entry into approx. 100 hook_theme() declarations that are currently in core. A little tedious, but do-able, though committing something like that might require a lot of patches in the queue to be re-rolled.

My concern with this is explaining it to new developers. The hook_theme($variables) patch potentially made this very easy:

"Anytime you need to print output to the screen, it should be wrapped in a theme_XXX function. All theme_XXX functions take a single argument -- $variables -- which contains any values you need to pass along inside."

Now, the explanation gets /way/ more convoluted, with a lot of "Except for..."s. I hate those! ;) However, I don't really have any better ideas. Moshe also pointed out in IRC that this approach has his +1.

I'd love to get Dries's thoughts on it, not only because it's an API change post-freeze, but also to see if he has any lovely alternative ideas. But many paths were explored on our way to this, so I'm not sure what else is left.

I think the recommendation text is fine as is. Trust me - there is no magical, elegant solution here. I'm not sure what the hesitation is. The state we are committing from is even worse.

When you define a new theme hook, decide whether you want that hook to receive a renderable element, or a set of non-element arguments (you have to pick -- it's one or the other). If you want an element, then just have one argument in your 'arguments' and name it 'element', 'elements', or 'form', or else set the 'render' key to THEME_ELEMENT. If you want non-element arguments, then don't call the first argument 'element', 'elements', or 'form', or else set the 'render' key to THEME_PROPERTIES. And if this is too confusing, then just set the 'render' key to what you want and don't worry about how the default is figured out.

My ears are bleeding from the burst blood vessel in my brain!!!!

Trust me - there is no magical, elegant solution here. I'm not sure what the hesitation is. The state we are committing from is even worse.

The hesitation is that we are currently in a WTF situation where the simplification of our theme() functions to always use one parameter called $variables has created turbulent waters for our improved render() system. [edit: Alex corrects me on this point below.] And this patch, rather then simplifying things, upgrades our WTF to a WTFBBQ. ;-)

Initially, to me, it would seem the best solution would be to put the theme parameters into "variables" array. But I see that solution was suggested by webchick in comment #31 of #373201: Enhance drupal_render() themeing. Return renderable array on tracker page., but rejected because it would add duplication to the array and be too hard for poor, lazy developers to copy array elements. Personally, I think this rejected solution is still better then our current state. But let me propose another option…

Looking at the patch in #32, the 'render' => THEME_PROPERTIES makes no sense to me. How are the variables passed as parameters to a theme function considered "properties". That's definitely the BBQ in this proposed patch. And why do we even need a whole new array key here? The "arguments" parameter name is already a WTF, because we call them variables everywhere else; even inside the description of "arguments" in the hook_theme() docblock! Why not kill two WTFs with one stone and replace the array key "arguments" with one of two keys, either "variables" or "renderable_elements" (or "elements" or "ponies" or whatever color bikeshed key name you guys want)? That way the hook_theme()'s definition of the theme function determines how theme() handles the array that is passed to it.

function hook_theme() {
  return array(
    'system_settings_form' => array(
      'renderable_elements' => array('form' => NULL), // Just pass the array as the first param to the theme function.
      'file' => 'system.admin.inc',
    ),
    'system_powered_by' => array(
      'variables' => array('image_path' => NULL), // Match the keys in the "variables" array with the corresponding #'ed key in render()
    ),
  );
}

IMO, we should either do the toggle based on the name of the key (not the value of some new "render" key), or stick all theme parameters in a '#variables' key. Either of those 2 solutions are better then the current state or the patch in #32.

Here's the beginnings of key-name-toggle patch. To make this my idea easier to review, I haven't updated any of the hook_theme functions and replaced the 'arguments' with 'variables as that would make the patch much bigger. So the patch will definitely fail until that part is added. This patch is based off of #32, so any place it used 'render' => THEME_ELEMENTS this one uses 'renderable_elements' => array('key_name' => NULL). BTW, is there currently any way to tell if a particular theme function expects a renderable array? It seems obvious there are exceptions to the rule that the arguments first key should be called "form", "element" or "elements".

Of course, feel free to flame me as I'm still really new to render(). :-)

And here's the patch with all the 'arguments' replaced with 'variables' or 'renderable_elements'. Much bigger patch, but it has a chance of passing the testing bot now. The patch in #37 is probably still easier to eyeball.

Quick re-roll to get theme() working.

Prior patches were based on an attempt to not make such a sweeping change too close to or after API freeze. However, I must say that I like this approach. John, just a reminder, which I explained in #6 and other comments, the problem existed before changing theme functions to take a single $variables. The problem is entirely due to the fact that we have two fundamentally different kinds of theme hooks we're trying to support -- one that takes 1 or more non-element variables and one that takes 1 element variable and that we want to make BOTH kinds work when called from drupal_render(). That said, I think your patch is a good way for hook_theme() to provide that information to theme().

+++ includes/common.inc	2009-10-19 16:57:24 +0000
@@ -5071,196 +5071,196 @@ function drupal_common_theme() {
-      'arguments' => array('page' => NULL),
+      'renderable_elements' => array('page' => NULL),

and elsewhere: drupal_render() only calls theme() on one element at a time, so this can be replaced with 'renderable_element' => 'page'

+++ includes/theme.inc	2009-10-19 16:28:36 +0000
@@ -365,11 +369,15 @@ function _theme_process_registry(&$cache
+      // Same for the 'renderable_elements' flag.

This isn't a flag.

I'm on crack. Are you, too?

Given that there's no way to fix the current situation without an API change of some sort anyway, I'd much rather an API change that is not going to require a 30-minute explanation. :P John's approach is interesting, in that regard. Though it would be good to get feedback on that from the people who were in support of #373201: Enhance drupal_render() themeing. Return renderable array on tracker page..

I am starting to once again find myself on the fence as to whether or not #373201: Enhance drupal_render() themeing. Return renderable array on tracker page. was a good idea... it seems like a DX improvement for people who understand the rendering system well, but a DX WTF for everyone else. Hm.

John, just a reminder, which I explained in #6 and other comments, the problem existed before changing theme functions to take a single $variables.

Right! I see that now. Thanks for the clarification. And, to be perfectly clear, I was just joking around with the “WTFBBQ” stuff. I used a lot of the previous patches in my patch. :-)

So, Alex wants to change 'renderable_elements' => array('element_name' => NULL), to 'renderable_element' => 'element_name'. That sounds reasonable to me. Any other opinions?

And, yes, the opinions of those who worked on #373201: Enhance drupal_render() themeing. Return renderable array on tracker page. is desirable.

I like John's patch as well. It will look slightly cleaner after eff's suggestion is incorporated.

Re-rolled given comments in #41 and #44.

Thinking a bit more - this introduces a WTF for a module author when writing his hook_theme(). The docs say that he should use either 'variables' or 'renderable element' (BTW, I think 'render element' is descriptive enough, and matches the render() function). So what should a author pick for for the 'links' theme hook? This patch uses 'variables' there but all node links are emitted with #links in a render element. See bottom of comment_node_view(), for example.

• If the theme function author expects its parameter, the $variables array, to contain keys without "#", the author should use 'variables'.
• If the theme function author expects its parameter, the $element array, to contain #'ed keys, the author should use 'renderable_element'. (Or 'render element', if it makes it more consistent with what render() uses.)

IMO, this is no more of a WTF then the fact that theme() already handles these two different kinds of arrays.

But, other then what I just said above, I have no easy-to-apply advice on which of the two options theme function authors should use. Personally, I'll be using 'variables' unless I'm theming parts of forms.

Hmm… it seems removing the array off of the 'renderable_element'’s value, caused some warnings. This might be related to the fact that some hook_theme functions used 'element_name' => array() and some used 'element_name' => NULL; I had to pick one of them to use for all renderable-element theme functions; see line 342 of the patch. But, I won't be able to re-roll for at least 5 hours while I get some work done.

I wrote a few parts of the improved render system so I thought I'd chime in here. I like John's patch. Some more comments:

1) We should standarize on naming. So huge +1 to renaming arguments to variables in hook_theme. As a name for an array that can be passed into render() I very much like moshe's suggestion render element. Short and concise and better than the renderable element we used in the documentation so far.

2) So what want is a way for a theme hook to define whether it

• a) expects one variable and one variable only. This one variable is a render element.
• b) expects one or several variables, which are either directly passed in via theme() or are taken from a render element's properties that are named the same as the expected variable names

That's it, there's no third option needed. So @moshe in #46: The "links" theme hook would chose b), as it expects several variables and not one render element. It doesn't care whether the variables come from a direct call to theme() or are taken from a render element's properties.

So John's patch is just fine IMO.

I would rename renderable_element to just render element (no underline needed).

Otherwise, the patch looks great.

re-rolled after RDF patch landed. Expanded the hook_theme docs a bit. Changed "renderable_element" to "render element".

+++ includes/theme.inc	2009-10-20 01:03:32 +0000
@@ -780,30 +788,28 @@ function theme($hook, $variables = array
+    if (isset($info['variables'])) {
+      foreach (array_keys($info['variables']) as $name) {
         if (isset($element["#$name"])) {
           $variables[$name] = $element["#$name"];
         }
       }
     }
+    else {
+      $variables[$info['render element']] = $element;
+    }

Although we have no use-cases in core of a single hook_theme() entry using both, and doing so would be poor design, I still think it's cleaner to make this two separate if statements rather than an if/else. i.e., if (isset($info['variables']) {} and if (isset($info['render element']) {}. This also better handles the situation of a 0-var theme hook that sets neither.

+++ includes/theme.inc	2009-10-20 01:03:32 +0000
@@ -780,30 +788,28 @@ function theme($hook, $variables = array
+  elseif (!empty($info['render element'])) {
+    $variables += array($info['render element'] => array());
   }

I'd prefer to see this gone. We should never call theme() on a theme hook that takes a single render element without passing it an explicit element. I'd rather see test failures for when this happens and correct them.

+++ includes/theme.inc	2009-10-20 01:03:32 +0000
@@ -990,9 +995,10 @@ function drupal_find_theme_functions($ca
+            $arg_name = isset($info['variables']) ? 'variables' : 'render element';
             $templates[$new_hook] = array(
               'function' => $match,
-              'arguments' => $info['arguments'],
+              $arg_name => $info[$arg_name],
             );

Any way to rework this so that a 0 variable theme hook (e.g., 'menu_local_tasks') can set neither 'variables' nor 'render element'?

+++ includes/theme.inc	2009-10-20 01:03:32 +0000
@@ -1093,10 +1098,11 @@ function drupal_find_theme_templates($ca
+          $arg_name = isset($info['variables']) ? 'variables' : 'render element';
           $templates[strtr($file, '-', '_')] = array(
             'template' => $file,
             'path' => dirname($files[$match]->uri),
-            'arguments' => $info['arguments'],
+            $arg_name => $info[$arg_name],
           );

same here

+++ modules/system/system.api.php	2009-10-20 00:19:34 +0000
@@ -849,12 +849,19 @@ function hook_permission() {
+ * - variables: (required if "render element" not present) An array of
+ *   variables that this theme hook uses. This value allows the theme layer to
+ *   properly utilize templates. Each array key represents the name of the
+ *   variable and the value will be used as the default value if it is not given
+ *   when theme() is called. Template implementations receive these arguments as
+ *   variables in the template file. Function implementations are passed this
+ *   array data in the $variables parameter.
+ * - render element: (required if "variables" not present) A string that is the
+ *   name of the sole renderable element to pass to the theme function. The
+ *   string represents the name of the "variable" that will hold the renderable
+ *   array inside any optional preprocess or process functions. Cannot be used
+ *   with the "variables" item; only one or the other, not both, can be present
+ *   in a hook's info array.

See my comments above. I see no reason for it to not be okay to set neither. And while I think it would be poor theme hook design to use both, I'm not sure if we should be saying that you can't.

Also, I would rework the 'render element' entry to more closely match the 'variables' entry with respect to how templates/functions get it, because in this regard, it's the same: A string that is the name of the variable that this theme hook uses to represent the element being rendered with the render() function. Template implementations receive this variable as a variable in the template file. Function implementations receive this variable as one of the entries within the $variables array.

I'm on crack. Are you, too?

Despite my comments above, I think this patch is quite good, and those little things can be debated and cleaned up later. It looks like we may be getting close to overlays being done, in which case, Dries' official public announcement about code freeze will be coming soon, and it would be nice to have this committed before then.

Given #44 and #49, as well as John's and my opinions, I'm calling this RTBC. No doubt, docs and minor code tweaking could be debated endlessly, and we'll have weeks if not months for that. My concern is this: ideally, we shouldn't tell module developers that the API is stable, and therefore, if they've been holding out on porting their modules, that we now really, really encourage them to start, until this is in. If there are other issues in the RTBC queue as sweeping as this one (in the sense of, is likely to break the majority of contrib modules once committed), then I feel the same way about those too.

Re-rolled.

Ok, had a chance to touch base with Dries to confirm that he's ok with this API change, since this is obviously a critical issue.

I had major reservations with the first approach (in general, if you need 10 lines of PHPDoc to explain something, we need to find a better solution ;)) but this second approach makes a lot more sense, and as a nice side-effect eliminates another DX WTF with the 'arguments' vs. 'variables' thing. I also agree that the sooner we get this in, the better, since it does affect all D7 modules that implement theme functions.

Therefore, committed to HEAD. Please ensure this is documented ASAP. I'll also shoot a note out via Twitter and on the development mailing list.

Here's a start: http://drupal.org/update/modules/6/7#hook_theme_render_changes. It could probably use some improvement, and also, I'm not sure which UNSTABLE-X header to put it into at the top of that page, and what the protocol is for adding it to http://drupal.org/node/394070. Any links towards how I can learn about that would be much appreciated.

394070 is being maintained by the coder maintainers, so no need to add there yourself. Those docs look fine to me - it's more a guide to what's changed, the docs for theme() itself should have the meat of the API anyway (and if they don't that's a separate bug).