Refactor drupal_render theming - docs

In proper words, the result of the discussion was that

• 1) We should completely seperate the #type property from presentation. A type is defined in hook_elements to supply elements with default values and it is the main distinguisher during form processing, but should not at the same time be the name of the element's main theme function.
• 2) We need the possibility for an element to have two theme functions assigned, one that renders the children and puts them into a single string, and one that wraps something around that string. Classical example is a form with custom theming: The custom theming function renders the children and puts them into a string, while the wrapper wraps the form tags around it.

The proposed properties explained:

#theme and #theme_wrapper. #theme is the major property, while #theme_wrapper is completely optional. The theme function in #theme is responsible for rendering all the element's children and putting them into a single string. If #theme_wrapper is set on an element, it will be called later than #theme with the string that #theme returned as an argument (plus the element itself). The string returned by #theme_wrapper is then returned by drupal_render().

So basically #theme is still nearly the same, while #type in the context of drupal_render is replaced by the #theme_wrapper property.

In hook_elements, types will usually set defaults for one or both of these properties.

Note that this is basically what we do now, but properly separated from form logic and with much better names and less confusion. It will also introduce new theming possibilities as it decouples form rendering from form processing quite a bit more (by making it possible to override #theme_wrapper, which wasn't really possible up to now, as that would have meant changing the #type which would fail form processing).

We can introduce a function drupal_render_children() that can be called by the #theme of an element to simply drupal_render all children and concatenate them into a single string.

We can also make #theme_wrapper an array, so for form elements, theme_form_element would simply be a key in there, making it finally simple to disable or replace theme_form_element for a single form element.

We also don't have to introduce a new flag to do what we want to do in the page rendering patch (which we would have to with the current system).

So here's a very first stab! Still very rough.
Lots of forms are still broken, but some already work, e.g. the module form or the login form. I did not yet convert any actual elements. But maybe this makes clearer what's the idea behind this.
Dunno if I'll have much time to work on this in the next weeks, though.

Now this one is much better already.

* theme_form_element is now just another entry in the #theme_wrapper array of form elements, which means it's finally simple and easy to disable or change theme_form_element for specific elements via hook_form_alter
* all core elements now have the #theme_wrapper property!

Also, the diffstat says 118 insertions(+), 142 deletions(-), and many of the additions are just comments. So we'll get a much cleaner drupal_render with less code, even!

I clicked around through lots of core forms, and they all seem to work. Haven't checked thoroughly so far, though.
I'm setting to needs review anyway to get review on the concept. If there are still broken forms (which is very likely), they will (hopefully) be easy to fix (mostly replacing a drupal_render by drupal_render_children or similar).

New patch attached! I caught several more forms that do a drupal_render($form) in their theme function and made them all use drupal_render_children instead. With the patch, drupal_render($element) in the $element's #theme function now causes an endless loop. It didn't previously because of some quite weird flag-setting inside of drupal_render. IMO, with the patch it's much much cleaner now - to render the not-yet-printed children of a form in its theme function, simply use drupal_render_children(). This also makes the theme functions more readable, as drupal_render_children just makes more sense.

So a lot more tests should pass now, hopefully, maybe even all.

Also incorporated moshes suggestions in #9 and made the input format fieldset reappear with the patch.

So here's a quick summary of how things are now in drupal_render():

• #type has nothing to do anymore with theming. #type simply is responsible for loading the right set of default values (including theming related properties).
• #theme: If this property is set to a theme function, it is called by drupal_render with the element as an argument. If the element is allowed to have children, the theme function is then responsible to render the children, either manually using drupal_render on an individual child, or by calling drupal_render_children, which concatenates all not-yet-rendered children into a string.
• If #theme is not set, the children are concatenated into a single string using drupal_render_children.
• For elements that are not allowed to have children, like textfields, #theme is the perfect place to create the element's markup.
• Now, in addition to #theme there is #theme_wrapper. #theme_wrapper is an array of theme functions. They are called after the children have been rendered, and can therefore wrap more markup around the string returned by #theme (or drupal_render_children). This is used e.g. by the #types form and fieldset to wrap their tags around the rendered children. It is also used for nearly all form elements to wrap the markup of theme_form_element around them. The latter allows finally to override this behaviour in an easy and clean way for individual elements.

Well, with the implementation in the patch, #theme_wrapper functions are called with theme(). But still, yes, I think in conjunction with the page rendering patch (http://drupal.org/node/351235), this could be used to wrap elements into json. You could then for example implement hook_page_alter, iterate over the contents of the content region, and add appropriate #theme_wrapper functions which then are going to add the required json stuff. And allowing other caller functions in place of theme() would be quite an easy follow up patch I'd guess, e.g. a #theme_wrapper_callback that defaults to 'theme' and then #theme_wrapper would actually be #theme_wrapper_callback_arguments, or something like that. Needs some more thought, of course, and I haven't really spend time figuring this out yet, but this patch, together with the hook_page_alter patch, is definitely a door-opener for a lot more flexibility when it comes to rendering stuff into different output formats IMO.

I don't really believe the bot, btw - installation went smooth for me locally. There are still some test failures, but a *lot* less then previously. Setting to CNR to give the testbot another chance.

* Rerolled for recent changes in HEAD
* Fixed the remaining TODOs I left in the patch
* We used to alter elements in theme functions before passing them to theme_form_element for radios and checkboxes. I moved those checks in a pre_render function to make them compatible with theme_form_element as #theme_wrapper entries.
* Fixed many failures and updated the remaining element definitions.

Now, this one should have 100% passes!

Don't really believe the testbot. The fail is in "File naming". Passes for me locally, and really has nothing to do with drupal_render(). Requesting retest.

Hm. So I just did a round of profiling. /node with 90 nodes, no caching
HEAD:
drupal_render 198 calls, 84 161 incl (6.7%), 28 698 (2.3%) exl
Patch:
drupal_render 198 calls, 104 124 incl (9.17%), 18 997 excl (1.67%)
drupal_render_children 50 calls, 21 406 incl (1.89 %), 5 327 excl (0.47 %)

So no. of calls is identical, little bit more time spend. I fear we're talking about microoptimizations here.. if anybody sees a part in the patch that could have an actual negative influence on drupal_render, please enlighten me (I couldn't really find something but I'll look into it more thorougly tomorrow),

So here we go, optimizing starts to be fun!

Main point were time is saved now is element_children. Also, we now pass the keys of the element's children to drupal_render_children, which saves another element_children() run.

Profiling screenshots attached. Profiling was done for node/xx/edit.

So now the patch is even faster for drupal_render than HEAD on the node edit page (by quite a lot), while on /node with 90 nodes it's the same time.

Fixed the issues catch raised.
* No more trailing whitespace
* sizeof() changed to !empty() (I didn't introduce that in this patch, but !empty() is still what we use normally), also moved a call do element_basic_defaults so that it doesn't get called each time
* Improved documentation and moved the important parts into the phpdoc block, where we had barely any interesting documentation at all before.

Reroll for the hook_page_alter patch (yay!) and the element sorting fix!

Fixed the new type #list and also a double label on the body field due to text format processing. Should pass all tests again now.

The tableselect element patch broke this. Reroll attached, should still pass all tests. I think this one is ready to go.

I guess I agree with eaton about #theme_wrapper. I first thought it would be needed as an array for form_element theming, but is not. So in the attached patch, #theme_wrapper is simply a string, which adds more simplifications and makes things consistent with #heme.
Also synced with HEAD and improved documentation here and there.

The $force parameter allows to print elements a second time on the same page, which wouldn't be possible otherwise without recursing over all children, as #printed would be set to TRUE not only for the element but also for all its children.

This one should be better.

Well, I think we definitely need to have the ability to have two theme functions per element, one for printing out the children and one for adding something around that.

We always had this, and we need this. What now is theme($element['#theme_wrapper']) used to be theme($element['#type']), which was just plain wrong (as we were overloading the type property, making it much less flexible). This patch just cleans up existing functionality.

We need two properties, for example for custom themed forms:
By default, the #type form has just #theme_wrapper set, #theme is empty. #theme_wrapper adds the form markup around the rendered child elements of the form. Now we can set #theme on an individual form (e.g. via hook_form_alter or in the form definition), where we control how and where the individual children are printed (we use this on many forms in core). But because of #theme_wrapper, we don't have to deal with the form markup, this just gets wrapped around the result of #theme. Same goes for fieldsets, or for individual form items, which are first themed and then the form_element markup is wrapped around the form element (which finally makes theme_form_element overridable on a per-element-basis. This used to be a pain for themers.).

So the patch doesn't really introduce new functionality, it just cleans up existing stuff.

#prefix and #suffix are used for different purposes (adding markup before or after an element). #prefix and #suffix should not have to interfere with the generation of the element itself. It would make things horribly complicated if #prefix and #suffix had to be used to theme the actual element.

Note, again, that #theme_wrapper is not actually new. Removing it would be a regression to Drupal 5 and 6.

Well, #theme / #theme_wrapper or #theme_children / #theme is basically bikeshedding IMO, except that for elements that are never allowed to have children (e.g. buttons or textfields), #theme and not #theme_wrapper is used to print the element, so renaming #theme to #theme_children might be more confusing here. So I vote for sticking with #theme and #theme_wrapper.

The attached patch adds another paragraph of documentation to the PHPDOC of drupal_render, giving an example on the difference between #theme and #theme_wrapper. Also removed the bikeshed tag, this issue is not at all about bikeshedding but about significant simplifications to a very important Drupal core function that is getting used more and more (and luckily so) ;)

chx, I thought about $force some more and agree that it's not needed currently. In an earlier stage of the patch I thought it might be useful for drupal_render_children, but is not and might not even work properly in conjunction with #theme functions.

So, $force is removed from the patch now.

added

drupal_render() theming properties changed

(issue) The value of the #type property of elements declared in hook_elements is no longer treated as a theming function. Instead, there are now two explicit properties that control HTML generation: #theme and #theme_wrapper. #theme is mostly the same as in Drupal 6, it can be used to render the element's children. #theme_wrapper is called afterwards and can be used to wrap markup around the rendered children, similar to the way the #type property used to be used.

If you implement hook_elements, you usually will have to set at least one of these properties for your elements, likely to the same value as #type.
In Drupal 6, in the #theme theme function of an element, it was common to call drupal_render() on the element itself to render all remaining children. This now causes an endless loop, use drupal_render_children() instead.

Also see the API docs for drupal_render()

to the upgrade docs.