I'll dbtngify the comment module. When i'm ready, i'll post a patch, i'm just creating the issue so nobody is doing redundant work.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Sweet, thanks! I look forward to seeing it.

CorniI’s picture

Status: Active » Needs review
FileSize
19.55 KB

So the first draft is open to review...
I hope there's not too much wrong :D

CorniI’s picture

FileSize
19.55 KB

With some help form RobLoach less debug comments...

RobLoach’s picture

There are also some db_select statements that don't quite warrent the dynamic query builder, but they do work.......

+    $query = db_select('comments', 'c');
+	$query->addField('c', 'nid', 'nid');
+	$query->addField('c', 'subject', 'subject');
+	$query->addField('c', 'cid', 'cid');
+	$query->addField('c', 'timestamp', 'timestamp');
+	$query->innerJoin('node', 'n', 'n.nid = c.nid');
+	$query->condition('c.nid',$nids,'IN')
+	->condition('c.status',COMMENT_PUBLISHED,'=')
+	->condition('n.status',1,'=')
+	->orderBy('c.cid', 'DESC')
+    ->range(0, $number);
+	$result = $query->execute();

But of course, it looks good to me!

Damien Tournoud’s picture

Status: Needs review » Needs work

The conversion looks good, but still need some work on some points:

- there are some debugging stuff

+    //Omit the trailing / from thread.

+        //return db_query('SELECT last_comment_timestamp, last_comment_name, comment_count FROM {node_comment_statistics} WHERE nid = :nid', array(':nid' => $node->nid), array('fetch' => PDO::FETCH_ASSOC));

- there are several code style issues, especially regarding spacing:

+  $query->condition('nc.comment_count',0,'>')

should be:

+  $query->condition('nc.comment_count', 0, '>')

and

