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.
- Refactoring node_view()
- First, we need to build the entire node structure as one of our standard #arrays, rather than just the node body.
- 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. - 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.)
- 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.
- 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.
- 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())
- 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.
- 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.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#145 | 134478.patch | 48.45 KB | RobLoach |
#144 | 134478-1.patch | 48.46 KB | Owen Barton |
#143 | rough-idea-134478-143c.patch | 51.49 KB | pwolanin |
#142 | rough-idea-134478-142.patch | 35.05 KB | pwolanin |
#134 | drupal_rendering.patch | 20.34 KB | recidive |
Comments
Comment #1
eaton CreditAttribution: eaton commentedAnother 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.
Comment #2
merlinofchaos CreditAttribution: merlinofchaos commentedsubscribing
Comment #3
kbahey CreditAttribution: kbahey commentedExcellent goals Eaton!
Subscribing.
Comment #4
Crell CreditAttribution: Crell commentedEaton, 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.
Comment #5
shunting CreditAttribution: shunting commentedSubscribe
Comment #6
webchicksubscribing. I'm VERY much in favour of this, and not only because of the signatures patch. ;)
Comment #7
Crell CreditAttribution: Crell commentedSo 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.
Comment #8
RobRoy CreditAttribution: RobRoy commentedMarked 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!
Comment #9
RobRoy CreditAttribution: RobRoy commentedWanted 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.
Comment #10
garrettc CreditAttribution: garrettc commentedSubscribing.
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.
Comment #11
eaton CreditAttribution: eaton commentedThanks 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. ;-)
Comment #12
radev CreditAttribution: radev commentedVery interesting.
Subscribing.
Comment #13
eaton CreditAttribution: eaton commentedThe 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?
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?
Other random lessons learned:
There's a lot to do. But it's promising, and feedback from others who are excited would be great.
Comment #14
eaton CreditAttribution: eaton commentedOne 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?
Comment #15
Crell CreditAttribution: Crell commentedWould 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.)
Comment #16
BioALIEN CreditAttribution: BioALIEN commentedSubscribing. I like where this is going :)
Comment #17
eaton CreditAttribution: eaton commentedAbsolutely. 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.
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedsubscribe
Comment #19
garrettc CreditAttribution: garrettc commentedAh, 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?
Comment #20
eaton CreditAttribution: eaton commentedThat'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:
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.
Comment #21
garrettc CreditAttribution: garrettc commentedInformation 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.
Comment #22
eaton CreditAttribution: eaton commentedThat 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
Comment #23
profix898 CreditAttribution: profix898 commentedsubscribing
Comment #24
garrettc CreditAttribution: garrettc commentedOkay, so here's a first pass after giving the problem some thought.
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.
Comment #25
webchickInteresting. 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.
Comment #26
add1sun CreditAttribution: add1sun commentedsubscribe
Comment #27
eaton CreditAttribution: eaton commentedDude, 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:
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.
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.
Comment #28
garrettc CreditAttribution: garrettc commentedI 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:
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 *:)
Comment #29
add1sun CreditAttribution: add1sun commentedHm, 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 ;)).
Comment #30
Crell CreditAttribution: Crell commentedMaybe 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.
Comment #31
kbahey CreditAttribution: kbahey commentedThemers may choose to display stuff before, after, on the side, or none at all.
So how about just "extra".
Comment #32
merlinofchaos CreditAttribution: merlinofchaos commentedRather than append, how about just 'pre' and 'post'?
Comment #33
eaton CreditAttribution: eaton commentedIn 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?
Comment #34
Crell CreditAttribution: Crell commented1) 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)?
Comment #35
garrettc CreditAttribution: garrettc commentedThat 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.
Comment #36
eaton CreditAttribution: eaton commentedJust 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. :)
Comment #37
jlinares CreditAttribution: jlinares commentedSubscribing.
Comment #38
moonray CreditAttribution: moonray commentedI would like to see a $node['url'] option as well, which would allow module developers to override the default
node/1
to something likenode/1?option=true
, and the theme would not have to guess at the destination of the teaser header for instance.Comment #39
eaton CreditAttribution: eaton commentedThat'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...
Comment #40
eaton CreditAttribution: eaton commentedThat'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...
Comment #41
moonray CreditAttribution: moonray commentedMaybe 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.Comment #42
eaton CreditAttribution: eaton commentedNot 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...
Comment #43
eaton CreditAttribution: eaton commentedNot 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...
Comment #44
tostinni CreditAttribution: tostinni commentedDoes this will help outputing things like pure XML to bound drupal content into external web services or other fancy stuff ?
Comment #45
eaton CreditAttribution: eaton commentedStage 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.
Comment #46
RobRoy CreditAttribution: RobRoy commented@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?
Comment #47
RobRoy CreditAttribution: RobRoy commentedHere's a re-roll that applies to HEAD.
Notes:
Hope this helps a bit until I can get another look.
Comment #48
RobRoy CreditAttribution: RobRoy commentedI'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.
Comment #49
eaton CreditAttribution: eaton commentedYou'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?
Comment #50
RobRoy CreditAttribution: RobRoy commentedYeah, 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
Comment #51
moshe weitzman CreditAttribution: moshe weitzman commentedAll I can do on this patch for now is add emotional support. Go RobRoy and Eaton! Thanks for working on this important patch.
Comment #52
garrettc CreditAttribution: garrettc commentedI'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.
Comment #53
eaton CreditAttribution: eaton commentedHere'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.)
Comment #54
garrettc CreditAttribution: garrettc commentedYep, 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
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?
Comment #55
eaton CreditAttribution: eaton commentedPrecisely!
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...
Comment #56
Dries CreditAttribution: Dries commentedI 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.
Comment #57
eaton CreditAttribution: eaton commentedI 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...
Comment #58
adrinux CreditAttribution: adrinux commentedworking towards theme nirvana...subscribing
Comment #59
webchickFollowing 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.
Comment #60
eaton CreditAttribution: eaton commentedAs 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.
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.
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.
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.
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.
Comment #61
RobRoy CreditAttribution: RobRoy commentedWait. 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.
Comment #62
jpetso CreditAttribution: jpetso commentedsubscribing.
Comment #63
moshe weitzman CreditAttribution: moshe weitzman commentedStatus, anyone? sure would be swell.
Comment #64
vivek.puri CreditAttribution: vivek.puri commentedsubscribing
Comment #65
Owen Barton CreditAttribution: Owen Barton commentedSubscribe
Comment #66
Frando CreditAttribution: Frando commentedLatest 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?
Comment #67
eaton CreditAttribution: eaton commentedFirst, 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.
Comment #68
eaton CreditAttribution: eaton commentedEr, whoops, ignore that. I thought I was replying in the node_styles issue. heh.
Comment #69
BioALIEN CreditAttribution: BioALIEN commentedEaton, if that's the case then we're still waiting for an answer to Frando's question ;)
Comment #70
alanburke CreditAttribution: alanburke commented+1 on this idea
Very useful for theme development
Alan
Comment #71
Zach Harkey CreditAttribution: Zach Harkey commentedSubscribing (with hopeful tears of joy in my eyes)
Comment #72
eaton CreditAttribution: eaton commentedThis 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!
Comment #73
dvessel CreditAttribution: dvessel commentedKeeping 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.
Comment #74
guardian CreditAttribution: guardian commentedsubscribing
Comment #75
Frando CreditAttribution: Frando commentedComment #76
smk-ka CreditAttribution: smk-ka commentedsubscribing
Comment #77
bjaspan CreditAttribution: bjaspan commentedSubscribe.
Comment #78
bjaspan CreditAttribution: bjaspan commentedI 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: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":
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?
Comment #79
merlinofchaos CreditAttribution: merlinofchaos commentedI 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.
Comment #80
somes CreditAttribution: somes commentedsubing
Comment #81
totalsense CreditAttribution: totalsense commentedSubscribe
Comment #82
totalsense CreditAttribution: totalsense commentedComment #83
keith.smith CreditAttribution: keith.smith commentedComment #84
mlncn CreditAttribution: mlncn commentedAll 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:
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.
Comment #85
eaton CreditAttribution: eaton commentedI 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.
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...
Comment #86
samuelwan CreditAttribution: samuelwan commentedSubscribing
Comment #87
mlncn CreditAttribution: mlncn commentedLarry 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:
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.
Comment #88
Owen Barton CreditAttribution: Owen Barton commentedI 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.
Comment #89
cpelham CreditAttribution: cpelham commentedsubscribing...
Comment #90
csevb10 CreditAttribution: csevb10 commentedWildly interested. Subscribing.
Comment #91
Senpai CreditAttribution: Senpai commentedSubscribing, with comments about Comments to follow...
Great ideas thus far!
Comment #92
Dave Cohen CreditAttribution: Dave Cohen commentedSubscribing, +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).
Comment #93
catchnothing of value to add, but I want to keep up with developments without just posting 'subscribe'.
Comment #94
OpenChimp CreditAttribution: OpenChimp commentedI hope a lot of headway can be made on this at the Drupal Code Sprint These ideas are awesome!
Comment #95
sinasalek CreditAttribution: sinasalek commentedLooks interesting
Comment #96
starbow CreditAttribution: starbow commentedWow, 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.
Comment #97
bcn CreditAttribution: bcn commentedtrack
Comment #98
ezra-g CreditAttribution: ezra-g commentedSubscribing.
Comment #99
birdmanx35 CreditAttribution: birdmanx35 commentedSubscribing.
Comment #100
swailsnet CreditAttribution: swailsnet commentedsubscribing
Comment #101
beeradb CreditAttribution: beeradb commentedway late to the party. subscribing anyway.
Comment #102
eaton CreditAttribution: eaton commentedWell, 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:
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.
Comment #103
Crell CreditAttribution: Crell commentedEaton: 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).
Comment #104
moshe weitzman CreditAttribution: moshe weitzman commented@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.
Comment #105
eaton CreditAttribution: eaton commentedi'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.
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?
Comment #106
Crell CreditAttribution: Crell commentedI 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.
Comment #107
gddsubscibing
Comment #108
Dave Cohen CreditAttribution: Dave Cohen commentedRE #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.
Comment #109
Wim LeersSubscribing.
Comment #110
coupet CreditAttribution: coupet commentedSubscribing. should consider performance improvements. the shortest (relative) path to rendering a node.
Comment #111
Liam McDermott CreditAttribution: Liam McDermott commentedSubscribing. Also #217806: Node Module Implements Comment Module has been marked a duplicate of this (great title though).
Comment #112
moshe weitzman CreditAttribution: moshe weitzman commentedregarding 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.
Comment #113
moshe weitzman CreditAttribution: moshe weitzman commentedOne more reply to Eaton's comment from ages ago:
that something is now a preprocess function:
MODULE_preprocess_page(&$variables)
. see comments above theme().Comment #114
Gábor Hojtsyre #105: why not modify how theme_links() takes the links, so we can pass the data in directly?
Comment #115
catchOn 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
Comment #116
robertDouglass CreditAttribution: robertDouglass commentedSubscribe.
Comment #117
David_Rothstein CreditAttribution: David_Rothstein commentedSubscribe.
Comment #118
dami CreditAttribution: dami commentedsubscribe
Comment #119
Susurrus CreditAttribution: Susurrus commentedWhat's the status on this? It's been over a year since the last patch...
Comment #120
cburschkaPlease, please get this into Drupal 7.
I can't remember how many hairs I've pulled out while theming nodes.
Comment #121
pwolanin CreditAttribution: pwolanin commentedSo, 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)
?Comment #122
pwolanin CreditAttribution: pwolanin commentedhere'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.
Comment #123
cburschkaReview, perhaps?
Comment #124
recidive CreditAttribution: recidive commentedAfter 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 nothook_nodeapi()
.Does that make any sense?
Comment #125
pwolanin CreditAttribution: pwolanin commentedWell, 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.
Comment #126
pwolanin CreditAttribution: pwolanin commentedchanging 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.
Comment #127
laken CreditAttribution: laken commentedsubscribing
Comment #128
cburschkapwolanin: 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().
Comment #129
pwolanin CreditAttribution: pwolanin commented@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.
Comment #130
wundo CreditAttribution: wundo commentedsubscribe
Comment #131
RobLoachUnsubscribing.
Comment #132
Susurrus CreditAttribution: Susurrus commentedIs there any progress here that can be reviewed as #129 says 20% of the way there?
Comment #133
pwolanin CreditAttribution: pwolanin commentedThis 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.
Comment #134
recidive CreditAttribution: recidive commentedRe-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.
Comment #135
pwolanin CreditAttribution: pwolanin commented@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.
Comment #136
starbow CreditAttribution: starbow commentedI 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.
Comment #137
catchAll 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?
Comment #138
pwolanin CreditAttribution: pwolanin commented@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.
Comment #139
alippai CreditAttribution: alippai commentedI can help with testing the patch (when it will be available). subscribing...
Comment #140
alippai CreditAttribution: alippai commentedIMHO this is CNW...
Comment #141
pwolanin CreditAttribution: pwolanin commentedyes - it's CNW for now.
Comment #142
pwolanin CreditAttribution: pwolanin commentedthis 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.
Comment #143
pwolanin CreditAttribution: pwolanin commentedActually, 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().
Comment #144
Owen Barton CreditAttribution: Owen Barton commentedRerolled patch (haven't had time to test it out - hopefully this week...)
Comment #145
RobLoachRerolled to head to remove some fuzz. I was still getting a blank page at admin/build/testing, not sure why...
Comment #146
JohnAlbinJust 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.phpI 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 adrupal_render()
call. :-\ Can you do that?Comment #147
starbow CreditAttribution: starbow commented@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?
Comment #148
Owen Barton CreditAttribution: Owen Barton commentedI 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.
Comment #149
Nick Lewis CreditAttribution: Nick Lewis commentedMerlinOfChaos's words from nearly a year ago still bear repeating:
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.
Comment #150
pwolanin CreditAttribution: pwolanin commented@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.
#prefix
element which is assumed to be XHTML markup.Comment #151
Crell CreditAttribution: Crell commentedI 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.
Comment #152
samirnassar CreditAttribution: samirnassar commentedCrell,
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:
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?
Comment #153
pwolanin CreditAttribution: pwolanin commented@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.
Comment #154
Crell CreditAttribution: Crell commented@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
Comment #155
recidive CreditAttribution: recidive commentedRestful 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?
Comment #156
samirnassar CreditAttribution: samirnassar commentedI 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.
Comment #157
scor CreditAttribution: scor commentedREST 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:
format
http://example.org/node/5?format=xml - the advantage is that we can read the format variable very easily in PHP.q
if the format is supported and continue the flow.One cons of this method is the double page load, but there might be a way to work around it in Drupal.
Comment #158
Jaza CreditAttribution: Jaza commentedre: 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:
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:
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.
Comment #159
Jaza CreditAttribution: Jaza commentedMay 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.What do you think, folks?
Comment #160
pwolanin CreditAttribution: pwolanin commented@Jaza - no, not for this patch. Let's focus on the essential elements here.
Comment #161
RobLoachIs anything going on here?
Comment #162
ultimateboy CreditAttribution: ultimateboy commentedSubscribe
Comment #163
dvessel CreditAttribution: dvessel commentedSorry 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.
Comment #164
Owen Barton CreditAttribution: Owen Barton commented@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.
Comment #165
korvus CreditAttribution: korvus commentedsubscribe
Comment #166
pwolanin CreditAttribution: pwolanin commentedwe already can have node-specific page templates in D6: page-node.tpl.php
Comment #167
moshe weitzman CreditAttribution: moshe weitzman commentedLets 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.)