Comment nodeapi unnecessary query

There is no patch attached.

Patch looks sensible, however you've not indented within the new if statement.

And this qualifies as a bug IMO.

I've fixed a code style issue in your patch, and I've committed it to CVS HEAD.

The patch is wrong: the display of comments on a particular node is governed not by the type-wide parameter 'comment_'.$node->type but by the node-specific parameter $node->comment. See node_show():

  if (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).

You're right, Daniel. I've rolled back the patch. This illustrates the need to write tests for patches.

We could presumably do the same thing with if ($node->comment) right?

@Dries: No hard feelings, but it also illustrates the importance of the community review :)

@catch: completely.

Yeah -- for read only you still display them so it's a nice catch.

Test looks and works good, and is worthwhile in it's own right.

Thanks for writing these tests. Much appreciated. Committed to CVS HEAD.

I've committed #22 to HEAD, but I leave #21 for Gabor to review. Lowering version to 6.x.

Thanks R.

patch at #21 is not necessary.

patch at #20 applies cleanly.

Is there a way I should test the patch further?

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;      

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.

@andypost: that's the way it currently works. Your module can really call comment_nodeapi() manually after altering the $node->comment parameter if needed.

@m3avrck: in fact it really should... so as _nodeapi($op = 'update index'), by the way.

Ok here are 3 patches...

• For D7, add $node->comment checking to search ops
• For D6, entire patch, with statistics and search ops
• For D5, ditto (I know this won't be committed but I am running it on MothersClick.com for anyone that may want it)

Bumping this still needs attention & review...

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.

The function signature of check_markup() changed.

Those queries might as well be converted to DBTNG when touching them.

So this is reroll with checking for hidden comments

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

Patch is here

reroll against current HEAD

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.

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

Looks good.

Committed to CVS HEAD. If we want to back-port this to Drupal 6 (per the tag), we'll have to re-roll.

reroll for D6 against current 6-dev