Wrong use of db_transaction() in block module [#1022416]

Since the official launch, I have been trying to learn some of the newly introduced features in Drupal 7. One of them is database transaction. Based on Database abstraction layer, the documentation code sample, snippet below, states that when $txn goes out of scope, it is automatically committed.

function my_transaction_function() {
  // The transaction opens here.
  $txn = db_transaction();

  .
  .
  .

  // $txn goes out of scope here.  Unless the transaction was rolled back, it
  // gets automatically commited here.
}

From my understanding of a database transaction, rollback should be the default action if no explicit commit action is given. In no case can a partial transaction be committed to the database since that would leave the database in an inconsistent state. Any exception could happen and cause the function to end prematurely and partial transaction will be automatically commited. Your next question might be, why not use try {} catch {}. Good question! =P

My next question is, why is try {} catch {} not used in the block module code below? Does db_transaction() make a difference here if an error occurs in the middle of the foreach loop? Can we safely assume that when $txn is destructed, all the work done in the transaction are successfully without us doing an explicit commit?

/**
 * Form submission handler for the main blocks administration form.
 *
 * @see block_admin_display_form()
 */
function block_admin_display_form_submit($form, &$form_state) {
  $txn = db_transaction();

  foreach ($form_state['values']['blocks'] as $block) {
    $block['status'] = (int) ($block['region'] != BLOCK_REGION_NONE);
    $block['region'] = $block['status'] ? $block['region'] : '';
    db_update('block')
      ->fields(array(
        'status' => $block['status'],
        'weight' => $block['weight'],
        'region' => $block['region'],
      ))
      ->condition('module', $block['module'])
      ->condition('delta', $block['delta'])
      ->condition('theme', $block['theme'])
      ->execute();
  }
  drupal_set_message(t('The block settings have been updated.'));
  cache_clear_all();
}

Comment	File	Size	Author
#4	1022416-5-wrap_block_transactions_in_try_catch.patch	5.34 KB	bfroehle

Comments

Comment #1

bfroehle commented 11 January 2011 at 05:15

Yes, the $transaction should have a try/catch wrapper (or be removed). Both block_admin_display_form_submit() and block_admin_configure_submit() have this problem.

I went through and scanned all other calls to db_transaction() and they seemed okay.

Comment #2

turboflash commented 11 January 2011 at 07:12

Hi bfroehle,

Thanks for confirming that.

Anyway, there is another point that I wish to highlight in my post actually. Should we require an explicit $txn->commit()? I just feel it is not right that Drupal is committing the transaction for me automatically if I did not tell it to do so and actual data is written on commit without my "consent". By putting in an explicit $txn->commit(), it will be akin to saying by the coder that this is the point all the work in the transaction are done and successful, and Drupal has the go ahead to commit the transaction.

This bug would have been picked up by the automated test if rollback is the default action when $txn is destructed. In fact, the code could work without try {} catch {} too if rollback is the default action and $txn->commit() is used. Just my additional two cents worth on the topic. =P

Comment #3

turboflash commented 11 January 2011 at 07:59

Just discovered something while reading up Constructors and Destructors.

The destructor will be called even if script execution is stopped using exit(). Calling exit() in a destructor will prevent the remaining shutdown routines from executing.

So if an exit() or drupal_exit() is called due to whatever reasons, it is the same as doing a commit, which might not be your intention, unless a $txn->rollback() is called beforehand.

Comment #4

bfroehle commented 11 January 2011 at 08:05

Status:

Active

» Needs review

Status	File	Size
new	1022416-5-wrap_block_transactions_in_try_catch.patch	5.34 KB

Attached patch puts the database code in try/catch statements. In block_admin_configure_submit(), I opted to include the

module_invoke($form_state['values']['module'], 'block_save', $form_state['values']['delta'], $form_state['values']);

call in try block.

Comment #5

turboflash commented 13 January 2011 at 04:01

Hi bfroehle,

Yup, that quick fix did solve the issue. However, it still did not address the issue which I have highlighted on the default commit when $txn goes out of scope. I did a more extensive read up and default commit is not the norm. Since Drupal is built upon PDO, I have attached a quote from PHP documentation below.

When the script ends or when a connection is about to be closed, if you have an outstanding transaction, PDO will automatically roll it back. This is a safety measure to help avoid inconsistency in the cases where the script terminates unexpectedly--if you didn't explicitly commit the transaction, then it is assumed that something went awry, so the rollback is performed for the safety of your data.

I do hope someone could re-evaluate this. I prefer to be safe and conservative with my data rather than take a risk with the assumption everything is successful. It will help anchor Drupal 7 towards an enterprise-level CMS.

Comment #6

bfroehle commented 13 January 2011 at 04:43

/**
 * A wrapper class for creating and managing database transactions.
 *
 * Not all databases or database configurations support transactions. For
 * example, MySQL MyISAM tables do not. It is also easy to begin a transaction
 * and then forget to commit it, which can lead to connection errors when
 * another transaction is started.
 *
 * This class acts as a wrapper for transactions. To begin a transaction,
 * simply instantiate it. When the object goes out of scope and is destroyed
 * it will automatically commit. It also will check to see if the specified
 * connection supports transactions. If not, it will simply skip any transaction
 * commands, allowing user-space code to proceed normally. The only difference
 * is that rollbacks won't actually do anything.
 *
 * In the vast majority of cases, you should not instantiate this class
 * directly. Instead, call ->startTransaction(), from the appropriate connection
 * object.
 */
class DatabaseTransaction {

In fact, directly committing is not even allowed:


  /**
   * Throws an exception to deny direct access to transaction commits.
   *
   * We do not want to allow users to commit transactions at any time, only
   * by destroying the transaction object or allowing it to go out of scope.
   * A direct commit bypasses all of the safety checks we've built on top of
   * PDO's transaction routines.
   *
   * @throws DatabaseTransactionExplicitCommitNotAllowedException
   *
   * @see DatabaseTransaction
   */
  public function commit() {
    throw new DatabaseTransactionExplicitCommitNotAllowedException();
  }

Comment #7

turboflash commented 13 January 2011 at 05:47

Thanks for the information. I am aware of that actually and I'm just checking the possibility for that to be changed.

The author of the class could have initially thought of making it as a default rollback but due to some reason, the author has make it otherwise. I hope to find out whether default rollback was ever considered when the class was written. If yes and there is reasons which I am not aware of for the class to be written this way, I'm happy to accept it. If not and there is no strong reason, shouldn't we explore the possibility of making it default rollback when object goes out of scope?

To me, Drupal is already a great CMS, why not make it the best. :)