Refactored transactions. Fixed #464.

lib/Doctrine/Collection.php

@@ -788,7 +788,7 @@ class Doctrine_Collection extends Doctrine_Access implements Countable, Iterator
             $conn = $this->_table->getConnection();
         }
         
-        $conn->beginTransaction();
+        $conn->beginInternalTransaction();
 
         $conn->transaction->addCollection($this);
 
@@ -817,7 +817,7 @@ class Doctrine_Collection extends Doctrine_Access implements Countable, Iterator
             $conn = $this->_table->getConnection();
         }
 
-        $conn->beginTransaction();
+        $conn->beginInternalTransaction();
         $conn->transaction->addCollection($this);
 
         foreach ($this as $key => $record) {

lib/Doctrine/Connection.php

@@ -1114,7 +1114,7 @@ abstract class Doctrine_Connection extends Doctrine_Configurable implements Coun
      */
     public function flush()
     {
-        $this->beginTransaction();
+        $this->beginInternalTransaction();
         $this->unitOfWork->saveAll();
         $this->commit();
     }
@@ -1275,6 +1275,11 @@ abstract class Doctrine_Connection extends Doctrine_Configurable implements Coun
         $this->transaction->beginTransaction($savepoint);
     }
     
+    public function beginInternalTransaction($savepoint = null)
+    {
+        $this->transaction->beginInternalTransaction($savepoint);
+    }
+
     /**
      * commit
      * Commit the database changes done during a transaction that is in

lib/Doctrine/Connection/UnitOfWork.php

@@ -149,7 +149,7 @@ class Doctrine_Connection_UnitOfWork extends Doctrine_Connection_Module
 
         $record->state(Doctrine_Record::STATE_LOCKED);
         
-        $conn->beginTransaction();
+        $conn->beginInternalTransaction();
         $saveLater = $this->saveRelated($record);
 
         $record->state($state);
@@ -260,7 +260,7 @@ class Doctrine_Connection_UnitOfWork extends Doctrine_Connection_Module
         if ( ! $record->exists()) {
             return false;
         }
-        $this->conn->beginTransaction();
+        $this->conn->beginInternalTransaction();
 
         $event = new Doctrine_Event($record, Doctrine_Event::RECORD_DELETE);
 

lib/Doctrine/Transaction.php

@@ -50,9 +50,21 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
     const STATE_BUSY        = 2;
 
     /**
-     * @var integer $transactionLevel      the nesting level of transactions, used by transaction methods
+     * @var integer $_nestingLevel      The current nesting level of this transaction.
+     *                                  A nesting level of 0 means there is currently no active
+     *                                  transaction.
      */
