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.
Comment | File | Size | Author |
---|---|---|---|
#31 | 350984.patch | 3.5 KB | moshe weitzman |
#26 | kick-comment-rendering-node-module.patch | 21.68 KB | dmitrig01 |
#25 | kick-comment-rendering-node-module.patch | 10.95 KB | dmitrig01 |
#21 | mw.patch | 21.6 KB | moshe weitzman |
#18 | mw.patch | 22.81 KB | moshe weitzman |
Comments
Comment #1
catchlooks good on a first pass, subscribing.
Comment #2
dmitrig01 CreditAttribution: dmitrig01 commentedVisually looks great, but i can't say about functionality. Let's wait for test results and then see what happens.
Comment #4
David StraussThe 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.
Comment #5
David StraussLet's fix that exception. :-)
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commented@David there are some seemingly unrelated changes in the diff regarding comment mode. What was the cause of the exception?
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commentedNow I see that the comment node wrapper is further separation of node from comment. Nice work. Whats the comment_update_index() change for?
Comment #8
David Strauss@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.
Comment #9
moshe weitzman CreditAttribution: moshe weitzman commentedNice work. Tests pass, and code looks good.
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commentedJust 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().
Comment #12
catchThe search ranking changes are causing the fail.
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedIn 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.
Comment #14
David StraussI'll try to babysit this patch over the weekend.
Comment #15
moshe weitzman CreditAttribution: moshe weitzman commentedI had misrun my tests - all looks good except for one assertion in SearchRankingTestCase. Hopefully David can have a look at that.
Comment #16
David Strauss@moshe I'll take a look tomorrow at work.
Comment #17
moshe weitzman CreditAttribution: moshe weitzman commentedThe 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.
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedThis 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.
Comment #19
David StraussRunning tests now...
Comment #20
David StraussI ran all tests twice, and I got failure both times on "First record inserted as expected."
Comment #21
moshe weitzman CreditAttribution: moshe weitzman commentedThat 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.
Comment #22
dmitrig01 CreditAttribution: dmitrig01 commentedCode looks good, functionality works, all tests passed for catch. It's good!
Comment #23
catchAll 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 :)
Comment #24
catchCross-posted, I agree this is RTBC.
Comment #25
dmitrig01 CreditAttribution: dmitrig01 commentedUntested but should do the trick (re: dbtng conversion)
Comment #26
dmitrig01 CreditAttribution: dmitrig01 commentedOops sorry, not unidiff
Comment #27
moshe weitzman CreditAttribution: moshe weitzman commentedAll tests pass, except for an unrelated failure in 'Comment approval'. New code looks good - thanks.
Comment #28
Dries CreditAttribution: Dries commentedThis was committed to CVS HEAD.
Comment #29
dmitrig01 CreditAttribution: dmitrig01 commentedw00t! thanks Dries!
Comment #30
sunSpacing/newline errors.
Line exceeds 80 chars.
$comments are only output if $links are output for pushbutton.
Comment #31
moshe weitzman CreditAttribution: moshe weitzman commented@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.
Comment #32
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #34
sunCatch 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.
Comment #35
moshe weitzman CreditAttribution: moshe weitzman commentedtha_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.
Comment #36
Damien Tournoud CreditAttribution: Damien Tournoud commentedInteresting, 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().