Building $node->body with arrays like FAPI for viewing

RobRoy, I've been giving some thought to this and if it's done right, I think it can be a huge improvement. I'll brain-dump my thinking on the subject so far and perhaps we'll come up with something that works.

1. The solution should allow module-provided data to be kept separate from rendered HTML
2. The solution should preserve the original body and teaser values from the database (if any) for use by themers
3. The solution should allow individual pieces of node data or the entire assembled node to be filtered and/or processed for viewing
4. The fully rendered teaser and body should still be stored in $node->teaser and $node->body once everything has been processed.
5. The solution should mirror existing Drupal systems (notably FormAPI) where possible. A new 'style' of data structures is bad.

I'd like to propose the following. Instead of modules just jamming properties onto the $node during nodapi $op load, jamming HTML into $node->body, and doing strange things with it on $op view, we adopt the same model formapi does:

nodeapi $op load gives modules a chance to return a structured array of the data they want appended to the node. This includes #weight information, possibly a #theme callback, etc. For example, the upload module might do something like:

$data['files'] = array(
  '#weight' => 20,
  '#theme' => 'theme_upload_file_list',
);

 // this is the data for a single file attachment
$data['files'][13412] = array(
  '#fid' => 13412,
  '#nid' => 744,
  '#filename' => 'img1.JPG',
  '#filepath' => 'files/img1.JPG',
  '#filemime' => 'image/jpeg',
  '#filesize' => 215892,
  '#vid' => 745,
  '#description' => 'image number one',
  '#list' => 1,
);

return $data;

This would be merged into an array o nthe node object, perhaps $node->data = array() or $node->fields = array().

The module responsible for the node-type itself (blog.module, etc) would be responsible for keeping the body, teaser, and any other necessary fields in a default entry in the data array, perhaps $node->data['core']. After all participants have had a chance to stick their data into the data array, hook_node_alter() is called with an op of 'data'. This gives modules a chance to muck around with the loaded data.

At this point, nothing more *needs* to happen. Some modules only want to load a node, not display it as HTML, and they wouldn't have to go any farther. Once theme_node() is called, though, the $node->data array is walked. Every time a chunk with a '#theme' value is encountered, the theme function is called and the chunk of the data array is passed to it, along with the node. The HTML that the function returns is added to the portion of the array as the '#html' element. Functions can return NULL, indicating that the data should be invisible. (some might do this if it's in $teaser mode, or something like that).

Once all the theme functions have been called, the result is the *same* structured array, but with fully populated '#teaser_html' and/or '#html' elements. Now the node module walks through the array, respecting the '#weight' elements, and builds up the complete HTML for the teaser and the body.

After THAT point, I think it might be appropriate to fire off a final call for filtering or something like that. Modules that dynamically insert data based on inlined tags or what not would be able to operate on the whole assembled node body at this point.

What would this approach accomplish?

1. Individual chunks of pre-rendered data, and fully-rendered data would be preserved.
2. Modules would have an opportunity to override raw data or themed data inserted into the node
3. Themed data would not overrwrite raw data
4. Modules would be forced to make their additional data fully themable
5. Themes that want to block out certain portions of module-added node data, or restructure the body of the node entirely, would NOT be forced to re-process and re-render all the other form data just to build $node->body again.

This idea is very rough, and I'm sure there are plenty of holes in it. But I think formapi has cleared a path for us, conceptually if not functionally.

After some thought, I realized it might make sense to include '#teaser' => TRUE/FALSE and '#body' => TRUE/FALSE as default properties. That would allow $node->body and $node->teaser to be built intelligently from the rendered pieces.

This type of dual storage of data and rendered HTML representation is how CCK handles things. Making the entire node operate that way would be a lot more consistent, and bring some of the same benefits to the entire system. A set of _node_alter, _node_post_render, etc hooks would allow the same level of flexibility that we have today while ensuring that only modules that REALLY need to hack the final 'post-processing' HTML would try to do so.

This is a very, very early and rough version of the patch. The changes are as follows:

hook_view() for module defined node types should build the initial $content array as modeled in node_prepare().

