In Drupal 5, we took the first steps toward building nodes the way we build forms -- structured arrays with useful meta-data, rendered to HTML as needed. The work that made it into that version was only a first step, though, and there are still major weaknesses in our current system that need to be resolved. Webchick's epic signatures patch (http://drupal.org/node/132446) is being held up by one of these weaknesses, for example, and crufty bits of duplicated code still litter Drupal core in places where the 5.0 system wasn't flexible enough to deal with various rendering scenerios. (Print-friendly versions and RSS feed generation are two glaring examples).

The problems

  • We build portions of the node as a structured array, but immediately collapse it to text -- the structural information is lost to themers and template designers.
  • Our node rendering workflow is still not flexible enough to remove hard-coded calls to comment.module and ugly functions like node_show(), stymieing anyone who wants to display comments in a nonstandard style.
  • Themers who want to print out small portions of a node, or 'reshuffle' the layout, go back to square one, printing out raw properties from the node obejct and taking on the burden of security and output filtering at the theming layer
  • When building nodes for different purposes (feeds, print-friendly mode, etc.) modules that modify the node via nodeapi have no way of knowing the intended use of the node, save the coarse 'teaser/full' boolean

The proposed solutions

  • Rather than building just the node's body and teaser with a structured array, build the entire node using our array data structures. Title, submitted information, main content, links, comments, etc.
  • Rather than rendering this to a string and then passing it on to the theming engine, pass the structured array itself to theme_node().
  • Use this richer pool of information in the theme engine layer to expose more options for template designers without forcing them to circumvent our normal output-cleaning and rendering pipeline.
  • 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.
  • End hunger, war, and strife.

This sounds big -- and it would be foolish to say that it's a simple set of steps. But the foundation for all of these changes has already been laid. I'd like to propose a three-phase patch process to get these changes into core. If any of them make it in, we'll benefit quite a bit, but if all three somehow make it in, I believe that Ewoks will dance happily.

  1. Refactoring node_view()
    1. First, we need to build the entire node structure as one of our standard #arrays, rather than just the node body.
    2. Next, we migrate nodeapi op view and nodeapi op alter to to the node-standard drupal_alter('node', $node_array); syntax. This lets modules use hook_node_alter() to add stuff to nodes, while nodeapi focuses on CRUD operations.
    3. Next, we eliminate the node_show() function and move its hard-coded calls to comment.module to a standard will allow comments to be added to the node the same way all other data is, not using ugly hard-coded hacks like node_show(). (Look that function up. Read it and weep.)
    4. Refactor theme_node() to accept our structured array rather than a raw node object. Map top-level array elements like 'submitted' and 'content' directly to variables for template designers.
  2. Allow template designers to manipulate the structured data, not just pre-rendered bits of it
    • Right now, our theme engines expose a discrete set of variables for themers. We should expose helper functions that let template designers selectively print sub-chunks of the element they're designing a template for. We do this already when writing hard-coded theme_* functions for forms -- drupal_render($foo['bar']), drupal_render($bar), etc. Something along the lines of output('content-image-2') or output('comments') or output('links-new-comment') would be ideal. Obviously, exposing drupal_render() itself at the template layer is not desired. But the theme engine's job should be mapping drupal_render()'s flexibility to whatever tag/language syntax the template designers need... not heavy-lifting bits of node data from one place to another.
    • This portion of the plan needs to be fleshed out, and I'm SURE will be the subject of debate. I am not married to an specific course of action, just the end goal: exposing the rich nested metadata we build to themers who can properly utilize it.
  3. Replace the hard-coded 'teaser', 'page', and 'links' params in node_view with a flexible $style param, and an $options array: node_view($node, $style = 'full', $options = array())
    1. Expose hook_node_styles(), allowing modules to expose their own custom node rendering styles. node.module will expose the four core defaults: teaser, full, print, and feed.
    2. Expose node_get_styles(), a function similar to node_get_types(). Modules that currently allow users to select behavior based on teaser/full state should instead allow users to configure behaviors for each of the new styles.
    3. Optional overrides (like, 'build this in full mode, but don't show the comments,' or 'don't show the links for this node') can be configured by flags in the $options parameter. These are little-used flags, and collapsing them into a single optional array mirrors the new technique used by the l() function. It's more flexible and easier to use in edge cases without disrupting the function's signature.

