Currently the main difference between the two modules is the ability of comment subscribe to track and send comments to any nth level parent of a comment. As part of merging the modules (#224782) I have created this patch that builds this functionality into the comment notify module. I will be creating a patch that will enable comment subscribe users to migrate to comment notify, after this patch is tested and verified.
Modifications inside comment_notify.install file.
Added one more field 'parents' to the comment_notify table
Populated this field for all existing comments.
Modifications inside comment_notify.module file
Queries have been modified to handle the newly added parents fields.
Fixed issue #360960
Regards
Zyxware
Comment | File | Size | Author |
---|---|---|---|
#20 | comment_notify_threaded.patch | 2.44 KB | zyxware |
#19 | 361449_provide_full_threaded_notifications.patch | 1.84 KB | greggles |
#18 | comment_notify_threaded.patch | 7.7 KB | zyxware |
#14 | comment_notify_patch_1.patch | 7.41 KB | zyxware |
#4 | comment_notify_comment_subscribe_merge_3.patch | 11.29 KB | zyxware |
Comments
Comment #1
zyxware CreditAttribution: zyxware commentedAs per discussions over IRC I am posting the modified patch with corrections on comments and code formating. Please ignore the previous patch and use the attached one.
Comment #2
greggles$num_rows_result-> num
Should that have a space in it?
The initial query concerns me for really large sites, but we can try it and see what happens.
$ifcondition should be $if_condition
$notifyval is initialized but never used?
I don't think we can use
IF(
in the query. The current comment_notify code just pulls back c.mail and u.mail and then tests for which one to use in PHP. That's much better for database independence.The zc alias in the query would be more intuitive as cn.
I think this is getting close to ready to commit. I'll try to get aclight to review as well. Thanks for your work on this.
Comment #3
aclight CreditAttribution: aclight commentedI will try to take a look at this this upcoming weekend if I have time. However if you don't hear from me by then, it's unlikely I'll have time to review this before, say, August. I am preparing for dissertation hyperdrive very soon and so probably won't have a lot of time for drupal related things.
Comment #4
zyxware CreditAttribution: zyxware commentedI have uploaded the latest patch as per your suggestions and drupal coding standards.
Comment #5
anoopjohn CreditAttribution: anoopjohn commentedI am updating the status of the new patch to [code needs review]
Comment #6
aclight CreditAttribution: aclight commentedSome comments:
1.
Why are we using a VARCHAR for the parents field? And shouldn't it be 'parent' not 'parents'?
2. It seems to me that db_result() would be more appropriate here than db_fetch_query().
I think the same applies here:
and here
3. This code looks really weird. Won't this end up with SQL that looks like
IF c1.pid...
? What table has columns named in such a way? If the code is right some better code comments explaining what is going on would be nice, because this is not so obvious at first glance.4. I would think that you'd want to put this update function so that it works as a batch. What if a site has millions of comments? Won't the update time out?
5. This needs a little work to make it make sense:
and this:
6. Looks like you need an additional space of indentation:
This is all I have time to look through right now.
To be honest I don't really see a point to having this functionality trying to be merged in. Can you provide a use case? You might have talked to Greggles about this already but if so it's not clear to me.
Comment #7
gregglesThanks for the awesome review, aclight. Most of the things you point out seem like they should be fixed, though some probably stem from us not giving a motivation for this patch (like the parents field should be a varchar). I can provide insight on this one part:
Yes. Basically comment_subscribe is a fork of comment_notify module but it has one change in functionality: it provides subscriptions for all replies to your comment on a threaded site. I implemented a similar feature in comment_notify but it only goes one level deep: a reply to a comment sends a mail, but a reply to a reply to a comment does not. The varchar parents field holds all of the parents of a comment. This makes it easy to find which notification mails to send.
Comment #8
aclight CreditAttribution: aclight commentedCan't we just use {comments}.thread to determine where in the thread a given comment is and get the parents based on that? It seems like adding {comment_notify}.parents would be duplicating data in an unnecessary way.
Comment #9
gregglesWow. I never knew such a column existed. On initial review it seems like that will work.
@anoopjohn - what do you think? This could reduce the size and complexity of the codebase we have to maintain quite a bit.
Comment #10
anoopjohn CreditAttribution: anoopjohn commentedI checked the column and it should take care of the issue I believe. But can't we do the merge first and then fix this issue afterwards. If we are to change the architecture now, we will have to implement it twice once here and then again in comment_subscribe before we can merge again. Also we have new support issues coming up in comment_subscribe and we can save on development time if we merge as early as possible.
Comment #11
aclight CreditAttribution: aclight commentedI don't think it makes sense to merge code into comment_notify that is overly complicated and unnecessary, unless it is extremely temporary. If using {comments}.thread means that the proposed {comment_notify}.parents column is no longer necessary, then I don't see why it would be necessary to merge twice anyway. It seems that all of the data necessary to determine the parentage of a comment is already in the database itself, so all that needs to change is the code that checks to see what the parents of a comment are and then sends emails as appropriate.
Comment #12
anoopjohn CreditAttribution: anoopjohn commentedI guess what you said makes sense aclight. We don't need to merge twice. All we need to import from comment subscribe is the subscription status information. I was thinking about storing the parent information collected by comment_subscribe which we won't need to because it is already present in the comments table.
If that is the case we will then work on modifying comment notify to use the {comments}.thread field. Once that is implemented we will post the patch here.
Comment #13
gregglesYes, I think this provides the clearest path forward. Thanks to both of you for your help.
Comment #14
zyxware CreditAttribution: zyxware commentedHi All,
Like greggles said we have worked on patch to use with thread field in {comments} table and the patch is attached with this post.
Thanks
zyxware
Comment #15
gregglesSomething about this patch seems pretty weird. Can you try posting another one?
Comment #16
zyxware CreditAttribution: zyxware commentedCould you please clarify your concern.
Comment #17
gregglesSure, the patch doesn't apply:
On visual inspection, it looks weird to me. Using the patch utility...doesn't apply.
Comment #18
zyxware CreditAttribution: zyxware commentedRevised patch is attached.
Comment #19
gregglesCan you clarify why you add
$parent_thread_ids[] = $nid;
?I'm also concerned that
if(!empty($format_string)) {
means that it will never send notifications for people who subscribed to "all comments" and then a comment is posted at the top level.I think that the way you find the list of people to notify based on thread in the query is interesting and may perform well, but it also appears somewhat buggy and feels overly complex to me (the loop that builds up all possible parent items is what feels complex to me). I've never heard of complaints about this query being particularly slow, so I'm not worried about the potential performance issues (and with a query as complex as this could get...it could cause performance problems the other way).
Here's a slightly less complex version. Can you review/test?
Comment #20
zyxware CreditAttribution: zyxware commentedAssigning nid to array is to store all variables which are passed to the query in an array.
An empty $format_string only means that there are no parents but yes that will miss commenters who have subscribed to all comments. We have corrected this also.
The way we have extracted the full path to the root parent of a comment is the only real way of using the thread other than querying in a loop which would query n times where n is the depth of the thread. Single join will only retrieve the direct parent unless queried in a loop like mentioned above.
Comment #21
gregglesDid you test my proposed patch? It seems to do the same job with less code.
What do you see as the benefits/drawbacks of each approach? I think they both are functional so it is a question of the code:
1. simplicity - ease of a new developer to understand
2. performance
etc.
Comment #22
zyxware CreditAttribution: zyxware commentedYes the patch was tested & reviewed and both are functionally the same.
Simplicity wise your approach is simpler than the split thread ids approach
Regarding performance I believe the approach of splitting threadids might be slightly faster as it requires fewer fetches from the database - i.e. only those comments that are parents of the comment or those that have notify-to-all-comments set would be fetched from the database.
Comment #23
gregglesGreat then, - Fixed for 6.x!. I committed the patch from #19 because I think performance is not an issue and the code is simpler.
Thanks, zyxware. Now to migrate the users from comment_subscribe.
Comment #24
gregglesbetter status in case someone wants to fix 5.x
Comment #25
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.