anything that imlements nodeapi $op 'view' should return, instead of rendered HTML, a formapi style array element or elements. '#type' can be anything supported by formapi (like 'markup' if you're sticking simple values in, or even 'form' if you stick a full form array in it). or, it can be the name of a theme function that will render the chunk to HTML later. '#weight' is supported, and nesting (ala formapi) is supported, so you can put things before or after the normal body text.

Further updates coming soon.

This version of the patch sidesteps some of the issues with rendering, and simply creates #markup elements. book.module and upload.module both work now, and upload.module demonstrates the new hook_node_content_alter. It gives modules a chance to reshuffle, alter, and otherwise tweak the $content array before it is becomes $node->body.

For modules that make simple additions to $node, all that's really necessary is returning a minmal '#type' => 'markup' array snippet rather than appending directly to $node->body.

Also fixed some workflow ordering issues that kept $node->log from displaying. Also, $node->log is now themable. (yay).

I just tested this patch against cvs head. I think this is something we really need. It fits with Adrian's original intention for FormAPI. It gives us a lot of power over node body/teaser rendering. It requires minimal code changes. It seem to almost be net loss in lines of code. The code style seems good.

so the big pluses are
hook_content_alter for changing content arrays before rendering
the power of form api goodies like #process, #after_build, #type
more consistant system for rendering node body and teeasers

drawbacks,
allota modules need to be brought in line. but this is far less invasive than the last formapi/renderingapi whatchamacallit conversion.

Most of all.. 'It works for me!(tm)'. +1

dopry, it's worth noting that #after_build, #process, and a number of other elements are handled by form_builder(), which we don't use here. form_render(), which is being used, respects #theme, #prefix, #suffix, and a few other things. It's been noted by Adrian that naming it render(), or something more generic, would make sense: there's nothing in it that assumes a form, just a structured array.

FormAPI was the first real application of the ambitious structured array concept. It took a long time to shake out the additional elements and flags that were needed to support a robust forms system, and all of them are handled in form_builder(). Viewing content is simple in comparison, but I think there may be a couple of flags that are needed. A hook for processing the full text of the node, after all rendering work has been done, would be a big boon. (And would make modules like inline much more reliable.)

As we look to use the underlying API for more tasks, I think it's worth breaking out certain things, like handling of hook_whatever_alter, #after_build, and so on into functions that don't assume the content array is a form, a node body, or anything like that. That's a matter for another patch, though -- just noting that this is an example of how many drupal subsystems can begin to benefit from work that we do in that area.

Performance also needs some careful attention, but the best way to approach that, IMO, is to implement decent caching support in the core form_render() function.

If people think that this approach for node_viewing is worth pursuing (and I'm very, very much in favor of it) I'll continue to expand the patch to include all the other core modules that implement nodeapi $op view and hook_view. The changes are simple but the potential gain for themers and module developers is huge.

most definately worth pursuing the full patch. great work, eaton.

RobRoy, I'm assigning this issue to myself if there are no objections. I'm working on a next revision of the patch that incorporates the following changes:

1. Provisions for modules that want to post-process the entire rendered node body (like inline.module)
2. Provisions for modules that need to see both body and teaser contents during their alter() operations (like premium.module)
3. Provisions for optionally caching chunks of rendered code for use by other subsystems (re-displaying a portion of the node in a block without re-rendering it)
4. Moving more core modules to the new system

subrsciption...
This sounds really exciting :-)

The attached patch offers a number of minor improvements and (unless I've missed something) converts all modules in core over to the new rendering method.

upload.module in particular demonstrates the use of a new form_render() tool: the #after_render handler. Like #after_build for forms, it gives modules a change to post-process the rendered output of an element. Something like inline.module would definitely want to use it.

A couple of things worth noting.

1. With this system, modules that use the old way of altering $teaser and $body won't break -- their content just won't show up in the node body until they're converted.
2. Themes that want to print one chunk but not another can set $node->content['foo']['#printed'] = TRUE on elements they want to hide, call form_render($node->content), and output it. This flag can be set and unset, and form_render called over and over, if they want to output different chunks in very different places in the HTML. In some situations, if the element is deeply nested in a complex node array, it may be necessary to unset the #children flags on the parent elements. There's a convenience function (form_strip_key(&$elements, $key_to_strip)) that handles that recursively, though I'm trying to think of a simpler way to do it. At least, using this system, it's POSSIBLE. It wasn't before.
3. I'm still looking for a good way to preserve the rendering stuff after the view work is done. Perhaps node_view should cache $content arrays the way node_load caches $node objects. That would allow blocks and pages to share some of the same work. Any ideas are much appreciated.

Also, I need to do some quick investigation into the use of #weight and how it should be defaulted -- I just realized that form_builder() does some work to keep things in order that form_render doesn't handle automatically.

I'd probably recommend that every module specify a weight for its elements, though -- putting things at the end versus the beginning versus 'right under the body text' is a pretty common need.

+1 on this new functionality.

Structured data is easier to theme.

Its coming along nicely...

few questions,
1) would unsetting #after_render work in place of adding #after_render done?

2) I don't see the reference to #printed in the patch. I thought there was already a #rendered or something similar. It seems to be something that can be shared by both forms and content output.

