Starting with 6.x the presave op/hook was added to node. This patch adds a presave hook also to comments so that modules can change comments before they are inserted/updated.

Comments

berdir’s picture

Status: Needs review » Needs work
+function hook_comment_presave(&$comment) {

The & is not necessary, in fact, it results in a warning in PHP 5.3

claudiu.cristea’s picture

Status: Needs work » Needs review
StatusFileSize
new1 KB

Yes... You're right...

Updated and set back to "needs review"

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Makes perfect sense. Consistency++. And useful too.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
+++ modules/comment/comment.module	2009-08-11 08:40:03.000000000 -0400
@@ -1235,6 +1235,8 @@ function comment_save($comment) {
+  // Allow modules to alter the comment before saving.
+  module_invoke_all('comment_presave', $comment);
   field_attach_presave('comment', $comment);
 

Actually, I think it makes more sense to run this op after the fields have been attached. This is consistent with where our alter hooks are elsewhere.

+++ modules/comment/comment.api.php	2009-08-11 08:40:32.000000000 -0400
@@ -12,6 +12,18 @@
+  // Change here the comment object before is saved...

Not quite proper grammar. Probably just:

// Change the comment object before it is saved.

However, it'd be nice to have an actual use case here. What's your motivation behind rolling this patch? What specifically are you trying to do in op presave? Can you make it a 2-3 line example here?

This review is powered by Dreditor.

yched’s picture

"run this op after the fields have been attached"
+1. Node module runs hook_node_* after field_attach_*.
Also, those two lines shouldn't be grouped under the same "// Allow modules to alter the comment before saving." comment.

claudiu.cristea’s picture

Status: Needs work » Needs review
StatusFileSize
new992 bytes

After submitting the patch, I was thinking too to place the hook call after fields were attached... It makes perfectly sense to have the comment object "ready" before presaving ops. Updated the patch (also the grammar issue).

@webchick

However, it'd be nice to have an actual use case here.

I want to perform some operations on data before data is stored in the DB. An example is the module Safe HTML. For node I used $op == 'submit' (5.x) and $op == 'presave' (6.x). For comments I had to run 2 additional queries (on hook_comment(), $op == 'insert' & $op = 'update') after the comment body was stored in the DB because I cannot intercept the comment before INSERT or UPDATE. And, yes... there is also a matter of consistency between Drupal objects.

Status: Needs review » Needs work

The last submitted patch failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
StatusFileSize
new1014 bytes

Retrying...

eojthebrave’s picture

Status: Needs review » Needs work

The example hook_comment_presave() should have some example code in it that shows how one might alter the $comment object and then return it. See hook_node_presave() as an example.

Otherwise, +1, and the rest of the patch looks good.

claudiu.cristea’s picture

Status: Needs work » Needs review
Issue tags: +comment, +Drupal core hooks
StatusFileSize
new1.04 KB

OK. Added also a simple example...

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Thanks. RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs documentation

Committed to HEAD. Thanks, Claudiu! :)

Marking "needs work" because this needs to be documented in the 6 to 7 upgrade guide. Once this is done, please mark back to "fixed."

eojthebrave’s picture

I think the example hook_comment_presave() code is still a little off. This line,

+function hook_comment_presave($comment) {

Should show that the $comment variable needs to be passed by reference, &$comment, if you don't pass by reference any action taken on the $comment object inside your presave hook will be lost.

webchick’s picture

eojthebrave: Actually, no. Objects no longer need the & to be passed by reference as of PHP 5 and this will in fact create a warning in PHP 5.3.

eojthebrave’s picture

@webchick, good to know. Thanks.

claudiu.cristea’s picture

Status: Needs work » Fixed

I've updated the documentation but I don't understand what's the deal with those categories UNSTABLE-x, so I posted my piece in the top of the list at http://drupal.org/update/modules/6/7#comment_presave_hook

berdir’s picture

You should do the opposite and add it to the end :)

claudiu.cristea’s picture

Well... it was not obvious :)

Moved to the bottom.

claudiu.cristea’s picture

Well... it was not obvious :)

Moved to the bottom.

Status: Fixed » Closed (fixed)
Issue tags: -Needs documentation, -comment, -Drupal core hooks

Automatically closed -- issue fixed for 2 weeks with no activity.