There's a lot in there. An awful lot. This is a proposal, though, and while I have pseudocode for most of the steps in here, I want to get others in the community on board to hammer out the details and help figure out where the pitfalls are.

What do we gain from this refactoring effort? More consistency, reduced duplication of code, and the preservation of meaningful metadata later in the rendering pipeline. That, in turn, means more possibilities for robust theming engines and more possibilities for template designers who don't want to learn to hack PHP and write hook_foo_alter() functions to get things done.

Code coming shortly -- discussion encouraged in the meantime.

Comments

eaton’s picture

Another note: I broke this proposal up into the three discrete steps above because I believe that they can be handled as three separate patches, rather than one monolithic one.

merlinofchaos’s picture

subscribing

kbahey’s picture

Excellent goals Eaton!

Subscribing.

Crell’s picture

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.

shunting’s picture

Subscribe

webchick’s picture

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

Crell’s picture

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.

RobRoy’s picture

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!

RobRoy’s picture

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.

garrettc’s picture

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.

eaton’s picture

Thanks for the excellent feedback so far! Sorry there hasn't been any progress code-wise; I'm at a work related workshop right now but will have a chance to dive in agressively after today. Code is, in fact, coming shortly. ;-)

radev’s picture

Very interesting.
Subscribing.

eaton’s picture

FileSize
15.28 KB

The attached patch is an extremely rough first stab at the new approach. It is NOT in any way ready for rigorous review -- it's here to demonstrate what kind of approach is being imagined, and to hilight some of the challenges. But if you're reading this issue, you're probably already one of the render-geeks, so let's get down to brass tacks. ;-)

First, node_view() is now a svelte 2 lines. It calls node_build(), and drupal_render(). That's it. node_show() still exists (for now) but does NOT have the old hard-coded call to comment module. (And the people rejoice).

What's changed functionally?

  1. node_build() doesn't make a string and stick it into $node->body, the way node_build_content() used to. It builds a complete structured array to hold all the node's data, and sticks the raw node object into the #node property for use when necessary. Ugly. Needs work.
  2. hook_view() now operates on a byref copy of that array rather than returning a node object.
  3. hook_nodeapi() op 'view' no longer exists. It's gone. Departed. Instead, hook_node_alter() is called. It might make sense to pass the node->type along as an additional parameter to that too, to make switching simpler.
  4. Lots of previously hard-coded stuff -- node links, comments -- are just part of the node array built by node_build(). This means that hook_node_alter() can shuffle where links appear in the node body, hide comments, add a new #theme function to comments, whatever.
  5. This is important: theme_node() no longer receives the raw node object with everything jammed into $node->body. It receives the structured array we've just built and altered. This means that the theming layer receives the fully structured data. Right now I'm hard-coding drupal_render() calls in the variables to populate $content, $links, $etc with the content that they used to have in the old system, but that's just because I haven't refactored things in the individual themes yet. What we ultimately want to do is pass *pieces of the structural data* to the template file, so render($content) would print out one chunk, but render($content['signature']) would print just one little sub-chunk.

There is a lot of stuff that is changing here. A lot. But it's needed to happen for a while -- $node->body was a useless bin full of text by the time the theme system got to it, and themers were left picking around in raw $node properties to reconstruct all the work of node_view() if they needed to control things at a more granular level.

What needs to happen?

  1. All the code needs to work ;-) This is alpha-level demo code. Let's think through how it's being done and look for opportunities to make the overall approach cleaner.
  2. Because of how we're now building the structured array, we now have a blank slate to imagine what to hand off to the theme and how to best structure it. This is a huge deal. $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?
  3. The search system. Holy moly. As Robert Douglas noted, Drupal's search API is awesome -- and the implementation that node module slaps on top of it is a mess of hard-coding. Someone (Robert! cough cough!) with experience in the search system is needed to help consult on ways to best translate our new structured data into good search data.

