Refactor page (and node) rendering

subscribing

Excellent goals Eaton!

Subscribing.

Eaton, I think I've already made it clear to you that I love this idea. If it makes Ewoks dance, so much the better. :-)

Standardizing another system on an _alter() hook makes a lot of sense. I recommend that we avoid the issues that followed Form API with hook_nodeapi and remove the view and alter ops entirely to avoid confusion.

For step 2, the big question will be what works best for themers. I'm going to see if I can explain this concept to the themers I work with and get some feedback from them. I'll let you know what I find out.

Subscribe

subscribing. I'm VERY much in favour of this, and not only because of the signatures patch. ;)

So I just did an informal survey around the office. The sample group includes 1 programmer with about 4 weeks Drupal experience, 1 themer with about 6 months Drupal experience, and 2 themers with about 4 weeks Drupal experience.

I explained the goal, and presented them with three potential syntax options:

x($content['first_image']);

print x($content['first_image']);

x('content-first_image');

The general consensus seemed to favor options 1 and 2. None of this sample group had an issue with the nested arrays. One person made a good case that option 3 would be easier for really novice people, but all agreed that knowing a little PHP was important for themers anyway.

They also were in universal agreement that we need to be able to print node properties in the page template. We need to do this all the time. :-) (2/3 sites we've done so far, with more coming.) If we can make that possible with this setup, that would be golden.

Marked the comments/node_show() issue at http://drupal.org/node/87326 a duplicate of this one.

I am definitely behind this as well. This should complete Eaton's initial node rendering patch and make the world a better place. The implementation looks good, can't wait to test some of this!

FYI, this is something that I've run into many times and will make theming sooo much easier, especially once some CCK field integration is flushed out. Right now, I'm trying to hack this together with D5. I have a a helper module that adds a bunch of content pieces to a node using nodeapi('view') but don't have a clean way to pull a piece out of content and place them into a certain region of my node.tpl.php.

Say I want to have a "node panel" (2 columns on top, 1 big column in middle, and 2 columns on bottom). I could pull some panels CSS into my node-whatever.tpl.php, then include print drupal_render($content['top_left']); in the top-left region, etc. This would allow us to do a follow-up patch, to dynamically and panel to a node-type and then map CCK fields to certain regions for that node's panel. And we could do this all through the UI on the Display fields tab. We could map fields to regions and weight them just like the panels admin interface.

Ooh, way down the road, we could even include all the submitted, node title, comments, links, etc. and have a weighted node field UI and we could rearrange all that stuff through the UI allowing us to customize node display per node-type. We could pull the "Show submitted info" out of the obscure themes config page and into the node display page so it's all in one place...but don't let me get ahead of myself.

Clearly, this is H-U-G-E and paves the way for a much more usable and extendible Drupal. Eaton, as always, you're the man!

Wanted to jot down some more of my excitement for this goodness. It's really cool that we will be pulling links, submitted info, etc. into this. I was just doing some content building in nodeapi('view') and started to add some link stuff into nodeapi('view') as well then realized that I had to go to hook_link() and add the links. Those are both just groups of content and getting rid of the hook_link() et al will just make it that much easier for new devs to work with Drupal.

Subscribing.

As a themer who's been wrestling with these sort of issues recently I'd love to see this make it in to 6.x. In conjunction with the new theme registry it would give us an incredible amount of flexibility.

My PHP isn't strong enough to get involved in the coding side but I'm more than willing to test patches and help thrash out any issues from a themers point of view if needed. I'm in #drupal-themes (nick: garrettc) most days if you need to get hold of me.

Very interesting.
Subscribing.

Would hook_node_alter() also get to do different things based on the style, or is that bad mixing of display logic and data logic? (I can see a performance benefit, as it can skip SQL calls, but that also reduces the flexibility for some theme layer styling.)

Subscribing. I like where this is going :)

subscribe

$node->terms might be replaced by $node['meta']['terms'], for example, so that $node['meta'] can contain data added by other modules like technorati, etc. What kinds of 'standard chunks' should it have?

Ah, now this is an interesting idea.

...thinking out loud here:

Under $node['meta'] we could have 'terms', 'author', 'created' and 'updated'. Under $node['content'] we could have entries for 'teaser' and 'body', or in the case of a customised CCK type, an entry for each field.

Would we want $node['links'], or is that being too specific? Should they sit under $node['meta']? My gut says no as they're not really information *about* the node, more supplementary content *for* the node. We need to be careful that 'meta' doesn't just become a dumping ground for everything that's not in 'content'. $node['supplementary'] perhaps?

Information architecture geek, that would be me then *:)

I've got some spare time this weekend so I'll bust out Omnigraffle and see if I can come up with something rough for us to start playing around with.

Re #4 and what to call 'links'. I hit this issue a while back on my own site (link in my profile) and ended up labelling it "tools" which was the closest fit I could think of.

subscribing

Okay, so here's a first pass after giving the problem some thought.

1. I've created both a 'properties' and a 'meta' section. But I'm now thinking these are essentially the same thing
2. I really don't like the name 'additional'. I'm hoping if we can get some clear use-cases for this section then a better name will present itself (or even if we need it at all)

One thing this has highlighted is how limited my knowledge of the steps that go into constructing a node is. I've written a few very basic modules that only deal with one or two hooks at most, so if anything in this diagram is stupid, then that's my stupid and no one elses *:)

We might also want to keep an eye on what's achievable for v6 and and possible roadmap towards v7 for anything that we can't do right now.

