comment_notify is sending two copies of every notification

#4 comment_notify-1278556.patch1.36 KBmoonray
Members fund testing for the Drupal project. Drupal Association Learn more


Joenet’s picture

Same Problem. It has been reported in #24 by the patch #14 here

moonray’s picture

Same problem.

Tracked it down to the following 2 hooks sending emails: comment_notify_comment_insert() and comment_notify_comment_publish()

One of the 2 hooks should be enough to get the email sent out.

moonray’s picture

Did some more research...

Looks like the problem is the following: When _comment_notify_mailalert() does its thing it marks the comment as notified using comment_notify_mark_comment_as_notified(). However, between comment_notify_comment_insert() and comment_notify_comment_publish() the comment object is not reloaded from db, thus not triggering comment_notify_comment_load() which populated the $comment->notified flag.

We need to update the comment object itself, in addition to writing the flag to db.

moonray’s picture

Status: Active » Needs review
1.36 KB

And here's a patch that fixes this.

greggles’s picture

Thanks for the research, moonray.

@joelko, @dusov can you test this?

greggles’s picture

I should have said that this looks sane to me :)

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Subscribing. Same problem. I thought I was doing something wrong :)

Tested the patch in #4. Works perfectly. Doesn't apply cleanly though, due to bad paths in the patch. That's easy enough for @greggles to deal with though: just use patch -p6 < patchfile.

Vikom’s picture

Subscribing. Have the same problem, patch in #4 seems to have solved it.

greggles’s picture

Title: Two copies of notification » Two copies of notification, mark as notified only updates DB, not passed object
Status: Reviewed & tested by the community » Fixed

I added some comments to explain why the change is necessary.

Now committed -

Thanks moonray and Wim Leers for the work and review!

Wim Leers’s picture

Hurray! :)

P.S.: is that the new d.o standard notation for attributing commits? A pipe symbol instead of just a comma and/or "and"? Just asking out of curiosity :)

greggles’s picture

The | is to separate the reviewer from the author. There's discussion on more specific formats for that, but this works now so I use it :)

I also use the --author flag on the commit to give credit to the author.

Wim Leers’s picture

The latter I know (and use), the former I've never heard of. Where is this documented? :)

greggles’s picture

It's not super documented. Dries and Angie did it for core for a while and then stopped doing it, I think.

Status: Fixed » Closed (fixed)

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