Move node links into $node->content
moshe weitzman - November 27, 2008 - 18:58
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | node system |
| Category: | task |
| Priority: | normal |
| Assigned: | moshe weitzman |
| Status: | closed |
| Issue tags: | Quick fix |
Description
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
| Attachment | Size |
|---|---|
| mw.patch | 15.59 KB |
| Testbed results | ||
|---|---|---|
| mw.patch | failed | Failed: Failed to apply patch. Detailed results |
#1
I like this. Less special cases.
Subscribing.
#2
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?#3
why did you remove the $cid argument from comment_render...?
#4
@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.
#5
I certainly like the general idea of removing the special casing for links.
#6
OK, here is a patch with proper indentation and with all instances of hook_link('node') converted.
#7
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.
#8
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.
#9
Moshe - are you sure it's used for comment links? - see #112514: hook_link_alter doesn't work for comment links
#10
@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.
#11
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?
#12
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')
#13
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.
#14
IMO, preprocess is clearly code, and not template. Also this is how user profiles work and unifying the two is a good idea.
#15
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?
#16
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.
#17
Architecture is nice, has a test and it passed.
#18
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.
#19
The last submitted patch failed testing.
#20
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.
#21
oops
#22
The last submitted patch failed testing.
#23
Thanks Moshe. Committed to CVS HEAD.
#24
Added upgrade docs - http://drupal.org/node/224333#node_links
#25
please roll-back. This was CNW, not RTBC due to failing tests.
#26
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?
#27
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.
#28
pwolanin: The node revision failures are #283246: Node revisions test fails if admin has a personal timezone offset.
#29
yes, I think that's it - sorry for my confusion.
#30
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.
#31
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.
#32
Added sentence about hook_ndoe_view_alter()
#33
Please someone also follow-up with the missing hook_node_view_alter documentation in #349469: Document missing hook_node_view_alter() .
#34
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 ?
#35
The last submitted patch failed testing.
#36
Ok, looks like changing theme.inc was a bad move, changing tpl files instead.
#37
Looks like a good fix.
#38
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.
#39
The last submitted patch failed testing.
#40
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.
#41
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:
<?phpif (!empty($node->content['teaser'])) {
+ $variables['content'] = drupal_render($node->content['teaser']);
+ }
?>
#42
Yeah, that looks like cruft from a prior version of the patch. Please review.
#43
The last submitted patch failed testing.
#44
#45
Ran tests, manually verified in the other issue that this was dead code. RTBC.
#46
Committed to CVS HEAD. Thanks.
#47
Automatically closed -- issue fixed for two weeks with no activity.
#48
FYI, #367214: hook_node_alter almost disappeared ?