Interesting. Based on that diagram, I would say:

- Ditch 'meta', and stick with 'properties' ... we did away with .meta files, and instead went with .info files because it's an easier term. And heck, we already call this content 'nodes' so no need to make it more complex again. ;)
- Ditch 'links' and go with 'additional' (or 'supplementary' or something...)... this would allow us to make a distinction between what is actually part of the node content, and what is being added to do something *with* the content (voting widgets, links to go elsewhere...)

Terms I would put in 'additional' since it's not part of the content, but rather information about the content. But not sure. There is definitely a lot of murky grey area. I guess you could even ditch everything but 'properties' (information about the content) and 'content' (the content itself), if you wanted a truly simplistic view.

subscribe

I prefer your diagramming, it's quicker *:)

The idea of #properties had me confused there for a while. Is there a reason why those particular elements are treated that way? I understand it in the case of FAPI as something like #collapsible isn't renderable as such, but is a state, or an attribute, of the element.

It seems a bit strange to still treat things like the title and the uid as special cases when we're talking about making $node much more structured and malleable. The title is part of the content no? And as a themer, I would like to have the ability to render out pretty much anything in the node at some point. If I know I'm just dealing with arrays then that's easier for me than having to learn that it's 90% arrays, but there are a few special cases.

(I'm not from a programming background so if this is a lack of understanding on my part then that's cool. If it's a case of "well we could but it's too much work for v6" then that's cool too. I'd rather get part of the way towards a solution for v6 than nothing at all.)

I agree about 'footer' feeling wrong for exactly the reason you've said, but like 'additional' it's tricky to think of something generic yet descriptive. I almost want to call it 'attached' but that's going to lead to too much confusion with file attachments.

So what do we know so far? Nodes have:

• Metadata: author, created date, modified date, taxonomy, ratings.
• Content: Comprising of a title, then various other fields built from core/CCK and additional modules. Examples: amazon tools, file attachments
• Links: Tool or task orientated items that affect or involve the node in some way. Examples: add a comment, servicelinks module
• Additional: Things that don't fit into either of the 3 sections above. Examples: comments, signatures.

This is making my head hurt... Webchick's idea about just having the two master arrays of content and not-content is looking more attractive by the minute *:)

Hm, yeah, not a big fan of footer. Related is OK, but I think I prefer webchick's additional or addon or something since I think we are basically saying these are things we are sort of appending to the node. Related sounds more like they are standalone things that just happen to relate (as if comments were nodes or something for goodness' sake ;)).

Maybe add1sun has the right word there: append? That implies both "add" and "after", which is the most common workflow.

content: The node itself.
info: stuff about the node itself.
append: stuff added to the node that's not the node itself.

Themers may choose to display stuff before, after, on the side, or none at all.

So how about just "extra".

Rather than append, how about just 'pre' and 'post'?

1) Extra works for me.

2) I'm still not sure why links gets top billing and other stuff doesn't, but I have no strong opinion.

3) Would modules be able to add new top-level elements, or would the 4-part breakdown be enforced somehow (beyond docs saying not to add more of them)?

I'm starting to lean favorably towards "info", "content", "links" and "extra". With comments being in the "extra" chunk.

That works for me. And it's not like we can't tweak this further during the v7 development cycle after we've got some practical experience with it.

@Crell: I think the rational for links being a top level entry is that it fits in with the exisiting hooks in core for manipulating that data, so the impact of the changes is minimal.

Subscribing.

I would like to see a $node['url'] option as well, which would allow module developers to override the default node/1 to something like node/1?option=true, and the theme would not have to guess at the destination of the teaser header for instance.

Maybe I wasn't entirely clear. But perhaps this needs to happen more upstream.

The basic idea is this.

If this node's nid = 1, and I would like to link to this node's page, I would create a link like this: 'node'.$node->nid. The problem is that currently it happens in the theme's page.tpl. If the property $node->url existed (or as a structured array: $node['url']), it could be overridden before it hits the theme.

Does this will help outputing things like pure XML to bound drupal content into external web services or other fancy stuff ?

@Eaton, I have a few cycles to help on this this week. Does it make sense to assign me a couple lower-hanging fruitish tasks that I can power through?

Here's a re-roll that applies to HEAD.

Notes:

