Currently, node_view() is limited to the node itself, and a couple of specific parameters -- $teaser, $page, and $links. There's a lot of other context that we can't capture with it -- should comments be displayed below the node? Are we being rendered for use in an RSS feed? etc.

That means that quite a few places in Drupal are simply copy-and-paste duplicates of node_view, with necessary tweaks. rss feed building and book.module's print-friendly formatting are two standout examples. Piling additional little-used parameters onto the end of the function is horrible and ugly, but we need context.

This patch replaces the existing $teaser, $page, and $links parameters with a more flexible system that lets modules define and use specific 'styles' for node display.

function node_view($node, $teaser = FALSE, $page = FALSE, $links = TRUE) {...}

Becomes...

function node_view($node, $style = 'full', $options = array()) {...}

$style is a textual key corresponding to defined rendering styles for views. Core defines four of them: full, teaser, feed, and print, to match the four ways we're currently building nodes in core. Other modules, by implementing hook_node_styles(), can define their own ('postcard', 'mail', and others are possibilities. Esoteric node styles like 'xml' could be implemented as well). By calling node_get_styles(), modules can pull up a list of all the currently defined styles and their descriptions. This means that modules that add chunks of content to nodes can customize their output for each rendering style, in the same way that modules can use node_get_types() to present options for each node type.

The $options array is a collection of values that controls finer-grain options in the node viewing process. $page and $links have been replaced by $options['page'] and $options['links']; other flags currently in the patch are $options['mark_read'], which determines whether a particular viewing of a node increments its statistics counter and the 'new' flag for readers. Any module can add new flags like this to the default options -- forum module demonstrate this in the patch, by adding $options['show_forum_navigation'] = TRUE to the definition for the standard 'full' viewing style. Modules can add style-specific options and global default options that apply to every style unless overridden.

One of the modules that benefits most obviously from this approach is CCK -- currently, it allows site builders to specify how each field should appear when displayed in teaser mode or full mode. There's no way to control other contexts, though, like RSS feeds. With this system, CCK would be able to simply add a column to its UI for each defined node style, and let users choose the field display settings for each one. Other modules that use nodeapi to add content can use the same approach to give greater flexibility and control over output WITHOUT nasty hacking.

In addition, this adds the ability to specify node.tpl.php files not just per content type, but per rendering style as well. node-teaser.tpl.php is possible, for example, eliminating the need for complex switching logic. node-feed.tpl.php is another example, allowing users to customize the display of nodes just in RSS feeds.

The current state of the patch is 'core node types work, but rss feeds and print-friendly mode haven't yet been upgraded.' Those fixes are coming shortly, but the patch is very testable.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drewish’s picture

subscribing. i think this is an excellent idea. i banged my head against this trying to RSS feeds a while back.

drewish’s picture

for those that didn't want to parse the patch, i just wanted to highligh that hook_nodeapi is also affected by this:
function hook_nodeapi(&$node, $op, $teaser, $page) {
becomes:
function hook_nodeapi(&$node, $op, $style = 'full', $options = array()) {

yched’s picture

+      drupal_set_message('blarghl');

Ah, so you do like strange debug messages too :-)

It seems hook_nodeapi('view') can be opted out ($options['skip_alter']) ?
Doesn't this introduce a weird exception wrt other hook_nodeapi ops ?

Aside from this, two possibly dumb suggestions :
- would it be better performance-wise to declare at least core default styles ('teaser', 'full', 'rss'...) as define()'ed integer constants, and let if ($style == 'teaser') be integer comparison ? Or is it minor and we don't care ?
- node_get_styles() will always return the same results. Is it a candidate for db-cache ?

I salivate thinking of all this patch opens up.
(and shiver thinking of the CCK UI work, but well...)

eaton’s picture

Ah, so you do like strange debug messages too :-)

Heheh. Yeaaaaaah, I noticed that after I posted. Heh. ;-)

