Does Zend_Db_Adapter::beginTransaction() stack?

Question

Consider the following:

/** (Cas_Template_Tree::DeleteNode)
 * Deletes the given node from the tree, and all of it's children.
 *
 * @static
 * @throws Exception
 * @param Cas_Template_Node $node
 * @return void
 */
public static function DeleteNode(Cas_Template_Node $node)
{
    $table = new Cas_Table_Templates();
    $adapter = $table->getAdapter();
    $leftStr = $adapter->quoteIdentifier('Left');
    $rightStr = $adapter->quoteIdentifier('Right');
    try
    {
        $adapter->beginTransaction();
        $row = $table->find($node->GetId())->current();
        $dependantRowSelector = array(
            "$leftStr >= ?" => $row->Left,
            "$rightStr <= ?" => $row->Right
        );
        //Get the rows removed so that we can nuke the ACLs later.
        $rowsToDelete = $table->fetchAll($dependantRowSelector)->toArray();
        //Delete the rows.
        $table->delete($dependantRowSelector);
        //Delete access control lists on those rows.
        foreach ($rowsToDelete as $rowToDelete)
        {
            Cas_Acl::CreateExisting($rowToDelete['Acl'])->Delete();
        }
        $left = (int)$row->Left;
        $right = (int)$row->Right;
        $difference = $right - $left + 1;
        $table->update(array('Left' => new Zend_Db_Expr("$leftStr - $difference")),
                       array("$leftStr > ?" => $right));
        $table->update(array('Right' => new Zend_Db_Expr("$rightStr - $difference")),
                       array("$rightStr > ?" => $right));
        $adapter->commit();
    }
    catch (Exception $ex)
    {
        $adapter->rollBack();
        throw $ex;
    }
}

/** (Cas_Acl::Delete)
 * Removes this ACL (and all of its dependent access control entries) from the database.
 * @return void
 */
public function Delete()
{
    $aclTable = new Cas_Table_AccessControlLists();
    $aceTable = new Cas_Table_AccessControlEntries();
    $adapter = Zend_Db_Table_Abstract::getDefaultAdapter();
    $identifierName = $adapter->quoteIdentifier('Identifier');
    $aclName = $adapter->quoteIdentifier('Acl');
    try
    {
        $adapter->beginTransaction();
        $aceTable->delete(array("$aclName = ?" => $this->GetId()));
        $aclTable->delete(array("$identifierName = ?" => $this->GetId()));
    }
    catch (Exception $ex)
    {
        $adapter->rollBack();
        throw $ex;
    }
}

Notice how both of these require that transactions work, because otherwise the operation would not be atomic (which would be bad ;) ) However, there are two transaction blocks going on here. The original DeleteNode method calls Cas_Acl::Delete(), which also attempts to execute itself inside of a transaction block. Ideally, Zend_Db would be smart enough to recognize this case, and for this particular call ignore the begin transaction and commit/rollback calls inside Cas_Acl::Delete.

Would the above code be safe? Can it be significantly improved in any way?

Do you sometimes call `Delete()` directly? Or is it only called within `DeleteNode()` ? — Radek Benkel, Feb 08 '11 at 07:22
@singles: Yes -- `Cas_Acl::Delete` is called whenever an ACL needs to be deleted. And there's an ACL attached to most every object managed by this app. Some of those objects use transactions, while others don't need to. — Billy ONeal, Feb 08 '11 at 08:38
Your code raised my a question, like the haddock's beard over or under the sheet in tintin http://omg.wthax.org/haddock.jpg, I think the "$adapter->beginTransaction();" should be set before the "try {} catch" and not inside — regilero, Feb 08 '11 at 12:49

score 2 · Answer 1 · answered Feb 08 '11 at 08:42

AFAIK Zend_Db is not able to recognize nested transactions. Look a the code.

public function beginTransaction()
{
    $this->_connect();
    $q = $this->_profiler->queryStart('begin', Zend_Db_Profiler::TRANSACTION);
    $this->_beginTransaction();
    $this->_profiler->queryEnd($q);
    return $this;
}

There's no code here to recognize another transaction (but maybe profiler could be used to do such thing) and _beginTransaction relies on PDO's beginTransaction.