3) We don't have weight control features in output yet anyway. Its all appended/prepended to node->body anyway. I think that almost constitutes a feature creep for this patch. I think we should get the default node rendering using this technique first. Then we can refine it some more as we approach code freeze.

+1, and 'It still works for me!(tm)'

oh and it still work as has my +1.

dopry and I were chatting and felt it would be useful to clarify the new rendering pipeline for nodes:

1. node_view() calls node_build_content()
2. node_build_content calls $node->type_view(), or node_prepare(), if the first is unavailable
3. $node->type_view() returns a keyed array containing the base elements of the node. In the case of node_prepare, that's the body or teaser, and the log message if it exists.
4. node_build_content() calls nodeapi with op = 'view'. Modules have a chance to return their own keyed elements to be merged into the content array.
5. node_build_content() calls hook_node_content_alter(), giving modules a chance to rearrange, manipulate, or otherwise mess with the completed content array.
6. node_view() gets the content array back from node_build_content, passes it into form_render().
7. if any chunks of the array have an #after_render function set, the contents of that chunk are passed to said function for processing. Modules like inline or insert_view that do substitution based on keys can easily do their work here.
8. The completed content array is stored in $node->content, and the rendered output is stored in $node->body or $node->teaser as appropriate. Themes and modules that look in those places now won't break, but they CAN look in $node->content for discrete chunks if they need to.

As I wrote earlier : sounds great !

just a silly question :
shouldn't the 'form_' functions (form_render, etc...) be renamed to something more abstract ?

yched, yes. Adrian commented on that a while ago, and I think renaming (at least) form_render() to something more generic makes sense. That, though, is something I would like to keep for another patch. While it wouldn't be TOO complicated, it would touch quite a few other parts of Drupal and I want to keep this patch small and as focused as possible.

Updated version.

1. chx noted that the rendering function assumes anything without a #type is a #markup element, so those redundant declarations have been removed
2. #after_render was setting #after_render_done, in the same way that #after_build works. Since this is a post-processing function and isn't saved in the rendered array, though, it should always be run if the element it's on is re-rendered. #after_render_done is no longer set or checked.
3. upload.module had a couple of dumb last-minute mistakes. Among other things, it wasn't taking in the $content text by reference, so it was never doing its substitution. All better now.

This code needs reviews.

I like the general idea quite a lot, but I am concerned about performance. Can somebody provide some benchmarks?

Just ran some simple test with ab on my shared hosting server. I installed a fresh copy of HEAD and used devel.module to generate a few users, a pile of nodes and comments, then attached a few files to a 'test node.'

Before the patch:
Front page: 335ms avg, 296ms med, 3272ms max
Single node: 263ms avg, 234ms med, 1735ms max

After the patch:
Front page: 314ms avg, 270ms med, 1934ms max
Single node: 262ms avg, 233ms med, 1988ms max

I'm not saying it makes things *faster*, but at least on my shared hosting server the differences seem to be negligable.

with node_render_2.patch...
some ab testing results... -c1

requests/second mean time per request(ms)

100 w/patch 267.783
100 w/o patch 266.175

1000 w/patch 316.438
1000 w/o patch 284.984

There is a minor difference at 100 requests... There is a larger difference at 1000/requests. May be exaggerated by i/o bottlenecks on my laptop. But does indicate that there is some trade-off.

I think there is more room for caching improvements with the fapi rendering method. Plus the flexibility is also a key factor.

same thing at 500 requests

w/patch 269.560 ms/request
w/o patch 267.315 ms/request

I think the 1000 requests hits some bottle necks on my laptop. I don't think it is actually a 10% impact.

Subscription...

Very exciting idea!

Hmm, this could mostly replace filter tags, couldn't it? Although using filter tags as a textual representation when _editing_ a node might work nicely.