It seems hook_nodeapi('view') can be opted out ($options['skip_alter']) ?
Doesn't this introduce a weird exception wrt other hook_nodeapi ops ?

That might actually be something to toss; I was experimenting with different options and I'm not sure that's really the best way to handle it. I was just kicking around the idea of a way to get a fast, stripped down 'just the basics' version of a node. Might be a bad idea.

- would it be better performance-wise to declare at least core default styles ('teaser', 'full', 'rss'...) as define()'ed integer constants, and let if ($style == 'teaser') be integer comparison ? Or is it minor and we don't care ?
- node_get_styles() will always return the same results. Is it a candidate for db-cache ?

Possibly. I'm not super concerned about the int vs. string comparisons, but I could be overlooking something important. I think there's value in consistency, and having 'core' styles as ints and 'module' styles as strings seems like a bad precedent.
There's definitely an option for db-caching with the style information. I deliberately left that out for now as I wanted to focus on the functionality in this round, but node_get_styles() is absolutely a central spot for potential caching.

I salivate thinking of all this patch opens up.
(and shiver thinking of the CCK UI work, but well...)

Try this. Do a search for _content_admin_display_contexts() in content_admin.inc. Replace it with node_get_style_names(). ;-)

Ta-da. Your work is done. ;-)

eaton’s picture

FileSize
30.53 KB

Voila -- now RSS feeds and printer-friendly book pages go through the system now as well.

Due to the trickiness of our rendering system, feed generation and book rendering still use a few shortcuts (ie, they use node_build_content() rather than node_view()). They still use the new rendering system, however, and the result is loads of additional configurability. CCK, for example, can selectively choose whether to add fields to RSS feeds, use a different formatter, etc. In addition, the ugly 'nodeapi op print' has been eliminated; since $style is set to 'print' modules have all the context they need to do special versions of the content.

http://drupal.org/node/134478 (the node rendering refactoring project) is complimentary to this patch, and the results of the two together will be crazy powerful for themers and developers. Rar.

eaton’s picture

Status: Needs work » Needs review

Setting this to 'needs review'. This version includes a system update that tweaks a system variable (feed_item_length), but nothing will explode in flames if you forget to apply it for a bit.

eaton’s picture

FileSize
30.88 KB

Whoops. Missed the search index node render call. In theory, we could have the search index be yet one more render style, but that's a matter of style.

webchick’s picture

Status: Needs review » Needs work

Preliminary results of testing. I'm going to need to try this again tomorrow when I'm more coherent, because this stuff doesn't make any sense:

1. I first added a blog entry and a book page, and tested teaser/full/print/rss views. All looked fine.
2. Then, I hit cron.php and attempted to do a search. The search returned no results, even though it should've.
3. I reverted the patch, and tried the search again. Still no results.
4. I hit the re-index button. I can't remember if I did that with the patch applied or not. I think it was probably not applied.
5. After that, I was getting results without the patch, so I tried it again with.
6. Now, search results were returned, but yielded the following notices:

 * notice: Undefined index: page in /Applications/MAMP/htdocs/head/modules/blog/blog.module on line 222.
 * notice: Undefined index: body_field in /Applications/MAMP/htdocs/head/modules/node/node.module on line 774.
 * notice: Undefined index: show_book_navigation in /Applications/MAMP/htdocs/head/modules/book/book.module on line 448.

Also, the teasers were just "..."
7. I hit admin/content/node and unpublished the book page. This seemed to work fine.
8. But, I went back to the home page where I received the following additional warning/notices:

 * warning: Invalid argument supplied for foreach() in /Applications/MAMP/htdocs/head/includes/theme.inc on line 1557.
 * notice: Undefined index: style in /Applications/MAMP/htdocs/head/includes/theme.inc on line 1568.
 * notice: Undefined index: style in /Applications/MAMP/htdocs/head/includes/theme.inc on line 1598.
 * notice: Undefined index: style in /Applications/MAMP/htdocs/head/includes/theme.inc on line 1599.

And now it's not displaying the title of the blog post (?!?)