• There was some user_access('RSS feed') stuff in there which didn't apply and I didn't try and put it back. Don't want anything holding this up. (Perm should be 'access RSS feeds' instead IMO, but let's leave that for http://drupal.org/node/62007).
• Also, some code in there for if ($page == 0) { "!title by !name"... Wasn't sure what to do with that.
• Some of the theme_node hunk was left out as a lot of that has moved, needs a closer review probably.
• In template_preprocess_node() you were overriding 'node' at the end so the tpl files didn't get the structure. We need to also look at tpl variable name space pollution, like in that foreach loop. I'm still a bit fuzzy, so will have to look again later.
• I started to remove node_show but didn't get too far, so put it back, but left out the cid arg (does this even get used? if so we need to put this functionality in comment.module somewhere).
• Got a little bit of the way with node.tpl.php (started with Garland for ease, let's do core/others after). So comments work, but have to force the render for now since #printed == TRUE, I guess since the render happens once in node_prepare? A whole lot of rendering going on, still fuzzy on when to render/not render and print the already rendered stuff if ever.
• $node['links'] need work and I didn't really touch anything else besides comments.

Hope this helps a bit until I can get another look.

I'm a little bit fresher now, no time to code on this this morning. What happens when a module adds some content to $node->content['foo'] and I want to pull that out in my theme? Since if I do a drupal_render($node->content) above a drupal_render($node->content['foo']) it will be shown only on top. If I use the new $force = TRUE, it will be shown twice. So the plan is to make people do a $foo = drupal_render($node->content['foo']) in their template.php for example?

Also, it seems like there is a lot of duplicate rendering going on, with node_prepare() and template_preprocess_node() and then in the tpl file itself. Then we have similar functions for displaying the node theme('node'), node_view, node_show, node_page_view, etc. I just want to know what your ideas on this are on consolidating some of this. I'm thinking we want to just pass the structured node object all the way to theme('node') then render pieces in the theme layer, but I'm sure I'm overlooking something.

Enlighten me Dr. Eaton.

Yeah, I don't think it is too much to ask a themer to learn a few helper functions. I say something like hide(), show() just an alias for render($var, TRUE), and render(). I think I need to get on a discussion to figure out what the hell to do with node_prepare, node_show, etc. Are we just scrapping all of that and only using theme('node')? If you have a good idea how to proceed with that and want me to tackle it, let me know here as I'm liable to dig myself in a hole and I have been postponing real work a bit too much lately. :P

All I can do on this patch for now is add emotional support. Go RobRoy and Eaton! Thanks for working on this important patch.

Also, it seems like there is a lot of duplicate rendering going on, with node_prepare() and template_preprocess_node() and then in the tpl file itself. Then we have similar functions for displaying the node theme('node'), node_view, node_show, node_page_view, etc.

I'm going to be a bit of a dunce here, but after following this thread and reading Merlin's post with a themeing example from drupal 6 I'm getting confused about how the relationship of events involving render(), _preprocess(), hide(), show() etc is going to play itself out.

It does seem like we're ending up with many possible paths to the same result (separation of business logic from the rendering of content), but I could be missing something which is leading to my confusion.

Nine times out of ten, any complex theming work requires discarding the rendered version and manually printing out the node, property by property.

Yep, I have a lot of tpl files that contain things like:

print $node->content['body']['#value'];

Navigating all those nested arrays gets to be a pain. Sometimes it's #value, sometimes it's view. Lots of print_r required. Do I need check_markup() on the #value of a cck field? etc etc

merlinofchaos's theming patches help eliminate a lot of the security holes that come along with that approach by acting as a 'filter' layer between that raw node and the theming layer.

So this is the preprocess demonstration on his site, and if I've got this right, also the place where we can do a lot of business logic and, for example, eliminate if() statements from our tpl files.

The wrapper functions like render(), could be used in the tpl file for people who didn't want to bother with the preprocess stage but just wanted to safely print out discrete chunks of the $node?

I had a quick look at this patch to give you my initial impression/thoughts -- it is certainly not a definite look, but it might help you make some progress or it might spark some ideas/discussion. I intend to have a closer look at this soon.

1. The $force parameter is a bit weird, and the PHPdoc wasn't clear to me -- it doesn't help me understand when or when not to use it. I'll want to look at that more to understand the need for this flag. I think I'd rather not have it this flag (it sounds hack-ish) but I have to look at the code some more to be sure. That or the PHPdoc should be a little bit more explanatory about the use-cases.

2. There are some tabs in the patch's code. Doesn't really matter -- I can remove those.

3. There are two TODOs in the code.

4. It's weird to read $node['#node']->comment -- one would expect to see $node->comment. In fact, we have both.

5. The use of # seems to be inconsistent -- it's $node['links'] but $node['#title']. Is there a reason that we write $node['#readmore'] instead of the simpler $node->readmore? I'm sure there is a good reason for this, but for someone who doesn't "see" the underlying reason, this looks sloppy rather than smart. ;) Also, I currently don't understand the underlying reason.

6. We still have node_prepare($node, $teaser), node_build($node, $teaser, $page), drupal_render($node), etc. Shouldn't node_prepare() be eliminated in favor of the new theme preprocesser stuff that Earl implemented? Is that the long-term goal or should that be part of this patch?

7. We still have "node api view" although that seems deprecated now by drupal_alter('node', $node, $teaser, $page)?

All in all, this patch takes us in the *right* direction. I believe this is a very important patch, especially in the long term. This patch is key, and every developer should forced to review this. :)

However, at this point, it somewhat feels like this patch leaves us very much in an intermediate state that is a bit confusing for the developer. There is a lot of preparing, altering and rendering going on at various levels.

As I said, I only had a quick look at it. To fully understand what is going on, I'll have to spend some time investigating the surrounding code. If possible, I'd steer towards making some of this stuff easier to understand and use -- possibly be deprecating some old mechanisms or removing some crufty code that can be simplified now (if any).

Either way, this is an important patch, and I plan to have another look at this soon! Great work guys.

working towards theme nirvana...subscribing

Following up from IRC...

regarding the $force parameter:

drupal_render() is smart in that it tracks when it's already output something, and won't output it again. This is very useful, because in form theme functions, you can control the order that fields are rendered, but also just print out "the rest" at the bottom to get tokens, submit buttons, and whatnot.

The $force parameter would indicate, "No, I don't care if I already told you to render this, do it again anyway." So this would allow someone to print the same node property twice in their node.tpl.php file.

We discussed on IRC and are pretty much agreed that this would be a seldomly-used feature, and the API would be simpler without it. It would affect those who are putting things like print $links; above and below the node body... they now need to store the result of a drupal_render into a variable and print that variable elsewhere in the page.

As webchick noted, this is an unnecessary flag and can be eliminated.

Wait. We still need something like this (Eaton, you still thinking about using hide/show?) or to agree to a workaround. If there is a chunk of content in $node->content that you want to print on the footer, when your node.tpl.php hit drupal_render($content) or whatever it would print that chunk out, so you'd either need to do something like this at the top of your tpl:

$footer_content = drupal_render($content['foo']) or hide($content['foo'])

then

print $footer_content or show($content['foo']) at the bottom of your tpl.

subscribing.

Status, anyone? sure would be swell.

subscribing

Subscribe

Latest patch still applies (with some offsets). However, whenever viewing a node with the patch applied, instead of its content, just "Array" is printed. So, it's somewhere broken. I tried to look into it, but haven't found the broken part yet. Eaton?
PS: How are chances on still getting this into D6 somehow?

Eaton, if that's the case then we're still waiting for an answer to Frando's question ;)

+1 on this idea
Very useful for theme development

Alan

Subscribing (with hopeful tears of joy in my eyes)

Keeping tabs.

This would have helped a lot for converting user profiles into template files. It'll be littered with drupal_render() when it's customized.

subscribing

subscribing

Subscribe.

I spewed a bunch of thoughts over at http://drupal.org/node/145551#comment-321310 and Moshe pointed me here. It seems (not surprisingly) like Eaton was several months ahead of me on these ideas, but there is one part that either's has been mentioned here yet or I haven't seen it called out explicitly.

Currently, and in the last version of this patch, menu callbacks return rendered content. Starting up at index.php, Drupal's logic is: (a) call the menu callback, get back $return; (b) if $return is an error code, display an error page; (c) print theme('page', $return). So right now, for example, the D6 function node_page_view() still basically returns theme('node', node_load(arg(1)).

I assert that menu callbacks are for business logic: turning the path, _GET/_POST data, the database, and whatever else into a digested format that is easy to render (read: a clean data structure). The theme layer is for turning that digested data into rendered output in whatever format is requested: HTML, JSON, XML, Braille, whatever.

Therefore, I think menu callbacks ought to return structured data, not rendered data. The last line of node_page_view() should basically be return array('#theme' => 'node', '#value' => $node_data_array). 'node' in this case is the recommended theme hook. $node_data_array is a drupal_render()-style nested content array with #value, #theme, #weight, etc. For example, it might contain something like:

$node_data_array = array(
  'title' => array('#theme' => 'plain_text', 'Drupal rocks.'),
  'body' => array('#theme' => 'check_markup', '#format' => 1, '<p>CNN.com reports that 37% of all web sites are using Drupal. Blah blah blah.</p>'),
  'comments' => array('#theme' => 'comments', array(....)),
  'links' => array('#theme' => 'links', '#value' => array(
     'add_new_comment' => array('#path' => 'node/1/comment/add', '#title' => 'Add new comment'),
     'subscribe' => array('#path' => 'subscribe/1', '#title' => 'Subscribe to this post'),
  );

So, every element contains an optional #theme and #value or other #elements for data that the #theme function expects.

index.php's logic then changes. (a) call the menu callback, get back $return. (b) Decide based on the path, HTTP headers, whatever, which output format (HTML, XML, etc.) is being requested, call that $format. (c) print theme_$format_page($return).

theme_html_page() would invoke the page template and, where currently print $content, it would print drupal_render_html($return) (though the theme preprocessor for the page hook would already have set $content = drupal_render_html($return)). The default theme_html_node() would just return drupal_render($node_data). But you could override it, e.g. for the "comments come first theme":

comments_come_first_theme_html_node($node_data) {
  return drupal_render_html($node_data['comments']) . drupal_render_html($node_data);
}

If theme_$format_$hook doesn't exist, them theme_$format gets called, so (e.g.) theme_json() ends up getting called for basically everything when JSON output is requested. But if some particular module needs a custom JSON output for some particular data, it can define theme_json_somehook(). Or a theme could override it with mytheme_json_somehook().

I'm intermixing the purpose of drupal_render() and theme() here; clarification is needed. The summary of the idea, though is this:

1. Menu callbacks return structured data that indicates the theme hooks for each piece. Not just node_page_callback, but every callback.

2. Rendering into the requested output format happens completely outside the menu callbacks. There are a parallel set of theme functions for each possible output format. HTML will have the most. Hopefully, tagged-data formats will almost always get by with a single theme function plus a handful of exceptions.

Does this fit with what has been under discussion here?

I like this, but I'm tired of arrays being used as objects with undocumentable magic keywords.

If we're going to go with this, I'd like to have a $page object that we seed with this data instead, or something along that lines.

subing

Subscribe

All right, let's make this happen for Drupal 7! We have an all-star cast of subscribers--- Can someone smarter and more organized than me summarize where we need to go?

As most here, I think, I'd love an array or an object that represents "data to be shown" that is then accessible for modification at the theme layer no matter what the output type is. That part of bjaspan's proposal makes a lot of sense to me, theme_this being handled by theme_html_this and theme_feed_this and theme_xml_this and theme_whatever_this.

So the system ought to be able to envision example.com/node/23/voice just like we can add /feed to taxonomy terms, and a module implementing that would be doing server-side text-to-audio or something. I think that also means modules need to get their shot at putting functions into the theme namespace.

All this highlights something that bothered my sense of clean separation from Eaton's original specification:

Replace the teaser/full boolean flag with a richer 'style' parameter, supporting four options by default: teaser, full, print, and feed. Allow other modules to expose new styles for their own purposes.

Teaser versus full is a another question, in my opinion, that should stay in its own flag with 'style' or 'output' or whatever being handled as above or otherwise separately: html, print, json, xml, custom can all be either teaser or full.

Putting it in the theme layer gives us handling /user/123 pages etc. sort of for free.

Other things to keep track of: a place for nodes via theme and/or modules to affect breadcrumbs and site title and other page-wide variables. Logically these would be theme functions connected to the menu system, which defines unique paths...?

And performance implications of everything!

There, that and $1.50 will get you a Euro.

Subscribing

Larry Garfield puts into a code example the style versus mode aspects (coincidentally) brought up here, embedded in a much larger proposal for a Data API:

function theme_node($node, $style = 'html', $mode = 'full') {
  foreach ($node->fields() as $field) {
    $return[$field->name()] = theme('field_'. $field->type(), $field, $style, $mode);
  }
  return $return;
}

I'm all right putting this in the theme layer because is about display, broadly, and Drupal's great approach is to put the theme functions in the module code, and only override them if necessary.

I still like bjspan's idea in #78 of extending the theme_something() to be theme_style_something()

I agree that the menu system, either with an extensios like .rss or just /feed on the end of a path should decide how we display something, but it should be a theme_ function that does the actual display.

The thrust of this thread is to make a big array or object available to the themer if they decide they want to do something different.

And now maybe I'm getting silly and unweildy, and proving Jeff's point, but I often want to override another module's theme_ from my module.

This way I can make a module that creates mymodule_theme_morse_page(), mymodule_theme_morse_node() and on and on to output entire sites as Morse code.

We wouldn't even have to declare anywhere that we created a new output style-- just call something through the menu system with morse on the end, and theme_morse_ functions are tried first. And now I see Jeff's point about doing this with a .ext rather than a /ext to separate it from variables passed in.

I like the idea of using .ext (.rss, .xml etc) to control content format - if we wanted to get really fancy we could also use 'accept' http headers here too ;)

In terms of extending the theme_ callbacks in this way, I am also a little concerned about that, because a lot of the renderers for structured content types really want to work on a much more granular level. That said, if the theme function found no function at a particular level it could just pass the structured array up a level, and so you could essentially define a new format, and a single page level theme function that had access to the whole array. Other formatters (e.g. RSS) could call node level theme functions to get them into html format and then iterate over them at the page level.

subscribing...

Wildly interested. Subscribing.

Subscribing, with comments about Comments to follow...

Great ideas thus far!

Subscribing, +1.

Also, I believe when rendering nodes for different purposes, it may be useful to apply different markup filters to the node body. I'm not sure how best to address it, but I'm throwing the thought out there. Perhaps this structured representation should include the body and teaser before the filter is applied (as well as after the filter).

nothing of value to add, but I want to keep up with developments without just posting 'subscribe'.

I hope a lot of headway can be made on this at the Drupal Code Sprint These ideas are awesome!

Looks interesting

Wow, this, #218770: Drupal Pipes, and #145551: Enable loading and rendering into multiple formats (html, xml, json, atom, etc.), are going to make for some interesting conversations in Boston.

track

Subscribing.

Subscribing.

subscribing

way late to the party. subscribing anyway.

Eaton: Both links and comments should be added to the $content array, I'd think. There's no reason either of them should do any sort of string concatenation.

The other real killer is that with CCK fields, if you want to print *one* of them separately from the $content structure then you really have to print all of them. You can't just remove one from the $content variable, as far as I'm aware.

Just that much would make node rendering a lot more consistent, in preparation for later efforts to make it rock (this or otherwise).

@Eaton - nice summary. Lets just nail #1 as soon as possible (will you work on that)?

If you can nail that, I pledge to work on #2. I really would love to delay node render to the theme layer. I agree that this is a stretch for a May code freeze but not September.

I concur with option 3. I'd rather have extra layers of indirection with flexibility we never use than something that isn't flexible enough because there aren't enough layers to hook into.

subscibing

RE #105, I concur that the full element type for links is best. Its important that modules can alter this structure reliably.

RE #102, my personal pet peeve involves wildly different output formats. But I agree these are seperate issues and it makes sense to tackle them independently. Improvements to any of the three is much appreciated.

Subscribing.

Subscribing. should consider performance improvements. the shortest (relative) path to rendering a node.

Subscribing. Also #217806: Node Module Implements Comment Module has been marked a duplicate of this (great title though).

regarding eaton's #105. i think we can make #2 work. with that option, we "Use a 'links' element #type, which would map the renderable element straight to theme_links()." In order for that to work, we could use a preprocess function for the theme('links') hook. that preprocess will only do anything if it receives a fapi style links array. it would do the transformation you mentioned so the data can be consumed by theme_links().

i might take a stab at this if i feel frisky later. there are only 11 calls to node_view() in core.

One more reply to Eaton's comment from ages ago:

The other trick is that quite a few modules use nodeapi op view as a place to set breadcrumbs, change the sitewide title, and so on. there's still the need for SOMETHING that gets called at that point. Not sure what makes the most sense for that, though.

that something is now a preprocess function: MODULE_preprocess_page(&$variables). see comments above theme().

re #105: why not modify how theme_links() takes the links, so we can pass the data in directly?

On theme_links I just opened a feature request for grouping of links (admin links, visitor links, etc.) - if we're going to break it anyway we might as well get something out of it. http://drupal.org/node/255686

Subscribe.

Subscribe.

subscribe

What's the status on this? It's been over a year since the last patch...

Please, please get this into Drupal 7.

I can't remember how many hairs I've pulled out while theming nodes.

So, even before reading Barry's post, it was clear to me that looking at this issue: http://drupal.org/node/145551 we needed a more fundamental solution. I think the tact of returning structured data from the callback is the right approach With forms, it's easy to do with minimal changes.

The thing I've been struggling a bit with is that some types of output (e.g. JSON) are fundamentally different from XHTML/XML/RSS/ec. It seems that with JSON you need to keep the data as an array until the final step (like theme('page')) when you encode the entire structre. In contract, string-based output like the current XHTML can be concatenated as you build it up as the current drupal_render() does.

So, I'd initially been thinking about how to overload the current theme() system, but it almost seems as though you need parallel output systems and at least a couple versions of drupal_render (string vs. array). For example, If I want the full page in XML, I'd imagine the final output of the links (or whatever) should not be the html version. Perhaps drupal_render calls theme_xml('$element['#theme'], $element)?

here's a patch which rolls in a little of render-switching code in from http://drupal.org/node/145551 , and starts implementing the ideas above. Most pages still work even (after many rounds of re-coding, re-thinking, and stopping to write the above).

The render switching-code in drupal_render_page should possibly do some sort of module-invoke-all to finder the rendering function, rather than using a magic naming convention, but I'm open to ideas.

Review, perhaps?

After a some thought on the subject. It seems that make sense to generalize hook_nodeapi()'s $op = 'rss item' so it wouldn't be specific for RSS rendering, but also provide the ability for modules to add custom (sanitized) data to the node element which is about to be rendered in other formats than html. This way we could move RSS to its own renderer. And maybe another function would be needed, for modules which implement node types but not hook_nodeapi().

Does that make any sense?

Well, in D6 we now have the node build mode flag, so I think most of these special ops for hook_nodeapi should go away I think.

I think we are going to need something parallel to the "theme" functions that provide clean output.

changing title - really refactor "everything" rendering :-)

Here's another patch that shows where I am now. I have not started on the node part yet - I want to look at some of the previous code.

subscribing

pwolanin: I have only looked that patch over, not tested it, but you're calling render() on everything in page.tpl.php except the right sidebar, where you call drupal_render().

@Arancaytar - thanks, there is still a bunch of stuff in the tempate files that will need to be fixed - please note the name of the patch :-) This is at best ~20% of the work.

subscribe

Unsubscribing.

Is there any progress here that can be reviewed as #129 says 20% of the way there?

This patch needs to be re-rolled. Also, this related patch sends to be re-rolled and pushed forward to make the rendering process a little cleaner: http://drupal.org/node/252013 That's a really simple patch to work on if anyone has time.

Re-rolling the patch and fixing a parse error in theme.maintenance.inc.

@pwolanin: I think your patch got slight out of the scope of this issue. While it's tempting to refactor "everything rendering" in the same patch, I guess it's more likely to get in faster if we focus on getting node rendering more flexible for themers. Surely the other rendering patch will benefit from this one. But I think those patches can finely be developed in parallel. So why we don't a) focus on more flexible node rendering on this path and b) leave the renderer discovery and page rendering to the other issue?

While this patch is not complete yet it needs review on the approach. So changing back to CNR.

@recidive - ok, let me clarify. I only expect this patch will really refactor page rendering, and node and form pages. Things like user profiles can wait for later patches, but this patch will enable their refactoring.

I was eyeballing this patch, and it doesn't look flexible enough to handle what I am trying to do over at #193311: Ajax Popups in Drupal 7: Adding Modal Dialogs to Help, Confirmations and Filter tips (Unified). The drupal_render_page needs to be able to handle rendering the page in more than one json format. You can look at Nate's patch over at http://drupal.org/node/218830#comment-791590 for a way to do this.

All the changes so far in this patch look pretty good to me. Putting things into $output[] instead of concatenating everything is nice and tidy.

I'm also not quite clear what the scope of this patch is, or how exactly it relates to the other one - is the idea to get this in, then the rendering patch, then popups? If we only fix node rendering here it'd be a major achievement.

It needs another re-roll for #value/#markup changes. What's the next step with this?

@catch - yeach, so now that we have #markup, I want to re-roll this.

I agree we need some way to make parallel "theme" systems as pointed out in http://drupal.org/node/218830#comment-791590, since (e.g.) we might want to render some other XML rather than XHTML. And I had some similar thought about "theme modes" or some such declared by a registry hook, so I think nedjo are basically thinking along similar lines.

The approach I was thinking about is this:
any theme function must have as it's first argument a renderable structured array. As it's second, option, arg is the render mode which defaults to XHTML. Function named theme_hook would render XHTML. Functions named theme_MODE_hook would render in another mode. drupal_render would take the same argument. So as long as all theme functions pass this additional argument down, we avoid the need for a global. We can also then have mixed rendering in some cases (?). I am not so fond of hacking this parameter into a global.

I can help with testing the patch (when it will be available). subscribing...

It needs another re-roll for #value/#markup changes. What's the next step with this?

@catch - yeach, so now that we have #markup, I want to re-roll this.

IMHO this is CNW...

yes - it's CNW for now.

this is still very much CNW, but something new to look at.

Starting to transform theme() calls so that all pass a single parameter. However, forms are all still broken.

Actually, making that a blanket change to theme is too much work and not really that useful.

Making some progress on conversion of theme('item_list') and theme('table') calls.

Also, what I wrote in #138 re: parallel theme modes is not what I actually started putting in the patch. Here we have a param to drupal_render that lets you substitute a different callback in place of theme().

Rerolled patch (haven't had time to test it out - hopefully this week...)

Rerolled to head to remove some fuzz. I was still getting a blank page at admin/build/testing, not sure why...

Just found this issue while looking at #306358: Add $classes to templates with new theme_process_hook functions and trying to figure how to get a data structure passed through the preprocess functions without either rendering it too early or sticking a specific PHP function that renders it inside the tpl. i.e. I hate code like the <?php print theme('links', $primary_links, array('class' => 'links primary-links')); ?> that sits in page.tpl.php

I like the <?php render($variable); ?> functions I see in the tpl.php files of this patch. Manipulating data in the preprocess functions instead of strings is a big +1. But I don't like seeing render() mixed with print in the tpl files, ala <?php print $varA; ?><?php render($varB); ?>. That's going to lead to confusion rather quickly.

/me has never tried converting <?php print theme('links', $primary_links, array('class' => 'links primary-links')); ?> to a drupal_render() call. :-\ Can you do that?

@pwolanin: Hi Peter, I keep meaning to ask, how was your talk at DrupalCon recieved? I watched the video, but I couldn't hear what the audience was saying during Q&A. Is there an emerging consensus that this should be done, and the correct approach, or are we still in the throwing around ideas stage?

I think now would be a good time to look at breaking this into smaller chunks, because I am sure that this patch (once done at least!) will count as eating kittens to Webchick :)

I am not sure what is a sensible unit to break this into, or if there is a way we could convince the 2 rendering systems to coexist for a while so we could complete the other patches module by module.

MerlinOfChaos's words from nearly a year ago still bear repeating:

I like this, but I'm tired of arrays being used as objects with undocumentable magic keywords.

If we're going to go with this, I'd like to have a $page object that we seed with this data instead, or something along that lines.

I've gone down the path of creating giant $page[$region][$block][$element] arrays, and the fact that I've returned to the land of HTML templates should be telling. They prevent me from having too many ways to alter any given element on a page, and keep me honest in the sense of sending the right variables to the template file, instead of passing giant objects that are so large that my web browser locks up if I print_r them.

Now, I'm an idiot, but whatever we go with, it shouldn't encourage people to be as irresponsible as me with how they go about changing a node's title element.

@Starbow - it went well enough, but I was happier with my presentation earlier in the week. Of course, I have since had very little time to dive into this and re-roll. I'm hoping to have some time by next week, if not sooner, for this. What I need suggestions on is basic architecture of how to render the page data.

• We will switch modes on the query string (re-direct based on header per my discussion with scor that each qutput should come from a unique path).
• A mode with be a combination of two pieces:
  1. a rendering function (like drupal_render) that can recurse through the data and do something to each element
  2. A data-to-output function (like theme) that is applied to some or all of the data elements by the rendering function
• The existing patch contains this notion of the 2 pieces, but there are some things that just don't work as-is. E.g. the #prefix element which is assumed to be XHTML markup.
• Among other things, I think it would be useful to figure out how pass query-string arguments to construct content in a piecemeal way - e.g. if only the content needs to be populated, or only one of the two sidebars, or a single block?

We will switch modes on the query string (re-direct based on header per my discussion with scor that each output should come from a unique path).

I firmly believe that is a fundamentally wrong approach. A given piece of content should have a unique URI, but not each different rendering of it. See this excellent and humorous writeup.

We're basically talking about turning Drupal into a proper REST server, so let's do it right.

Crell,

Bear with me because this RESTful thing is new to me. It is somewhat hard for me to understand and I suspect it is hard to understand for many of us who have become accustomed to things like:

• RSS feed: example.com/post-title/rss
• Atom feed: example.com/post-title/atom
• Trackback: example.com/post-title/trackback
• Comment: example.com/post-title/comment

Are are saying that your preferred method would be something along the lines of a feed reader accessing example.com/post-title at which point Drupal figures out what the feed reader wants and serves up what the feed reader wants? Another example would be for a comment or pingback request to be submitted to example.com/post-title and Drupal figuring out what to display.

Is this correct?

@Crell - well, we must have a query-string representation, and we need headers-based detection. Whether or not we redirect to the first based on the second I don't have very stong feelings about.

@samirnassar: Essentially. The underlying point is that a given request for a page consists of several parameters.

1) GET - verb
2) URL - noun
3) Request type header - I guess this would be the adjective. :-)