This wouldn't replace filter tags, really. They're best for indicating what kind of content is allowed inside a given database field, or processing fields in a way that can be cached and stored for later retrieval.

The post-render processing that upload.module is doing must, by definition, be dynamic because it happens when clean URLs are switched on or off, and possibly when the uploaded files are in the temp folder during previewing before the node is actually saved. For a lot of cases (like, say, bbcode filtering or stripping of javascript) the actual 'filter' mechanism is good. The #post_render step just allows modules that were previously string replacing on $node->body to do their thing more reliably.

Did some benchmarking. Found that on my old alpha POS that the patch was much much slower but that box does weird things. My amd64 had more interesting results.

Single nodes seemed to be very close in results. The following results where run with ab2 concurrency 100 1000 tests

Large node- 32ms without patch and 34ms with patch
Small node- 27ms without patch and 34ms with patch (strange consistency there)
Front page node list- .847ms without patch .862ms with patch

So no grand difference. There was a strange result when I bumped concurrency up to 1000 though. I only test the list but the results where 43ms without patch and 1.9ms with patch. This is pretty drastic so its likely to be some strange phenomena of my setup but I thought it worth noting and it was repeatable.

So there's my results for what they're worth. Its not a hard core test but hopefully it helps.

Also I really like the idea of the patch. form_api has saved me countless hours of work(while providing us all countless hours of converting). I have no doubt the same could be said for the same thing in node rendering. I'm really looking forward to playing with it in the image module.

--
James

a short line to register to this issue.

This is IMO a great improvement -purely conceptual, since I did not yet read thouroughly trough the code- for Drupal.

having hardcoded $node->body (or teaser) is far too blog centric. Another step closer to a framework.

^^that was me, with a wrong session.... Anyway, fwiw, I am trying to get something like this into flexinode too:

in your theme you can say= foo: render_field(123) -- bar: render_field(234) // baz render_all_the_rest()
A big problem right now, is that as soon as you print fields in specific places in the theme/node, you can no longer rely on your admin interface for flexinode or CCK, becaus if you add/remove fields there you need to adjust the code in your theme too.

http://pastie.caboo.se/7240

/**
 * Maintain a list of already rendered fields
 * Call this function for all fields you have rendered.
 * @param $field_id id of the flexinode field
 * @param $ctype_id flexinode ID of the content-type
 * @param $place 'teaser' or 'body'.
 */
function flexinode_register_rendered_fields($ctype_id, $field_id = NULL, $place = 'body') {
  static $unrendered_fields;

  //fill the registration variable if we don't get a field_id.
  if (!$field_id) {
    $unrendered_fields[$place] = array();
    $ctype =  flexinode_load_content_type()
    foreach ($ctype->fields as $field) {
      if ($type->teaser) {
        $unrendered_fields['teaser'][$field->field_id] = $field;
      }
      else {
        $unrendered_fields['body'][$field->field_id] = $field;
      }
    }
    return $unrendered_fields[$place];
  }
  elseif (isset($unrendered_fields[$place][$field_id])) {
    unset($unrendered_fields[$place][$field_id]);
    return $unrendered_fields[$place];
  }
  else {
    return $unrendered_fields[$place];
  }
}

is what I added to flexinode in a development (non committed) branch.

in your theme you can say= foo: render_field(123) -- bar: render_field(234) // baz render_all_the_rest()
A big problem right now, is that as soon as you print fields in specific places in the theme/node, you can no longer rely on your admin interface for flexinode or CCK, becaus if you add/remove fields there you need to adjust the code in your theme too.

Very true, berkes. I *believe* that this patch accomplishes exactly what you're trying to do. You can simply pass the part of the node $content arry you want into form_render(), and get the html back.

Both of our approaches share one problem: if there are any nodeapi based post-processing filters like inline.module, or in the case of the patch if there are any #after_render functions added at the very top level, they have to be manually applied to the fields by the theme. That's something I think can be solved, later, but I'd have to think it through.

Here's another subtly improved version of the patch, after a longish conversation with chx.

1. the weird helper function for stripping elements is gone. It was never directly used anyways.
2. more of the node preparation work is now handled in node_build_content(), and that function now operates on a byref copy of $node rather than just returning its value. It also has an optional last parameter, $rebuild. By default, if $content array has already been built during the page-execution cycle, it will not waste time rebuilding it. Passing in TRUE will force it to always rebuild.

