Fatal error when using the Comment module's "Unpublish comment containing keyword(s)" action

Thanks for the report. Could you post specific steps to reproduce this issue on a clean install of D7?

Thanks @fasdalf@fasdalf.ru! Does this happen if you follow those steps on a clean install?

That's a bona fide major bug then, unfortunately. Filing against 7.x since trigger has been removed from core in D8.

Tagging novice to add an automated test, since the steps in #2 can be used to write a functional test fairly easily.

hi

hopefully I can give a solution for this but first some thoughts.

The error is in comment_unpublish_by_keyword_action.

This function is making a call for drupal_render($comment) but $comments is a commentobject and not render array

first try:

function comment_unpublish_by_keyword_action($comment, $context) {
    comment_build_content($comment, null);
    $text = drupal_render($comment->content);
    foreach ($context['keywords'] as $keyword) {
      if (strpos($text, $keyword)) {
        $comment->status = COMMENT_NOT_PUBLISHED;
        watchdog('action', 'Unpublished comment %subject.', array('%subject' => $comment->subject));
        break;
      }
    }
}

first line is added and second line is changed.

first line isn't good imho due to sending a null for a variable is wrongly and also creates an error in log.

it also seems that the comment, even though the status is corretly set to COMMENT_NOT_PUBLISHED,is published.

hi

the php error of this issolved but

ok hopefully that will solve it now

Thanks @filijonka. Looks like a nice simple fix, although having to do a full node_load() is a little worrisome. I wonder if we can mock the node object? It just looks like it's being used to determine how the comment form and links will be displayed, which may not be relevant.

First though let's see what the bot thinks and write a test for it.

#9: drupal-trigger_comment_by_keywords-1461732.patch queued for re-testing.

yes you're right about the node_load and it's quite sufficient to just initialize an object.

Here's a test, it fails without the patch in #9 and passes with it. Improvements welcome, especially since this is my first attempt at writing an automated test.

The test looks great, and the correct test failure is exposed on testbot. The only things I'd change are two minor points about the assertion message (and in both case the rest of trigger.test is providing the bad example):

