In D5, we made a huge improvement by building up the node body as a structured array using the $node->content element. We didn't have time to include node links then - this patch fixes that. After this goes in, it will be straightforward to remove the comment rendering from node_show() and add that to $node->content as well.

This patch is pretty similar to the initial versions of #134478: Refactor page (and node) rendering. That patch got way too ambitious later on and I consider it littered beyond hope. Thus we start fresh and small in this issue. I have also marked #255686: Provide an option to display link groups in themes as a dupe since we solve that need too. Some notes:

  • hook_link() is nearly extinguished. It is no longer used for op=node or op=taxonomy_terms. taxonomy_terms was a nasty special case which this patch happily extinguishes. Modules now add node links just like other kinds of content. They use hook_nodapi_view() to append stuff into $node->content['links'].
  • A new 'form' element called node_links is introduced. It is a small wrapper around the existing theme('links')
  • Rendering of the node has been delayed from node_build_content() to template_preprocess_node(). This matches how we now do user profile rendering.
  • The theme system is unchanged.
  • 7431 passes, 0 fails, and 0 exceptions
Files: 
CommentFileSizeAuthor
#42 preprocess.patch1.04 KBmoshe weitzman
Passed: 8857 passes, 0 fails, 0 exceptions
[ View ]
#36 339929.patch1.63 KBswentel
Failed: Failed to apply patch.
[ View ]
#34 339929.patch1.2 KBswentel
Failed: 7986 passes, 5 fails, 60 exceptions
[ View ]
#21 mw.patch25.37 KBmoshe weitzman
Failed: 7641 passes, 1 fail, 0 exceptions
[ View ]
#16 mw.patch25.66 KBmoshe weitzman
Failed: Failed to apply patch.
[ View ]
#12 mw.patch24.18 KBmoshe weitzman
Failed: Failed to apply patch.
[ View ]
#8 mw.patch23.95 KBmoshe weitzman
Failed: Failed to apply patch.
[ View ]
#6 mw.patch24.44 KBmoshe weitzman
Failed: Failed to apply patch.
[ View ]
#4 mw.patch14.87 KBmoshe weitzman
Failed: Failed to apply patch.
[ View ]
mw.patch15.59 KBmoshe weitzman
Failed: Failed to apply patch.
[ View ]

Comments

kbahey’s picture

I like this. Less special cases.

Subscribing.

quicksketch’s picture

I like it also. What advantage do we get by the drupal_alter('node_view', $node, $teaser, $page); that we don't have with the hook_nodeapi() call directly before it?

chx’s picture

Status:Needs review» Needs work

why did you remove the $cid argument from comment_render...?

moshe weitzman’s picture

Status:Needs work» Needs review
StatusFileSize
new14.87 KB
Failed: Failed to apply patch.
[ View ]

@chx - good catch. I had pushed this patch further so that comment_render() was no longer mixed in with node_show(). I eventually scaled back for now but forgot to remove those bits. New patch attached.

@nate - thats just a place where sites can add some custom fiddling after contrib modules have built up $node->content. It stands in place of the existing nodeapi('alter') hook.

Also, I forgot to mention that this patch does have some deliberate indentation buglets. I left those in order to keep the diff readable. I'll fix that soon - just wanted to get some eyes on this beforehand.

pwolanin’s picture

I certainly like the general idea of removing the special casing for links.

moshe weitzman’s picture

StatusFileSize
new24.44 KB
Failed: Failed to apply patch.
[ View ]

OK, here is a patch with proper indentation and with all instances of hook_link('node') converted.

catch’s picture

Category:feature» task
Status:Needs review» Needs work

This looks great. Looks like the $teaser parameter to hook_link() is unnecessary now, so that should be removed completely. I think we already have test coverage for at least some of the hook_link implementations so that may be covered.

moshe weitzman’s picture

Status:Needs work» Needs review
StatusFileSize
new23.95 KB
Failed: Failed to apply patch.
[ View ]