This gives themes and blocks and modules that want to share a copy of $node a bit more flexibility (and the ability to render separate chunks without colliding w/the standard node view.)

Minor style tweak in node_view().

After much discussion on #drupal with chx, berkes, merlinofchaos, and a number of others, this revision makes some final changes in the workflow:

1. modules can again use nodeapi op $view to directly alter the node. BUT, instead of simply editing $node->body they are still expected to add elements to $node->content
2. hook_node_content_alter() is gone, and has been replaced by nodeapi op 'alter'. This gives modules a chance to both add their own custom fields to the node, AND modify the final 'fully built' node if they need to, without creating two very different syntaxes.
3. poll.module actually works. (ahem)
4. search indexing and book TOC generation use the new functions.
5. In general, anywhere anyone was loading a node, calling node_prepare(), and printing $node->body should really be calling node_build_content(), and printing form_render($node->content) now.

Re-rolled to patch cleanly against the current HEAD. Also,

1. hook_node(), node_prepare(), and node_build_content() now *return* a copy of $node that's been altered rather than altering the byref copy. Previously, only node_prepare did this. The byrefs were unecessary, as hook_view always gets a crack at the node before anything else.
2. poll.module was calling poll_view with 1, 0, 1 instead of TRUE, FALSE, TRUE. Fixed.

Re-rolled to apply with no fuzz or cruft against the new HEAD.

tested and code reviewed. sensational patch.

i don't doubt the RTBC, and this looks great. i'm just curious... why this:

+      '#value' => theme('node_log_message', filter_xss($node->log)),

instead of:

+      '#value' => theme('node_log_message', check_plain($node->log)),

otherwise, the patch looks fine (i'm sure moshe's review was more thorough) and applies cleanly to HEAD. under limited, basic testing, things appear to work fine in core.

mostly, i'm just asking about the output filtering, not adding anything real to the reviewing/testing for this patch... sorry i don't have time to do a more detailed review, but i trust moshe on such things, anyway. ;)

thanks,
-derek

The filter_xss call was the same one being made by the existing node module as it assembled the log message; My patching dealt specifically with the style of rendering and content-building, not the specific filtering/security implications of how we treat the log messages. It's quite possible that another security or filtering related patch might see fit to replace that with something different. For now, though... I keep filter_xss! :-)

Please review the consistency of your code comments. Some are proper sentences starting with capital letters and punctuations, others are not. Proper sentences are preferred, especially for longer comments.

The comment module also glues stuff onto the node body, not? People have asked me how they can show comments next to the node (eg, in the sidebar). Can't we clean up the following function in node.module now:

function node_show($node, $cid) {
  $output = node_view($node, FALSE, TRUE);

  if (function_exists('comment_render') && $node->comment) {
    $output .= comment_render($node, $cid);
  }

  // Update the history table, stating that this user viewed this node.
  node_tag_new($node->nid);

  return $output;
}

It would be a good use case ...

Dries,

I'd really like to see that happen. The comments case is slightly more complex, though. Right now the fully-rendered node on a page is made up of four blocks of data:

$node->title/metadata (added by the theming function)
$node->body/teaser (built by this code)
$node->links (built by older code and not considered part of the node body)
$comments (added by node->show)

If you're willing to see $node->links put into the $content array, and $comments put into it as well, I'm all in favor. It would definitely increase the scope of the patch and risk breaking a lot more external code, though, because all of those things would become part of the content array.

Specifically, CSS that's built based on the current tag structure (including most of our themes) may choke. I'll have to investigate that. In addition, if we make comments part of the node content we have to roll links in as well -- since they separate the two. The format of the $links array is slightly different -- among other things it doesn't use # prefixes for its values, and uses a completely different rendering mechanism. That was decided during the Great Links Array patch to keep things simple.

I'll take a closer look today and see what I can do.

Dries, what you ask is a severe case of feature creep. That may be a next patch. This one is good to go as it stands (after minor code comment stuff).

Here's a cleaned up copy of the patch with just the fixes to the code comments.

I tend to agree with chx that the integration of comments into this is a very big deal code-wise, and should probably be the domain of a second follow-up patch. If we go that route, I think we should talk about entirely different approaches to node rendering that give us more flexibility than 'is this a teaser/is this a page/should I show links.'

