Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, comment_approve_event.patch, failed testing.

polosatique’s picture

I have fixed patch format.

polosatique’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, rules-comment_approve_event-1421058.patch, failed testing.

polosatique’s picture

Status: Needs work » Needs review
FileSize
973 bytes

I have fixed patch format.

maartendeblock’s picture

I can confirm this patch works on 7.x-2.0. I would love to see this getting picked up.

fago’s picture

Status: Needs review » Needs work

Unfortunately the comment publish hook is rather broken, i.e. it's not always invoked when it should be. I remember I've opened a core issue for that some time ago.

Anyway, because of that I don't think we should add that event. However, you can check for that using the regular pre-save event + conditions on the comment status. I'd be happy to include a new condition "Comment is published" analogously to the node condition though, as well as a "Publish comment" action.

ruess’s picture

I'd be happy to include a new condition "Comment is published" analogously to the node condition though, as well as a "Publish comment" action.

This would be great!

mitchell’s picture

Title: Rules comment approve event » Event: Comment is published
Version: 7.x-2.0 » 7.x-2.x-dev

This looks like the issue referenced in #7: #900396: Remove separate comment publishing hooks or fix them.

Still waiting on rewrite of #5 with the hooks described in #7.

mitchell’s picture

Component: Provided Rules integration » Rules Core
Status: Needs work » Postponed (maintainer needs more info)

What about entity is changed, type is comment, status of submitted comment is 0, and status of changed comment is 1?

mvlabat’s picture

Status: Postponed (maintainer needs more info) » Needs review
fago’s picture

Title: Event: Comment is published » Add comment published conditions and actions
Category: feature » task
Status: Needs review » Needs work

We do not have entity is changed events and we should try to avoid theme for performance reasons. The more fine-granular events are, the faster it is. Also, for UX the entity* stuff sucks. In the long term I think we should move back to per entity-type CRUD actions as well.

Anyway, because of that I don't think we should add that event. However, you can check for that using the regular pre-save event + conditions on the comment status. I'd be happy to include a new condition "Comment is published" analogously to the node condition though, as well as a "Publish comment" action.

Let's do that, as this is the way it currently works for all other entity types (e.g. nodes) as well.

PatchRanger’s picture

Let's do that, as this is the way it currently works for all other entity types (e.g. nodes) as well.

Done.
The first file contains only new test.
The second one has test and functionality both.
Please review.

PatchRanger’s picture

Status: Needs review » Needs work

Test bot stuck.
Let's re-trigger testing.

PatchRanger’s picture

Status: Needs work » Needs review

Let's see if it helps.

PatchRanger’s picture

Nope, it didn't. Ok, let's do it this way: re-attach patches.

PatchRanger’s picture

Strange. Somebody, please help me to tame the bot!:)
What's wrong with my patches?

PatchRanger’s picture

Ok, I've understood what is wrong.
The problem is that simpletest tests of the whole Rules 7.x-2.x branch failed.
See #1797576: Fix simpletest tests for more information.

Status: Needs review » Needs work
savingstrangers’s picture

Did this ever get followed up on? I was hoping when I just downloaded the 7.x-2.3 version of Rules it might include a "Comment is Published" or "Comment is Approved" event, but doesn't, and I would LOVE this... I need an email sent out to node author upon someone commenting on their content, but the comments must be approved by admin first, so I don't want it emailing them once the comment is created, but rather once it's published/approved!

savingstrangers’s picture

For anyone else needing to do what I described in post #20... I ended up using the action "After Updating an Existing Comment" and it worked out to where it would be okay in my situation, I realize it would not always work for others (in case someone may actually update comments, which I'm not going to do) , but it worked for my situation! Again, would like to see "Upon Comment Approval" or "Upon Comment Published" events in future Rules :)

deggertsen’s picture

Rather odd that this isn't in more demand. I guess most people try to use the notifications module instead? Anyways, the patches in #16 applied cleanly for me, but then I got this error:

Additional uncaught exception thrown while handling exception.

Original

ReflectionException: Function rules_condition_comment_is_published() does not exist in ReflectionFunction->__construct() (line 1691 of /sites/all/modules/rules/includes/rules.core.inc).

Additional

FacesExtendableException: There is no method process for this instance of the class RulesAction. in FacesExtendable->__call() (line 133 of /sites/all/modules/rules/includes/faces.inc).

I'll spend a bit of time seeing if I can figure out what the problem is... It's likely that the patches are old and something has changed in rules that would make it necessary to update.

deggertsen’s picture

Alright, I think I figured out that the patch in #16 was messed up because it did not include a new comment.eval.inc file that I think was intended to be included. There is only one function that I can guess would have been in that file so I felt there is little need to add an extra file just for the one condition function. Instead, I simply added it to the same comment.rules.inc file. If somebody thinks this is bad for some reason then go ahead and move the rules_condition_comment_is_published() function to a new comment.eval.inc file. As far as I can tell it gives the same result (as long as you include the comment.eval.inc file if you go that route). There was also a bunch of code included in the patch for tests, which I do not think is necessary for this issue at least (correct me if I'm wrong).

Anyways, here is the patch for the condition "Comment is published" and the actions "Publish comment" and "Unpublish comment"

I am also using the patch from #5 to get the action "After approving a new comment", because I thought the issue discussed in #7 might be fixed by now as I was unable to locate the issue. I'll report back if I run into any problems, but at this point everything appears to be working as expected.

PatchRanger’s picture

Status: Needs work » Needs review

