DBTNG: book module

The multiple order by bug is handled in #302308: Multiple orderBy SQL syntax error, complete with unit test.

* is an expression, so you'd use $query->addExpression('*'); However, you really shouldn't use * anyway. Just add the fields you know you want. You get a more efficient query that way.

book.module:

Line 213 and following, You can't chain most of db_select() because we need to return aliases. Sorry. :-( You can chain the execute()->fetchField(), and that can be on a single line like so:

$select = db_select('node');
$select->addField('node', 'title');
$select->condition('nid', $node->book['bid']);
$select->addTag('node_access');

$title = $select->execute()->fetchField();

Line 274 and following: No need to loop there. You can just do:

$nids = db_query("...")->fetchCol();

Line 280, that doesn't need to be a dynamic select, except for the annoyance of the IN clause. :-( Blargh. That's being worked on over in #314464: Convert db_placeholders() to DBTNG. I'm not sure how we want to handle that. The select query looks fine, it just should be unnecessary here.

Line 488, if you're going to explicitly specify a fetch mode as part of the method call then use ->fetchAssoc(), not ->fetch(PDO::FETCH_ASSOC). The former is easier to read. There are a couple of cases of that. (I think a lot of people are using fetchObject(), too, rather than the default of fetch().)

Line 501-505, the ->execute() should go on its own line, as should ->fields(). Like so:

      db_insert('book')
        ->fields(array(
          'nid' => $node->nid,
          'mlid' => $node->book['mlid'],
          'bid' => $node->book['bid'],
        ))
        ->execute();

Line 538 and 770, all built queries should be split across multiple lines, even if short. There are probably other instances of that I'm not catching at the moment, so please check for 'em.

Lines 1131 and following: I really dig the way you've abstracted the query to handle whatever depth is currently defined as the maximum. :-) Nicely done there.

book.pages.inc:

Line 213, another query that should be broken across multiple lines.

Thanks, John!

Hm. Yes, anything that needs node_access functionality (or any other query_alter) must be a dynamic select, and that is not going to change anytime soon. So yeah, if you need to tag it then it needs to be dynamic.

Patch still applies with offset.

book.module line 1172 and following: We've since improved the API to make adding multiple fields easier. You can now use ->fields($tablename, array(the fields)). Should be an easy change.

Unfortunately, contrary to the test report in #9 I do get a number of simpletest failures on the book module with this patch. :-(

Patch applies, book tests pass. I noticed no problem in normal usage of the book module either.

This won't work:

    db_update('book')
      ->fields('bid', $book_link['bid'])
      ->condition('mlid', $mlids, 'IN')
      ->execute();

UpdateQuery::fields() only takes an associative array. The second line should be ->fields(array('bid' => $book_link['bid']).

In book_nodeapi_delete(), I don't get why we're fetching an associative array there. The legacy code was, but not for any valid reason as it's just a single column. :-) Let's switch that to an object so we can get rid of the options array there.

That's all I see on visual inspection. So close!

Committed to CVS HEAD.

The fact that the tests didn't catch that typo (associative array), suggests that the book tests need more work. Care to follow-up on that and to work on a book.test patch?

The bot likes it, and it looks OK on visual inspection.

The last code block of the test could use a code comment. Please be a bit more verbose. Thanks!

Committed to CVS HEAD. Thanks.

Tagging.