Thing that you might to do is add second param to Delete() method which determines whether to use transactions or not and call it with false param in DeleteNode():

//RB called as Cas_Acl::CreateExisting($rowToDelete['Acl'])->Delete(false);
public function Delete($useTransaction = true)
{
    $aclTable = new Cas_Table_AccessControlLists();
    $aceTable = new Cas_Table_AccessControlEntries();
    $adapter = Zend_Db_Table_Abstract::getDefaultAdapter();
    $identifierName = $adapter->quoteIdentifier('Identifier');
    $aclName = $adapter->quoteIdentifier('Acl');
    try
    {
        if ($useTransaction === true) {
            $adapter->beginTransaction();
        }
        $aceTable->delete(array("$aclName = ?" => $this->GetId()));
        $aclTable->delete(array("$identifierName = ?" => $this->GetId()));
        //BTW isn't commit() should be called here? 
    }
    catch (Exception $ex)
    {   
        if ($useTransaction === true) {
            $adapter->rollBack();
        }
        throw $ex;
    }
}

Would probably use a strategy object instead of a boolean here (Polymorphism > conditionals). Otherwise good answer (+1) — Billy ONeal, Feb 08 '11 at 16:04

score 1 · Accepted Answer · answered Feb 08 '11 at 12:31

1

I use a custom transactionManager object and add this logic (start transaction if not yet started) there.

I found this type of object (Delegation?) quite useful. I use it as well to use specific DB connection with right access. All write and read query inside a transaction are using this TransactionManager object.

Call to roolback are done in the controller, but via the TransactionManager as well, which detects if it should really do it, and it is very useful as well because calling Rollback twice will make most of existing databases cry.

Finally we try to use a generic rule, make the transaction control code very "high-level" in the code (only on controllers) and not in more abstract sub-levels objects. So that generally we can call any ->Delete() action on most objecs and we know this object is not trying to handle the transaction and that WE should do it. But, effectively, this is only a general rule and sometimes enclosing transactions happen and the TransactionManagers help us to hide the problem (and can log them with stacktrace).

answered Feb 08 '11 at 12:31

regilero

29,806
6
60
99

+1 - I'm probably going to go with moving all transaction logic into the controllers. :( I don't like the idea of a "transaction manager" 1. because PHP doesn't have deterministic destruction (and therefore RAII is out of the question), and 2. because any class containing the word "manager" is a code smell. – Billy ONeal Feb 08 '11 at 16:00
@Billy ONeal: you have the register_shutdown function, and anyway if you do not use persistent connexion the transaction will timeout in case of problem, so deterministic destruction is not a problem. And you can call it TransactionAdapter if you want, so no manage or handle in the name, and a real Pattern. – regilero Feb 08 '11 at 17:04
@regilero: It's less a name and more what the class actually does. Words like "manager" are "nice filler words because we don't know exactly what this class should be doing" -- and usually indicates an SRP violation. (Disclaimer: I'm in a glass house here -- the first method above is in `Cas_Template_Locator`, which is just as bad). – Billy ONeal Feb 08 '11 at 17:40
@Billy ONeal:yes, quite right about SRP violation, but here, talking about the transaction responsability, the good design is at least to exclude transaction handling from DAO and put the transaction responsability in a single object which is responsible for transaction handling (handling the try/catch and retry loop around the transaction code -as nice transaction get break by tiemouts and deadlocks sometime), a complex code, better to write it only one time. – regilero Feb 08 '11 at 20:24
@regilero: Sorry -- my response was a knee-jerk reaction -- too much looking at students' code and seeing them add the word "manager" to *everything* :( I agree that it makes sense to make a separate object though. Perhaps "RefCountedTransaction" or "TransactionSerializer" makes more sense. – Billy ONeal Feb 08 '11 at 20:55
Apparently [PHP **does** support RAII](http://stackoverflow.com/questions/4938679/does-php-support-the-raii-pattern-how/4938780#4938780), so now the natural name is "ScopedTransaction". – Billy ONeal Feb 08 '11 at 21:53

Does Zend_Db_Adapter::beginTransaction() stack?

2 Answers2