Move node links into $node->content

I like this. Less special cases.

Subscribing.

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?

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

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

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 - are you sure it's used for comment links? - see #112514: hook_link_alter doesn't work for comment links

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?

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.

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?

Architecture is nice, has a test and it passed.

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.

Thanks Moshe. Committed to CVS HEAD.

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

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?

pwolanin: The node revision failures are #283246: Node revisions test fails if admin has a personal timezone offset.

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

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.

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.

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

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 ?

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

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.

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.

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:

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

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

Committed to CVS HEAD. Thanks.

FYI, #367214: hook_node_alter almost disappeared ?