Merge pull request #3480 from morozov/transaction-void

Transaction-related Statement methods return void

UPGRADE.md

 # Upgrade to 3.0
 
+## BC BREAK Transaction-related `Statement` methods return `void`.
+
+`Statement::beginTransaction()`, `::commit()` and `::rollBack()` no longer return a boolean value. They will throw a `DriverException` in case of failure.
+
 ## MINOR BC BREAK `Statement::fetchColumn()` with an invalid index.
 
 Similarly to `PDOStatement::fetchColumn()`, DBAL statements throw an exception in case of an invalid column index.

lib/Doctrine/DBAL/Connection.php

@@ -9,6 +9,7 @@ use Doctrine\DBAL\Cache\CacheException;
 use Doctrine\DBAL\Cache\QueryCacheProfile;
 use Doctrine\DBAL\Cache\ResultCacheStatement;
 use Doctrine\DBAL\Driver\Connection as DriverConnection;
+use Doctrine\DBAL\Driver\DriverException;
 use Doctrine\DBAL\Driver\PingableConnection;
 use Doctrine\DBAL\Driver\ResultStatement;
 use Doctrine\DBAL\Driver\ServerInfoAwareConnection;
@@ -502,12 +503,11 @@ class Connection implements DriverConnection
      *
      * @see   isAutoCommit
      *
-     * @param bool $autoCommit True to enable auto-commit mode; false to disable it.
+     * @throws ConnectionException
+     * @throws DriverException
      */