+++ b/modules/trigger/trigger.testundefined
@@ -642,6 +642,33 @@ class TriggerOtherTestCase extends TriggerWebTestCase {
+    $this->assertTrue(variable_get($action_id, FALSE), t('Check that a comment can be added when "Unpublish comment containing keyword(s)" action is triggered.'));

Most of our assertion messages just indicate the expected result, so in this case I'd go with simply "A comment can be added when...". The second thing is that (unlike almost any other text string) assertion messages do not need to be translated; see http://drupal.org/simpletest-tutorial-drupal7#t. (And yeah, the rest of the module's tests doesn't follow either of these suggestions.)

Want to reroll with those changes, and then upload both the test patch by itself and another patch with the test combined with #9? I'd still prefer to work around the node_load() but we can at least confirm that #9 causes #14 to pass.

Thanks!

ignore #9 and use this one instead.
We actuallydon't need a node for this one,we just need an object.

Thanks for the review @xjm!

Re: #17

+++ b/modules/comment/comment.moduleundefined
@@ -2591,8 +2591,10 @@ function comment_unpublish_action($comment, $context = array()) {
+  comment_build_content($comment,$node);

I believe there should be a space between the two arguments here.

+++ b/modules/comment/comment.moduleundefined
@@ -2591,8 +2591,10 @@ function comment_unpublish_action($comment, $context = array()) {
+  $text = drupal_render($comment->content);
   foreach ($context['keywords'] as $keyword) {

Should there be a blank line between these two lines?

I'll do a re-roll this evening.

blank line?

sorry for the mistake,felt so bad i hadn't ul the patch so made it in a hurry

Here's the revised test, with a better assertion message and not wrapped in t() - thanks @xjm.

Two patches, one with the test only and one with the patch in #19. The first should fail, the second should pass.

Uh oh, the test seems to be failing on my local sandbox with Undefined property: stdClass::$comment.

hey

kind realised now that this has been marked component trigger.module but the error is actually in comment.module and we should have done this for 8.x, not7.x and backporting to 7.x.

so remarking and will do new patch for 8.x

The patch for7.x is still valid and should be tested for.

could you please explain why this new patch?

It looks like #26 is a performance fix. #24 (and the current code) is performing the same render repeatedly for every comment, which is unnecessary.

However, #26 has several code style errors which should be fixed. Also, we need to figure out a way to test this in Drupal 8 with trigger removed; maybe the actions module has some tests we could extend? Finally, I'm concerned about the exception that was exposed when the test was added in #20; we need to fix that as well.

sorry @xjm but you're for once wrong, the rendering in #24 is only done once,it's not declared within the foreachloop.

+++ b/core/modules/comment/comment.moduleundefined
@@ -2393,8 +2393,11 @@ function comment_unpublish_action($comment, $context = array()) {
   foreach ($context['keywords'] as $keyword) {
-    $text = drupal_render($comment);

@filijonka ^ Sorry, that is what I meant. And you're right, I guess the deletion is in the earlier patch as well. I have no idea what the additional patch is for, then. :) We still need to get it passing the test, though.

@xjm that's alright,i'm just happy you're wrong for once:)

and yes I'll take a look at whenI got the time.probably gonna need to ask Q though.

Updating tags per http://drupal.org/node/1517250

Ok patch with test only, combined patch to follow shortly.

And combined version.

#37: 1461732.patch queued for re-testing.

#37: 1461732.patch queued for re-testing.

This looks good and has test coverage. Changing to RTBC and will trigger a re-test to set it back if it doesn't apply anymore but I doubt that code has changed.

#37: 1461732.patch queued for re-testing.

This looks fine except one hunk in the tests:

+    $comment = $this->postComment($this->node, $keyword_2, $this->randomName());
+
+    // The actual comment so as we have status.
+    $comment = comment_load($comment->id);

So $this->postComment) doesn't return a real comment, not introduced by this patch though.

However the code comment doesn't appear to be an actual sentence. Should it be "Load the full comment so that status is available."?

Yes that is what it should be.

#45: 1461732.patch queued for re-testing.

Looks good now, RTBC and will trigger retest just to be sure.

#45: 1461732.patch queued for re-testing.

#45: 1461732.patch queued for re-testing.

Rerolled

Setting to RTBC again, will come back NW in case there's something wrong.

This should do it - commentEntityNG conversion made the new test crash.

#55 looks good, just tweaked the test method and docblock.

Looks good.

Looks great. Committed/pushed to 8.x, moving to 7.x for backport.

Working on the 7.x backport.

Backported #56 to D7.

It wasn't the most straightforward backport since the Action module and its functions which are used in the D8 patch don't exist in D7, at least as far as I know. I'm not familiar with actions or their code, so I maybe I don't know what I'm doing. I did what I could to replicate the missing functionality.

Switched back to using comment_build_content() instead of comment_view(). Set node->comment = 0 in order to prevent the exception in comment_links() which was mentioned in #21.

#63: 1461732-63-comment-action-test.patch queued for re-testing.

+++ b/modules/comment/comment.moduleundefined
@@ -2615,8 +2615,11 @@ function comment_unpublish_action($comment, $context = array()) {
+  $node = new stdClass();
+  $node->comment = 0;
+  comment_build_content($comment, $node);

That looks like a rather ugly hack. Why not do a $node = node_load($comment->nid)?

See #10, #13, #16, and #17. xjm preferred this hack to loading an entire node object.

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

Issue summary updated

Issue summary updated (webwarrior)

patch no longer applies.

#70 can you give some more information than that it doesn't applies? in what way doesn't it applies any longer?

63: 1461732-63-comment-action-test.patch queued for re-testing.

For the record, the xjm of two years ago was prematurely optimizing, and it's possible folks should instead listen to Berdir.

I've been testing this now for awhile and the patch #63 solves the issue but it seems that the comment is posted anyway even if the action is triggered

after reading through this issue carefully, taking in all opinions and debugging the system with patch #63 i came to conclusion that patch weren't correct so I rewrote it.

there is no test with this but it will come. The main problem with the earlier patches (even the d8 one which is no longer interesting since comment is rewritten) is that it only sets the comment as unpublished but it actually never saves it.

A test for previous patch

sorry ehm forgott that I made changes...

ok got a bit wiser thanks to berdir..needed a combined patch and not a test only patch

Thanks!!

The patch is working fine...

Removed double issue summary template.

1. +++ b/modules/comment/comment.module
   @@ -2603,7 +2603,7 @@ function comment_unpublish_action($comment, $context = array()) {
   + * @param object $comment
     *   Comment object to modify.
   

   Should we typehint \Drupal\comment\CommentInterface?

2. +++ b/modules/comment/comment.module
   @@ -2615,10 +2615,13 @@ function comment_unpublish_action($comment, $context = array()) {
   +  $node = node_load($comment->nid);
   +  comment_build_content($comment, $node);
   ...
   +      comment_save($comment);
   
   +++ b/modules/comment/comment.test
   @@ -1974,6 +1974,45 @@ class CommentActionsTestCase extends CommentHelperCase {
   +    $hash = drupal_hash_base64($callback);
   ...
   +    $result = db_query("SELECT aid, type, callback, parameters, label FROM {actions}")->fetchObject();
   ...
   +    $action = db_query("SELECT aid, type, callback, parameters, label FROM {actions} "
   ...
   +    $this->assertEqual(comment_load($comment->cid)->status, COMMENT_NOT_PUBLISHED, 'Comment was unpublished');
   

   It feels a bit wrong to use all that old methods instead of using the proper alternative already.

ignore prev comment, chat with @dawehner and he had missed this is a D7 patch

79: 1461732-78-comment-action.patch queued for re-testing.

why just retest this without any comments on why or changin tags or anything @joshi.rohit100?

@filijonka, sorry for that. But it was by mistake.

89: 1461732-89-comment-action.patch queued for re-testing.

retesting to confirm if it really needs reroll.

Working on a reroll.

I was able to cleanly apply the patch in #89 to HEAD, don't think this needs a reroll.

89: 1461732-89-comment-action.patch queued for re-testing.

Followed steps to reproduce 1-6 (from original issue above) with 1461732-89-comment-action.patch applied and confirm there is no error now.

The patch works, no errors, comments get correctly published/unpublished, THX!

 function comment_unpublish_by_keyword_action($comment, $context) {
+  $node = node_load($comment->nid);
+  comment_build_content($comment, $node);

Sorry, I'm not clear why this is using comment_build_content() rather than comment_view() like the Drupal 8 patch did...

The difference could (in some cases) be important because comment_view() invokes some hooks that allow the data it returns to be changed.

I see there were some PHP notices and such when that was tried in one of the earlier patches, but it should be possible to track down why....? comment_view() really seems like the correct way to get the same version of the rendered comment that users actually see on the site.

hmm ok it's a change by dcam between comment/patch in 61 and 63 done two years back.

question is if this is such a significant problem and if so how to prove it so we have to do yet another patch and wait yet another x months/years.

I don't remember why I did that. My suspicion is that comment_view() wouldn't work with the $node parameter being an empty stdClass object, but comment_build_content() did. As @Berdir mentioned in #66, this was all just an ugly hack which I attempted to implement because @xjm didn't want to load an entire node object. She later retracted that in #73. So, there probably isn't any reason why we can't load the node object and use comment_view() now.

Since we're loading the node now, let's see if it works with comment_view().

Good we are adding a test to cover this now. It's quite odd that we tried to render a comment object like that...

nothing chnaged so setting back this to green per #112

per #112

Committed to 7.x - thanks!

I fixed a few small issues in the test on commit (removed an unused $result variable, and fixed a few code style issues):

--- a/modules/comment/comment.test
+++ b/modules/comment/comment.test
@@ -13,7 +13,7 @@ class CommentHelperCase extends DrupalWebTestCase {
   function setUp() {
     parent::setUp('comment', 'search');
     // Create users and test node.
-    $this->admin_user = $this->drupalCreateUser(array('administer content types', 'administer comments', 'administer blocks','administer actions'));
+    $this->admin_user = $this->drupalCreateUser(array('administer content types', 'administer comments', 'administer blocks', 'administer actions'));
     $this->web_user = $this->drupalCreateUser(array('access comments', 'post comments', 'create article content', 'edit own comments'));
     $this->node = $this->drupalCreateNode(array('type' => 'article', 'promote' => 1, 'uid' => $this->web_user->uid));
   }
@@ -1976,7 +1976,7 @@ class CommentActionsTestCase extends CommentHelperCase {
   /**
    * Tests the unpublish comment by keyword action.
    */
-  public function testCommentUnpublishbyKeyword() {
+  public function testCommentUnpublishByKeyword() {
     $this->drupalLogin($this->admin_user);
     $callback = 'comment_unpublish_by_keyword_action';
     $hash = drupal_hash_base64($callback);
@@ -1988,10 +1988,7 @@ class CommentActionsTestCase extends CommentHelperCase {
 
     $this->drupalPost("admin/config/system/actions/configure/$hash", $edit, t('Save'));
 
-    $result = db_query("SELECT aid, type, callback, parameters, label FROM {actions}")->fetchObject();
-
-    $action = db_query("SELECT aid, type, callback, parameters, label FROM {actions} "
-                     . "WHERE callback = :callback", array(':callback' => $callback))->fetchObject();
+    $action = db_query("SELECT aid, type, callback, parameters, label FROM {actions} WHERE callback = :callback", array(':callback' => $callback))->fetchObject();
 
     $this->assertTrue($action, 'The action could be loaded.');
 
@@ -2004,12 +2001,11 @@ class CommentActionsTestCase extends CommentHelperCase {
 
     comment_unpublish_by_keyword_action($comment, array('keywords' => array($keywords)));
 
-    // We need to make sure that the comment has been
-    // saved with status unpublished.
-    $this->assertEqual(comment_load($comment->cid)->status, COMMENT_NOT_PUBLISHED, 'Comment was unpublished');
-    $this->assertWatchdogMessage('Unpublished comment %subject.', array('%subject' => $comment->subject), 'Found watchdog message');
+    // We need to make sure that the comment has been saved with status
+    // unpublished.
+    $this->assertEqual(comment_load($comment->cid)->status, COMMENT_NOT_PUBLISHED, 'Comment was unpublished.');
+    $this->assertWatchdogMessage('Unpublished comment %subject.', array('%subject' => $comment->subject), 'Found watchdog message.');
     $this->clearWatchdog();
-
   }
 
   /**