Other random lessons learned:

  1. If you feed something into drupal_render(), #theme is important, not just #type. When rendering, if an array has its #theme property set, it's immediately handed off to that theming function. If NOT, drupal_render FIRST renders all the element's children, THEN hands it off to theme($element['#type']). Why is this difference important? In the first case, our theme function gets the chance to control the way all the children get rendered, what order, etc. If we leave out #theme and rely on the automatic mapping via the #type property, all the children are already rendered by the time out theme function gets called. That's not useful for the purpose of templating. Not a huge deal, but it's good to remember. :-)

There's a lot to do. But it's promising, and feedback from others who are excited would be great.

eaton’s picture

Status: Active » Needs work

One idea that i'm kicking around right now, by the way, is the change from node_view($node, $teaser, $page, $links) to the afore-mentioned node_view($node, $style, $options) format. It was slated for stage 3 of this patch, but I'm also realizing that converting over to that approach earlier rather than later will also make it easier to handle the search/indexing issues. 'index' or 'search' would just be a new node rendering style, allowing modules to change what data they expose when the search index is being built. Any thoughts?

Crell’s picture

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.)

BioALIEN’s picture

Subscribing. I like where this is going :)

eaton’s picture

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.)

Absolutely. Now that I think about it, this iteration of the code should probably pass $teaser, $page, $link into hook_node_alter() as well as just the raw node object. In the 'future', $style and $options would get passed in in a similar fashion.

moshe weitzman’s picture

subscribe

garrettc’s picture

$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?

eaton’s picture

That's a good question. The node structure can be abused the same way form related stuff can be stuck in the wrong locations. we a couple general classes of things to think about:

  1. information about the node that is NOT given an explicit element (submitted-info, title) but is stored in dedicated #properties. node_theme() manually builds $submitted and $node-url from these different pieces.
  2. Information 'about' the node that isn't specifically its content. $terms is the easiest example to find.
  3. The main content -- the node body, forum module's navigation pager, book module's index of sub-pages... these are all things that would get jammed in there.
  4. Links -- this is just the current 'links' variable, really. If we expand this, I'm not sure what to call it. It seems to be dedicated to actions to take on the node? Humm.
  5. Footer stuff -- node comments are the biggest example of this (maybe the only example?) in other scenerios, though, related nodes or other suggested content might be placed in this location.

As I mentioned, it's tricky -- we've got a blank slate right now, and we also have the flexibility of making the structure different depending on the context. (A search-specific structure is one possibility). Call your drupal friends! Call information architecture geeks! Let's get some hardcore drupal-heads thinking about this one.

garrettc’s picture

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.

eaton’s picture

That would definitely be awesome. Thanks!

I lean towards keeping "links" as is for now because in the context of web content it's a pretty common requirement, and it maps cleanly to a whole suite of callbacks we currently use -- hook_link and hook_links_alter, etc.

But yeah. It's tricky and complicated. I'm curious to iron out some of these with everybody. :D

profix898’s picture

subscribing

garrettc’s picture

FileSize
43.54 KB

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.

webchick’s picture

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.

add1sun’s picture

subscribe

eaton’s picture

Dude, thank you so much for that diagram. I'll post subsequent revisions to it in that form as well. *grin*

First off: I'm hesitant to use 'properties' as an actual top-level renderable element. At the moment, things like the author and the title of the node and so on are being stored as #properties, not in a collection named 'properties'. If that's confusing, let me break it down using the earlier structure I proposed:

Node structure
  |--#uid
  |--#title
  |--meta
  |    |--terms
  |    |--fivestar-rating
  |    |--etc
  |--content
  |    |--body
  |    |--forum-nav
  |    |--etc
  |--links
  |--footer
  |    |--comments
  |    |--etc

In the above example, things with a pound-sin prefix, like #uid and #title, are actual properties of the node structure. They don't render out automatically, but they can be used by theme_node() when building the node itself. They're equivalent to things like #collapsible and #default_value in a form api element.

Elements without the #prefix are just nested structures, with each 'parent' being a discernable chunk of the node's structure. $node['meta']['terms'], $node['content']['forum-nav'], and so on. Currently, I'm getting around the more complex structure by doing a drupal_render() on each top level element in the node structure. In the example above, that would be meta, content, links, and footer.

I like the diagram's use of a larger collection, although I think the 'about' section might need more fleshing out. 'meta', I was thinking, would just be a place to dump descriptive information about the node that is not necessarily a part of the node's proper *content*.

