DBTNG: book module
John Morahan - August 31, 2008 - 11:23
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | database system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
| Issue tags: | DBTNG Conversion |
Description
Lots of stuff that still needs to be done here:
* I haven't tested this at all because simpletest is horribly broken on my eeepc at the moment;
* I have no clue what to do with db_rewrite_sql or db_placeholders (which isn't actually used but the corresponding implode is a few times);
* There's at least one query with implode(' AND ', $match) that I didn't get around to replacing
I'll try to work on this some more when I get home.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| book.dbtng_.patch | 7.8 KB | Idle | Failed: Failed to apply patch. | View details | Re-test |
#1
Fixed most of the above. I still haven't touched anything with db_rewrite_sql; setting CNR so someone can tell me how to deal with those. Note that this includes a fix to SelectQuery - it wasn't inserting a comma between multiple ORDER BY fields.
#2
The multiple order by bug is handled in #302308: Multiple orderBy SQL syntax error, complete with unit test.
#3
Ok, cool. Here it is without that part. Apply both patches to test.
#4
The new docs-in-progress mention $query->addTag('node_access') so I added that.
I didn't see any obvious way to convert "SELECT * FROM {table}" to db_select, so I used this sort of thing:
<?php$book_schema = drupal_get_schema('book');
foreach (array_keys($book_schema['fields']) as $field) {
$query->addField('b', $field, $field);
}
?>
That's hardly the best way to do this, though?
#5
* 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.
#6
I used $query->addExpression('*') as these are both public API functions so we don't know what specific fields their callers will need.
I vaguely recall that something was still wrong here but all the tests pass and I've let this sit on my laptop for too long.
#7
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:
<?php
$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:
<?php$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:
<?phpdb_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!
#8
Thanks for the review! This one should be a bit better. I've left the db_placeholders one as a dynamic select for now - but actually, doesn't that need to be dynamic for node_access anyway?
#9
Oh bloody hell, not this again. I *know* I attached a patch.
#10
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.
#11
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. :-(
#12
Sorry for the delay.
#13
Patch applies, book tests pass. I noticed no problem in normal usage of the book module either.
#14
This won't work:
<?phpdb_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!
#15
#16
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?
#17
Here's a test, along with a fix for the typo which actually was still broken (I misread Crell's comment, sorry). The test fails because of a different bug in book.module though, so marking CNW to prevent the bot reviewing it for now.
#18
Let's fix both issues together so there can be tests and they can pass too.
#19
The bot likes it, and it looks OK on visual inspection.
#20
The last code block of the test could use a code comment. Please be a bit more verbose. Thanks!
#21
Added a comment.
#22
Committed to CVS HEAD. Thanks.
#23
Automatically closed -- issue fixed for two weeks with no activity.
#24
Tagging.