For those that don't like this patch/feature for some reason (I doubt there are any people) a short note why this patch is very good too:
Consider CCK (alike modules), who now only need to bother about appending data do $node->body. In fact: after this patch we will have nearly all CCK features already! in core!

CCK's use of structured arrays to store its fields was the inspiration for this patch, in fact. Part of the goal was to integrate CCK's way of doing things in a cleaner fashion, so it would eventually be able to benefit from clean teaser processing, filtering modules like inline, and so on.

Quick note. When I said:

I think we should talk about entirely different approaches to node rendering

...I meant that we should re-evaluate the entire use of node_show() to display a 'proper' node page, and see whether node_view should accept a more flexible set of parameters than the current $teaser and $page flags, etc. The actual rendering pipeline used in this patch isn't what I would want to change. ;)

Still has my +1, the comments look good now.

@Dries, fun things like placing comments in the sidebar will probably have to wait until we take this another step further and start doing full page rendering using formAPI.

I'll leave it to a wiser man to RTBC.

Dopry spotted a missing period.

I'mg going to tentatively set this back to RTBC, Dries. After looking over more of the system, I think that moving links and comments to this rendering mechanism is a good thing but something that, realistically, should happen in a next patch. It's part of migrating *page* rendering to the structured-array system, not node bodies.

If you disagree, I can go back to the drawing board, but I think it's an important distinction.

Perhaps I'm missing something, but I interpreted Dries comment above as simply suggesting something like changing the code in node.module from:

