diff --git a/src/EntityManager.php b/src/EntityManager.php index f7d47d7b12..da09ed1b77 100644 --- a/src/EntityManager.php +++ b/src/EntityManager.php @@ -32,7 +32,6 @@ use Doctrine\Persistence\Mapping\MappingException; use Doctrine\Persistence\ObjectRepository; use InvalidArgumentException; -use Throwable; use function array_keys; use function class_exists; @@ -246,18 +245,22 @@ public function transactional($func) $this->conn->beginTransaction(); + $successful = false; + try { $return = $func($this); $this->flush(); $this->conn->commit(); - return $return ?: true; - } catch (Throwable $e) { - $this->close(); - $this->conn->rollBack(); + $successful = true; - throw $e; + return $return ?: true; + } finally { + if (! $successful) { + $this->close(); + $this->conn->rollBack(); + } } } @@ -268,18 +271,22 @@ public function wrapInTransaction(callable $func) { $this->conn->beginTransaction(); + $successful = false; + try { $return = $func($this); $this->flush(); $this->conn->commit(); - return $return; - } catch (Throwable $e) { - $this->close(); - $this->conn->rollBack(); + $successful = true; - throw $e; + return $return; + } finally { + if (! $successful) { + $this->close(); + $this->conn->rollBack(); + } } } diff --git a/src/UnitOfWork.php b/src/UnitOfWork.php index 39ba6b68b7..97d80862e3 100644 --- a/src/UnitOfWork.php +++ b/src/UnitOfWork.php @@ -49,7 +49,6 @@ use Exception; use InvalidArgumentException; use RuntimeException; -use Throwable; use UnexpectedValueException; use function array_chunk; @@ -427,6 +426,8 @@ public function commit($entity = null) $conn = $this->em->getConnection(); $conn->beginTransaction(); + $successful = false; + try { // Collection deletions (deletions of complete collections) foreach ($this->collectionDeletions as $collectionToDelete) { @@ -478,16 +479,18 @@ public function commit($entity = null) throw new OptimisticLockException('Commit failed', $object); } - } catch (Throwable $e) { - $this->em->close(); - if ($conn->isTransactionActive()) { - $conn->rollBack(); - } + $successful = true; + } finally { + if (! $successful) { + $this->em->close(); - $this->afterTransactionRolledBack(); + if ($conn->isTransactionActive()) { + $conn->rollBack(); + } - throw $e; + $this->afterTransactionRolledBack(); + } } $this->afterTransactionComplete(); diff --git a/tests/Tests/ORM/EntityManagerTest.php b/tests/Tests/ORM/EntityManagerTest.php index c3ad9f559e..c9b85f6b4f 100644 --- a/tests/Tests/ORM/EntityManagerTest.php +++ b/tests/Tests/ORM/EntityManagerTest.php @@ -21,9 +21,12 @@ use Doctrine\ORM\UnitOfWork; use Doctrine\Persistence\Mapping\Driver\MappingDriver; use Doctrine\Persistence\Mapping\MappingException; +use Doctrine\Tests\Mocks\ConnectionMock; +use Doctrine\Tests\Mocks\EntityManagerMock; use Doctrine\Tests\Models\CMS\CmsUser; use Doctrine\Tests\Models\GeoNames\Country; use Doctrine\Tests\OrmTestCase; +use Exception; use Generator; use InvalidArgumentException; use stdClass; @@ -31,6 +34,7 @@ use function get_class; use function random_int; +use function sprintf; use function sys_get_temp_dir; use function uniqid; @@ -382,4 +386,34 @@ public function testDeprecatedFlushWithArguments(): void $this->entityManager->flush($entity); } + + /** @dataProvider entityManagerMethodNames */ + public function testItPreservesTheOriginalExceptionOnRollbackFailure(string $methodName): void + { + $entityManager = new EntityManagerMock(new class extends ConnectionMock { + public function rollBack(): bool + { + throw new Exception('Rollback exception'); + } + }); + + try { + $entityManager->transactional(static function (): void { + throw new Exception('Original exception'); + }); + self::fail('Exception expected'); + } catch (Exception $e) { + self::assertSame('Rollback exception', $e->getMessage()); + self::assertNotNull($e->getPrevious()); + self::assertSame('Original exception', $e->getPrevious()->getMessage()); + } + } + + /** @return Generator */ + public function entityManagerMethodNames(): Generator + { + foreach (['transactional', 'wrapInTransaction'] as $methodName) { + yield sprintf('%s()', $methodName) => [$methodName]; + } + } } diff --git a/tests/Tests/ORM/UnitOfWorkTest.php b/tests/Tests/ORM/UnitOfWorkTest.php index ee475e729d..ae6b1d282f 100644 --- a/tests/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Tests/ORM/UnitOfWorkTest.php @@ -41,6 +41,7 @@ use Doctrine\Tests\Models\GeoNames\Country; use Doctrine\Tests\OrmTestCase; use Doctrine\Tests\PHPUnitCompatibility\MockBuilderCompatibilityTools; +use Exception; use PHPUnit\Framework\MockObject\MockObject; use stdClass; @@ -971,6 +972,43 @@ public function testItThrowsWhenApplicationProvidedIdsCollide(): void $this->_unitOfWork->persist($phone2); } + + public function testItPreservesTheOriginalExceptionOnRollbackFailure(): void + { + $this->_connectionMock = new class extends ConnectionMock { + public function commit(): bool + { + return false; // this should cause an exception + } + + public function rollBack(): bool + { + throw new Exception('Rollback exception'); + } + }; + $this->_emMock = new EntityManagerMock($this->_connectionMock); + $this->_unitOfWork = new UnitOfWorkMock($this->_emMock); + $this->_emMock->setUnitOfWork($this->_unitOfWork); + + // Setup fake persister and id generator + $userPersister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata(ForumUser::class)); + $userPersister->setMockIdGeneratorType(ClassMetadata::GENERATOR_TYPE_IDENTITY); + $this->_unitOfWork->setEntityPersister(ForumUser::class, $userPersister); + + // Create a test user + $user = new ForumUser(); + $user->username = 'Jasper'; + $this->_unitOfWork->persist($user); + + try { + $this->_unitOfWork->commit(); + self::fail('Exception expected'); + } catch (Exception $e) { + self::assertSame('Rollback exception', $e->getMessage()); + self::assertNotNull($e->getPrevious()); + self::assertSame('Commit failed', $e->getPrevious()->getMessage()); + } + } } /** @Entity */