Attached patch converts includes/batch.inc to DBTNG. It should have no other effect. To test, run, um, some unit test. :-)

CommentFileSizeAuthor
#4 313152.patch1.71 KBrobloach
dbtng-batch.patch1.75 KBCrell

Comments

Anonymous’s picture

Anonymous (not verified) commented

Consistent coding:

Should we strive to be consistent with line breaks and indentation for the use of ->?

I.E.: Should

   if ($batch['progressive']) {
-    db_query("DELETE FROM {batch} WHERE bid = %d", $batch['id']);
+    db_delete('batch')->condition('bid', $batch['id'])->execute();
   }

Change to:

   if ($batch['progressive']) {
-    db_query("DELETE FROM {batch} WHERE bid = %d", $batch['id']);
+    db_delete('batch')
+      ->condition('bid', $batch['id'])
+      ->execute();
   }

to match:

   if ($batch = batch_get()) {
-    db_query("UPDATE {batch} SET batch = '%s' WHERE bid = %d", serialize($batch), $batch['id']);
+    db_update('batch')
+      ->fields(array('batch' => serialize($batch)))
+      ->condition('bid', $batch['id'])
+      ->execute();
Crell’s picture

Crell commented

If the query is short and doesn't cross the 80 character boundary, there's no need to break it across multiple lines. The first query doesn't. The second query does. I defer to webchick on that convention, however.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

I prefer consistency, so putting all of them on their own lines is better from my perspective. Also, it makes diffs more meaningful if we ever change the query properties.

robloach’s picture

robloach
he/him
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 313152.patch1.71 KB

The only change I made is the whitespace change suggested at #1. In addition to that, tests are passing, so setting this to RTBC.

Should we use db_select here?

  // Retrieve the current state of batch from db.
  $batch = db_query("SELECT batch FROM {batch} WHERE bid = :bid AND token = :token", array(
    ':bid' => $_REQUEST['id'],
    ':token' => drupal_get_token($_REQUEST['id']))
  )->fetchField();
Crell’s picture

Crell commented

@Rob: Nope, because the query structure itself is not dynamic. It's just easier to read if we put the array in long form, like FAPI and menu items. db_select() should only be used when the structure of the query itself needs to change at runtime.

Thanks for the patch fix!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Awesome. Changes look great, and ran through a few SimpleTests with no problems. Committed. Thanks!

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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

Crell’s picture

Crell commented
Issue tags: +DBTNG Conversion

Tagging.