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.It looks like this file accidentally got added to the repository in the 5.x-2.x branch, and needs to be deleted.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | comment_notify_no_patch.patch | 4.16 KB | recidive |
Comments
Comment #1
gregglesThanks for the report.
I believe this is a required part of installation for the 5.x branch. For 6.x the patch was included in core and is no longer needed.
Comment #2
aclight CreditAttribution: aclight commentedAh, sorry about that. I missed that the patch was against core.
Comment #3
recidive CreditAttribution: recidive commented@greggles: can you adivise why is it necessary to patch comment.module? Since other comment notification modules doesn't require that.
Comment #4
gregglesOther comment notification modules send notifications about spam. This one, largely, doesn't.
This is part of the code that I inherited and never really investigated. I'd be happy to commit a patch to comment_notify if it removes this dependency, but don't plan to fix that myself.
Comment #5
recidive CreditAttribution: recidive commentedOk, here is a patch to make it work without patching core's comment.module.
Looking at patched comment_save() function, the new logic seems the same as with the patch.
I did some basic testing, and it's working fine.
Comment #6
gregglesYes, that makes sense. I believe that the ability to see if comments are published was fixed up with the 5.3 SA: http://drupal.org/node/184354
It would be nice to get more testers/reviews prior to committing it. I'll add it to all my sites and test it out now.
Comment #7
aclight CreditAttribution: aclight commentedI tested this on a site running the latest comment_notify 5.x-2.x code and this worked as expected. The code also looks good to me. Not requiring users to patch core is a good idea.
One potential problem, however, is that if a user gets the new code, assuming they had previously patched the core comment.module, they'll now have 2 emails sent out for each comment, once when hook_comment($op=publish) is called and again when hook_comment($op=insert or $op=update) is called.
I think that if #186025: Distinguish between new comments and comment edits got in first that this wouldn't be a problem.
Another related problem--this patch removes the comment_notify_patch_to_comment.patch file, so it's going to be harder for administrators to reverse their previous patch to comment.module without the patch being present in the download of comment_notify anymore.
Bumping priority to critical because requiring patches to core is bad.
Comment #8
gregglesThis is now committed to 6.x - http://drupal.org/cvs?commit=178442
I think it makes sense to remove the patch. If people need a new version of the comment.module file then they should just download core...they have to do that often enough with security updates anyway.
Comment #9
gregglesThis module is no longer actively developed for Drupal 5. If someone wishes to take over as the 5.x maintainer I would consider it, but these days everyone should really upgrade to Drupal 6.x.