From 16a8f10fd2fca952b08a656e909dd0fa0a86baed Mon Sep 17 00:00:00 2001 From: Matthias Pigulla Date: Wed, 9 Oct 2024 15:37:04 +0200 Subject: [PATCH 1/4] Remove a misleading comment (#11644) --- src/Query/Exec/SingleTableDeleteUpdateExecutor.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Query/Exec/SingleTableDeleteUpdateExecutor.php b/src/Query/Exec/SingleTableDeleteUpdateExecutor.php index 7f7496e397a..4a4a2986467 100644 --- a/src/Query/Exec/SingleTableDeleteUpdateExecutor.php +++ b/src/Query/Exec/SingleTableDeleteUpdateExecutor.php @@ -14,8 +14,6 @@ * that are mapped to a single table. * * @link www.doctrine-project.org - * - * @todo This is exactly the same as SingleSelectExecutor. Unify in SingleStatementExecutor. */ class SingleTableDeleteUpdateExecutor extends AbstractSqlExecutor { From 51be1b1d526342543f783f18352862cbdd2a8551 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Paris?= Date: Wed, 9 Oct 2024 15:11:06 +0200 Subject: [PATCH 2/4] Run risky code in finally block catch blocks are not supposed to fail. If you want to do something despite an exception happening, you should do it in a finally block. Closes #7545 --- src/EntityManager.php | 29 ++++++++++++-------- src/UnitOfWork.php | 19 ++++++++------ tests/Tests/ORM/EntityManagerTest.php | 34 ++++++++++++++++++++++++ tests/Tests/ORM/UnitOfWorkTest.php | 38 +++++++++++++++++++++++++++ 4 files changed, 101 insertions(+), 19 deletions(-) diff --git a/src/EntityManager.php b/src/EntityManager.php index f7d47d7b12e..da09ed1b77a 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 39ba6b68b7f..97d80862e39 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 c3ad9f559e7..c9b85f6b4f1 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 ee475e729d0..ae6b1d282f3 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 */ From b6137c89110952664cdc0b2e4df7f98716ff3f93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Paris?= Date: Thu, 10 Oct 2024 10:40:01 +0200 Subject: [PATCH 3/4] Add guard clause It maybe happen that the SQL COMMIT statement is successful, but then something goes wrong. In that kind of case, you do not want to attempt a rollback. This was implemented in UnitOfWork::commit(), but for some reason not in the similar EntityManager methods. --- src/EntityManager.php | 8 ++++++-- tests/Tests/ORM/EntityManagerTest.php | 28 +++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/src/EntityManager.php b/src/EntityManager.php index da09ed1b77a..b677c79dc13 100644 --- a/src/EntityManager.php +++ b/src/EntityManager.php @@ -259,7 +259,9 @@ public function transactional($func) } finally { if (! $successful) { $this->close(); - $this->conn->rollBack(); + if ($this->conn->isTransactionActive()) { + $this->conn->rollBack(); + } } } } @@ -285,7 +287,9 @@ public function wrapInTransaction(callable $func) } finally { if (! $successful) { $this->close(); - $this->conn->rollBack(); + if ($this->conn->isTransactionActive()) { + $this->conn->rollBack(); + } } } } diff --git a/tests/Tests/ORM/EntityManagerTest.php b/tests/Tests/ORM/EntityManagerTest.php index c9b85f6b4f1..88ff5ed924f 100644 --- a/tests/Tests/ORM/EntityManagerTest.php +++ b/tests/Tests/ORM/EntityManagerTest.php @@ -29,6 +29,7 @@ use Exception; use Generator; use InvalidArgumentException; +use PHPUnit\Framework\Assert; use stdClass; use TypeError; @@ -409,6 +410,33 @@ public function rollBack(): bool } } + /** @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 { From bac1c17eabef3aad449197a442380f024f01be7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Paris?= Date: Thu, 10 Oct 2024 11:05:30 +0200 Subject: [PATCH 4/4] Remove submodule remnant This should make a warning we have in the CI go away. > fatal: No url found for submodule path 'docs/en/_theme' in .gitmodules --- docs/en/_theme | 1 - 1 file changed, 1 deletion(-) delete mode 160000 docs/en/_theme diff --git a/docs/en/_theme b/docs/en/_theme deleted file mode 160000 index 6f1bc8bead1..00000000000 --- a/docs/en/_theme +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 6f1bc8bead17b8032389659c0b071d00f2c58328