The reason that I'm hoping we can provide a couple of clearly defined chunks that are always there is so that template designers who DON'T want to burrow deep into the node structure still have a couple of pieces they know that they can safely rearrange just by calling render($content), render($footer), and so on. The more top-level elements we put in by default, the greater the chance that the designers will have to resort to brute force things like render($node) to get all the other left-over bits. For people doing lots of heavy customization, both with modules and their themes, that's no big loss, but it can make it baffling for folks on the theming end of things.

Perhaps 'info', 'content', 'links', and 'footer' might be good choices.

Node structure
  |--#uid
  |--#title
  |--info
  |    |--submitted
  |    |--terms
  |    |--fivestar-rating
  |--content
  |    |--body
  |    |--forum-nav
  |    |--etc
  |--links
  |--footer
  |    |--comments
  |    |--etc

I admit that 'footer' feels a little ugly considering its position implications rather than 'structural'. Perhaps a 'related' chunk woudl also make sense, as a place for modules to put information about things that are related to the current node? Hmmm.

garrettc’s picture

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 *:)

add1sun’s picture

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 ;)).

Crell’s picture

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.

kbahey’s picture

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

So how about just "extra".

merlinofchaos’s picture

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

eaton’s picture

In reading over this stuff I'm starting to lean favorably towards "info", "content", "links" and "extra". With comments being in the "extra" chunk. Pre and Post imply position, which is what I'm trying to avoid with the structural divisions...

Anyone? Thoughts?

Crell’s picture

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)?

garrettc’s picture

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.

eaton’s picture

Just a heads up: this patch is currently on hold while http://drupal.org/node/137236 goes through the commit process: THAT patch makes a lot of the ugly theme-layer code that was goign to be in this patch automatic. That's a good, happy thing.

More updates coming shortly. :)

jlinares’s picture

Subscribing.

moonray’s picture

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.

eaton’s picture

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.

That's not really inside the scope of this patch attempt, I'm afraid. The system outlined here would be a starting point for something like that existing in contrib, though...

eaton’s picture

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.

That's not really inside the scope of this patch attempt, I'm afraid. The system outlined here would be a starting point for something like that existing in contrib, though...

moonray’s picture

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.

eaton’s picture

Not really, unfortunately. That's tied into more than just node rendering -- it's also the menu system, path handling, and so on. node URLs are rarely displayed by loading the entire node object and theming, rather by calling the l() function with just the nid. I like that idea, but I think it's goign to have to happen in a totally different part of the system... it's closer to path aliasing really...

eaton’s picture

Not really, unfortunately. That's tied into more than just node rendering -- it's also the menu system, path handling, and so on. node URLs are rarely displayed by loading the entire node object and theming, rather by calling the l() function with just the nid. I like that idea, but I think it's goign to have to happen in a totally different part of the system... it's closer to path aliasing really...

tostinni’s picture

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

eaton’s picture

Stage three of the patch (not yet coded) will probably help with that quite a bit. There are no directprovisions for it in this proposed set of changes but the infrastructure for rendering nodes in various "styles" provides some infrastructure that's definitely useful for that.

I haven't had a chance to work much on this patch as I've been focusing on http://drupal.org/node/138706, but once that lands I'd like to see if I can get these changes in before the freeze. We'll see... I can at least give it a try.

RobRoy’s picture

@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?

RobRoy’s picture

FileSize
14.54 KB

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.

RobRoy’s picture

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.

eaton’s picture

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.

You're exactly right. After talkign to merlinofchaos on #drupal, it's clear that really is the best approach for this. The code I had in place when I last rolled the patch was before his theme changes, and it wasn't yet clear whether I should 'pre-render' everything or leave it for the theme layer. The latter really does look like the best approach...

You're correct, though, that the trickiness with rendering, printing, storing and rearranging and hiding, is a thorny one. a PHP-savvy themer could easily set up a series of $top = drupal_render($node['foo']['bar']); print $top; style statements but it may be worth it to include a set of themer-friendly wrapper functions like render(), to render AND print a chunk of content, hide() to mask a chunk of content, and render($foo, TRUE) to force it to print even after it's been hidden. Perhaps that's too complex; asking peopel to populate variables manually when they are doing a lot of shuffling doesn't seem TOO daunting, but who knows?