So GET node/$nid with a header of text/html *should* return something different than node/$nid with a header of application/json or xml/rss (or whatever the mimetypes are). That is in theory how the system is supposed to work.

Of course, since so many developers can't think beyond a simple web browser (NOT in reference to anyone here, to be clear) the implementation is not always the best, and a lot of things end up being sent with a wrong header. My stance on the matter is that we can and should do things right first, then add on functionality to allow the lesser systems to play too (such as a GET parameter that can substitute for the header).

See my thoughts on implementation over at http://drupal.org/node/218830#comment-792841

Restful Drupal is just infeasible, did you ever imagine Drupal without sessions?

The most we can get near to RESTful design is what they call "REST enabled", i.e. what google did for their Google Data API.

Almost every Drupal paths resolve to a full page, with headers, sidebars, main content, and footer, with exception for AJAX callbacks. So why not have a different path when we need to get a specific object. E.g. /node/123 get a full page while /some-meaningful-path/node/123 you get just a node representation?

I am keeping my eye on this issue because it affects the implementation of #230710: Centralized module to control feeds and feed types, which would also address #28337: Add permissions to disable RSS feeds and #202018: Offer ATOM (RFC 4287) in addition to RSS.

My implementation of a Syndication core module is not RESTful. I was advised that the outcome of this issue greatly influences how to go about implementing a Syndication module.

