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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous 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 CreditAttribution: 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 🇨🇦
CreditAttribution: webchick 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
CreditAttribution: RobLoach commented
Status: Needs review » Reviewed & tested by the community
FileSize
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 CreditAttribution: 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 🇨🇦
CreditAttribution: webchick 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) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

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

Crell’s picture

Crell CreditAttribution: Crell commented
Issue tags: +DBTNG Conversion

Tagging.