RobRoy’s picture

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

moshe weitzman’s picture

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

garrettc’s picture

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.

eaton’s picture

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.

Here's the quick summary version.

Right now, we render the node as a big chunk of text, then pass that text to the theming layer -- along with the full object version of the node. Nine times out of ten, any complex theming work requires discarding the rendered version and manually printing out the node, property by property. 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. We're still stuck with the split between 'pre-rendered text' and 'the raw node object.'

This patch is designed to change that, building the node as a structured array (just like forms) that can be intelligently rendered, piece by piece, using the drupal_render() command, without violating nasty bits of security. The discussion of wrapper functions like render(), hide(), and so on are just being kicked around as possible ways to give themers a handful of simpler ways to play with that array, just as developers would do using PHP logic (looping through, rendering things in a different order, etc.)

garrettc’s picture

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?

eaton’s picture

Precisely!

Also, http://drupal.org/node/144608 implements the 'node rendering styles' work that was discussed as part of this patch. They're related, but I think they can easily be split...

Dries’s picture

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.

eaton’s picture

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.

I agree wholeheartedly. Thanks for the look, Dries. This patch is in a much rougher state, and while I think that it could be completed before freeze by RobRoy and I, there are a number of those issues that need conceptual answers. Some brainstorming with interested parties would be great...

adrinux’s picture

working towards theme nirvana...subscribing

webchick’s picture

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.

eaton’s picture

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.

As webchick noted, this is an unnecessary flag and can be eliminated. The idea was to keep themers from having to render little pieces into variables if they wanted to print them out of order or twice, or what not... but for now I'm not too worried about it.

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

Indeed. The trick is that rendering things this way means building a structured, FAPI-style array to contain all the data needed during rendering. Our structured arrays use $thing['#property'] rather than $thing->property. An earlier version of the code automatically translated all $node->property data into the $node['#property'] style, so it could be passed along cleanly. I was concerned about potential namespace collisions, though, and for the time being put things in $node['#node']. There is a better solution, I'm sure. I just haven't figured out if it's to go back to direct ->property to ['#property'] mapping, or something else.

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.

Right now, it IS sloppy. :-) One problem is that right now, our array structures are more useful than basic object properties, but they are also more verbose ($foo['#bar'] rather than $foo->bar). There are three philosophies currently being mixed in the patch:

1) convert $node_object->property to $node_structure['#property']
2) put $node_object into $node_structure['#node'] for retrieval
3) Ugly mixture of both

$node->title was getting the #1 treatment, for example. $node->links should always be part of the structured array, though, because it is actually a nested renderable data structure. If we turned the title into a #value type element that's directly renderable, for example, $node['title'] would be consistent. This is what some of the 'data structure' discussions in earlier comments were about.

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?

I lean towards 'long term goal.' node_prepare() is a generic fallback function for modules that don't implement hook_view() for their node types. node_build() is an overall traffic cop function, coordinating the various hooks and alter operates that are needed.

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

Yes. Although I think that eliminating that hook will require us to support some sort of 'after render' operation in drupal_render() for code that needs to do post-processing on the raw text of the node after it's rendered. See upload.module's URL fixing code, for example. We explored some of this in Drupal 5 but it didn't work, partly because we weren't rendering the entire node. Now that we're doing that, we'll be able to.

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.

RobRoy’s picture

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.

jpetso’s picture

subscribing.

moshe weitzman’s picture

Status, anyone? sure would be swell.

vivek.puri’s picture

subscribing

Owen Barton’s picture

Subscribe

Frando’s picture

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’s picture

First, it's necessary to change themes and return to the page, refreshing the Theme Cache. Without that, the params for theme_node etc won't match up and you get the error above.

I don't think there's much chance of a D6 release, really. Given that the objections voiced above were fundamental philosophical ones, and no real alternative approach has been conceived of or articulated yet (save a D7-level refactoring of node module), I think it would be a mistake. If the approach used in this patch were re-evaluated, and enough people thought it was worth using, it would probably need 'search indexing' added as another node style to round out the set.

eaton’s picture

Er, whoops, ignore that. I thought I was replying in the node_styles issue. heh.