So.. I don't really know *what* the problems are, but I know that this patch needs a bit of work. ;)

eaton’s picture

FileSize
30.82 KB

A very important note: this patch changes the variable mapping for theme_node(). tpl.php files shouldn't need to be changed, but you WILL need to switch to another theme and back for things to match up. That's a consequence of the new theme function registry.

Also, silly parse error in Chameleon now fixed.

moshe weitzman’s picture

I gave this a pretty a good workout and code read and it works/looks good to me. lets resolve the issues webchick mentioned and then move it along.

eaton’s picture

Status: Needs work » Needs review

Just talked to webchick and her problems appear to trace back to the theme cache issue as well. She suggested setting it back to Code Needs Review since they disappeared when she switched themes.

Further testing still needed, but it looks like the weirdness was due to that.

RobRoy’s picture

Status: Needs review » Needs work

Last patch doesn't apply for me. Is that just me?

RobRoy’s picture

Status: Needs work » Needs review
FileSize
31.13 KB

I gotta run, but here is a hasty re-roll with the chameleon fix and some Doxygen.

add1sun’s picture

subscribing. w00t!

yched’s picture

Status: Needs review » Needs work

Eaton, your system_update_6017() forgets to actually return the (empty) $ret array - will cause warnings on update.

yched’s picture

Status: Needs work » Needs review
FileSize
31.08 KB

I guess I can fix this myself, instead of setting back to CNW...

yched’s picture

FileSize
28.26 KB

Wait a minute - RobRoy's patch in #13 seems to be missing some bits of eaton's latest patch in #9