+        if(empty($edit['pid'])) {

should be:

+        if (empty($edit['pid'])) {

(full reference http://drupal.org/coding-standards)

- equality is the default operator for condition, so '=' can be removed in all those three examples:

->condition('c.status',COMMENT_PUBLISHED,'=')
->condition('nid', $node->nid, '=')
->condition('cid', $edit['cid'], '=')

- some queries don't have to be dynamic:

-      $query = 'SELECT c.cid, c.pid, c.nid, c.subject, c.comment, c.format, c.timestamp, c.name, c.mail, c.homepage, u.uid, u.name AS registered_name, u.signature, u.picture, u.data, c.status FROM {comments} c INNER JOIN {users} u ON c.uid = u.uid WHERE c.cid = %d';
-      $query_args = array($cid);
+      $query = db_select('comments', 'c');
+      $query->addField('c', 'cid', 'cid');
+      $query->addField('c', 'nid', 'nid');
+      $query->addField('c', 'pid', 'pid');
+      $query->addField('c', 'comment', 'comment');
+      $query->addField('c', 'subject', 'subject');
+      $query->addField('c', 'format', 'format');
+      $query->addField('c', 'timestamp', 'timestamp');
+      $query->addField('c', 'name', 'name');
+      $query->addField('c', 'mail', 'mail');
+      $query->addField('c', 'homepage', 'homepage');
+      $query->addField('c', 'status', 'status');
+      $query->addField('u', 'uid', 'uid');
+      $query->addField('u', 'name', 'registered_name');
+      $query->addField('u', 'signature', 'signature');
+      $query->addField('u', 'picture', 'picture');
+      $query->addField('u', 'data', 'data');
+      $query->addField('u', 'status', 'status');
+      $query->innerJoin('users', 'u', 'c.uid = u.uid');
+      $query->condition('c.cid', $cid, '=');

And I think we could omit the alias if equal to the name of the column.

Thanks for your great work on this!

RobLoach’s picture

Status: Needs work » Needs review
FileSize
18.42 KB

Addressed some of the issues brought up by DamZ at #5 and fixed some write space problems.

The last query you mentioned actually does need the dynamic builder because of the following if statement:

      $query = db_select('comments', 'c')
        ->addField('c', 'cid', 'cid')
        ->addField('c', 'nid', 'nid')
        ->addField('c', 'pid', 'pid')
        ->addField('c', 'comment', 'comment')
        ->addField('c', 'subject', 'subject')
        ->addField('c', 'format', 'format')
        ->addField('c', 'timestamp', 'timestamp')
        ->addField('c', 'name', 'name')
        ->addField('c', 'mail', 'mail')
        ->addField('c', 'homepage', 'homepage')
        ->addField('c', 'status', 'status')
        ->addField('u', 'uid', 'uid')
        ->addField('u', 'name', 'registered_name')
        ->addField('u', 'signature', 'signature')
        ->addField('u', 'picture', 'picture')
        ->addField('u', 'data', 'data')
        ->addField('u', 'status', 'status')
        ->innerJoin('users', 'u', 'c.uid = u.uid')
        ->condition('c.cid', $cid);

      if (!user_access('administer comments')) {
        $query->condition('c.status', COMMENT_PUBLISHED);
      }
CorniI’s picture

FileSize
20.88 KB

@RobLoach: sorry, your patch broke it completly, as addField doesn't return the actual query object but the field alias...
I scrapped it and started from my patch again with the hints by Damien.
@omitting the alias for addField: This would break all the code, as the default alias for a field is tablealias_tablefield, but all code needs just tablefield (the alias is the name in the result object, too).
@the unneeded '=' in conditions: I prefer having it there and actually seeing it, I know it can be ommitted. Maybe someone other can have a 3rd opinion (Crell?)
Most if not all from the whitespace issues should be fixed now, I reformatted some of the queries, too.

Crell’s picture

For convention I'd prefer to not have the '=' specified explicitly, as it is the default and I can't see that changing. We generally don't specify defaults we don't have to. I don't care strongly enough to hold back the entire patch for it, though. webchick may. :-)

Re the alias for addField(), Cornil is correct that the default is $table_$field, to help ensure uniqueness. You could arguably accept the default and then use the returned value as the field, as the unit tests do, if you wanted. The odds of that making a difference here are extremely remote, however, so I don't care much either way.

I'll try to give this a thorough review this weekend.

Damien Tournoud’s picture

@Crell: wouldn't it make sense to change the default addField() to default to $field, which is more inline with what most of core does?

CorniI’s picture

I'll remove the '='s today, but imho this shouldn't stop the review. Imho, the default alias of $table_$field isn't so user-friendly as you see eg from my code, because elsewhere just the field name and not $table_$field is used, so defaulting to $field, then $table_$field and then to whatever you want makes sense (too me...)

Crell’s picture

CorniI’s picture

FileSize
20.79 KB

Sorry for beeing late, but finally here it is :)

Crell’s picture

Status: Needs review » Needs work

Gr. Several broken hunks. :-(

jsaints’s picture

Status: Needs work » Needs review
FileSize
19.14 KB

Re-roll patch. It now applies cleanly against HEAD and passes all tests.

I also fixed line in the patch that caused line 288 of comment.test to fail.
Error was in the patch at line:

$max = db_query("SELECT MAX(thread) FROM {comments} WHERE thread LIKE :thread AND nid = :nid", array(':thread' => $parent->thread .'.%', ':nid' => $edit['nid']))->fetchField();
Crell’s picture

Status: Needs review » Needs work

comment.module:

line 320, that query doesn't need to be dynamic. It can just be a a db_query_range() call. Also, the foreach() loop is unnecessary. Just call ->getCol() on the result object, which can be chained directly on the db_query_range() call.

$nids = db_query_range("SELECT ...")->fetchCol();

Line 337 and following, those lines need to be indended within the if statement. Also, you can now just use fields() rather than addField to add multiple fields from a single table.

Line 385, as discussed with webchick we can break long queries across multiple lines in the same string, like so:

$result = db_query_range('(SELECT thread 
  FROM {comments} 
  WHERE nid = :nid  
    AND status = 0 
  ORDER BY timestamp DESC) 
  ORDER BY SUBSTRING(thread, 1, (LENGTH(thread) - 1))', array(':nid' => $node->nid), 0, $new_replies)
  ->fetchField();

Line 387, that query has multiple arguments so the argument array should be broken across multiple lines.

Lines 620, 621, 660, 661, I believe webchick wanted those broken across multiple lines, even if they're short. Anything longer than one chained method should be multi-line.

Line 760, break argument array across multiple lines.

Line 979 and following, use fields() there instead of addField(). Much easier to read, and that's I think the reason that fields() was added. :-) (See #11 above.)

Line 1016 and following, there's 2 unconverted dynamic queries there. It looks like they use db_rewrite_sql and pager. Bah. Until we get pager queries updated to the new API it's impossible to convert that properly. Please leave a TODO on them in the code, with a link to the appropriate issue ("Extenders" and the db_rewrite_sql issues) so that we know to fix it later.

Line 1155, argument array should be broken out across multiple lines.

Line 1182 and 1564, the array( should not be on its own line. It should be on the same line as the db_query( portion. Same applies to the various places above that need their args broken out.

Line 1188, the } should be on its own line. Looks like an accidental backspace to me. :-)

Line 1225, those chained methods should be on their own lines.

Lines 1915, 1930, array( should not be on its own line.

The function comment_operations() is unconverted, and it's weird enough that I'm not entirely sure how to convert it properly. :-)

comment.pages.inc: I think this file was skipped entirely. It only has a few queries so let's go ahead and do it while we're at it. Note line 125, which calls the weirdness that is comment_operations().

comment.admin.inc: Ibid. Let's convert this while we're here.

jsaints’s picture

FileSize
19.84 KB

I rolled a new patch with the changes listed above, except for the queries relating to comment_operations(). All tests pass. This is progress, but its not complete.

Here are some thoughts and questions on the changes I made:

  1. In the code snipit below, which is preferable, db_query_range() or something like $query->range(5, 10) ?
    $nids = db_query_range("SELECT ...")->fetchCol();
    
  2. The DBTNG docs say "fields() does not support specifying an alias for a field" so on line 993 I left in one $query->addField('u', 'name', 'registered_name');
  3. I do not see a good way to DBTNG the comment_operations(). Perhaps we could get some input from the comment.module author as to how we might best go about refactoring for DBTNG here. I will do the work, just need some input from the group.
  4. There is also still a pager_query in comment.admin.inc that needs to have a TODO comment put on it.

Its getting close. Thanks.

cburschka’s picture

Status: Needs work » Needs review
Crell’s picture

#1: db_query_range() is fine to use there, and probably a bit faster than db_select()->range().

#2: Great. That's exactly how it's supposed to work. :-)

#3: I don't know that there is a single author of comment.module anymore, really. I suggest posting to the dev list with a link here and a request for help from anyone with strong comment.module-fu.

jsaints’s picture

Status: Needs review » Needs work

On my run this morning , I thought of a clean way to do the update to DBTNG that will not require changing comment_operations().

This way we can talk about comment_operations() in a separate thread and patch if we want.

I will work on this today and have the patch soon.

jsaints’s picture

Status: Needs work » Needs review
FileSize
24.58 KB

Changes are made to comment_operations(). Made some other touchups.

All tests pass. It now applies to HEAD cleanly. This is ready for review.

cburschka’s picture

Just going over that... am I reading it wrong, or are you executing the same query twice here?

// comment.module:725
        db_query("UPDATE {comments} SET status = %d, timestamp = %d, subject = '%s', comment = '%s', format = %d, uid = %d, name = '%s', mail = '%s', homepage = '%s' WHERE cid = %d", $edit['status'], $edit['timestamp'], $edit['subject'], $edit['comment'], $edit['comment_format'], $edit['uid'], $edit['name'], $edit['mail'], $edit['homepage'], $edit['cid']);
        db_update('comments')
          ->fields(array(
            'status' => $edit['status'],
            'timestamp' => $edit['timestamp'],
            'subject' => $edit['subject'],
            'comment' => $edit['comment'],
            'format' => $edit['comment_format'],
            'uid' => $edit['uid'],
            'name' => $edit['name'],
            'mail' => $edit['mail'],
            'homepage' => $edit['homepage']
          ))
          ->condition('cid', $edit['cid'])
          ->execute();
cburschka’s picture

Status: Needs review » Needs work
+      
+      //TODO Convert these queries to dynamic queries once the pager query is updated to the new API
+

Comments need to end in a period, as far as I know.

jsaints’s picture

Status: Needs work » Needs review
FileSize
24.58 KB

Good catch. Here is a new one.

Thanks so much for your feedback.

Dries’s picture

Status: Needs review » Fixed

I gave this a good review and it certainly looked good to me. Additional improvements can go in a follow-up patch. Thanks all.

If this break the comment module, please submit a test with your fix.

Status: Fixed » Closed (fixed)

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

Crell’s picture

Tagging.