Fix incorrect handling of transactions using deferred constraints

Author: simPod

src/Connection.php

@@ -1283,16 +1283,39 @@ public function transactional(Closure $func)
 
         try {
             $res = $func($this);
-            $this->commit();
 
             $successful = true;
-
-            return $res;
         } finally {
             if (! $successful) {
                 $this->rollBack();
             }
         }
+
+        $this->commit();
+//        $successful = false;
+//        try {
+//            $this->commit();
+//
+//            $successful = true;
+//        } finally {
+//            if (! $successful) {
+//                $this->rollBack();
+//            }
+//        }
+
+        return $res;
+
+//        try {
+//            $res = $func($this);
+//        } catch (Throwable $e) {
+//            $this->rollBack();
+//
+//            throw $e;
+//        }
+//
+//        $this->commit();
+//
+//        return $res;
     }
 
     /**
@@ -1424,12 +1447,21 @@ public function commit()
 
         $connection = $this->getWrappedConnection();
 
-        if ($this->transactionNestingLevel === 1) {
-            $result = $this->doCommit($connection);
-        } elseif ($this->nestTransactionsWithSavepoints) {
-            $this->releaseSavepoint($this->_getNestedTransactionSavePointName());
+        try {
+            if ($this->transactionNestingLevel === 1) {
+                $result = $this->doCommit($connection);
+            } elseif ($this->nestTransactionsWithSavepoints) {
+                $this->releaseSavepoint($this->_getNestedTransactionSavePointName());
+            }
+        } finally {
+            $this->updateTransactionStateAfterCommit();
         }
 
+        return $result;
+    }
+
+    private function updateTransactionStateAfterCommit(): void
+    {
         --$this->transactionNestingLevel;
 
         $eventManager = $this->getEventManager();
@@ -1446,12 +1478,10 @@ public function commit()
         }
 
         if ($this->autoCommit !== false || $this->transactionNestingLevel !== 0) {
-            return $result;
+            return;
         }
 
         $this->beginTransaction();
-
-        return $result;
     }
 
     /**
@@ -1467,7 +1497,11 @@ private function doCommit(DriverConnection $connection)
             $logger->startQuery('"COMMIT"');
         }
 
-        $result = $connection->commit();
+        try {
+            $result = $connection->commit();
+        } catch (Driver\Exception $e) {
+            throw $this->convertExceptionDuringQuery($e, 'COMMIT');
+        }
 
         if ($logger !== null) {
             $logger->stopQuery();

src/Driver/PDO/PDOException.php

@@ -6,6 +6,11 @@
 
 use Doctrine\DBAL\Driver\Exception as DriverException;
 
+use function explode;
+use function is_numeric;
+use function str_replace;
+use function strpos;
+
 /**
  * @internal
  *
@@ -17,10 +22,26 @@ final class PDOException extends \PDOException implements DriverException
 
     public static function new(\PDOException $previous): self
     {
-        $exception = new self($previous->message, 0, $previous);
+        if (isset($previous->errorInfo[2]) && strpos($previous->errorInfo[2], 'OCITransCommit: ORA-02091') === 0) {
+            // With pdo_oci driver, the root-cause error is in the second line
+            //ORA-02091: transaction rolled back
+            //ORA-00001: unique constraint (DOCTRINE.GH3423_UNIQUE) violated
+            [$firstMessage, $secondMessage] = explode("\n", $previous->message, 2);
+
+            [$code, $message] = explode(': ', $secondMessage, 2);
+            $code             = (int) str_replace('ORA-', '', $code);
+        } else {
+            $message = $previous->message;
+            if (is_numeric($previous->code)) {
+                $code = (int) $previous->code;
+            } else {
+                $code = $previous->errorInfo[1] ?? 0;
+            }
+        }
+
+        $exception = new self($message, $code, $previous);
 
         $exception->errorInfo = $previous->errorInfo;
-        $exception->code      = $previous->code;
         $exception->sqlState  = $previous->errorInfo[0] ?? null;
 
         return $exception;

tests/Functional/TransactionTest.php

@@ -5,7 +5,6 @@
 use Doctrine\DBAL\Driver\Exception as DriverException;
 use Doctrine\DBAL\Platforms\AbstractMySQLPlatform;
 use Doctrine\DBAL\Tests\FunctionalTestCase;
-use PDOException;
 
 use function sleep;
 
@@ -29,8 +28,8 @@ public function testCommitFalse(): void
         sleep(2); // during the sleep mysql will close the connection
 
         try {
-            self::assertFalse(@$this->connection->commit()); // we will ignore `MySQL server has gone away` warnings
-        } catch (PDOException $e) {
+            self::assertFalse(@$this->connection->commit()); // we will ignore `Packets out of order.` error
+        } catch (\Doctrine\DBAL\Exception\DriverException $e) {
             self::assertInstanceOf(DriverException::class, $e);
 
             /* For PDO, we are using ERRMODE EXCEPTION, so this catch should be