BioALIEN’s picture

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

alanburke’s picture

+1 on this idea
Very useful for theme development

Alan

Zach Harkey’s picture

Subscribing (with hopeful tears of joy in my eyes)

eaton’s picture

Status: Needs work » Postponed

This has officially been postponed until the next version of Drupal. There's just not enough consensus on the details to get something out the door right now, and something this big needs to be done right the first time. We'll keep the issue updated as it evolves!

dvessel’s picture

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.

guardian’s picture

subscribing

Frando’s picture

Version: 6.x-dev » 7.x-dev
Status: Postponed » Needs work
smk-ka’s picture

subscribing

bjaspan’s picture

Subscribe.

bjaspan’s picture

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, '

CNN.com reports that 37% of all web sites are using Drupal. Blah blah blah.

'), '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?

merlinofchaos’s picture

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.

somes’s picture

subing

totalsense’s picture

Subscribe

totalsense’s picture

Title: Refactor node rendering » subscribe
keith.smith’s picture

Title: subscribe » Refactor node rendering
mlncn’s picture

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.

eaton’s picture

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.

I agree, and this is something brought up by Steven in his critique of the related patch. The two axis we're talking about are 'different output formats' and 'different presentations of the node data'. I think that both should be expandable, but yeah, they need to be treated as different things.

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

I'm a little worried about overloading the theme layer in this way, as up until now it's been used almost exclusively for output of pure HTML. Are we using the theme layer to format lists of nodes as RSS? I can't remember. In any case, I'm thinking that this may be a case where we need to examine the possibility of a very different kind of mechanism. Rails' use of different .foo extensions on paths is promising, and it's in line with other suggestions that have been made. Menu callbacks, for example, could define a 'default renderer' if they wanted to, and all paths would fall back to 'html' if none is specified. Adding an extension at the end of the path (like .rss or .xml or .swf) would trigger a different rendering mechanism? Something like that?

I'm not sure, but I think that connecting it to the menu system, and extending our path metadata to support the idea of a given 'rendering scheme' or 'output format' might be a good way to go...

samuelwan’s picture

Subscribing

mlncn’s picture

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.

Owen Barton’s picture

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.

cpelham’s picture

subscribing...

csevb10’s picture

Wildly interested. Subscribing.

Senpai’s picture

Subscribing, with comments about Comments to follow...

Great ideas thus far!

Dave Cohen’s picture

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).

catch’s picture

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

OpenChimp’s picture

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

sinasalek’s picture

Looks interesting

starbow’s picture

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.

bcn’s picture

track

ezra-g’s picture

Subscribing.

birdmanx35’s picture

Subscribing.

swailsnet’s picture

subscribing

beeradb’s picture

way late to the party. subscribing anyway.

eaton’s picture

Well, it definitely looks like there's interest around this. The problem is, node rendering in Drupal is a pretty huge component. If you start tugging at the thread, you start pulling apart the entire Drupal-sweater.

There are three separate aspects to this, in order of increasing difficulty:

  1. Remove the hard-coded assumptions from node.module
  2. Make the entire structure of the node easier to change and alter at display time
  3. Make it easier to output wildly different formats (XML, JSON, etc)

I think we can get the most bang for our buck right now focusing on #1 -- #2 and #3 are tangled up in DataAPI and our overall page rendering/menu dispatch system, and I think it's best to allow those solutions to mature before we tear apart nodes to make something happen in this part of the code.

The biggest offender right now is the node_show() function and the default page rendering for the node/x page. It hard-codes calls to comment module, and so on. Our biggest barrier is the fact that the $node->links array is hard rendered in, and the comments are added after it, separate from the normal $node->content array.

If we can get the $links array that hangs off of each node converted into a standard renderable structure, then stick it into $node->content with a very heavy weight, comment module would be able to take back control of its own display -- just adding the comments to the end fo the $node->content array below the links. We could eliminate the bulk of the hard-coded assumptions about comment.module from the node module.

And that, in turn, would make a lot of new tweaks easier. Putting comments on a separate page, putting other stuff like pingbacks and trackbacks down with the comments, etc.

Any thoughts on this narrowly focused approach for D7? I think that it makes more sense, right now, to go for the doable easy win than to take another stab at a massive restructuring that depends on other components that aren't really ready yet.