function node_show($node, $cid) {
  $output = node_view($node, FALSE, TRUE);

  if (function_exists('comment_render') && $node->comment) {
    $output .= comment_render($node, $cid);
  }
...

And doing the comment rendering earlier so it becomes part of the structured node content, like:

function node_show($node, $cid) {

  if (function_exists('comment_render') && $node->comment) {
        $node->content['comments'] = array(
        '#value' =>  comment_render($node, $cid);,
        '#weight' => 1000, );
  }
  $output = node_view($node, FALSE, TRUE);
...

OR maybe do it in comment_nodeapi?

Not quite. If I understood Dries correctly, he was asking if we could remove the special-case handling of comment.module from node.module entirely, putting the work of adding comments to the node display in the comments moduel where it (philosophically) belongs. I am all in favor of this idea. It would require greatly enlarging the scope of the patch, though, because of the way that the $node->links array, separate from the content, is currently built and placed between the node body and its comments.

It's a good idea in the long term, but it would basically mean that node_show() would be a do-nothing function, and that all existing modules and themes that interact with node_view() would have to change to adapt to the new presence of comments in the default $node->view list.

I'd prefer that work go into a separate patch, though, where its impact can be weighed separately from the general rendering changes to $node->body and $node->content. But as I said -- if Dries feels that is a showstopper and that integration of links and comments must be part of this patch, I'm willing to go back and hit it again.

I think we should go ahead and commit this patch at this point. The code has been +1'd by a lot of folk. Lets not add any other features this patch. Lets definately not set it back to needs review so we can add a feature.

dopry's right. there's already much goodness here, which should get into HEAD asap so we can start to take advantage of it (e.g. for CCK fields rendering as fieldsets in node bodies http://drupal.org/node/57483 or other improvements). the other changes being proposed are great ideas, but we should see if we want to tackle them in the next 4 weeks, or whatever release comes after that. it would be tragic if this work, as it currently stands, didn't hit core because we sent eaton off on a massive re-do-everything chase, which he might not be able to finish in time. in generally, i prefer more, smaller patches, with more limited scope, since it's easier to test, manage, back-out if necessary, etc.

thanks,
-derek

After spending about an hour or so tinkering with this, I will say that using this new rendering approach to decouple node.module from comments.module possible but needs a thorough architectural review to avoid performance hits and unpleasant workflow side effects.

node_show, right now at least, takes the already-rendered node (post-PHPTemplate theming, even!) and appends the comments to it. That means that nothing that uses node_view needs to worry about incurring the considerable performance hit of rendering a large stack of comments. That would change with this, and I think a lot more attention will need to be paid to developing optimal ways of selectively rendering and building sub-chunks of structured arrays. It's possible now but will need work if we make the approach integral to core.

In addition, node.module has tons of conditional logic for administration, searching, and so on all built around '..And if comments.module is there, do this too...' That's bigger than just rendering.

In conclusion, I think a 'decouple comments.module from node.module' issue/task can probably be opened after 4.8/5.0 is out the door and we have more time to work through the implications and work out the best answer.

Eaton et al: thanks for researching the comments issue. Let's postpone this for future work.

1. If the form functionality is used to render non-forms, maybe we should consider to rename part of the form functionality. It looks weird to read

-    $output .= $node->body;
+    $output .= form_render($node->content);

Not something super-important, but interesting nonetheless.

2. It also begs the question, is it useful to reuse the form rendering code, or does it way too much? That is, are we only using a fraction of the form rendering functionality? I'd have to investigate this some more to figure out the answer, but maybe someone has an answer ready ...

All in all, I do support this functionality provided by this patch. I just want to make sure we got the implementation right.

2. It also begs the question, is it useful to reuse the form rendering code, or does it way too much? That is, are we only using a fraction of the form rendering functionality? I'd have to investigate this some more to figure out the answer, but maybe someone has an answer ready ...

The form rendering in particular is very, very cleanly isolated from the rest of FormAPI's handling code. When I first started working on the patch I was concerned about the same thing and I was very very pleasantly startled. All the Forms specific stuff actually happens in drupal_build_form() and form_builder(). Given that, I agree that form_render() probably makes more sense as drupal_render().

I'd like to take a look at how we can make the infrastructure more efficient by centralizing specific kinds of tasks (like walking the structured arrays and setting/unsetting specific elements, calling specific functions, etc) that we may want to do frequently. With this patch we now have two uses cases for it -- forms and nodes -- and the possibility of others (links? comments? we'll see). it's a lot easier to see what things are commonalities, and what things are specific to a particular use of the structured arrays (like submission/validation).

A point of clarification regarding the rendering stuff -- early on, I also spent an hour or two picking Adrian's brain about the nature of the rendering code and the form processing code. It was explicitly designed to support these kinds of uses, so the clean separation makes sense.

Using the structured arrays to generate forms was just the 'biggest pill to swallow' from a refactoring perspective, and also the one where we were feeling the most pain in the 4.6 era. Creating another rendering pipeline would've been a tragic waste of a well-architected system. :-)

My only real concern (right now, at least) is the fact that themers still need to pay attention to the #post_render array when they manually spit out small pieces of the form rather than just reshuffling it. It's still a major improvement over the old approach, but it's an area for potential improvement down the line.

Attached is a version of the patch with one subtle difference: all instances of form_render in HEAD have been replaced with drupal_render. It's a simple switch but (in my opinion, at least) should make it clearer where the distinction between forms work and the generalized code lies.

Good idea.

excellent. drupal_render() belongs in theme.inc or common.inc IMO

An excellent point, moshe.

Here's one more re-rolled version of the patch. What's new?

form_render() is now drupal_render().
_form_sort() is now _element_sort() -- it was only used within form/drupal_render().

In addition, the following functions:
drupal_render()
_element_sort()
element_properties()
element_child()
element_children()

have been moved into common.inc. PHPDoc comments have been updated to make it clear that these functions are generic, not form-specific.

Some have suggested that a new file -- render.inc, or elements.inc, or something along those lines -- should be created. I can understand that reasoning, but I'm concerned that any more poking and tweaking and scope creep will just mean longer delays for the core rendering patch while we discuss the abstract philosophy of what we should name our rendering library. :-) This patch is already 3x larger than the one in #49, with no change in functionality, just function-shuffling and renaming. If there are any concerns about the viability of ANYTHING past the patch on #49, just commit it and leave these rename-and-reshuffle tasks for another issue.

Looks like this is ready for review again.

Get this in. Really.

Some explanation why I RTBC'd -- the patch was ready and the functionality move / search&replace could not break it.

Committed to CVS HEAD. This needs to be documented. Marking 'code needs work'.

Awesome, Dries. Thanks for the guidance and questions, too. The additional investigation regarding comment.module helped clarify quite a bit. I've attached a one-liner for CHANGELOG and am writing up docs for the 4.7->4.8 conversion guide.

Thanks for the documentation. Committed the CHANGELOG.txt change.

http://drupal.org/node/64279 has been updated with preliminary instructions on updating hook_view and hook_nodeapi functions for the change.

developer docs need to be updated too:

http://api.drupal.org/api/HEAD/function/hook_view

updated docs:

http://drupal.org/cvs?commit=46305

And for a modern idea inspired by this: https://www.youtube.com/watch?v=GAMo4kQK-Io