Posted by moshe weitzman on December 24, 2008 at 9:37pm
8 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | node system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | moshe weitzman |
| Status: | closed (fixed) |
Issue Summary
This patch build upon #339929: Move node links into $node->content. It moves comment rendering out of node_show(). Instead comment module inserts comments itself into the $node->content structured array. In template_preprocess_node(), comments are extracted into a simple $comments variable which is printed at bottom of node.tpl.php by default. Themers are free to move it as desired.
I marked #52088: Can't easily move comment form in a theme and #152402: Make node_show() themeable for comment positioning as duplicates of this issue. There are surely more.
There is of course more than can be done, but I'm stopping here.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| mw.patch | 7.29 KB | Idle | Failed: 7991 passes, 0 fails, 1 exception | View details |
Comments
#1
looks good on a first pass, subscribing.
#2
Visually looks great, but i can't say about functionality. Let's wait for test results and then see what happens.
#3
The last submitted patch failed testing.
#4
The exception you got in testing is further evidence that the poll module is mostly around to cause trouble, and I mean in a good way that shows us what's broken. Also, subscribing and interested in seeing this land in core.
#5
Let's fix that exception. :-)
#6
@David there are some seemingly unrelated changes in the diff regarding comment mode. What was the cause of the exception?
#7
Now I see that the comment node wrapper is further separation of node from comment. Nice work. Whats the comment_update_index() change for?
#8
@moshe I moved the code out of node.module that reads the max number of comments on nodes, which is used for ranking search results. The node module should ideally be completely oblivious to the whole comment system.
#9
Nice work. Tests pass, and code looks good.
#10
Just to see where this is headed, I have posted my next patch after this one goes in. That patch defers all all theming for the homepage and the node detail page until index.php. Thats pretty much a holy grail, and we are getting very very close. See #351235: hook_page_alter().
#11
The last submitted patch failed testing.
#12
The search ranking changes are causing the fail.
#13
In order to stop showing comments on node previews, it is convenient to simply get rid of the $page parameter that we pass around in nodeapi('view') and other hooks. Thats a prehistoric relic of Drupal. Replaced by (bool)menu_get_object(). That variable is still available to templates since I've added it to template_preprocess_page().
This patch has lots of test failures. At quick glance, they look unrelated to this patch. Perhaps someone can work on those. I am not doing dev this weekend.
#14
I'll try to babysit this patch over the weekend.
#15
I had misrun my tests - all looks good except for one assertion in SearchRankingTestCase. Hopefully David can have a look at that.
#16
@moshe I'll take a look tomorrow at work.
#17
The search test had hard coded node_update_index assuming that it was the only relevant implementation of the hook. That is no longer true, with the new comment_update_index. Changed that to a module_invoke_all() and now we are good. search cron() just begged for a little cleanup while i was there.
#18
This patch fixes a couple of test failures. Now I am seeing two failures: one in SQL handling tests and one in Path aliases with translated nodes. I suspect those are due to an unclean testing environment and not true failures. Since the testbot is down, it would be great is someone could run the test suite and review the code and RTBC if all looks good.
#19
Running tests now...
#20
I ran all tests twice, and I got failure both times on "First record inserted as expected."
#21
That test failure happens for me without the patch as well. Perhaps it is mysql version/configuration related.
Attached patch gets rid of useless node_comment_mode() and comment_node_mode() functions. They were useless wrappers. I reran most tests and all looks OK.
#22
Code looks good, functionality works, all tests passed for catch. It's good!
#23
All the code looks good - there's one db_result which needs converting to dbtng but it's a copy/paste from elsewhere so will get caught at some point. All tests pass too (can't reproduce the failures reported earlier with or without the patch). Lots and lots of cruft removed :)
#24
Cross-posted, I agree this is RTBC.
#25
Untested but should do the trick (re: dbtng conversion)
#26
Oops sorry, not unidiff
#27
All tests pass, except for an unrelated failure in 'Comment approval'. New code looks good - thanks.
#28
This was committed to CVS HEAD.
#29
w00t! thanks Dries!
#30
$variables['title'] = check_plain($node->title);+ $variables['page'] = (bool)menu_get_object();
...
// Render all remaining node links.
$variables['links'] = !empty($node->content['links']) ? drupal_render($node->content['links']) : '';
+
+ // Render any comments.
+ $variables['comments'] = !empty($node->content['comments']) ? drupal_render($node->content['comments']) : '';
...
+
...
-}
+}
\ No newline at end of file
Spacing/newline errors.
@@ -392,22 +390,21 @@ function hook_nodeapi_validate($node, $f...
* will be called after hook_view(). The structure of $node->content is a renderable
Line exceeds 80 chars.
+++ themes/pushbutton/node.tpl.php 31 Dec 2008 00:43:06 -0000<?php if ($links): ?>
<div class="links">» <?php print $links ?></div>
+
+ <?php print $comments; ?>
+
<?php endif; ?>
$comments are only output if $links are output for pushbutton.
#31
@Sun - not sure if you realize that this issue was committed already.
The attached patch fixes Sun's bugs and also prevents comments from rendering during node preview (another bug fix). Please review.
#32
Committed to CVS HEAD. Thanks.
#33
Automatically closed -- issue fixed for two weeks with no activity.
#34
Catch was so kind to enlighten and point me to this issue from #410674: Remove $page parameter from hook_node_view() implementations. This patch introduced magic we don't really want.
<tha_sun|pissed> It seems like the $page argument was removed in D7 (good), but replacing it with menu_get_object() === bad.
tha_sun|pissed: not everything that isn't a teaser is a page.
<tha_sun|pissed> Why was $page removed then?
tha_sun|pissed: actually, page isn't removed - it's still available in templates.
tha_sun|pissed: but menu_get_object() is a standard way to know if you're on the official page for the thing you're working with.
tha_sun|pissed: $page was just a special case version of that for nodes.
<tha_sun|pissed> So, nodes can't ever be displayed in full on an arbitrary path anymore.
tha_sun|pissed: erm, why not?
<tha_sun|pissed> Because each and every module manually checks menu_get_object()
<tha_sun|pissed> You've a Panels page on my/foo/path/that/is/NOT/node, tada.
<tha_sun|pissed> You want to expose a full node page view to an external service, on cron runs, impossible.
tha_sun|pissed: ah I see, set $page to true manually, can't do that any more.
#35
tha_sun - thats true, but don't blame this patch. this was barely related to $page
i suspect that the best solution for embedded nodes is to expose a new BUILD_MODE and have the admin specify which fields appear in that build mode. In D6, thats done in CCK. In D7, we are in transition. The book footer and all the other $page stuff should transition from menu_get_object() to BUILD_MODE. Thats out of scope for this patch though.
The point of this patch was not to get rid of page, I just did because it is a needed step toward better BUILD_MODE support.
#36
Interesting, I just opened #445902: (bool) menu_get_object() is *not* an equivalent of $page. Let's move the discussion there.
I believe that the good place for $page is somewhere in an alter hook, probably not in hook_page_alter(), as I suggested there, but on a special alter hook to be added at the bottom of node_show().