Crell’s picture

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).

moshe weitzman’s picture

@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.

eaton’s picture

i'm willing to give it a solid stab, moshe. there's only one real question that stands in my way: how to treat the $links array.

Right now, we have a one-to-one correlation between the contents of the $links array and what theme_links() accepts. Since the #render system uses hash-prefixed keys to distinguish between properties and nested elements, our current links structure is incompatible. If we want the links to be a renderable element in the content array, we have a couple courses of action.

  • Just render the links and put them in a markup #type element. This has the downside of being kinda crummy, because we're collapsing nice structured data to plain old HTML before we really need to.
  • Use a 'links' element #type, which would map the renderable element straight to theme_links(). Then, make theme_links() smart enough to recognize the difference between an incoming links array and an incoming links collection *element*. (for example, if there is a #type = links key/value pair, and a #data key contains an array of links, just pull it out and treat the #data array as if it had been passed in as the first param. This has the serious downside of breaking existing theme_links overrides, and it's kinda janky.
  • Create a full element type for #links, and allow its theme function to just map the links to the normal theme_links() function. This is disappointing because it adds an unecessary level of indirection between the raw data and a theme function that REALLY just needs a thin translation layer to shuffle some attributes.

I'm not happy with any of the options, but #3 feels like the safest and wisest option. As we build more renderable structures, I think this is a question we're going to have to deal with more and more. I know the Amazon module has two sets of theming functions -- the 'real' ones, and the ones that only map #renderable hash-prefixed properties to the usual data structure that the normal theme function uses.

Thoughts? Anyone?

Crell’s picture

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.

heyrocker’s picture

subscibing

Dave Cohen’s picture

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.

Wim Leers’s picture

Subscribing.

coupet’s picture

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

Liam McDermott’s picture

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

moshe weitzman’s picture

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.

moshe weitzman’s picture

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().

Gábor Hojtsy’s picture

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

catch’s picture

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

robertDouglass’s picture

Subscribe.

David_Rothstein’s picture

Subscribe.

dami’s picture

subscribe

Susurrus’s picture

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

cburschka’s picture

Please, please get this into Drupal 7.

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

pwolanin’s picture

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)?

pwolanin’s picture

FileSize
12.07 KB

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.

cburschka’s picture

Status: Needs work » Needs review

Review, perhaps?

recidive’s picture

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?

pwolanin’s picture

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.

pwolanin’s picture

Title: Refactor node rendering » Refactor page (and node) rendering
FileSize
22.86 KB

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.

laken’s picture

subscribing

cburschka’s picture

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().

pwolanin’s picture

@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.

wundo’s picture

subscribe

RobLoach’s picture

Unsubscribing.

Susurrus’s picture

Status: Needs review » Needs work

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

pwolanin’s picture

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.

recidive’s picture

Status: Needs work » Needs review
FileSize
20.34 KB

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.

pwolanin’s picture

@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.

starbow’s picture

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.

catch’s picture

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?

pwolanin’s picture

@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.

alippai’s picture

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

alippai’s picture

Status: Needs review » Needs work

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...

pwolanin’s picture

yes - it's CNW for now.

pwolanin’s picture

FileSize
35.05 KB

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.

pwolanin’s picture

FileSize
51.49 KB

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().

Owen Barton’s picture

FileSize
48.46 KB

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

RobLoach’s picture

FileSize
48.45 KB

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

JohnAlbin’s picture

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?

starbow’s picture

@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?

Owen Barton’s picture

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.

Nick Lewis’s picture

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.

pwolanin’s picture

@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?
Crell’s picture

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.

samirnassar’s picture

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?

pwolanin’s picture

@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.

Crell’s picture

@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

recidive’s picture

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?

samirnassar’s picture

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.

scor’s picture

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:

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

Jaza’s picture

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.

Jaza’s picture

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?

pwolanin’s picture

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

RobLoach’s picture

Is anything going on here?

ultimateboy’s picture

Subscribe

dvessel’s picture

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.

Owen Barton’s picture

@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.

korvus’s picture

subscribe

pwolanin’s picture

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

moshe weitzman’s picture

Status: Needs work » Closed (duplicate)

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.)