diff --git a/src/EntityManager.php b/src/EntityManager.php index f7d47d7b12..b677c79dc1 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,24 @@ 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(); + if ($this->conn->isTransactionActive()) { + $this->conn->rollBack(); + } + } } } @@ -268,18 +273,24 @@ 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(); + if ($this->conn->isTransactionActive()) { + $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..88ff5ed924 100644 --- a/tests/Tests/ORM/EntityManagerTest.php +++ b/tests/Tests/ORM/EntityManagerTest.php @@ -21,16 +21,21 @@ 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 PHPUnit\Framework\Assert; use stdClass; use TypeError; use function get_class; use function random_int; +use function sprintf; use function sys_get_temp_dir; use function uniqid; @@ -382,4 +387,61 @@ 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()); + } + } + + /** @dataProvider entityManagerMethodNames */ + public function testItDoesNotAttemptToRollbackIfNoTransactionIsActive(string $methodName): void + { + $entityManager = new EntityManagerMock( + new class extends ConnectionMock { + public function commit(): bool + { + throw new Exception('Commit exception that happens after doing the actual commit'); + } + + public function rollBack(): bool + { + Assert::fail('Should not attempt to rollback if no transaction is active'); + } + + public function isTransactionActive(): bool + { + return false; + } + } + ); + + $this->expectExceptionMessage('Commit exception'); + $entityManager->$methodName(static function (): void { + }); + } + + /** @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 */