Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I have implemented comment_approve event for Rules. Patch is in attachment.
Please, review and give your feedback.
Thanks,
Comments
Comment #2
polosatique CreditAttribution: polosatique commentedI have fixed patch format.
Comment #3
polosatique CreditAttribution: polosatique commentedComment #5
polosatique CreditAttribution: polosatique commentedI have fixed patch format.
Comment #6
maartendeblock CreditAttribution: maartendeblock commentedI can confirm this patch works on 7.x-2.0. I would love to see this getting picked up.
Comment #7
fagoUnfortunately 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.
Comment #8
ruess CreditAttribution: ruess commentedThis would be great!
Comment #9
mitchell CreditAttribution: mitchell commentedThis 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.
Comment #10
mitchell CreditAttribution: mitchell commentedWhat about entity is changed, type is comment, status of submitted comment is 0, and status of changed comment is 1?
Comment #11
mvlabat CreditAttribution: mvlabat commentedComment #12
fagoWe 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.
Let's do that, as this is the way it currently works for all other entity types (e.g. nodes) as well.
Comment #13
PatchRanger CreditAttribution: PatchRanger commentedDone.
The first file contains only new test.
The second one has test and functionality both.
Please review.
Comment #14
PatchRanger CreditAttribution: PatchRanger commentedTest bot stuck.
Let's re-trigger testing.
Comment #15
PatchRanger CreditAttribution: PatchRanger commentedLet's see if it helps.
Comment #16
PatchRanger CreditAttribution: PatchRanger commentedNope, it didn't. Ok, let's do it this way: re-attach patches.
Comment #17
PatchRanger CreditAttribution: PatchRanger commentedStrange. Somebody, please help me to tame the bot!:)
What's wrong with my patches?
Comment #18
PatchRanger CreditAttribution: PatchRanger commentedOk, 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.
Comment #20
savingstrangers CreditAttribution: savingstrangers commentedDid 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!
Comment #21
savingstrangers CreditAttribution: savingstrangers commentedFor 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 :)
Comment #22
deggertsen CreditAttribution: deggertsen commentedRather 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:
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.
Comment #23
deggertsen CreditAttribution: deggertsen commentedAlright, 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.
Comment #24
PatchRanger CreditAttribution: PatchRanger commentedYou are right, it was missing, my fault.
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.
Comment #25
deggertsen CreditAttribution: deggertsen commentedI'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?
Comment #26
mvlabat CreditAttribution: mvlabat commentedGetting the same error as deggertsen.Comment #27
mvlabat CreditAttribution: mvlabat commentedUpd. 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
2. Open "comment.rules.inc" file and delete this code
and in the beginning, just after line 8, add this
If someone could merge this fix and the patch in #23, it'll be great.
Comment #28
deggertsen CreditAttribution: deggertsen commentedHere's the rerolled patch based on the suggested changes in #27. Thanks @mvlabat!
Please review and mark as RTBC once tested.
Comment #29
deggertsen CreditAttribution: deggertsen commentedPatch #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.
Comment #30
deggertsen CreditAttribution: deggertsen commentedFinally 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.
Comment #31
lukusHi - I'm interested in this functionality .. is this likely to be committed soon?
Comment #32
deggertsen CreditAttribution: deggertsen commented@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.
Comment #33
gynekolog CreditAttribution: gynekolog commented#30 is working fine for me, thank you.
Comment #34
deggertsen CreditAttribution: deggertsen commentedThanks for testing @gynekolog. I'm using this on 2 production sites now. Marking as RTBC.
Comment #35
fagoProblem 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.
Comment #36
deggertsen CreditAttribution: deggertsen commentedThanks 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.
Comment #37
donSchoe CreditAttribution: donSchoe commentedHi @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:
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 :-)
Comment #38
GuillaumeDuveauInterested too
Comment #39
darragh97 CreditAttribution: darragh97 as a volunteer commentedI'm very interested too
Comment #40
alfazaz CreditAttribution: alfazaz commentedI'm very interested too...
Comment #41
TR CreditAttribution: TR commented@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.
Comment #42
TR CreditAttribution: TR commented