REST wrt content negotiation is still not clearly defined and none of the various approaches solve all the problems (maybe it's meant to be that way!). In particular, the question of request header vs. query string is still problematic.

@crell: your idea relies on having control over the client accept header, which is a fine assumption in some cases, but not always. Can you safely assume that all clients will implement the accept headers properly? How do you point to a specific representation of a resource? For example what if you want to provide a link to a PDF version of a resource?

What I discussed with pwolanin is the following idea. conneg is sometimes implemented as a 30x redirect to the right data format and language URI. What's important here is that you always "publish" the original URI as identifier for the resource, so that it is still this main URI that will be used by the system to point to the resource. Then it's just a matter of HTTP redirect. This is the approach we use in Neologism to publish RDF vocabularies. There is a conneg behind each URI and an HTTP redirect to the right format (HTML, RDF/XML or N3). The ptlis.net PHP content negotiation library is used for the conneg.

In terms of query string for Drupal, I can see at least 4 approaches:

• additional query string parameter format http://example.org/node/5?format=xml - the advantage is that we can read the format variable very easily in PHP.
• slash approach http://example.org/node/5/xml - here the problem is that we might conflict with the menu and url aliases, although we could match the last arg against the supported formats, remove it from q if the format is supported and continue the flow.
• file extension approach http://example.org/node/5.xml - the url is nice
• some implementations like dbpedia have a different URI pattern like http://dbpedia.org/resource/London which gets redirected to http://dbpedia.org/page/London for the HTML output.

One cons of this method is the double page load, but there might be a way to work around it in Drupal.

re: content negotiation, why don't we just follow the example given by the language system in D6 core? For language negotiation, it follows this approach:

1. Check sub-domain name (if we're using per-language sub-domains).
2. Check the first part of the path (i.e. before the 1st slash).
3. Check the HTTP response headers.
4. Fall back to the default language.

With the exception of the sub-domains (who would want a sub-domain for each rendering format that their site supports?), I think we can apply this to content negotiation pretty easily:

1. Check the first part of the path (i.e. before the 1st slash).
2. Check the HTTP response headers.
3. Fall back to the default rendering mode.

I'd rather exclude the option of an additional query string parameter, personally. This can be handled fine within $_GET['q'], and it looks cleaner as well. I think we should settle on one way of putting it into $_GET['q'], and I would go for putting it at the start, before the first slash. Nothing wrong with putting it at the end (after a slash or after a dot) per se, just a question of preference.

We should be able to make it work with the menu routing and the URL aliasing, same way as the i18n system does. I don't see major problems there. For sites that have both multi-rendering and i18n, the rendering mode should probably go first in the URL, as it's a lower-level thing that should be given priority.

May I also cautiously suggest that this new API is an ideal opportunity — should we wish to seize it — for a first experiment at implementing the ideas in http://jaspan.com/dx-files-abandon-anonymous-arrays-attributes in core. This could be the first new Drupal API in three years that isn't based on nested array('#key' => $value) structures. Trying it out in a brand new API will be much safer, and much less work, than re-writing an existing API.

$elements = new PageElements();
$elements->title = drupal_get_title();
$elements->theme = 'page';
$elements->show_blocks = $show_blocks;
$elements->show_messages = $show_messages;
$elements->success = $success;
$elements->content = $content;

What do you think, folks?

@Jaza - no, not for this patch. Let's focus on the essential elements here.

Is anything going on here?

Subscribe

Sorry if this has been addressed in this super long discussion but I have one question..

Is the scope of the node refactor confined to the node theming hook? Or does it extend to the page? I ask because if it does extend to the page, how would themers handle that? Do we start having page templates specific to carrying nodes since there would be node specific chunks of output instead of just $content? It's often requested to have comments available right inside the page template. And in the future if other types of content not related to nodes want to give their own set of arrays for the page what happens there? page.tpl.php could grow into an unmanageable mess. Maybe use theming patterns for the 'page'?

So, does the node data spill over to the page hook? That is my question.

@dvessel - this all happens before we call any theme functions, so this shouldn't affect how theming works from the perspective of a regular themer much at all. Basically we are just collecting all the names of the theme functions and the parameters/input to them in one but drupal_render friendly array, and then it iterates, calling theme functions and appending string until it reaches the final page HTML.

There are some definite advantages for "advanced" themers, or modules that want to play around with page structure however (think panels 3!), because they get to alter the final array of content data before it is actually themed, and radically effect the page. Right now, if you do this you basically have to write your own code to load and build the page from scratch.

subscribe

we already can have node-specific page templates in D6: page-node.tpl.php

Lets mark this as a dupe since a lot of this is implemented in #351235: hook_page_alter() (very close to commit.) and output format work can move to #145551: Enable loading and rendering into multiple formats (html, xml, json, atom, etc.)