Posted by R.Muilwijk on April 24, 2008 at 2:08pm
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | comment.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | needs backport to D6 |
Issue Summary
In the nodeapi the comment statistics are loaded for every node_load.
function comment_nodeapi(&$node, $op, $arg = 0) {
switch ($op) {
case 'load':
return db_fetch_array(db_query("SELECT last_comment_timestamp, last_comment_name, comment_count FROM {node_comment_statistics} WHERE nid = %d", $node->nid));
break;Why not make it something like this?
function comment_nodeapi(&$node, $op, $arg = 0) {
switch ($op) {
case 'load':
if (variable_get('comment_'.$node->type, COMMENT_NODE_READ_WRITE) != COMMENT_NODE_DISABLED) {
return db_fetch_array(db_query("SELECT last_comment_timestamp, last_comment_name, comment_count FROM {node_comment_statistics} WHERE nid = %d", $node->nid));
}
break;NB: I found this in drupal 5.7 but checked and it still applies to 6 and 7-dev.
Comments
#1
There is no patch attached.
#2
My bad here it is
#3
Patch looks sensible, however you've not indented within the new if statement.
#4
And this qualifies as a bug IMO.
#5
Sorry catch I am not really familiar yet with the categories.
New patch is up
#6
Huh somehow I changed it back to feature request. Back to bug report
#7
I've fixed a code style issue in your patch, and I've committed it to CVS HEAD.
#8
The patch is wrong: the display of comments on a particular node is governed not by the type-wide parameter
'comment_'.$node->typebut by the node-specific parameter$node->comment. Seenode_show():<?phpif (function_exists('comment_render') && $node->comment) {
$output .= comment_render($node, $cid);
}
?>
Oh, by the way: the current comment count (as shown for example in the tracker) is actually wrong (it should be 0 when comments are disabled).
#9
You're right, Daniel. I've rolled back the patch. This illustrates the need to write tests for patches.
#10
We could presumably do the same thing with if ($node->comment) right?
#11
@Dries: No hard feelings, but it also illustrates the importance of the community review :)
@catch: completely.
#12
new patch making the code like this:
function comment_nodeapi(&$node, $op, $arg = 0) {switch ($op) {
case 'load':
if ($node->comment != COMMENT_NODE_DISABLED) {
return db_fetch_array(db_query("SELECT last_comment_timestamp, last_comment_name, comment_count FROM {node_comment_statistics} WHERE nid = %d", $node->nid));
}
break;
#13
Could someone please review the patch applied above.
Here are the backported patches for 6-dev and 5-dev.
#14
Yeah -- for read only you still display them so it's a nice catch.
#15
the 14th a comment code cleanup was submitted. Patch is still working however.
patch -p0 < comment_nodeapi_load_1.patch
patching file modules/comment/comment.module
#16
http://drupal.org/files/issues/comment_nodeapi_load_1.patch is ready to be comitted. Hope to get it backported to 6 soon
#17
The patch only applied with a offset. I added a test for checking whether or not the comment pager works. Comment pager uses the information form nodeapi load so it should test whether or not this damages anything.
The test passes before and after applying patch. Because of this addition the status is back to patch (code needs review).
#18
Test looks and works good, and is worthwhile in it's own right.
#19
Thanks for writing these tests. Much appreciated. Committed to CVS HEAD.
#20
6 doesn't have the .test file so the patch for 6 only contains the .module patch.
#21
Unfortunatly there was a little typo in my patch. For head the following patch is provided. The patch for 6.x is already fixed and valid.
#22
#23
I've committed #22 to HEAD, but I leave #21 for Gabor to review. Lowering version to 6.x.
Thanks R.
#24
patch at #21 is not necessary.
patch at #20 applies cleanly.
Is there a way I should test the patch further?
#25
Can't this patch be further expanded to improve the search index and results as well?
@@ -412,15 +417,23 @@
case 'update index':
$text = '';
- $comments = db_query('SELECT subject, comment, format FROM {comments} WHERE nid = %d AND status = %d', $node->nid, COMMENT_PUBLISHED);
- while ($comment = db_fetch_object($comments)) {
- $text .= '<h2>'. check_plain($comment->subject) .'</h2>'. check_markup($comment->comment, $comment->format, FALSE);
+ if ($node->comment != COMMENT_NODE_DISABLED) {
+ $comments = db_query('SELECT subject, comment, format FROM {comments} WHERE nid = %d AND status = %d', $node->nid, COMMENT_PUBLISHED);
+ while ($comment = db_fetch_object($comments)) {
+ $text .= '<h2>'. check_plain($comment->subject) .'</h2>'. check_markup($comment->comment, $comment->format, FALSE);
+ }
}
return $text;
case 'search result':
- $comments = db_result(db_query('SELECT comment_count FROM {node_comment_statistics} WHERE nid = %d', $node->nid));
- return format_plural($comments, '1 comment', '@count comments');
+ if ($node->comment != COMMENT_NODE_DISABLED) {
+ $comments = db_result(db_query('SELECT comment_count FROM {node_comment_statistics} WHERE nid = %d', $node->nid));
+ return format_plural($comments, '1 comment', '@count comments');
+ }
+ else {
+ return '';
+ }
+ break;
#26
Suppose comments should be loaded anyway because I can change $node->comment by editing node params for example to stop flame but for history purposes I need ability to watch them. This patch stops ability to load old comments and only way put duplicate code of nodeapi in own module.
#27
@andypost: that's the way it currently works. Your module can really call
comment_nodeapi()manually after altering the$node->commentparameter if needed.@m3avrck: in fact it really should... so as _nodeapi($op = 'update index'), by the way.
#28
Ok here are 3 patches...
#29
Bumping this still needs attention & review...
#30
Seems like the search patch should go into D7 (where it'll get more attention), then it'll just be a straight backport of all changes to Drupal 6. So bumping this back up for a bit.
#31
The last submitted patch failed testing.
#32
The function signature of check_markup() changed.
Those queries might as well be converted to DBTNG when touching them.
#33
So this is reroll with checking for hidden comments
#34
Before print link in RSS there should be a check if comments enabled
So maybe CommentRSSUnitTest should be changed? or this for another issue #335893: Option to not include comment element in Feeds
#35
Patch is here
#36
The last submitted patch failed testing.
#37
#38
reroll against current HEAD
#39
Patch makes sense to me. It's a slight change to search behaviour - currently we index comments with nodes even if they're hidden, but hide them on view (but you could still see the comment at comment/1 for example). Now they'll not be indexed either.
Doesn't look like there's any existing tests for comments appearing in the search index (or those nodeapi hooks in general). Would you be up for writing those? Looking at the search test, it seems like it'd be mainly copying and pasting from one of the existing tests which creates a node and indexes it, but adding a comment and changing the settings.
#40
So here patch with tests included
Added test to modules/search/search.test
Logic - change node to hide comments, reindex search, try to find comment body
#41
Looks good.
#42
Committed to CVS HEAD. If we want to back-port this to Drupal 6 (per the tag), we'll have to re-roll.
#43
reroll for D6 against current 6-dev
#44
Works oke for me on d6
#45
The last submitted patch, comment-query.patch, failed testing.