Alright, I think I figured out that the patch in #16 was messed up because it did not include a new comment.eval.inc file that I think was intended to be included.

You are right, it was missing, my fault.

Instead, I simply added it to the same comment.rules.inc file. If somebody thinks this is bad for some reason then go ahead and move the rules_condition_comment_is_published() function to a new comment.eval.inc file. As far as I can tell it gives the same result (as long as you include the comment.eval.inc file if you go that route).

I've put this code into a separate file by an example of other code: see Rules sources for reference.
Personally I wouldn't mind merging this - let's see how it passes tests and what will be said by maintainers.
Switching to 'needs review' to launch testing.

deggertsen’s picture

I'm getting this error when I try to use the "rules_condition_comment_is_published" condition. I've been trying to figure out the problem, but haven't had any luck thus far.

Warning: call_user_func_array() expects parameter 1 to be a valid callback, function 'rules_condition_comment_is_published' not found or invalid function name in FacesExtendable->__call() (line 123 of /home/teaching/public_html/sites/all/modules/rules/includes/faces.inc).

Any help?

mvlabat’s picture

Getting the same error as deggertsen.

mvlabat’s picture

Upd. The error disappeared when I replaced rules_condition_comment_is_published function to the separate file. I don't know why, but when a function is declared in the same file it causes errors.

To fix the problem do the following:
1. Create file called "comment.eval.inc" and copy this code to it

<?php

/**
 * @file
 * Contains rules integration for the comment module needed during evaluation.
 *
 * @addtogroup rules
 * @{
 */

/**
 * Condition: Check if the comment is published
 */
function rules_condition_comment_is_published($comment) {
  return $comment->status == 1;
}

/**
 * @}
 */

2. Open "comment.rules.inc" file and delete this code

/**
 * Condition: Check if the comment is published
 */
function rules_condition_comment_is_published($node) {
  return $node->status == 1;
}

and in the beginning, just after line 8, add this

/**
 * Implements hook_rules_file_info() on behalf of the comment module.
 */
function rules_comment_file_info() {
  return array('modules/comment.eval');
}

If someone could merge this fix and the patch in #23, it'll be great.

deggertsen’s picture

Here's the rerolled patch based on the suggested changes in #27. Thanks @mvlabat!

Please review and mark as RTBC once tested.

deggertsen’s picture

Status: Needs review » Needs work

Patch #5 needs to be rolled in with patch #28. I'll try to do this at some point, but if somebody else could take care of it that would be great.

deggertsen’s picture

Status: Needs work » Needs review
FileSize
3.26 KB

Finally got around to combining the two patches. I've been using this on a couple sites for awhile now with no issues. Please review and move to RTBC once tested. I think it's ready to commit.

lukus’s picture

Hi - I'm interested in this functionality .. is this likely to be committed soon?

deggertsen’s picture

@lukus, have you tested it? If not, please apply the patch and test it. If it's working for you then mark this issue as RTBC, at that point it might be committed. I'm a bit disappointed that it didn't make it into the most recent stable release myself.

gynekolog’s picture

Status: Reviewed & tested by the community » Needs review

#30 is working fine for me, thank you.

deggertsen’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for testing @gynekolog. I'm using this on 2 production sites now. Marking as RTBC.

fago’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
+function rules_comment_publish($comment) {
		
+ rules_invoke_event('comment_approve', $comment);
		
+}

Problem is, the hook is quite broken, isn't it? It fires every time the comment is saved and published. Also publishing and approval isn't necessarily the same either. So maybe just leave out the event for now.

The actions look good to me, however we'll need to add test coverage for them. We've a Rules core integration test case, which should receive respective test coverage for comment module.

deggertsen’s picture

Thanks for looking at this @fago. Here's a patch without the event. Is there a way to trigger an event on comment approval without the issue you pointed out? Maybe we can create a separate issue for that.

Is there documentation on how to add test coverage? I'm not familiar with that.

donSchoe’s picture

Issue summary: View changes

Hi @deggertsen - The patch in #30 was working quite well for me. Now I just updated to #36 but it's breaking stuff. I did a quick diff on the both patches and noticed in addition to the code in #35, this is missing:

@@ -51,7 +58,58 @@ function rules_comment_event_info() {
         'comment' => array('type' => 'comment', 'label' => t('deleted comment')),
       ),
     ),
+    'comment_approve' => $defaults + array(
+      'label' => t('After approving a new comment'),
+      'variables' => array(
+        'comment' => array('type' => 'comment', 'label' => t('approve comment')),
+      ),
+    ),
+  );
+}
+
/**
 * Implements hook_rules_condition_info() on behalf of the comment module.

Was this intentional?

Edit: OK just figured this is related to #35. I'm not satisfied, #36 is breaking my custom rules which used to work with #30 :-)

GuillaumeDuveau’s picture

Interested too

darragh97’s picture

I'm very interested too

alfazaz’s picture

I'm very interested too...

TR’s picture

@guix, @darragh97, @alfazaz:
Saying you're 'interested' in seeing this feature doesn't help get the issue resolved. If you really are interested, then:
1) Try the patch, see if it provides the feature you need, see if you encounter any bugs, report back in this issue whether the patch worked for you or not with details of the errors you got (if any).
2) Fix the patch. The testbot has identified 25 coding standards issues that were introduced in the patch. The patch needs to be rewritten to correct those.
2) Work towards addressing the missing pieces - this issue is "Needs work" because it needs tests written. So write them, or hire someone to write them.

TR’s picture

Category: Task » Feature request