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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

zyxware’s picture

Status: Active » Needs review
FileSize
10.84 KB

As 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.

greggles’s picture

Status: Needs review » Needs work

$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.

aclight’s picture

I 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.

zyxware’s picture

I have uploaded the latest patch as per your suggestions and drupal coding standards.

anoopjohn’s picture

Status: Needs work » Needs review

I am updating the status of the new patch to [code needs review]

aclight’s picture

Status: Needs review » Needs work

Some comments:

1.

+  db_add_field($ret, 'comment_notify', 'parents', array('type' => 'varchar', 'length' => 255));

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().

$number_rows_result = db_fetch_object(db_query("SELECT COUNT(*) AS num FROM {comments} c"));

I think the same applies here:

+      $result = db_fetch_array(db_query($sql));

and here

+      $parents_of_parent = db_fetch_object(db_query("SELECT parents FROM  {comment_notify} WHERE cid = %d", $parent_result->pid));

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.

+    $if_condition = "IF (c". $level .".pid <> 0, CONCAT(c". $level .".pid, ','),'')";
+    $table = "c". $level;

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:

+    // In the while loop it was joined because we wanted the series 
+    // of joins to finally return null record set but when we update 
+    // we need to generated the complete set of records for the full depth.

and this:

+      // Get pid from comments table  for current cid, get the parents for this cid.

6. Looks like you need an additional space of indentation:

+     $parent = $parent_result->pid .',';

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.

greggles’s picture

Title: Merging Comment Subscribe to Comment Notify » Merging Comment Subscribe to Comment Notify by providing a "subscribe to _all_ replies" style feature

Thanks 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:

Can you provide a use case? You might have talked to Greggles about this already but if so it's not clear to me.

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.

aclight’s picture

Can'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.

greggles’s picture

Wow. 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.

anoopjohn’s picture

I 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.

aclight’s picture

I 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.

anoopjohn’s picture

I 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.

greggles’s picture

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.

Yes, I think this provides the clearest path forward. Thanks to both of you for your help.

zyxware’s picture

Hi 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

greggles’s picture

Something about this patch seems pretty weird. Can you try posting another one?

zyxware’s picture

Could you please clarify your concern.

greggles’s picture

Sure, the patch doesn't apply:

gjkmbp:comment_notify greg$ cvs up -A
cvs update: Updating .
cvs update: Updating translations
gjkmbp:comment_notify greg$ wget http://drupal.org/files/issues/comment_notify_patch_1_0.patch
--2009-03-25 06:22:45--  http://drupal.org/files/issues/comment_notify_patch_1_0.patch
Resolving drupal.org... 140.211.166.6
Connecting to drupal.org|140.211.166.6|:80... connected.
HTTP request sent, awaiting response... 200 OK
Length: 7586 (7.4K) [text/plain]
Saving to: `comment_notify_patch_1_0.patch'

100%[===============================================================================================================================================>] 7,586       --.-K/s   in 0.1s    

2009-03-25 06:22:45 (77.5 KB/s) - `comment_notify_patch_1_0.patch' saved [7586/7586]

gjkmbp:comment_notify greg$ patch -p0 < comment_notify_patch_1_0.patch 
patching file comment_notify.module
Hunk #1 FAILED at 373.
1 out of 1 hunk FAILED -- saving rejects to file comment_notify.module.rej
gjkmbp:comment_notify greg$ 

On visual inspection, it looks weird to me. Using the patch utility...doesn't apply.

zyxware’s picture

Status: Needs work » Needs review
FileSize
7.7 KB

Revised patch is attached.

greggles’s picture

Can 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?

zyxware’s picture

Assigning 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.

greggles’s picture

Did 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.

zyxware’s picture

Yes 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.

greggles’s picture

Status: Needs review » Fixed

Great 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.

greggles’s picture

Version: 7.x-1.x-dev » 5.x-2.x-dev
Status: Fixed » Patch (to be ported)

better status in case someone wants to fix 5.x

greggles’s picture

Status: Patch (to be ported) » Closed (fixed)

This 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.