The email received by subscribers contains a link to unsubscribe which doesn't work; it leads to a "sorry, there was a problem." error. According to my log, I have these problems:

Notice: Undefined offset: 0 in comment_notify_unsubscribe_by_hash() (line 268 of public_html/drupal/sites/all/modules/comment_notify/comment_notify.inc)

Notice: Trying to get property of non-object in comment_notify_unsubscribe_by_hash() (line 268 of public_html/drupal/sites/all/modules/comment_notify/comment_notify.inc).

These are the errors immediately after installing the latest version of comment notify.

CommentFileSizeAuthor
#4 comment_notify-unsubscribe_by_hash_error-2328055-4.patch1.08 KBivanstegic
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

evelien’s picture

evelien CreditAttribution: evelien commented
Priority: Normal » Major

problem still exists. I think this isn't priority normal, but major. People can't unsubscribe and will therefore receive unwanted e-mails.

evelien’s picture

evelien CreditAttribution: evelien commented
Priority: Major » Normal

found something more, changing to normal again. Error occurs when the original comment is removed. It seems that while removing nodes and/ or comments, the DB for comment notification isn't updated and therefore you'll get an error. It isn't pretty, but isn't major either.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Title: Unsubscribe by hash error » Unsubscribe by hash error when a comment is deleted
Issue tags: -unsubscribe, -PHP, -bound, -error

Thanks for updating with the causes of this issue. Updating the title and removing some issue tags that don't seem relevant to me.

ivanstegic’s picture

ivanstegic CreditAttribution: ivanstegic at TEN7 commented
Status: Active » Needs review
FileSize
comment_notify-unsubscribe_by_hash_error-2328055-4.patch1.08 KB

I can confirm that I've seen this behavior on a live production site. In our experience, it isn't that the unsubscribe link didn't work, it was that it was already used once before, and the user had already been unsubscribed. This normally works just fine, but there is this edge case where a deleted anonymous comment makes a difference. The attached patch fixes this by (1) checking for an array element, which prevents the nasty error message, and (2) tweaking the unsubscribe messaging to go from

Sorry, there was a problem unsubscribing from notifications.

to

Sorry, there was a problem unsubscribing from notifications. You may already be unsubscribed.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at CARD.com commented

Those seems like improvements. Thanks!

I wonder if that can be improved further so that the message just says "You are already unsubscribed." in the cases where we believe that to be the case?

ivanstegic’s picture

ivanstegic CreditAttribution: ivanstegic at TEN7 commented

@greggles I don't think that's the case since the error message is generated in a place where, potentially, something else could have gone wrong so just including "You are already unsubscribed." may be misleading. I might be wrong about how I'm reading that function though, so I'm open to listen :)

lexfunk’s picture

lexfunk CreditAttribution: lexfunk at TEN7 commented

I reviewed the patch and it works for the problems described.

Thanks