-    public function setAutoCommit($autoCommit)
+    public function setAutoCommit(bool $autoCommit) : void
     {
-        $autoCommit = (bool) $autoCommit;
-
         // Mode not changed, no-op.
         if ($autoCommit === $this->autoCommit) {
             return;
@@ -1191,7 +1191,7 @@ class Connection implements DriverConnection
     /**
      * {@inheritDoc}
      */
-    public function beginTransaction()
+    public function beginTransaction() : void
     {
         $connection = $this->getWrappedConnection();
 
@@ -1201,15 +1201,17 @@ class Connection implements DriverConnection
 
         if ($this->transactionNestingLevel === 1) {
             $logger->startQuery('"START TRANSACTION"');
-            $connection->beginTransaction();
-            $logger->stopQuery();
+
+            try {
+                $connection->beginTransaction();
+            } finally {
+                $logger->stopQuery();
+            }
         } elseif ($this->nestTransactionsWithSavepoints) {
             $logger->startQuery('"SAVEPOINT"');
             $this->createSavepoint($this->_getNestedTransactionSavePointName());
             $logger->stopQuery();
         }
-
-        return true;
     }
 
     /**
@@ -1218,11 +1220,12 @@ class Connection implements DriverConnection
      * @throws ConnectionException If the commit failed due to no active transaction or
      *                                            because the transaction was marked for rollback only.
      */
-    public function commit()
+    public function commit() : void
     {
         if ($this->transactionNestingLevel === 0) {
             throw ConnectionException::noActiveTransaction();
         }
+
         if ($this->isRollbackOnly) {
             throw ConnectionException::commitFailedRollbackOnly();
         }
@@ -1235,8 +1238,12 @@ class Connection implements DriverConnection
 
         if ($this->transactionNestingLevel === 1) {
             $logger->startQuery('"COMMIT"');
-            $result = $connection->commit();
-            $logger->stopQuery();
+
+            try {
+                $connection->commit();
+            } finally {
+                $logger->stopQuery();
+            }
         } elseif ($this->nestTransactionsWithSavepoints) {
             $logger->startQuery('"RELEASE SAVEPOINT"');
             $this->releaseSavepoint($this->_getNestedTransactionSavePointName());
@@ -1246,18 +1253,19 @@ class Connection implements DriverConnection
         --$this->transactionNestingLevel;
 
         if ($this->autoCommit !== false || $this->transactionNestingLevel !== 0) {
-            return $result;
+            return;
         }
 
         $this->beginTransaction();
-
-        return $result;
     }
 
     /**
      * Commits all current nesting transactions.
+     *
+     * @throws ConnectionException
+     * @throws DriverException
      */
-    private function commitAll()
+    private function commitAll() : void
     {
         while ($this->transactionNestingLevel !== 0) {
             if ($this->autoCommit === false && $this->transactionNestingLevel === 1) {
@@ -1273,11 +1281,11 @@ class Connection implements DriverConnection
     }
 
     /**
-     * Cancels any database changes done during the current transaction.
+     * {@inheritDoc}
      *
      * @throws ConnectionException If the rollback operation failed.
      */
-    public function rollBack()
+    public function rollBack() : void
     {
         if ($this->transactionNestingLevel === 0) {
             throw ConnectionException::noActiveTransaction();
@@ -1290,12 +1298,16 @@ class Connection implements DriverConnection
         if ($this->transactionNestingLevel === 1) {
             $logger->startQuery('"ROLLBACK"');
             $this->transactionNestingLevel = 0;
-            $connection->rollBack();
-            $this->isRollbackOnly = false;
-            $logger->stopQuery();
 
-            if ($this->autoCommit === false) {
-                $this->beginTransaction();
+            try {
+                $connection->rollBack();
+            } finally {
+                $this->isRollbackOnly = false;
+                $logger->stopQuery();
+
+                if ($this->autoCommit === false) {
+                    $this->beginTransaction();
+                }
             }
         } elseif ($this->nestTransactionsWithSavepoints) {
             $logger->startQuery('"ROLLBACK TO SAVEPOINT"');

lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php

@@ -230,31 +230,31 @@ class MasterSlaveConnection extends Connection
     /**
      * {@inheritDoc}
      */
-    public function beginTransaction()
+    public function beginTransaction() : void
     {
         $this->connect('master');
 
-        return parent::beginTransaction();
+        parent::beginTransaction();
     }
 
     /**
      * {@inheritDoc}
      */
-    public function commit()
+    public function commit() : void
     {
         $this->connect('master');
 
-        return parent::commit();
+        parent::commit();
     }
 
     /**
      * {@inheritDoc}
      */
-    public function rollBack()
+    public function rollBack() : void
     {
         $this->connect('master');
 
-        return parent::rollBack();
+        parent::rollBack();
     }
 
     /**

lib/Doctrine/DBAL/Driver/Connection.php

@@ -54,23 +54,23 @@ interface Connection
     /**
      * Initiates a transaction.
      *
-     * @return bool TRUE on success or FALSE on failure.
+     * @throws DriverException
      */
-    public function beginTransaction();
+    public function beginTransaction() : void;
 
     /**
      * Commits a transaction.
      *
-     * @return bool TRUE on success or FALSE on failure.
+     * @throws DriverException
      */
-    public function commit();
+    public function commit() : void;
 
     /**
      * Rolls back the current transaction, as initiated by beginTransaction().
      *
-     * @return bool TRUE on success or FALSE on failure.
+     * @throws DriverException
      */
-    public function rollBack();
+    public function rollBack() : void;
 
     /**
      * Returns the error code associated with the last operation on the database handle.

lib/Doctrine/DBAL/Driver/IBMDB2/DB2Connection.php

@@ -137,31 +137,39 @@ class DB2Connection implements Connection, ServerInfoAwareConnection
     /**
      * {@inheritdoc}
      */
-    public function beginTransaction()
+    public function beginTransaction() : void
     {
-        db2_autocommit($this->conn, DB2_AUTOCOMMIT_OFF);
+        if (! db2_autocommit($this->conn, DB2_AUTOCOMMIT_OFF)) {
+            throw new DB2Exception(db2_conn_errormsg($this->conn));
+        }
     }
 
     /**
      * {@inheritdoc}
      */
-    public function commit()
+    public function commit() : void
     {
         if (! db2_commit($this->conn)) {
             throw new DB2Exception(db2_conn_errormsg($this->conn));
         }
-        db2_autocommit($this->conn, DB2_AUTOCOMMIT_ON);
+
+        if (! db2_autocommit($this->conn, DB2_AUTOCOMMIT_ON)) {
+            throw new DB2Exception(db2_conn_errormsg($this->conn));
+        }
     }
 
     /**
      * {@inheritdoc}
      */
-    public function rollBack()
+    public function rollBack() : void
     {
         if (! db2_rollback($this->conn)) {
             throw new DB2Exception(db2_conn_errormsg($this->conn));
         }
-        db2_autocommit($this->conn, DB2_AUTOCOMMIT_ON);
+
+        if (! db2_autocommit($this->conn, DB2_AUTOCOMMIT_ON)) {
+            throw new DB2Exception(db2_conn_errormsg($this->conn));
+        }
     }
 
     /**

lib/Doctrine/DBAL/Driver/IBMDB2/DB2Exception.php

@@ -2,8 +2,8 @@
 
 namespace Doctrine\DBAL\Driver\IBMDB2;
 
-use Exception;
+use Doctrine\DBAL\Driver\AbstractDriverException;
 
-class DB2Exception extends Exception
+class DB2Exception extends AbstractDriverException
 {
 }

lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php

@@ -174,27 +174,29 @@ class MysqliConnection implements Connection, PingableConnection, ServerInfoAwar
     /**
      * {@inheritdoc}
      */
-    public function beginTransaction()
+    public function beginTransaction() : void
     {
         $this->conn->query('START TRANSACTION');
-
-        return true;
     }
 
     /**
      * {@inheritdoc}
      */
-    public function commit()
+    public function commit() : void
     {
-        return $this->conn->commit();
+        if (! $this->conn->commit()) {
+            throw new MysqliException($this->conn->error, $this->conn->sqlstate, $this->conn->errno);
+        }
     }
 
     /**
-     * {@inheritdoc}non-PHPdoc)
+     * {@inheritdoc}
      */
-    public function rollBack()
+    public function rollBack() : void
     {
-        return $this->conn->rollback();
+        if (! $this->conn->rollback()) {
+            throw new MysqliException($this->conn->error, $this->conn->sqlstate, $this->conn->errno);
+        }
     }
 
     /**

lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php

@@ -177,37 +177,33 @@ class OCI8Connection implements Connection, ServerInfoAwareConnection
     /**
      * {@inheritdoc}
      */
-    public function beginTransaction()
+    public function beginTransaction() : void
     {
         $this->executeMode = OCI_NO_AUTO_COMMIT;
-
-        return true;
     }
 
     /**
      * {@inheritdoc}
      */
-    public function commit()
+    public function commit() : void
     {
         if (! oci_commit($this->dbh)) {
             throw OCI8Exception::fromErrorInfo($this->errorInfo());
         }
-        $this->executeMode = OCI_COMMIT_ON_SUCCESS;
 
-        return true;
+        $this->executeMode = OCI_COMMIT_ON_SUCCESS;
     }
 
     /**
      * {@inheritdoc}
      */
-    public function rollBack()
+    public function rollBack() : void
     {
         if (! oci_rollback($this->dbh)) {
             throw OCI8Exception::fromErrorInfo($this->errorInfo());
         }
-        $this->executeMode = OCI_COMMIT_ON_SUCCESS;
 
-        return true;
+        $this->executeMode = OCI_COMMIT_ON_SUCCESS;
     }
 
     /**

lib/Doctrine/DBAL/Driver/PDOConnection.php

@@ -126,25 +126,25 @@ class PDOConnection implements Connection, ServerInfoAwareConnection
     /**
      * {@inheritDoc}
      */
-    public function beginTransaction()
+    public function beginTransaction() : void
     {
-        return $this->connection->beginTransaction();
+        $this->connection->beginTransaction();
     }
 
     /**
      * {@inheritDoc}
      */
-    public function commit()
+    public function commit() : void
     {
-        return $this->connection->commit();
+        $this->connection->commit();
     }
 
     /**
      * {@inheritDoc}
      */
-    public function rollBack()
+    public function rollBack() : void
     {
-        return $this->connection->rollBack();
+        $this->connection->rollBack();
     }
 
     /**

lib/Doctrine/DBAL/Driver/SQLAnywhere/SQLAnywhereConnection.php

@@ -64,13 +64,11 @@ class SQLAnywhereConnection implements Connection, ServerInfoAwareConnection
      *
      * @throws SQLAnywhereException
      */
-    public function beginTransaction()
+    public function beginTransaction() : void
     {
         if (! sasql_set_option($this->connection, 'auto_commit', 'off')) {
             throw SQLAnywhereException::fromSQLAnywhereError($this->connection);
         }
-
-        return true;
     }
 
     /**
@@ -78,15 +76,13 @@ class SQLAnywhereConnection implements Connection, ServerInfoAwareConnection
      *
      * @throws SQLAnywhereException
      */
-    public function commit()
+    public function commit() : void
     {
         if (! sasql_commit($this->connection)) {
             throw SQLAnywhereException::fromSQLAnywhereError($this->connection);
         }
 
         $this->endTransaction();
-
-        return true;
     }
 
     /**
@@ -185,30 +181,24 @@ class SQLAnywhereConnection implements Connection, ServerInfoAwareConnection
      *
      * @throws SQLAnywhereException
      */
-    public function rollBack()
+    public function rollBack() : void
     {
         if (! sasql_rollback($this->connection)) {
             throw SQLAnywhereException::fromSQLAnywhereError($this->connection);
         }
 
         $this->endTransaction();
-
-        return true;
     }
 
     /**
      * Ends transactional mode and enables auto commit again.
      *
-     * @return bool Whether or not ending transactional mode succeeded.
-     *
      * @throws SQLAnywhereException
      */
-    private function endTransaction()
+    private function endTransaction() : void
     {
         if (! sasql_set_option($this->connection, 'auto_commit', 'on')) {
             throw SQLAnywhereException::fromSQLAnywhereError($this->connection);
         }
-
-        return true;
     }
 }

lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvConnection.php

@@ -146,7 +146,7 @@ class SQLSrvConnection implements Connection, ServerInfoAwareConnection
     /**
      * {@inheritDoc}
      */
-    public function beginTransaction()
+    public function beginTransaction() : void
     {
         if (! sqlsrv_begin_transaction($this->conn)) {
             throw SQLSrvException::fromSqlSrvErrors();
@@ -156,7 +156,7 @@ class SQLSrvConnection implements Connection, ServerInfoAwareConnection
     /**
      * {@inheritDoc}
      */
-    public function commit()
+    public function commit() : void
     {
         if (! sqlsrv_commit($this->conn)) {
             throw SQLSrvException::fromSqlSrvErrors();
@@ -166,7 +166,7 @@ class SQLSrvConnection implements Connection, ServerInfoAwareConnection
     /**
      * {@inheritDoc}
      */
-    public function rollBack()
+    public function rollBack() : void
     {
         if (! sqlsrv_rollback($this->conn)) {
             throw SQLSrvException::fromSqlSrvErrors();

tests/Doctrine/Tests/DBAL/ConnectionTest.php

@@ -251,8 +251,6 @@ class ConnectionTest extends DbalTestCase
     {
         $this->connection->setAutoCommit(false);
         self::assertFalse($this->connection->isAutoCommit());
-        $this->connection->setAutoCommit(0);
-        self::assertFalse($this->connection->isAutoCommit());
     }
 
     /**
@@ -297,28 +295,6 @@ class ConnectionTest extends DbalTestCase
         self::assertTrue($conn->isTransactionActive());
     }
 
-    /**
-     * @dataProvider resultProvider
-     */
-    public function testCommitReturn(bool $expectedResult) : void
-    {
-        $driverConnection = $this->createMock(DriverConnection::class);
-        $driverConnection->expects($this->once())
-            ->method('commit')->willReturn($expectedResult);
-
-        $driverMock = $this->createMock(Driver::class);
-        $driverMock->expects($this->any())
-            ->method('connect')
-            ->will($this->returnValue($driverConnection));
-
-        $conn = new Connection([], $driverMock);
-
-        $conn->connect();
-        $conn->beginTransaction();
-
-        self::assertSame($expectedResult, $conn->commit());
-    }
-
     /**
      * @return bool[][]
      */

tests/Doctrine/Tests/DBAL/Functional/ConnectionTest.php

@@ -122,7 +122,7 @@ class ConnectionTest extends DbalFunctionalTestCase
                 self::assertEquals(2, $this->connection->getTransactionNestingLevel());
                 $this->connection->beginTransaction();
                 self::assertEquals(3, $this->connection->getTransactionNestingLevel());
-                self::assertTrue($this->connection->commit());
+                $this->connection->commit();
                 self::assertEquals(2, $this->connection->getTransactionNestingLevel());
                 throw new Exception();
                 $this->connection->commit(); // never reached

tests/Doctrine/Tests/DBAL/Functional/TransactionTest.php

-<?php
-
-namespace Doctrine\Tests\DBAL\Functional;
-
-use Doctrine\DBAL\Platforms\MySqlPlatform;
-use Doctrine\Tests\DbalFunctionalTestCase;
-use function sleep;
-
-class TransactionTest extends DbalFunctionalTestCase
-{
-    protected function setUp() : void
-    {
-        parent::setUp();
-
-        if ($this->connection->getDatabasePlatform() instanceof MySqlPlatform) {
-            return;
-        }
-
-        $this->markTestSkipped('Restricted to MySQL.');
-    }
-
-    protected function tearDown() : void
-    {
-        $this->resetSharedConn();
-
-        parent::tearDown();
-    }
-
-    public function testCommitFalse() : void
-    {
-        $this->connection->query('SET SESSION wait_timeout=1');
-
-        $this->assertTrue($this->connection->beginTransaction());
-
-        sleep(2); // during the sleep mysql will close the connection
-
-        $this->assertFalse(@$this->connection->commit()); // we will ignore `MySQL server has gone away` warnings
-    }
-}