Closed (fixed)
Project:
Comment Notify
Version:
5.x-2.x-dev
Component:
Code
Priority:
Critical
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
15 Nov 2008 at 15:09 UTC
Updated:
11 Mar 2010 at 00:03 UTC
Jump to comment: Most recent file
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 commentedAh, sorry about that. I missed that the patch was against core.
Comment #3
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 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 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.