Prior to the 5.x-2.0 release, Comment Notify used the comments table to store database information. Now, the comment_notify table is used. However, the information stored in the comments table by Comment Notify was never cleaned up. Specifically, the "notify" column should be removed.

This is particularly important when upgrading from 5.x-1.x to a DRUPAL-6 release, as the comments table schema will be incorrect.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greggles’s picture

Does it cause bugs during the upgrade to 6? If so, then yes, we should remove it.

I've only upgraded one site that uses comment_notify from 5.x to 6.x and didn't notice any problems, but...

greggles’s picture

EDIT: "If so, then yes, we should remove it soon. Otherwise we can leave it around a little longer until I get some more comfort that the migration to the new comment_notify table doesn't accidentally drop subscriptions."

Junyor’s picture

I don't know if it causes bugs, but wouldn't it make Drupal 6 report the incorrect schema on the Status page? Or maybe I'm just making that up and Schema module is required for that.

greggles’s picture

Status: Active » Needs review
FileSize
6.19 KB
838 bytes

Here's a patch for 5.x and 6.x

greggles’s picture

I should add that nobody has mentioned issues related to the upgrade so I feel like it's relatively safe to do this.

Junyor’s picture

@greggles: It looks like some unrelated stuff jumped into the 6x patch.

Junyor’s picture

Status: Needs review » Needs work
greggles’s picture

Status: Needs work » Needs review

Indeedy do - thanks for noticing that!

greggles’s picture

Junyor’s picture

Status: Needs review » Reviewed & tested by the community

The Drupal 5.x patch works as advertised.

greggles’s picture

Thanks for the review. I like this - I just want to wait on this a little more to see if anyone reports problems with the 5.x-1.x -to- 5.x-2.x upgrade process.

Dave Reid’s picture

Should actually be using this for D6:

$ret = array();
if (db_column_exists('comment', 'notify')) {
  db_drop_field($ret, 'comment', 'notify');
}
return $ret;
aclight’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.06 KB
1.92 KB

As Dave Reid suggests, we need to check to make sure that the {comments}.notify column exists before we try to drop it, or otherwise we'll get DB error messages. The attached two patches check for the column first and only try to drop it if it exists.

I've tested the attached patch on Drupal 5 with mysql. I haven't tested the pgsql case, nor have I tested this on D6.

aclight’s picture

The name of the update function needs to be comment_notify_update_5200, not comment_notify_update_5001.

Freso’s picture

Version: 5.x-2.1 » 5.x-2.x-dev

The patch looks good, with a sound logic. Not tested though.

greggles’s picture

Status: Needs review » Fixed

heh. There is no comment table - only comments, so the 6x patch was broken. Easy fix, obviously.

Committed to 6x http://drupal.org/cvs?commit=169154 and 5x http://drupal.org/cvs?commit=169156

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.