Thanks catch. If you search for module_invoke_all('link' in comment.module you will see that we still pass $teaser with different values so I don't think we can remove that quite yet. Let me know if I am wrong.

Attached patch adds back the docs for hook_link_alter() since that is still used for comment links.

catch’s picture

Moshe - are you sure it's used for comment links? - see #112514: hook_link_alter doesn't work for comment links

moshe weitzman’s picture

@catch - see 'drupal_alter('link', $links, $node)' in comment_render() ... there may be additional places that need an alter call but thats not in scope here.

pwolanin’s picture

Status:Needs review» Needs work

This doesn't really make sense to me:

+  if (isset($node->preview)) {
+    unset($node->content['links']);
+  }

Probably either here or where links are added, the code should be checking the build mode.

Also, why do you need drupal_alter('link')? You have this new alter hook in node_build_content():

+  // Allow modules to modify the structured node.
+  drupal_alter('node_view', $node, $teaser, $page);

Any module wanting to alter the links should be able to implement this hook, right?

moshe weitzman’s picture

Status:Needs work» Needs review
StatusFileSize
new24.18 KB
Failed: Failed to apply patch.
[ View ]

Attached patch uses NODE_BUILD_PREVIEW instead of $node->preview. I also extinguished two notices.

The drupal_alter('links') pwolanin refers to is for *comment links*, not node links. So it is not replaced by drupal_alter('node_view')

chx’s picture

Is drupal_render too heavy or not in a preprocess function? You know, logic and presentation separation. I know, preprocess is a bit of a grey between the two. But I think we are better off with putting the rendered pieces back into node.

moshe weitzman’s picture

IMO, preprocess is clearly code, and not template. Also this is how user profiles work and unifying the two is a good idea.

chx’s picture

Sorry to throw questions at you at such a slow pace -- I think the patch is now good but this is Drupal 7, do we have a means to write tests for this..? Catching the links with a link_alter test module?

moshe weitzman’s picture

StatusFileSize
new25.66 KB
Failed: Failed to apply patch.
[ View ]

I've added an assertion to upload.test that its link on node teasers is present and accurate. That assures that the link rendering system is working.

chx’s picture

Status:Needs review» Reviewed & tested by the community

Architecture is nice, has a test and it passed.

David_Rothstein’s picture

Status:Reviewed & tested by the community» Needs review

This looks like a really great patch, but I had some comments reading through the code:

-  // Set the proper node part, then unset unused $node part so that a bad
-  // theme can not open a security hole.

I don't understand why this (and its associated code) is being removed. It looks to me like the potential security hole is still there after this patch.

-    if ($cid && is_numeric($cid)) {
+    if (!empty($node->cid)) {

The above seems unrelated to anything else... I'm guessing it wasn't supposed to be left in the final version of the patch?

+  if (!empty($node->content['teaser'])) {
+    $variables['content'] = drupal_render($node->content['teaser']);
+  }

Maybe I'm just missing it, but I can't find where $node->content['teaser'] would ever get set...

+  book_nodeapi_view_link($node, $teaser, $page);

A minor point, but the above seems to be passing a parameter that the function does not have.

+  $variables['terms']     = !empty($node->taxonomy) ? drupal_render($node->content['links']['terms']) : '';

Minor point, but it seems like it would be cleaner to check for the existence of the array that is actually being rendered, rather than checking $node->taxonomy.

+  if ($node->taxonomy) {
+    $terms = drupal_render($node->content['links']['taxonomy']);
+  }

Same comment as above applies here.

Status:Needs review» Needs work

The last submitted patch failed testing.

moshe weitzman’s picture

Status:Needs work» Needs review

I have incorporated all of david's suggestions except the first one. That is legacy code, which no longer makes sense. It dates back to before CCK when all we had was a body. $node is now a large collection of properties and 99% of them are raw and unfiltered. It is inconsistent to unset one of them because we are afraid of what a theme might do. We are handing the theme a dangerous weapon, and might as well recognize it. Themes for the most part just print $content and other nicely prepared variables. When you stray, it is quite expected that you get see raw values.

moshe weitzman’s picture

StatusFileSize
new25.37 KB
Failed: 7641 passes, 1 fail, 0 exceptions
[ View ]

oops

Status:Needs review» Needs work

The last submitted patch failed testing.

Dries’s picture

Status:Needs work» Fixed

Thanks Moshe. Committed to CVS HEAD.

moshe weitzman’s picture

pwolanin’s picture

Status:Fixed» Needs work

please roll-back. This was CNW, not RTBC due to failing tests.

pwolanin’s picture

I get I get 2 fails in node revisions, and 1 in Path aliases with translated nodes. So, it's possible that's due to some other core change?

moshe weitzman’s picture

Status:Needs work» Fixed

The test bot reported one failure in upload test but it seems spurious because it ran fine for both dries and i. i think the current test failures are unrelated to this patch. If I am wrong, let me know and I will fix quickly.

Dave Reid’s picture

pwolanin’s picture

yes, I think that's it - sorry for my confusion.

boombatower’s picture

This cause the testing bot to be disabled and took me hours to trace down: #348111: Upload.test fails when run from run-tests.sh. was an issue in run-tests.sh...please trust the bot and save my time.

Dave Reid’s picture

Status:Fixed» Active

There should probably be some kind of note in the upgrade docs on how to replace hook_link_alter for node links. There's no mention of it in the doc page or any api examples anymore.

moshe weitzman’s picture

Status:Active» Fixed

Added sentence about hook_ndoe_view_alter()

Dave Reid’s picture

Please someone also follow-up with the missing hook_node_view_alter documentation in #349469: Document missing hook_node_view_alter().

swentel’s picture

Status:Fixed» Needs review
StatusFileSize
new1.2 KB
Failed: 7986 passes, 5 fails, 60 exceptions
[ View ]

Follow up patch:
Changing $variables['taxonomy'] to $variable['terms'] triggers php notices if taxonomy is disabled:

Notice: Undefined variable: taxonomy in include() (line 22 of /var/www/drupal/drupal-cvs/themes/garland/node.tpl.php.

Patch attached changes terms back to taxonomy in theme.inc, or am I missing something completely important here ?

Status:Needs review» Needs work

The last submitted patch failed testing.

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new1.63 KB
Failed: Failed to apply patch.
[ View ]

Ok, looks like changing theme.inc was a bad move, changing tpl files instead.

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

Looks like a good fix.

Dries’s picture

Committed #34 to CVS HEAD. Setting back to 'code needs review' because we're discussing some other issues, and because it is not clear if all documentation has been updated yet.

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

David_Rothstein’s picture

Status:Needs work» Fixed

I added a section about the theme changes (removal of the $taxonomy variable in node.tpl.php files) to the "Converting 6.x themes to 7.x" page (http://drupal.org/node/254940#taxonomy). Someone should probably review it for accuracy, though.

I think that's the only missing documentation for this issue, so I'm setting back to "fixed" for now.

catch’s picture

Status:Fixed» Needs work

As far as I can see, David Rothstein's comment about $node->content['teaser'] wasn't addressed - on both full views and teaser lists, this hunk doesn't run:

<?php
 
if (!empty($node->content['teaser'])) {
+   
$variables['content'] = drupal_render($node->content['teaser']);
+  }
?>
moshe weitzman’s picture

Status:Needs work» Needs review
StatusFileSize
new1.04 KB
Passed: 8857 passes, 0 fails, 0 exceptions
[ View ]

Yeah, that looks like cruft from a prior version of the patch. Please review.

Status:Needs review» Needs work

The last submitted patch failed testing.

catch’s picture

Status:Needs work» Needs review
catch’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:+Quick fix

Ran tests, manually verified in the other issue that this was dead code. RTBC.

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Committed to CVS HEAD. Thanks.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

AmrMostafa’s picture