Here's a HEAD reroll of #9 which, I think, should be complete.
I also changed a remaining if (!$page) { to if (!$options['page']) { in chameleon.theme.

Side note :
there's a bunch of code like the following in the patch, which seem very likely to raise 'undefined index' PHP warnings :

function forum_view(&$node, $style = 'full', $options = array()) {
  if ($options['page']) {
...

I did not change them for now, but we might want scan the patch for these.
There is no shortcut for , is there ?

eaton’s picture

yched, several of the options, like 'page' and 'body_field' and 'show_links' and so on are ALWAYS set to true or false. That's the magic of $styles['default']['options'] ;-) Those get array_merge'd into the options element of all node styles.

yched’s picture

Yes, but you provide a default value of $options = array() just the line above.
Either there is a chance of hook_view being called without $options param, or there is not.
If there is, you should secure $options['page'] with empty() or something.
If there is not, the default value should probably be removed.
What do you think ?

drewish’s picture

humm, this dosn't seem to be generating a teaser for the front page. i tried switching the theme thinking that might straighten it out but no luck.

i also ran into the following warnings.

using Chameleon/Marvin for node/X/view

notice: Undefined variable: teaser in D:\drupal\drupalhead\themes\chameleon\chameleon.theme on line 135.

using Chameleon/Marvin for previewing /node/X/edit

notice: Undefined variable: teaser in D:\drupal\drupalhead\themes\chameleon\chameleon.theme on line 135.
notice: Undefined property: stdClass::$links in D:\drupal\drupalhead\themes\chameleon\chameleon.theme on line 152.
drewish’s picture

Status: Needs review » Needs work

yeah the teasers are broken.

eaton’s picture

FileSize
30.07 KB

Attached is a version of the patch that fixes those two issues in Chameleon/Marvin. Are teasers displaying in any other themes? I've yet to be able to reproduce those problems after doing a theme-switch..

RobRoy’s picture

I get this now. Create a story node, views full node fine, teasers show up fine, but on /node this comes up. Looks like it's trying to merge $extra but that doesn't exists.

# notice: Undefined variable: extra in \modules\node\node.module on line 1998.
# warning: array_merge() [function.array-merge]: Argument #1 is not an array in \modules\node\node.module on line 1998.

Seems to be working besides that though.

RobRoy’s picture

FileSize
28.74 KB

This patch just adds an $extra = array(); above that while loop. Think that's the intended behavior, so leaving at CNW. Sorry about doing that last one too hastily, hopefully I don't blow this one line change. :)

eaton’s picture

Status: Needs work » Needs review
FileSize
30.12 KB

Actually I just looked closer and the issue with $extra is due to a dropped line in the RSS feed handling -- $extra should be the results of nodeapi op 'rss item', not an empty array... thus the array_merge on the next line. :D

This version includes that change.

RobRoy’s picture

FileSize
39.55 KB

Getting this when I go to /rss.xml then back to /.

warning: __clone method called on non-object in includes\common.inc on line 1436.

I debugged it a bit and attached a backtrace. Looks like it might be menu related, but included here just in case. Think this is still CNR, and may be close to RTBC!

eaton’s picture

"warning: __clone method called on non-object in /Users/jeff/Sites/menutest/includes/common.inc on line 1436." appears to be a menu system issue right now: I'm seeing it on a number of pages, including the first visit to /node after site installation, on a completely clean HEAD without the style patch.

eaton’s picture

FileSize
31.84 KB

Here's the patch with the doxygen comments rolled back in.

RobRoy’s picture

FileSize
31.65 KB

A couple Doxygen tweaks, and changed the def of poll_view_results so $block = FALSE as it was after required params. Can you try to apply this, and if Eclipse didn't bork it I think it's RTBC.

eaton’s picture

Just applied and tested this -- thanks for catching poll.module, as it was in an earlier version of the patch but slipped out.

This version appears to have nailed down all the issues associated with the patch thus far. I'm not able to set it RTBC given the fact that my name's on it, but anyone is welcome to. ;-)

RobRoy’s picture

Status: Needs review » Reviewed & tested by the community

RTBC even though I posted the last patch, it was just minor doc changes and one tiny tweak. Let's get this in and move on to http://drupal.org/node/134478. Hizzah!

yched’s picture

There are several places where we test $style against 'teaser' :
- in upload.module to decide whether to display download links
- in book.module to decide whether to display book navigation...
Wouldn't these be better off proposed as 'options' flags (like forum.module does for 'show_forum_navigation') ?
(Can probably be done in a subsequent patch, I'm just checking I get the concepts right...)
Actually, is there any sense in having any hardcoded if ($type == 'teaser')-like logic ?

eaton’s picture

There are several places where we test $style against 'teaser' :
- in upload.module to decide whether to display download links
- in book.module to decide whether to display book navigation...
Wouldn't these be better off proposed as 'options' flags (like forum.module does for 'show_forum_navigation') ?
(Can probably be done in a subsequent patch, I'm just checking I get the concepts right...)
Actually, is there any sense in having any hardcoded if ($type == 'teaser')-like logic ?

That's an excellent question. Book module should actually be using an options flag, and upload module would benefit from this approach as well. What probably comes next, if this patch is committed, is adding configuration options that acknowledge the range of different styles. An option in the upload config page that lets us choose which display styles include the list of attached files, versus a link in the $node->links, would be good...

I think that probably requires more discussion as it's a UI issue and could very quickly get into complex discussions about user preferences and so on. ;-)

But you're absolutely right that more and more stuff can be migrated into either $options, or node type settings that store a list of styles in the same way that many nodes currently store a list of types to operate on.

eaton’s picture

On further consideration, I think I can articulate what I think is the best approach.

  1. Ideally, modules should either operate under display styles, or provide a configuration page that allows users to choose which currently active style(s) should be acted upon.
  2. In addition, modules should recognize the presence of an $options flag (like 'show_forum_navigation') as a programmatic override to the normal user settings.
  3. Alternately, modules that operate in 'faceless' mode without configuration screens can add their default $options flags to the $styles['default']['options'] array using hook_node_styles(). (This is what was done with forum and book nav...)

Any thoughts?

yched’s picture

My take on this was to consider styles as 'meaningless' per se, and conveying some semantics only through the default set of $options they define. Meaning modules should not take any decision based on the style name, but rely only on the set of $options.

Then defining which style gets which options could ideally be done through a checkbox UI (that does not necessarily needs to live in core ?)

eaton’s picture

Yeah, I can see that. Basically the $options array was put in place to allow finer-grain control than JUST the display style. The abiliy to, when necessary, render a teaser *BUT* with one bit of additional information teasers don't usually have, lessens the need to create jillions of one-off display styles, cluttering up any configuration UI, because one code snippet needed to override the defaults. Also, we do need some sort of internal mapping system for the non-module-controlled stuff, like whether links are shown in a given display style etc.

eaton’s picture

*nudge*

Just wanted to give this a poke; there are two other related issues that are in a holding pattern depending on this one, dating back to the conversation Dries had with webchick about how to best solve the signatures issue. Any thoughts from core committers? The sooner this can get the thumbs/up thumbs down, the better chance we have of knowing how to proceed with those.

Dries’s picture

1. This patch has potential, but I'd like to see us think it through a bit more. For example, I'd love to see us solve the signature problem, and I'd like to see us put comments in a $node field as well. It's not clear how well that would work.

2. The style name 'default' is somewhat poor -- and I was confused by its purpose. Other people will get confused about the name 'default' too, and it isn't until you look at the implementation of node_view() that it becomes clear what default does. This is a small barrier for developers. I think it would be better, if we could get rid of the 'default' style. It would make things more explicit. I don't think we have 'dynamic' styles, have we? The number of styles are set in stone? If so, it doesn't seem excessive to get rid of 'default'. I'd also consider to make defines for the different style names.

3. I want to think some more about what this means, or could mean, for things like node_prepare(). I also want to think some more about what this means for the 'view' hook -- used to overwrite a node's view. Could any of this be simplified or eliminated?

Overall, I like the direction of this patch, and it is something I'd like to see land before D6. At the same time, I don't want to rush this and I want us to be a little bit more critic about how this can be improved. I encourage people to extend this patch so more parts of Drupal can take advantage of this.

Dreaming: ultimately, I want to have a per-node checkbox to toggle the display of author information, or to toggle taxonomy links, or to toggle the date. This patch seems to be a step towards facilitating that, but the path isn't 100% clear yet.

Let's take this patch a step further by seeing how we can deal with signatures, comments, avatars, etc. Let's also see if we can simplify other parts of Drupal -- like the view hook and node_prepare.

I'll keep thinking about this too. :)

eaton’s picture

2. The style name 'default' is somewhat poor -- and I was confused by its purpose. Other people will get confused about the name 'default' too, and it isn't until you look at the implementation of node_view() that it becomes clear what default does. This is a small barrier for developers. I think it would be better, if we could get rid of the 'default' style. It would make things more explicit. I don't think we have 'dynamic' styles, have we? The number of styles are set in stone? If so, it doesn't seem excessive to get rid of 'default'. I'd also consider to make defines for the different style names.

Nope, we do in fact support dynamic styles -- that's part of the point of the patch, and how it opens up a bit more flexibility for module developers. Postcard module is an example of this; it could expose a separate 'postcard' rendering style, and modules that use node_get_style_names() to provide a list of when their extra info should be displayed would automatically be able to treat 'postcard' rendering style differently than teaser, full, etc. There is no way to do this at present. That's why the default element is provided: for modules to support 'default' options that will be applied to all defined styles, even if they're not yet known.

CCK is another example of how this system is useful: there is no way to alter what fields are included in the RSS feed version of a node, as opposed to the normal teaser or full views. This patch changes that and would allow CCK to easily customize that. If a module were to add another style (say, 'postcard'), CCK and any other module using node_get_styles() would be able to work with them as easily as they current work with different content types.

For example, I'd love to see us solve the signature problem, and I'd like to see us put comments in a $node field as well. It's not clear how well that would work.

That work is going on in http://drupal.org/node/134478 -- the patch that currently works alongside this one. That patch refactors the node rendering system to support cleaner separation of those elements, while this one handles the addition of the new 'styles' concept. Based on past experience with major core patches, I thought it would have a better chance if we were to separate out the distinct chunks of functionality rather than implementing a single, monstrous patch.

3. I want to think some more about what this means, or could mean, for things like node_prepare(). I also want to think some more about what this means for the 'view' hook -- used to overwrite a node's view. Could any of this be simplified or eliminated?

Not really. Again, those changes would be in the realm of http://drupal.org/node/134478. This patch is a means of capturing context that we currently lack: 'what is this node being built for?' Teaser, full version, rss feed, print-friendly version, full-page mode, etc. Previously we've solved the problem by appending numerous optional parameters onto the end of the node_view() call, but that gets out of control quickly and makes it difficult for contrib modules to intelligently implement UIs for the different styles/modes nodes can be in..

Overall, I like the direction of this patch, and it is something I'd like to see land before D6. At the same time, I don't want to rush this and I want us to be a little bit more critic about how this can be improved. I encourage people to extend this patch so more parts of Drupal can take advantage of this.

I'm not quite what you mean by 'extending the patch so more parts of drupal can take advantage of it.' At present it adds a single new dimension to node viewing and rendering: the style the node is being rendered in. There are many ways modules can utilize that. Do you mean that the patch should add explicit support for different display options per-style in modules like Upload? ('display the attachment list in: teaser, print-friendly, feed, full, etc...') We deliberately decided to avoid that in order to keep scope creep out of the picture.

Dreaming: ultimately, I want to have a per-node checkbox to toggle the display of author information, or to toggle taxonomy links, or to toggle the date. This patch seems to be a step towards facilitating that, but the path isn't 100% clear yet.

That's outside the scope of this patch -- per-node configuration doesn't really have anything to do with the functionality that's in here. Tracking per-node display options would require additional tables, etc. Changing the display of those parts of the node based on the current rendering style is certainly possible, but again, there's work going on in http://drupal.org/node/134478 to do that.

I suppose the primary question is this: Should we merge this patch, and the http://drupal.org/node/134478 patch into a single one? This patch, regardless of whether that one gets in, provides some very valuable functionality to module developers. (See especially the 'CCK' case above.) Rolling them together as one seems like a violation of the 'keep separate functionality in separate patches' principle that was driven home during the Drupal 5 dev cycle.

Wim Leers’s picture

Subscribing.

Dries’s picture

Talked briefly with Steven about this patch and we have some additional ideas -- Steven said he'd give it a closer look tomorrow.

eaton’s picture

Awesome! Thanks, Dries.

Steven’s picture

Status: Reviewed & tested by the community » Needs work

The 'styles' as originally listed are "teaser, full, print, and feed". Justifying this as "this is how we currently use nodes in core" and saying that "modules can extend this any way they want" seems like a really bad way to go about this:

  • "Teaser" and "full" show different body content. "Print" actually has little to do with printer friendlyness (that's what a print stylesheet is for), and *really* just serves to aggregate a node with its related content (the child pages). This represents variation in what content to display.
  • "Teaser", "full" and "print" all output to HTML, however, the "print" HTML is used without a stylesheet. Aside from style differences, there are also UI differences: the appearance of 'add new comment' links, the display of the title, the repetition down the page, ...). This represents variation in how the content is themed.
  • "Feed" is completely different. It exports to RSS/XML, collects namespaces rather than JS/CSS and is unthemed. This represents variation in the content's format.

The current 4 styles are an odd mish-mash: for example, "feed" will actually display full nodes or teasers, depending on feed_item_length. There is no way to unify the "print" style with an actual print stylesheet as used on regular pages. And Dries' idea of "user-choosable node styles" only sounds really complicated, because there is no clear way to fit into this flat list.

It actually took me a while to realize that the only use of 'print' is to let other modules insert stuff in, while the actual logic of iterating over the child pages is external to that (unless I'm misreading the patch).

One important observation is that "formats" aren't really that. Due to the way the filter system works, you'll always have to filter to HTML first, and then transcode to something else. When you output something as themed HTML or as RSS, you're really defining the method of encapsulation: usually a body, with various pieces of metadata spread around. This makes sense when you consider that project.module sends out plain text emails, which try to maintain the semantics of the HTML version. The headers in the email can depend on node content (e.g. subject).

There is also a 4th encapsulation: "update index" HTML. When node.module indexes data for search, certain meta-data is collected into a single chunk of HTML, so that it can be indexed. The fact that we've had to start from the node view has always been a pain (we don't want forum navigation links in there, for example).

You could argue that "printer friendly", as it is implemented now, is just another 'encapsulation' rather than a display style.

When it comes to 'content' to display, the line between what is content and what isn't is fuzzy. For me, things like the forum navigation are UI, not content. The read more link could be considered content (e.g. you want it to show up in your RSS feeds), but you could easily theme it yourself or make it redundant (e.g. make the entire node clickable).

Furthermore, there is a difference in choosing how to display a certain field (e.g. teaser vs body) and choosing which fields to display in a certain context (e.g. node listing vs full page). This patch seems to merge those two together at the hip.

Finally, as far as actual theming goes, it completely depends on the complexity of the theme and the skill of the site owner. However, if you look at how theme settings are implemented now, allowing manual .tpl.php tweaks or out of order rendering isn't all that incompatible with providing user-controllable settings for display. Templates already need to do things like <?php if ($foo) print $foo; ?>, because the theme system may unset certain variables based on settings.

After processing all this, I can't help but feel that this is another situation where splitting up related patches leads to missing the big picture. The reasons and execution of this feature is intricately tied to the node rendering stuff, yet both are being treated as independent.

dodorama’s picture

I'd love to see something like this in D6.

eaton’s picture

Status: Needs work » Postponed

dodazzi, with about 36 hours before the code freeze I think there isn't much chance of this getting into D6. I would've liked it too -- I think it's critical for a number of important CCK developments -- but there's been relatively little support for it and some unanswered questions.

Once HEAD re-opens for development on Drupal 7, I'd like to revisit this issue and the node rendering patch.

Frando’s picture

Status: Postponed » Needs work
FileSize
32.59 KB

Is anyone working on this now that we got still 11 days until the freeze?
Eaton/RobRoy: feel free to set this back to postponed - I myself won't have time to deeply dive into that but I'd be glad to review this.
Latest patch didn't apply cleanly anymore (but only wrong newlines). Here's a reroll that applies to HEAD.

Very quick review:

When viewing a node with the patch applied, I get the following notices:

# warning: Invalid argument supplied for foreach() in /home/frando/www/drupal/HEAD/nr/includes/theme.inc on line 1582.
# notice: Undefined index: style in /home/frando/www/drupal/HEAD/nr/includes/theme.inc on line 1593.
# notice: Undefined index: style in /home/frando/www/drupal/HEAD/nr/includes/theme.inc on line 1623.
# notice: Undefined index: style in /home/frando/www/drupal/HEAD/nr/includes/theme.inc on line 162

The node title is not getting printed in teaser lists (e.g. front page).
RSS feeds seem to work!

Frando’s picture

FileSize
32.64 KB

Damn, didn't notice that there's an update function and thus forgot to increase its number.
After having done this and running update.php the notices dissappears and the title is printed correctly. Patch attached.
Played around some more and didn't notice any bugs so far.

alanburke’s picture

+1 on this idea.

I use some crazy logic (that really should be there) in my tpl.php files to get around the limitations that this would resolve.

Alan

bjaspan’s picture

Subscribe.

Pancho’s picture

Version: 6.x-dev » 7.x-dev

Unfortunately way too late for D6...

Gábor Hojtsy’s picture

Yay, but open for Drupal 7.

q0rban’s picture

Subscribing.

moshe weitzman’s picture

Status: Needs work » Closed (duplicate)