Split comment.timestamp into 'created' and 'updated' columns

We don't use /** documentation block comments within blocks of code, those need to be //.

We don't put personal credits in either, that is what the CVS log is for.

Changes which affect the site's content will not go in Drupal 5.x, which is the stable branch.

We need to do both at the same time: don't update (creation) timestamp but at the same time add a modified timestamp.
I'm very much for this move, but it can't be done in D6 anymore.

We now order by cid instead of timestamp for flat comments, however this still has the effect that edited comments show up as 'new' - drupal.org issue queue is one example. Hoping this would be a code slush patch.

First stab at this. Given in offering to the test bot god.

This one should already be better.

This one should pass. Careful review of the update code is required.

This will remove a tonne of wtfs when it's in.

The update had to be updated for $ret and update_sql() removal and I also removed a whitespace-only hunk from field.attach.inc

Tagging.

Sun gave me permission to add the super-critical this is a complete mess tag in irc.

+++ modules/comment/comment.admin.inc	9 Oct 2009 08:22:21 -0000
@@ -62,7 +62,7 @@ function comment_admin_overview($form, &
-    'time' => array('data' => t('Time'), 'field' => 'timestamp', 'sort' => 'desc'),
+    'time' => array('data' => t('Time'), 'field' => 'updated', 'sort' => 'desc'),

Shouldn't we call the column header "Updated" then? (to also make it more consistent with the Content overview table)

+++ modules/comment/comment.module	9 Oct 2009 08:22:22 -0000
@@ -1325,8 +1326,12 @@ function comment_save($comment) {
+    if (empty($comment->updated)) {
+      $comment->updated = $coment->created;
     }

Typo in "$coment" here.

I'm on crack. Are you, too?

Fixed.

Awesome, this looks ready to fly now.

+++ modules/comment/comment.install	9 Oct 2009 16:21:58 -0000
@@ -130,6 +130,37 @@ function comment_update_7006() {
+  return $ret;

We no longer need this return statement.

In the node module, we use 'changed' instead of 'updated'. I'm happy with either, but it feels like we want to be consistent to avoid a minor WTF.

Hm. Node module uses 'changed' in the db, but 'updated' in the UI, however let's make comment module consistent with node module for now, then try to make them consistent internally at a later time. Also removed the $ret.

All points covered.

Thanks for the reroll.

Committed to CVS HEAD!

This will require us to module update the upgrade instructions in the handbook. Marking this 'needs work'.

.

@@ -1539,9 +1545,9 @@ function comment_num_new($nid, $timestam
     $timestamp = ($timestamp > NODE_NEW_LIMIT ? $timestamp : NODE_NEW_LIMIT);
 
     // Use the timestamp to retrieve the number of new comments.
-    return db_query('SELECT COUNT(cid) FROM {comment} WHERE nid = :nid  AND timestamp > :timestamp AND status = :status', array(
+    return db_query('SELECT COUNT(cid) FROM {comment} WHERE nid = :nid  AND changed > :changed AND status = :status', array(
       ':nid' => $nid,
-      ':timestamp' => $timestamp,
+      ':changed' => $timestamp,
       ':status' => COMMENT_PUBLISHED,
       ))->fetchField();
   }

Shouldn't this continue to use :timestamp for the placeholder in the query, since that's what the changed date is being checked against?

This review is powered by Dreditor.

Here's the change in nomenclature I'm suggesting.
At the risk of straying off-topic, $timestamp is not a very informative name for the variable here. Something like $last_viewed might be more appropriate.

As Dries suggested, I've added documentation of this issue's essential change to the handbook page for Converting 6.x modules to 7.x: Comment.timestamp split into 'created' and 'changed'.

Hmm, that should be causing test failures somewhere, surprised we have zero coverage for comment_num_new().

Catch, the patch in #22 doesn't fix a bug. It only renames a placeholder variable. Or are you referring to something else?

Reviewing patches before caffeine again :(

Committed. Thanks.

Interesting that I read someone saying that D6 won't be fixed. Know that there is an enormous bug in D6 that makes comments lose their order, which was the problem spoken about in the original post here.

I have one page with ONLY 44 comments and the threads ran out of numbers!!! Yes! I get an overflow. It's all wrong.

There is the database content sorted by thread:

 cid |        thread        | timestamp  
-----+----------------------+------------
  86 | 00/                  | 1274892356
  99 | 00/                  | 1276884496
  88 | 00/                  | 1275222201
  89 | 00.00/               | 1275236161
  94 | 00.01/               | 1275853621
  95 | 00.01.00/            | 1275860733
  96 | 00.01.01/            | 1275887844
  97 | 00.01.01.00/         | 1275891281
   8 | 01/                  | 1259015162
   9 | 01.00/               | 1259576100
  13 | 01.00.01/            | 1259774358
  10 | 2101/                | 1259757558
  12 | 2101.00/             | 1259774249
  17 | 410101/              | 1261870329
  37 | 410102/              | 1265397905
  38 | 410102.00/           | 1265400755
  83 | 5k4vugw/             | 1274239305
  81 | 5k4vugw/             | 1273262152
  76 | 5k4vugw/             | 1272514125
  64 | 5k4vugw/             | 1270733125
  77 | 5k4vugw/             | 1272737246
  75 | 5k4vugw/             | 1272443197
  70 | 5k4vugw/             | 1272083801
  80 | 5k4vugw/             | 1273257675
  72 | 5k4vugw/             | 1272382397
  65 | 5k4vugw.00/          | 1270739128
  74 | 5k4vugw.01/          | 1272385629
  78 | 5k4vugw.02/          | 1272749979
  82 | 5k4vugw.03/          | 1274074506
  84 | 5k4vugw.04/          | 1274239921
  52 | 5oqzy7z/             | 1268719224
  56 | 5oqzy7z/             | 1269206336
  53 | 5oqzy7z.00/          | 1268720880
  58 | 5oqzy7z.00.00/       | 1270168515
  69 | 5oqzy7z.00.00.00/    | 1271497985
  59 | 5oqzy7z.00.01/       | 1270169096
  79 | 5oqzy7z.01/          | 1273048619
  61 | 5u0osu9/             | 1270550619
  62 | 5u0osu9.00/          | 1270573430
  50 | 5z03zz4/             | 1267559589
  51 | 5z03zz4.00/          | 1267870672
  60 | 5z03zz4.00.00/       | 1270238815
  63 | 5z03zz4.00.00.00/    | 1270573657
  85 | 5z03zz4.00.00.00.00/ | 1274295286
(44 rows)

As you may notice, the threads are in order up to cid #85. Then #86, 99 and 88 are at the top with 00/. Any idea why?! I will tell you why. The number '5z03zz4.00.00.00.00' represents: 5970886882825090468202.

And that number + 1, to go to the next thread level is: 5.97088688283E+21.

And when you try to convert that back to a thread number it becomes 00/.

You can try that yourself:

php > echo base_convert(substr('5z03zz4.00.00.00.00', 1), 36, 10);
5970886882825090468202
php > echo base_convert(substr('5z03zz4.00.00.00.00', 1), 36, 10) + 1;
5.97088688283E+21

The culprit is this code:

      elseif ($comment->pid == 0) {
        // This is a comment with no parent comment (depth 0): we start
        // by retrieving the maximum thread level.
        $max = db_query('SELECT MAX(thread) FROM {comment} WHERE nid = :nid', array(':nid' => $comment->nid))->fetchField();
        // Strip the "/" from the end of the thread.
        $max = rtrim($max, '/');
        // Finally, build the thread field for this new comment.
        $thread = int2vancode(vancode2int($max) + 1) . '/';
      }

The $max variable is set to the entire thread value. It should be limited to the first part. There are many ways to do so, there is an example:

$max = rtrim($max, '/');
$p = strpos($max, '.');
if ($p !== FALSE) {
  $max = substr($max, 0, $p);
}

Now, you may skip fixing this in D6, but I can tell you that threaded comments just won't work for any D6 user that receives around 50 comments! On my end, I have to fix my database. See you later.

Thank you.
Alexis Wilke

P.S. I have version 5.2 of PHP. Maybe base_convert() in 5.3 does not accept the period? I'd be surprised though.