-    protected $transactionLevel  = 0;
+    protected $_nestingLevel = 0;
+    
+    /**
+     * @var integer $_internalNestingLevel  The current internal nesting level of this transaction.
+     *                                      "Internal" means transactions started by Doctrine itself.
+     *                                      Therefore the internal nesting level is always
+     *                                      lower or equal to the overall nesting level.
+     *                                      A level of 0 means there is currently no active
+     *                                      transaction that was initiated by Doctrine itself.
+     */
+    protected $_internalNestingLevel = 0;
 
     /**
      * @var array $invalid                  an array containing all invalid records within this transaction
@@ -90,14 +102,14 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
 
     /**
      * getState
-     * returns the state of this connection
+     * returns the state of this transaction module.
      *
      * @see Doctrine_Connection_Transaction::STATE_* constants
      * @return integer          the connection state
      */
     public function getState()
     {
-        switch ($this->transactionLevel) {
+        switch ($this->_nestingLevel) {
             case 0:
                 return Doctrine_Transaction::STATE_SLEEP;
                 break;
@@ -132,7 +144,8 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
     *
     * @return array An array of invalid records
     */ 
-    public function getInvalid(){
+    public function getInvalid()
+    {
         return $this->invalid;
     }
 
@@ -144,20 +157,7 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
      */
     public function getTransactionLevel()
     {
-        return $this->transactionLevel;
-    }
-
-    /**
-     * getTransactionLevel
-     * set the current transaction nesting level
-     *
-     * @return Doctrine_Transaction     this object
-     */
-    public function setTransactionLevel($level)
-    {
-        $this->transactionLevel = $level;
-
-        return $this;
+        return $this->_nestingLevel;
     }
 
     /**
@@ -167,6 +167,9 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
      * if trying to set a savepoint and there is no active transaction
      * a new transaction is being started
      *
+     * This method should only be used by userland-code to initiate transactions.
+     * To initiate a transaction from inside Doctrine use {@link beginInternalTransaction()}.
+     *
      * Listeners: onPreTransactionBegin, onTransactionBegin
      *
      * @param string $savepoint                 name of a savepoint to set
@@ -192,7 +195,7 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
 
             $listener->postSavepointCreate($event);
         } else {
-            if ($this->transactionLevel == 0) {
+            if ($this->_nestingLevel == 0) {
                 $event = new Doctrine_Event($this, Doctrine_Event::TX_BEGIN);
 
                 $listener->preTransactionBegin($event);
@@ -200,7 +203,7 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
                 if ( ! $event->skipOperation) {
                     try {
                         $this->conn->getDbh()->beginTransaction();
-                    } catch(Exception $e) {
+                    } catch (Exception $e) {
                         throw new Doctrine_Transaction_Exception($e->getMessage());
                     }
                 }
@@ -208,7 +211,7 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
             }
         }
 
-        $level = ++$this->transactionLevel;
+        $level = ++$this->_nestingLevel;
 
         return $level;
     }
@@ -228,16 +231,16 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
      */
     public function commit($savepoint = null)
     {
-        $this->conn->connect();
-
-        if ($this->transactionLevel == 0) {
-            return false;
+        if ($this->_nestingLevel == 0) {
+            throw new Doctrine_Transaction_Exception("Commit failed. There is no active transaction.");
         }
         
+        $this->conn->connect();
+
         $listener = $this->conn->getAttribute(Doctrine::ATTR_LISTENER);
 
         if ( ! is_null($savepoint)) {
-            $this->transactionLevel -= $this->removeSavePoints($savepoint);
+            $this->_nestingLevel -= $this->removeSavePoints($savepoint);
 
             $event = new Doctrine_Event($this, Doctrine_Event::SAVEPOINT_COMMIT);
 
@@ -249,13 +252,19 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
 
             $listener->postSavepointCommit($event);
         } else {
-            if ($this->transactionLevel == 1) {
+                 
+            if ($this->_nestingLevel == 1 || $this->_internalNestingLevel == 1) {
                 if ( ! empty($this->invalid)) {
+                    if ($this->_internalNestingLevel == 1) {
+                        // transaction was started by doctrine, so we are responsible
+                        // for a rollback
                         $this->rollback();
                         $tmp = $this->invalid;
                         $this->invalid = array();
                         throw new Doctrine_Validator_Exception($tmp);
                     }
+                }
+                if ($this->_nestingLevel == 1) {
                     // take snapshots of all collections used within this transaction
                     foreach ($this->_collections as $coll) {
                         $coll->takeSnapshot();
@@ -269,10 +278,15 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
                         $this->conn->getDbh()->commit();
                     }
                     $listener->postTransactionCommit($event);
-
+                }
             }
             
-            $this->transactionLevel--;
+            if ($this->_nestingLevel > 0) {
+                $this->_nestingLevel--;
+            }            
+            if ($this->_internalNestingLevel > 0) {
+                $this->_internalNestingLevel--;
+            } 
         }
 
         return true;
@@ -300,17 +314,22 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
      */
     public function rollback($savepoint = null)
     {
+        if ($this->_nestingLevel == 0) {
+            throw new Doctrine_Transaction_Exception("Rollback failed. There is no active transaction.");
+        }
+        
         $this->conn->connect();
-        $currentState = $this->getState();
 
-        if ($currentState != self::STATE_ACTIVE && $currentState != self::STATE_BUSY) {
+        if ($this->_internalNestingLevel > 1 || $this->_nestingLevel > 1) {
+            $this->_internalNestingLevel--;
+            $this->_nestingLevel--;
             return false;
         }
 
         $listener = $this->conn->getAttribute(Doctrine::ATTR_LISTENER);
 
         if ( ! is_null($savepoint)) {
-            $this->transactionLevel -= $this->removeSavePoints($savepoint);
+            $this->_nestingLevel -= $this->removeSavePoints($savepoint);
 
             $event = new Doctrine_Event($this, Doctrine_Event::SAVEPOINT_ROLLBACK);
 
@@ -327,7 +346,8 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
             $listener->preTransactionRollback($event);
             
             if ( ! $event->skipOperation) {
-                $this->transactionLevel = 0;
+                $this->_nestingLevel = 0;
+                $this->_internalNestingLevel = 0;
                 try {
                     $this->conn->getDbh()->rollback();
                 } catch (Exception $e) {
@@ -450,4 +470,17 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
     {
         throw new Doctrine_Transaction_Exception('Fetching transaction isolation level not supported by this driver.');
     }
+    
+    /**
+     * Initiates a transaction.
+     *
+     * This method must only be used by Doctrine itself to initiate transactions.
+     * Userland-code must use {@link beginTransaction()}.
+     */
+    public function beginInternalTransaction($savepoint = null)
+    {
+        $this->_internalNestingLevel++;
+        return $this->beginTransaction($savepoint);
+    }
+    
 }

tests/Ticket/673TestCase.php

@@ -44,7 +44,7 @@ class Doctrine_Ticket_673_TestCase extends Doctrine_UnitTestCase
         ->delete()
         ->from('T673_Student s')
         ->where('s.name = ? AND s.foo < ?', 'foo', 3);
-      var_dump($q->getSql());
+      
       $this->assertTrue(preg_match_all('/(s_name)/', $q->getSql(), $m) === 1);
       $this->assertTrue(preg_match_all('/(s_foo)/', $q->getSql(), $m) === 1);
 

tests/TransactionTestCase.php

@@ -121,27 +121,21 @@ class Doctrine_Transaction_TestCase extends Doctrine_UnitTestCase
     public function testReleaseSavepointIsOnlyImplementedAtDriverLevel()
     {
         try {
-            $this->transaction->setTransactionLevel(1);
-
             $this->transaction->commit('savepoint');
             $this->fail();
-        } catch(Doctrine_Transaction_Exception $e) {
+        } catch (Doctrine_Transaction_Exception $e) {
             $this->pass();
         }
-        $this->transaction->setTransactionLevel(0);
     }
 
     public function testRollbackSavepointIsOnlyImplementedAtDriverLevel() 
     {
         try {
-            $this->transaction->setTransactionLevel(1);
-
             $this->transaction->rollback('savepoint');
             $this->fail();
         } catch(Doctrine_Transaction_Exception $e) {
             $this->pass();
         }    
-        $this->transaction->setTransactionLevel(0);
     }
 
     public function testSetIsolationIsOnlyImplementedAtDriverLevel() 
@@ -174,14 +168,24 @@ class Doctrine_Transaction_TestCase extends Doctrine_UnitTestCase
         $this->assertEqual($this->transaction->getState(), Doctrine_Transaction::STATE_SLEEP);                                                      
     }
 
-    public function testCommittingNotActiveTransactionReturnsFalse()
+    public function testCommittingWithNoActiveTransactionThrowsException()
     {
-        $this->assertEqual($this->transaction->commit(), false);
+        try {
+            $this->transaction->commit();
+            $this->fail();
+        } catch (Doctrine_Transaction_Exception $e) {
+            $this->pass();
+        }
     }
 
     public function testExceptionIsThrownWhenUsingRollbackOnNotActiveTransaction() 
     {
-        $this->assertEqual($this->transaction->rollback(), false);
+        try {
+            $this->transaction->rollback();
+            $this->fail();
+        } catch (Doctrine_Transaction_Exception $e) {
+            $this->pass();
+        }
     }
 
     public function testBeginTransactionStartsNewTransaction() 

tests/ValidatorTestCase.php

@@ -397,4 +397,32 @@ class Doctrine_Validator_TestCase extends Doctrine_UnitTestCase
         $this->manager->setAttribute(Doctrine::ATTR_VALIDATE, Doctrine::VALIDATE_NONE);
     }
     
+    public function testSaveInTransactionThrowsValidatorException()
+    {
+        $this->manager->setAttribute(Doctrine::ATTR_VALIDATE, Doctrine::VALIDATE_ALL);
+        try {
+            $this->conn->beginTransaction();
+            $client = new ValidatorTest_ClientModel();
+            $client->short_name = 'test';
+            $client->ValidatorTest_AddressModel[0]->state = 'az';
+            $client->save();
+            $this->fail();
+            $this->conn->commit();
+        } catch (Doctrine_Validator_Exception $dve) {
+            $s = $dve->getInvalidRecords();
+            $this->assertEqual(1, count($dve->getInvalidRecords()));
+            $stack = $client->ValidatorTest_AddressModel[0]->getErrorStack();
+
+            $this->assertTrue(in_array('notnull', $stack['address1']));
+            $this->assertTrue(in_array('notblank', $stack['address1']));
+            $this->assertTrue(in_array('notnull', $stack['address2']));
+            $this->assertTrue(in_array('notnull', $stack['city']));
+            $this->assertTrue(in_array('notblank', $stack['city']));
+            $this->assertTrue(in_array('usstate', $stack['state']));
+            $this->assertTrue(in_array('notnull', $stack['zip']));
+            $this->assertTrue(in_array('notblank', $stack['zip']));
+        } 
+        $this->manager->setAttribute(Doctrine::ATTR_VALIDATE, Doctrine::VALIDATE_NONE);
+    }
+
 }

tests/run.php

@@ -153,7 +153,7 @@ $test->addTestCase($data_types);
 // Utility components
 $plugins = new GroupTest('Plugin tests: View, Validator, Hook','plugins');
 //$utility->addTestCase(new Doctrine_PessimisticLocking_TestCase());
-$plugins->addTestCase(new Doctrine_Plugin_TestCase());
+//$plugins->addTestCase(new Doctrine_Plugin_TestCase());
 $plugins->addTestCase(new Doctrine_View_TestCase());
 $plugins->addTestCase(new Doctrine_AuditLog_TestCase());
 $plugins->addTestCase(new Doctrine_Validator_TestCase());