Search results show comment count for content types where comments are disabled

Unfortunately, the content type settings for comments are more suggestions than requirements -- on a per-item basis, you can turn comments on or off for that content item/node/page/story/etc. In other words, the setting is not per content type but per node.

It is also possible to turn off new comments (on the node) but still have old comments there -- that's what the setting actually does (it doesn't wipe out existing comments).

So it seems as though the logic that would make sense is:
- If there are some comments, display the comment information
- If there are 0 comments currently and comments are turned off for this particular node, don't display the comment information.

This works the same in Drupal 7, and should be fixed there and then ported back to D6.

Tagging #d7ux as it seems like a user experience issue to me...

If it is working correctly in Drupal 7, then we need to make this a D6 issue, not mark it as fixed. It's definitely not working correctly in Drupal 6.

This is not working correctly for me in Drupal 7. I have a page for which comments are closed and there are 0 comments, and the search results are still showing "0 comments".

http://api.drupal.org/api/function/comment_node_search_result/7 is the problem. It is checking for COMMENT_NODE_HIDDEN, but not for COMMENT_NODE_CLOSED.

And on my edit form for this node, I only have options for closed and open. The reason being that http://api.drupal.org/api/function/comment_form_alter/7 has this logic in it:

   // If the node doesn't have any comments, the "hidden" option makes no
    // sense, so don't even bother presenting it to the user.
    if (empty($comment_count)) {
      unset($form['comment_settings']['comment']['#options'][COMMENT_NODE_HIDDEN]);
      unset($form['comment_settings']['comment'][COMMENT_NODE_HIDDEN]);
      $form['comment_settings']['comment'][COMMENT_NODE_CLOSED]['#description'] = t('Users cannot post comments.');
    }

So there is no way to set this node to have its comments hidden, but the search results are presenting the 0 comments even though it is closed.

That's a bug.

There may be some related issues in http://api.drupal.org/api/function/comment_node_view/7 as well -- it doesn't look like it's hiding comments in teaser view if status is HIDDEN?

Here's a fix for the comment issue. Besides fixing the problem identified in this bug, it also gets rid of the "-" that was reported in #2.

If you disagree with how this was patched, then the logic in comment_form_alter() needs to be fixed instead so that even nodes with 0 comments can have their comment status set to "hidden".

Does this need a test? I suppose so, but anyway here's the patch to start with.

As far as adding tests, here is what I think we should be testing:
- Comments are marked "hidden" for a node: Node with 0 or 1+ comments does not have "N comments" showing in search results.
- Comments are marked "closed" for a node: Node with 1+ comments has the "N comments" string; node with 0 comments doesn't have it.
- Comments are marked "open" for a node: Node with 0 or 1+ comments does have "N comments" showing up in search results.

Where "N comments" should be taken to mean "0 comments" or "1 comment" or "# comments".

Trying to write a test for this in room 675

* Tests that comment count display toggles properly on comment status of node
*
* - Nodes with comment status set to Open should always how comment counts
* - Nodes with comment status set to Closed should show comment counts
* only when there are comments
* - Nodes with comment status set to Hidden should never show comment counts

Code patch and test patch are currently separate. I'm reviewing and will review final, combined patch later.

Here are the two patches rolled together.

I have reviewed the test and it looks good. The patch applies and the test runs successfully.

Applied the patch. No errors. I basically understand the code and it seems correct. Attached an image which shows two open articles: 1 with a comment, and the other with no comments.

Committed to CVS HEAD. Thanks.

We should now move this down to D6 for consideration.

On our hosting environment, when running this test, we get a "preg_split()" unknown error notice in the CommentToggle test:

GET http://simpletest.gardens-simpletest.acquia-sites.com/ returned 200 (7.84 KB). Browser search.test 1058 SearchCommentCountToggleTestCase->testSearchCommentCountToggle()
Valid HTML found on "http://simpletest.gardens-simpletest.acquia-sites.com/" Browser search.test 1058 SearchCommentCountToggleTestCase->testSearchCommentCountToggle()
Verbose message Debug search.test 1058 SearchCommentCountToggleTestCase->testSearchCommentCountToggle()
preg_split() [function.preg-split]: Unknown error Notice search.module 1306 search_simplify_excerpt_match()

I am unable to determine what the root cause is. strace isn't helping me here. I did find this potentially related bug:
http://www.mail-archive.com/php-bugs@lists.php.net/msg103869.html

If I make the change in the attached path, it works fine. Are the single quotes required there? I don't understand what they are doing in that case.

Sorry, took it from the wrong dir. This includes the other places where the same thing caused notices.

err....

Okay, this time I'm going to really upload the new patch :)

I'm guessing the quotes are there to ensure multi-word exact matches. I *think* using double instead od single quotes will bring the same result? In any case, the tests still pass if you just remove the quotes or switch to double quotes. And switching to double doesn't trigger the notice for me.

This looks like a completely unrelated bug, and the only slight relation to the issue reported here is that one segment of your proposed patch overlaps the patch here.

I think it should be filed as a separate issue, and maybe on that issue you can patch all the notices you are getting, since they seem to be from the same source?

Hi @jhodgdon

#1061208: On certain environments, preg_split() is throwing notices in search tests when single quotes are passed for this. It's a pretty simple patch.

Unfortunately, I have no more information on the noticed. I straced them and checked every log. That's realyl all I got.

Sorry to post here. I did a CVS blame and figured I start with the commit that caused it in the first place, but it is kinda unrelated.

Talked with Gabor (the Drupal 6 branch maintainer) and D6 issues are really not being committed unless they're really essential -- we really don't have a test system for Drupal 6 and it's too dangerous. So... putting this back to D7 / fixed.