From 16a8f10fd2fca952b08a656e909dd0fa0a86baed Mon Sep 17 00:00:00 2001 From: Matthias Pigulla Date: Wed, 9 Oct 2024 15:37:04 +0200 Subject: [PATCH 1/7] 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/7] 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/7] 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/7] 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 From bea454eefc4bd341e1550b0befd6052db2d175fe Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Thu, 10 Oct 2024 13:54:34 +0200 Subject: [PATCH 5/7] [GH-8471] undeprecate partials completly (#11647) * [GH-8471] Undeprecate all PARTIAL object usage. --- UPGRADE.md | 12 +++++++++--- src/Query/Parser.php | 8 -------- src/UnitOfWork.php | 10 ---------- 3 files changed, 9 insertions(+), 21 deletions(-) diff --git a/UPGRADE.md b/UPGRADE.md index a42be6a4378..a44bbd5a35b 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -8,10 +8,16 @@ change in behavior. Progress on this is tracked at https://github.com/doctrine/orm/issues/11624 . -## PARTIAL DQL syntax is undeprecated for non-object hydration +## PARTIAL DQL syntax is undeprecated -Use of the PARTIAL keyword is not deprecated anymore in DQL when used with a hydrator -that is not creating entities, such as the ArrayHydrator. +Use of the PARTIAL keyword is not deprecated anymore in DQL, because we will be +able to support PARTIAL objects with PHP 8.4 Lazy Objects and +Symfony/VarExporter in a better way. When we decided to remove this feature +these two abstractions did not exist yet. + +WARNING: If you want to upgrade to 3.x and still use PARTIAL keyword in DQL +with array or object hydrators, then you have to directly migrate to ORM 3.3.x or higher. +PARTIAL keyword in DQL is not available in 3.0, 3.1 and 3.2 of ORM. ## Deprecate `\Doctrine\ORM\Query\Parser::setCustomOutputTreeWalker()` diff --git a/src/Query/Parser.php b/src/Query/Parser.php index 2a95297efd9..388cfcc8e47 100644 --- a/src/Query/Parser.php +++ b/src/Query/Parser.php @@ -1850,14 +1850,6 @@ public function JoinAssociationDeclaration() */ public function PartialObjectExpression() { - if ($this->query->getHydrationMode() === Query::HYDRATE_OBJECT) { - Deprecation::trigger( - 'doctrine/orm', - 'https://github.com/doctrine/orm/issues/8471', - 'PARTIAL syntax in DQL is deprecated for object hydration.' - ); - } - $this->match(TokenType::T_PARTIAL); $partialFieldSet = []; diff --git a/src/UnitOfWork.php b/src/UnitOfWork.php index 46c2893dcc3..2969a3e0e0f 100644 --- a/src/UnitOfWork.php +++ b/src/UnitOfWork.php @@ -41,7 +41,6 @@ use Doctrine\ORM\Persisters\Entity\JoinedSubclassPersister; use Doctrine\ORM\Persisters\Entity\SingleTablePersister; use Doctrine\ORM\Proxy\InternalProxy; -use Doctrine\ORM\Query\SqlWalker; use Doctrine\ORM\Utility\IdentifierFlattener; use Doctrine\Persistence\Mapping\RuntimeReflectionService; use Doctrine\Persistence\NotifyPropertyChanged; @@ -2920,15 +2919,6 @@ private function newInstance(ClassMetadata $class) */ public function createEntity($className, array $data, &$hints = []) { - if (isset($hints[SqlWalker::HINT_PARTIAL])) { - Deprecation::trigger( - 'doctrine/orm', - 'https://github.com/doctrine/orm/issues/8471', - 'Partial Objects are deprecated for object hydration (here entity %s)', - $className - ); - } - $class = $this->em->getClassMetadata($className); $id = $this->identifierFlattener->flattenIdentifier($class, $data); From 39d2136f466187a8d1169c1fc307ebd8244a0e1a Mon Sep 17 00:00:00 2001 From: Matthias Pigulla Date: Thu, 10 Oct 2024 15:15:08 +0200 Subject: [PATCH 6/7] Fix different first/max result values taking up query cache space (#11188) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add a test covering the #11112 issue * Add new OutputWalker and SqlFinalizer interfaces * Add a SingleSelectSqlFinalizer that can take care of adding offset/limit as well as locking mode statements to a given SQL query. Add a FinalizedSelectExecutor that executes given, finalized SQL statements. * In SqlWalker, split SQL query generation into the two parts that shall happen before and after the finalization phase. Move the part that generates "pre-finalization" SQL into a dedicated method. Use a side channel in SingleSelectSqlFinalizer to access the "finalization" logic and avoid duplication. * Fix CS violations * Skip the GH11112 test while applying refactorings * Avoid a Psalm complaint due to invalid (?) docblock syntax * Establish alternate code path - queries can obtain the sql executor through the finalizer, parser knows about output walkers yielding finalizers * Remove a possibly premature comment * Re-enable the #11112 test * Fix CS * Make RootTypeWalker inherit from SqlOutputWalker so it becomes finalizer-aware * Update QueryCacheTest, since first/max results no longer need extra cache entries * Fix ParserResultSerializationTest by forcing the parser to produce a ParserResult of the old kind (with the executor already constructed) * Fix WhereInWalkerTest * Update lib/Doctrine/ORM/Query/Exec/PreparedExecutorFinalizer.php Co-authored-by: Grégoire Paris * Fix tests * Fix a Psalm complaint * Fix a test * Fix CS * Make the NullSqlWalker an instance of SqlOutputWalker * Avoid multiple cache entries caused by LimitSubqueryOutputWalker * Fix Psalm complaints * Fix static analysis complaints * Remove experimental code that I committed accidentally * Remove unnecessary baseline entry * Make AddUnknownQueryComponentWalker subclass SqlOutputWalker That way, we have no remaining classes in the codebase subclassing SqlWalker but not SqlOutputWalker * Use more expressive exception classes * Add a deprecation message * Move SqlExecutor creation to ParserResult, to minimize public methods available on it * Avoid keeping the SqlExecutor in the Query, since it must be generated just in time (e. g. in case Query parameters change) * Address PHPStan complaints * Fix tests * Small refactorings * Add an upgrade notice * Small refactorings * Update the Psalm baseline * Add a missing namespace import * Update Psalm baseline * Fix CS * Fix Psalm baseline --------- Co-authored-by: Grégoire Paris --- UPGRADE.md | 9 ++ psalm-baseline.xml | 14 +-- src/Query.php | 34 +++++++- src/Query/Exec/FinalizedSelectExecutor.php | 33 +++++++ src/Query/Exec/PreparedExecutorFinalizer.php | 28 ++++++ src/Query/Exec/SingleSelectSqlFinalizer.php | 64 ++++++++++++++ src/Query/Exec/SqlFinalizer.php | 26 ++++++ src/Query/OutputWalker.php | 28 ++++++ src/Query/Parser.php | 22 ++++- src/Query/ParserResult.php | 37 +++++++- src/Query/SqlOutputWalker.php | 29 +++++++ src/Query/SqlWalker.php | 85 ++++++++++--------- src/Tools/Pagination/CountOutputWalker.php | 11 +-- .../Pagination/LimitSubqueryOutputWalker.php | 71 ++++++++++++---- src/Tools/Pagination/RootTypeWalker.php | 16 +++- tests/Tests/Mocks/NullSqlWalker.php | 20 +++-- .../ParserResultSerializationTest.php | 38 +++++++-- tests/Tests/ORM/Functional/QueryCacheTest.php | 8 +- .../ORM/Functional/Ticket/GH11112Test.php | 79 +++++++++++++++++ .../Tests/ORM/Query/CustomTreeWalkersTest.php | 9 +- .../LimitSubqueryOutputWalkerTest.php | 23 +++++ .../ORM/Tools/Pagination/PaginatorTest.php | 6 +- .../Tools/Pagination/WhereInWalkerTest.php | 5 +- 23 files changed, 591 insertions(+), 104 deletions(-) create mode 100644 src/Query/Exec/FinalizedSelectExecutor.php create mode 100644 src/Query/Exec/PreparedExecutorFinalizer.php create mode 100644 src/Query/Exec/SingleSelectSqlFinalizer.php create mode 100644 src/Query/Exec/SqlFinalizer.php create mode 100644 src/Query/OutputWalker.php create mode 100644 src/Query/SqlOutputWalker.php create mode 100644 tests/Tests/ORM/Functional/Ticket/GH11112Test.php diff --git a/UPGRADE.md b/UPGRADE.md index a44bbd5a35b..9de8af04d38 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -1,5 +1,14 @@ # Upgrade to 2.20 +## Add `Doctrine\ORM\Query\OutputWalker` interface, deprecate `Doctrine\ORM\Query\SqlWalker::getExecutor()` + +Output walkers should implement the new `\Doctrine\ORM\Query\OutputWalker` interface and create +`Doctrine\ORM\Query\Exec\SqlFinalizer` instances instead of `Doctrine\ORM\Query\Exec\AbstractSqlExecutor`s. +The output walker must not base its workings on the query `firstResult`/`maxResult` values, so that the +`SqlFinalizer` can be kept in the query cache and used regardless of the actual `firstResult`/`maxResult` values. +Any operation dependent on `firstResult`/`maxResult` should take place within the `SqlFinalizer::createExecutor()` +method. Details can be found at https://github.com/doctrine/orm/pull/11188. + ## Explictly forbid property hooks Property hooks are not supported yet by Doctrine ORM. Until support is added, diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 2d96702abe2..28ed8aab049 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -1868,6 +1868,12 @@ _sqlStatements = &$this->sqlStatements]]> + + + + + + @@ -1996,6 +2002,9 @@ + + + @@ -2057,11 +2066,6 @@ - - - - - parameters)]]> diff --git a/src/Query.php b/src/Query.php index 2654e04b0cc..48464e52631 100644 --- a/src/Query.php +++ b/src/Query.php @@ -18,6 +18,8 @@ use Doctrine\ORM\Query\AST\SelectStatement; use Doctrine\ORM\Query\AST\UpdateStatement; use Doctrine\ORM\Query\Exec\AbstractSqlExecutor; +use Doctrine\ORM\Query\Exec\SqlFinalizer; +use Doctrine\ORM\Query\OutputWalker; use Doctrine\ORM\Query\Parameter; use Doctrine\ORM\Query\ParameterTypeInferer; use Doctrine\ORM\Query\Parser; @@ -33,6 +35,7 @@ use function count; use function get_debug_type; use function in_array; +use function is_a; use function is_int; use function ksort; use function md5; @@ -196,7 +199,7 @@ class Query extends AbstractQuery */ public function getSQL() { - return $this->parse()->getSqlExecutor()->getSqlStatements(); + return $this->getSqlExecutor()->getSqlStatements(); } /** @@ -285,7 +288,7 @@ private function parse(): ParserResult */ protected function _doExecute() { - $executor = $this->parse()->getSqlExecutor(); + $executor = $this->getSqlExecutor(); if ($this->_queryCacheProfile) { $executor->setQueryCacheProfile($this->_queryCacheProfile); @@ -813,11 +816,31 @@ protected function getQueryCacheId(): string { ksort($this->_hints); + if (! $this->hasHint(self::HINT_CUSTOM_OUTPUT_WALKER)) { + // Assume Parser will create the SqlOutputWalker; save is_a call, which might trigger a class load + $firstAndMaxResult = ''; + } else { + $outputWalkerClass = $this->getHint(self::HINT_CUSTOM_OUTPUT_WALKER); + if (is_a($outputWalkerClass, OutputWalker::class, true)) { + $firstAndMaxResult = ''; + } else { + Deprecation::trigger( + 'doctrine/orm', + 'https://github.com/doctrine/orm/pull/11188/', + 'Your output walker class %s should implement %s in order to provide a %s. This also means the output walker should not use the query firstResult/maxResult values, which should be read from the query by the SqlFinalizer only.', + $outputWalkerClass, + OutputWalker::class, + SqlFinalizer::class + ); + $firstAndMaxResult = '&firstResult=' . $this->firstResult . '&maxResult=' . $this->maxResults; + } + } + return md5( $this->getDQL() . serialize($this->_hints) . '&platform=' . get_debug_type($this->getEntityManager()->getConnection()->getDatabasePlatform()) . ($this->_em->hasFilters() ? $this->_em->getFilters()->getHash() : '') . - '&firstResult=' . $this->firstResult . '&maxResult=' . $this->maxResults . + $firstAndMaxResult . '&hydrationMode=' . $this->_hydrationMode . '&types=' . serialize($this->parsedTypes) . 'DOCTRINE_QUERY_CACHE_SALT' ); } @@ -836,4 +859,9 @@ public function __clone() $this->state = self::STATE_DIRTY; } + + private function getSqlExecutor(): AbstractSqlExecutor + { + return $this->parse()->prepareSqlExecutor($this); + } } diff --git a/src/Query/Exec/FinalizedSelectExecutor.php b/src/Query/Exec/FinalizedSelectExecutor.php new file mode 100644 index 00000000000..a97e6f428c5 --- /dev/null +++ b/src/Query/Exec/FinalizedSelectExecutor.php @@ -0,0 +1,33 @@ +sqlStatements = $sql; + } + + /** + * @param list|array $params + * @param array|array $types + */ + public function execute(Connection $conn, array $params, array $types): Result + { + return $conn->executeQuery($this->getSqlStatements(), $params, $types, $this->queryCacheProfile); + } +} diff --git a/src/Query/Exec/PreparedExecutorFinalizer.php b/src/Query/Exec/PreparedExecutorFinalizer.php new file mode 100644 index 00000000000..b89b124e352 --- /dev/null +++ b/src/Query/Exec/PreparedExecutorFinalizer.php @@ -0,0 +1,28 @@ +executor = $exeutor; + } + + public function createExecutor(Query $query): AbstractSqlExecutor + { + return $this->executor; + } +} diff --git a/src/Query/Exec/SingleSelectSqlFinalizer.php b/src/Query/Exec/SingleSelectSqlFinalizer.php new file mode 100644 index 00000000000..5180be9e7b2 --- /dev/null +++ b/src/Query/Exec/SingleSelectSqlFinalizer.php @@ -0,0 +1,64 @@ +sql = $sql; + } + + /** + * This method exists temporarily to support old SqlWalker interfaces. + * + * @internal + * + * @psalm-internal Doctrine\ORM + */ + public function finalizeSql(Query $query): string + { + $platform = $query->getEntityManager()->getConnection()->getDatabasePlatform(); + + $sql = $platform->modifyLimitQuery($this->sql, $query->getMaxResults(), $query->getFirstResult()); + + $lockMode = $query->getHint(Query::HINT_LOCK_MODE) ?: LockMode::NONE; + + if ($lockMode !== LockMode::NONE && $lockMode !== LockMode::OPTIMISTIC && $lockMode !== LockMode::PESSIMISTIC_READ && $lockMode !== LockMode::PESSIMISTIC_WRITE) { + throw QueryException::invalidLockMode(); + } + + if ($lockMode === LockMode::PESSIMISTIC_READ) { + $sql .= ' ' . $this->getReadLockSQL($platform); + } elseif ($lockMode === LockMode::PESSIMISTIC_WRITE) { + $sql .= ' ' . $this->getWriteLockSQL($platform); + } + + return $sql; + } + + /** @return FinalizedSelectExecutor */ + public function createExecutor(Query $query): AbstractSqlExecutor + { + return new FinalizedSelectExecutor($this->finalizeSql($query)); + } +} diff --git a/src/Query/Exec/SqlFinalizer.php b/src/Query/Exec/SqlFinalizer.php new file mode 100644 index 00000000000..cddad84e8a3 --- /dev/null +++ b/src/Query/Exec/SqlFinalizer.php @@ -0,0 +1,26 @@ +queryComponents = $treeWalkerChain->getQueryComponents(); } - $outputWalkerClass = $this->customOutputWalker ?: SqlWalker::class; + $outputWalkerClass = $this->customOutputWalker ?: SqlOutputWalker::class; $outputWalker = new $outputWalkerClass($this->query, $this->parserResult, $this->queryComponents); - // Assign an SQL executor to the parser result - $this->parserResult->setSqlExecutor($outputWalker->getExecutor($AST)); + if ($outputWalker instanceof OutputWalker) { + $finalizer = $outputWalker->getFinalizer($AST); + $this->parserResult->setSqlFinalizer($finalizer); + } else { + Deprecation::trigger( + 'doctrine/orm', + 'https://github.com/doctrine/orm/pull/11188/', + 'Your output walker class %s should implement %s in order to provide a %s. This also means the output walker should not use the query firstResult/maxResult values, which should be read from the query by the SqlFinalizer only.', + $outputWalkerClass, + OutputWalker::class, + SqlFinalizer::class + ); + // @phpstan-ignore method.deprecated + $executor = $outputWalker->getExecutor($AST); + // @phpstan-ignore method.deprecated + $this->parserResult->setSqlExecutor($executor); + } return $this->parserResult; } diff --git a/src/Query/ParserResult.php b/src/Query/ParserResult.php index e5d36c9bbe3..86a4e4d3b64 100644 --- a/src/Query/ParserResult.php +++ b/src/Query/ParserResult.php @@ -4,7 +4,10 @@ namespace Doctrine\ORM\Query; +use Doctrine\ORM\Query; use Doctrine\ORM\Query\Exec\AbstractSqlExecutor; +use Doctrine\ORM\Query\Exec\SqlFinalizer; +use LogicException; use function sprintf; @@ -20,15 +23,23 @@ class ParserResult 'sqlExecutor' => '_sqlExecutor', 'resultSetMapping' => '_resultSetMapping', 'parameterMappings' => '_parameterMappings', + 'sqlFinalizer' => 'sqlFinalizer', ]; /** * The SQL executor used for executing the SQL. * - * @var AbstractSqlExecutor + * @var ?AbstractSqlExecutor */ private $sqlExecutor; + /** + * The SQL executor used for executing the SQL. + * + * @var ?SqlFinalizer + */ + private $sqlFinalizer; + /** * The ResultSetMapping that describes how to map the SQL result set. * @@ -75,6 +86,8 @@ public function setResultSetMapping(ResultSetMapping $rsm) /** * Sets the SQL executor that should be used for this ParserResult. * + * @deprecated + * * @param AbstractSqlExecutor $executor * * @return void @@ -87,13 +100,33 @@ public function setSqlExecutor($executor) /** * Gets the SQL executor used by this ParserResult. * - * @return AbstractSqlExecutor + * @deprecated + * + * @return ?AbstractSqlExecutor */ public function getSqlExecutor() { return $this->sqlExecutor; } + public function setSqlFinalizer(SqlFinalizer $finalizer): void + { + $this->sqlFinalizer = $finalizer; + } + + public function prepareSqlExecutor(Query $query): AbstractSqlExecutor + { + if ($this->sqlFinalizer !== null) { + return $this->sqlFinalizer->createExecutor($query); + } + + if ($this->sqlExecutor !== null) { + return $this->sqlExecutor; + } + + throw new LogicException('This ParserResult lacks both the SqlFinalizer as well as the (legacy) SqlExecutor'); + } + /** * Adds a DQL to SQL parameter mapping. One DQL parameter name/position can map to * several SQL parameter positions. diff --git a/src/Query/SqlOutputWalker.php b/src/Query/SqlOutputWalker.php new file mode 100644 index 00000000000..e737e1c9c53 --- /dev/null +++ b/src/Query/SqlOutputWalker.php @@ -0,0 +1,29 @@ +createSqlForFinalizer($AST)); + + case $AST instanceof AST\UpdateStatement: + return new PreparedExecutorFinalizer($this->createUpdateStatementExecutor($AST)); + + case $AST instanceof AST\DeleteStatement: + return new PreparedExecutorFinalizer($this->createDeleteStatementExecutor($AST)); + } + + throw new LogicException('Unexpected AST node type'); + } +} diff --git a/src/Query/SqlWalker.php b/src/Query/SqlWalker.php index 4c25fb63a68..e4e2de5c5c1 100644 --- a/src/Query/SqlWalker.php +++ b/src/Query/SqlWalker.php @@ -16,7 +16,6 @@ use Doctrine\ORM\OptimisticLockException; use Doctrine\ORM\Query; use Doctrine\ORM\Utility\HierarchyDiscriminatorResolver; -use Doctrine\ORM\Utility\LockSqlHelper; use Doctrine\ORM\Utility\PersisterHelper; use InvalidArgumentException; use LogicException; @@ -49,8 +48,6 @@ */ class SqlWalker implements TreeWalker { - use LockSqlHelper; - public const HINT_DISTINCT = 'doctrine.distinct'; /** @@ -278,34 +275,48 @@ public function setQueryComponent($dqlAlias, array $queryComponent) /** * Gets an executor that can be used to execute the result of this walker. * + * @deprecated Output walkers should no longer create the executor directly, but instead provide + * a SqlFinalizer by implementing the `OutputWalker` interface. Thus, this method is + * no longer needed and will be removed in 4.0. + * * @param AST\DeleteStatement|AST\UpdateStatement|AST\SelectStatement $AST * * @return Exec\AbstractSqlExecutor - * - * @not-deprecated */ public function getExecutor($AST) { switch (true) { case $AST instanceof AST\DeleteStatement: - $primaryClass = $this->em->getClassMetadata($AST->deleteClause->abstractSchemaName); - - return $primaryClass->isInheritanceTypeJoined() - ? new Exec\MultiTableDeleteExecutor($AST, $this) - : new Exec\SingleTableDeleteUpdateExecutor($AST, $this); + return $this->createDeleteStatementExecutor($AST); case $AST instanceof AST\UpdateStatement: - $primaryClass = $this->em->getClassMetadata($AST->updateClause->abstractSchemaName); - - return $primaryClass->isInheritanceTypeJoined() - ? new Exec\MultiTableUpdateExecutor($AST, $this) - : new Exec\SingleTableDeleteUpdateExecutor($AST, $this); + return $this->createUpdateStatementExecutor($AST); default: return new Exec\SingleSelectExecutor($AST, $this); } } + /** @psalm-internal Doctrine\ORM */ + protected function createUpdateStatementExecutor(AST\UpdateStatement $AST): Exec\AbstractSqlExecutor + { + $primaryClass = $this->em->getClassMetadata($AST->updateClause->abstractSchemaName); + + return $primaryClass->isInheritanceTypeJoined() + ? new Exec\MultiTableUpdateExecutor($AST, $this) + : new Exec\SingleTableDeleteUpdateExecutor($AST, $this); + } + + /** @psalm-internal Doctrine\ORM */ + protected function createDeleteStatementExecutor(AST\DeleteStatement $AST): Exec\AbstractSqlExecutor + { + $primaryClass = $this->em->getClassMetadata($AST->deleteClause->abstractSchemaName); + + return $primaryClass->isInheritanceTypeJoined() + ? new Exec\MultiTableDeleteExecutor($AST, $this) + : new Exec\SingleTableDeleteUpdateExecutor($AST, $this); + } + /** * Generates a unique, short SQL table alias. * @@ -561,10 +572,15 @@ private function generateFilterConditionSQL( */ public function walkSelectStatement(AST\SelectStatement $AST) { - $limit = $this->query->getMaxResults(); - $offset = $this->query->getFirstResult(); - $lockMode = $this->query->getHint(Query::HINT_LOCK_MODE) ?: LockMode::NONE; - $sql = $this->walkSelectClause($AST->selectClause) + $sql = $this->createSqlForFinalizer($AST); + $finalizer = new Exec\SingleSelectSqlFinalizer($sql); + + return $finalizer->finalizeSql($this->query); + } + + protected function createSqlForFinalizer(AST\SelectStatement $AST): string + { + $sql = $this->walkSelectClause($AST->selectClause) . $this->walkFromClause($AST->fromClause) . $this->walkWhereClause($AST->whereClause); @@ -585,31 +601,22 @@ public function walkSelectStatement(AST\SelectStatement $AST) $sql .= ' ORDER BY ' . $orderBySql; } - $sql = $this->platform->modifyLimitQuery($sql, $limit, $offset); - - if ($lockMode === LockMode::NONE) { - return $sql; - } + $this->assertOptimisticLockingHasAllClassesVersioned(); - if ($lockMode === LockMode::PESSIMISTIC_READ) { - return $sql . ' ' . $this->getReadLockSQL($this->platform); - } - - if ($lockMode === LockMode::PESSIMISTIC_WRITE) { - return $sql . ' ' . $this->getWriteLockSQL($this->platform); - } + return $sql; + } - if ($lockMode !== LockMode::OPTIMISTIC) { - throw QueryException::invalidLockMode(); - } + private function assertOptimisticLockingHasAllClassesVersioned(): void + { + $lockMode = $this->query->getHint(Query::HINT_LOCK_MODE) ?: LockMode::NONE; - foreach ($this->selectedClasses as $selectedClass) { - if (! $selectedClass['class']->isVersioned) { - throw OptimisticLockException::lockFailed($selectedClass['class']->name); + if ($lockMode === LockMode::OPTIMISTIC) { + foreach ($this->selectedClasses as $selectedClass) { + if (! $selectedClass['class']->isVersioned) { + throw OptimisticLockException::lockFailed($selectedClass['class']->name); + } } } - - return $sql; } /** diff --git a/src/Tools/Pagination/CountOutputWalker.php b/src/Tools/Pagination/CountOutputWalker.php index 0929e2ab065..92615f8aab5 100644 --- a/src/Tools/Pagination/CountOutputWalker.php +++ b/src/Tools/Pagination/CountOutputWalker.php @@ -11,7 +11,7 @@ use Doctrine\ORM\Query\Parser; use Doctrine\ORM\Query\ParserResult; use Doctrine\ORM\Query\ResultSetMapping; -use Doctrine\ORM\Query\SqlWalker; +use Doctrine\ORM\Query\SqlOutputWalker; use RuntimeException; use function array_diff; @@ -36,7 +36,7 @@ * * @psalm-import-type QueryComponent from Parser */ -class CountOutputWalker extends SqlWalker +class CountOutputWalker extends SqlOutputWalker { /** @var AbstractPlatform */ private $platform; @@ -62,16 +62,13 @@ public function __construct($query, $parserResult, array $queryComponents) parent::__construct($query, $parserResult, $queryComponents); } - /** - * {@inheritDoc} - */ - public function walkSelectStatement(SelectStatement $AST) + protected function createSqlForFinalizer(SelectStatement $AST): string { if ($this->platform instanceof SQLServerPlatform) { $AST->orderByClause = null; } - $sql = parent::walkSelectStatement($AST); + $sql = parent::createSqlForFinalizer($AST); if ($AST->groupByClause) { return sprintf( diff --git a/src/Tools/Pagination/LimitSubqueryOutputWalker.php b/src/Tools/Pagination/LimitSubqueryOutputWalker.php index f92c1145d4f..0e858ce0ecb 100644 --- a/src/Tools/Pagination/LimitSubqueryOutputWalker.php +++ b/src/Tools/Pagination/LimitSubqueryOutputWalker.php @@ -14,15 +14,20 @@ use Doctrine\ORM\Mapping\QuoteStrategy; use Doctrine\ORM\OptimisticLockException; use Doctrine\ORM\Query; +use Doctrine\ORM\Query\AST\DeleteStatement; use Doctrine\ORM\Query\AST\OrderByClause; use Doctrine\ORM\Query\AST\PathExpression; use Doctrine\ORM\Query\AST\SelectExpression; use Doctrine\ORM\Query\AST\SelectStatement; +use Doctrine\ORM\Query\AST\UpdateStatement; +use Doctrine\ORM\Query\Exec\SingleSelectSqlFinalizer; +use Doctrine\ORM\Query\Exec\SqlFinalizer; use Doctrine\ORM\Query\Parser; use Doctrine\ORM\Query\ParserResult; use Doctrine\ORM\Query\QueryException; use Doctrine\ORM\Query\ResultSetMapping; -use Doctrine\ORM\Query\SqlWalker; +use Doctrine\ORM\Query\SqlOutputWalker; +use LogicException; use RuntimeException; use function array_diff; @@ -50,7 +55,7 @@ * * @psalm-import-type QueryComponent from Parser */ -class LimitSubqueryOutputWalker extends SqlWalker +class LimitSubqueryOutputWalker extends SqlOutputWalker { private const ORDER_BY_PATH_EXPRESSION = '/(?platform = $query->getEntityManager()->getConnection()->getDatabasePlatform(); $this->rsm = $parserResult->getResultSetMapping(); + $query = clone $query; + // Reset limit and offset $this->firstResult = $query->getFirstResult(); $this->maxResults = $query->getMaxResults(); @@ -158,11 +165,33 @@ private function rebuildOrderByForRowNumber(SelectStatement $AST): void */ public function walkSelectStatement(SelectStatement $AST) { + $sqlFinalizer = $this->getFinalizer($AST); + + $query = $this->getQuery(); + + $abstractSqlExecutor = $sqlFinalizer->createExecutor($query); + + return $abstractSqlExecutor->getSqlStatements(); + } + + /** + * @param DeleteStatement|UpdateStatement|SelectStatement $AST + * + * @return SingleSelectSqlFinalizer + */ + public function getFinalizer($AST): SqlFinalizer + { + if (! $AST instanceof SelectStatement) { + throw new LogicException(self::class . ' is to be used on SelectStatements only'); + } + if ($this->platformSupportsRowNumber()) { - return $this->walkSelectStatementWithRowNumber($AST); + $sql = $this->createSqlWithRowNumber($AST); + } else { + $sql = $this->createSqlWithoutRowNumber($AST); } - return $this->walkSelectStatementWithoutRowNumber($AST); + return new SingleSelectSqlFinalizer($sql); } /** @@ -174,6 +203,16 @@ public function walkSelectStatement(SelectStatement $AST) * @throws RuntimeException */ public function walkSelectStatementWithRowNumber(SelectStatement $AST) + { + // Apply the limit and offset. + return $this->platform->modifyLimitQuery( + $this->createSqlWithRowNumber($AST), + $this->maxResults, + $this->firstResult + ); + } + + private function createSqlWithRowNumber(SelectStatement $AST): string { $hasOrderBy = false; $outerOrderBy = ' ORDER BY dctrn_minrownum ASC'; @@ -203,13 +242,6 @@ public function walkSelectStatementWithRowNumber(SelectStatement $AST) $sql .= $orderGroupBy . $outerOrderBy; } - // Apply the limit and offset. - $sql = $this->platform->modifyLimitQuery( - $sql, - $this->maxResults, - $this->firstResult - ); - // Add the columns to the ResultSetMapping. It's not really nice but // it works. Preferably I'd clear the RSM or simply create a new one // but that is not possible from inside the output walker, so we dirty @@ -232,6 +264,16 @@ public function walkSelectStatementWithRowNumber(SelectStatement $AST) * @throws RuntimeException */ public function walkSelectStatementWithoutRowNumber(SelectStatement $AST, $addMissingItemsFromOrderByToSelect = true) + { + // Apply the limit and offset. + return $this->platform->modifyLimitQuery( + $this->createSqlWithoutRowNumber($AST, $addMissingItemsFromOrderByToSelect), + $this->maxResults, + $this->firstResult + ); + } + + private function createSqlWithoutRowNumber(SelectStatement $AST, bool $addMissingItemsFromOrderByToSelect = true): string { // We don't want to call this recursively! if ($AST->orderByClause instanceof OrderByClause && $addMissingItemsFromOrderByToSelect) { @@ -260,13 +302,6 @@ public function walkSelectStatementWithoutRowNumber(SelectStatement $AST, $addMi // https://github.com/doctrine/orm/issues/2630 $sql = $this->preserveSqlOrdering($sqlIdentifier, $innerSql, $sql, $orderByClause); - // Apply the limit and offset. - $sql = $this->platform->modifyLimitQuery( - $sql, - $this->maxResults, - $this->firstResult - ); - // Add the columns to the ResultSetMapping. It's not really nice but // it works. Preferably I'd clear the RSM or simply create a new one // but that is not possible from inside the output walker, so we dirty diff --git a/src/Tools/Pagination/RootTypeWalker.php b/src/Tools/Pagination/RootTypeWalker.php index dc1d77a51c7..ec45dbd6fc1 100644 --- a/src/Tools/Pagination/RootTypeWalker.php +++ b/src/Tools/Pagination/RootTypeWalker.php @@ -5,7 +5,10 @@ namespace Doctrine\ORM\Tools\Pagination; use Doctrine\ORM\Query\AST; -use Doctrine\ORM\Query\SqlWalker; +use Doctrine\ORM\Query\Exec\FinalizedSelectExecutor; +use Doctrine\ORM\Query\Exec\PreparedExecutorFinalizer; +use Doctrine\ORM\Query\Exec\SqlFinalizer; +use Doctrine\ORM\Query\SqlOutputWalker; use Doctrine\ORM\Utility\PersisterHelper; use RuntimeException; @@ -22,7 +25,7 @@ * Returning the type instead of a "real" SQL statement is a slight hack. However, it has the * benefit that the DQL -> root entity id type resolution can be cached in the query cache. */ -final class RootTypeWalker extends SqlWalker +final class RootTypeWalker extends SqlOutputWalker { public function walkSelectStatement(AST\SelectStatement $AST): string { @@ -45,4 +48,13 @@ public function walkSelectStatement(AST\SelectStatement $AST): string ->getEntityManager() )[0]; } + + public function getFinalizer($AST): SqlFinalizer + { + if (! $AST instanceof AST\SelectStatement) { + throw new RuntimeException(self::class . ' is to be used on SelectStatements only'); + } + + return new PreparedExecutorFinalizer(new FinalizedSelectExecutor($this->walkSelectStatement($AST))); + } } diff --git a/tests/Tests/Mocks/NullSqlWalker.php b/tests/Tests/Mocks/NullSqlWalker.php index 9d54e703547..90cf0c6e926 100644 --- a/tests/Tests/Mocks/NullSqlWalker.php +++ b/tests/Tests/Mocks/NullSqlWalker.php @@ -7,12 +7,14 @@ use Doctrine\DBAL\Connection; use Doctrine\ORM\Query\AST; use Doctrine\ORM\Query\Exec\AbstractSqlExecutor; -use Doctrine\ORM\Query\SqlWalker; +use Doctrine\ORM\Query\Exec\PreparedExecutorFinalizer; +use Doctrine\ORM\Query\Exec\SqlFinalizer; +use Doctrine\ORM\Query\SqlOutputWalker; /** * SqlWalker implementation that does not produce SQL. */ -class NullSqlWalker extends SqlWalker +class NullSqlWalker extends SqlOutputWalker { public function walkSelectStatement(AST\SelectStatement $AST): string { @@ -29,13 +31,15 @@ public function walkDeleteStatement(AST\DeleteStatement $AST): string return ''; } - public function getExecutor($AST): AbstractSqlExecutor + public function getFinalizer($AST): SqlFinalizer { - return new class extends AbstractSqlExecutor { - public function execute(Connection $conn, array $params, array $types): int - { - return 0; + return new PreparedExecutorFinalizer( + new class extends AbstractSqlExecutor { + public function execute(Connection $conn, array $params, array $types): int + { + return 0; + } } - }; + ); } } diff --git a/tests/Tests/ORM/Functional/ParserResultSerializationTest.php b/tests/Tests/ORM/Functional/ParserResultSerializationTest.php index 0841c5ff3a0..7b2ae59119a 100644 --- a/tests/Tests/ORM/Functional/ParserResultSerializationTest.php +++ b/tests/Tests/ORM/Functional/ParserResultSerializationTest.php @@ -6,6 +6,8 @@ use Closure; use Doctrine\ORM\Query; +use Doctrine\ORM\Query\Exec\FinalizedSelectExecutor; +use Doctrine\ORM\Query\Exec\PreparedExecutorFinalizer; use Doctrine\ORM\Query\Exec\SingleSelectExecutor; use Doctrine\ORM\Query\ParserResult; use Doctrine\ORM\Query\ResultSetMapping; @@ -35,18 +37,40 @@ protected function setUp(): void * * @dataProvider provideToSerializedAndBack */ - public function testSerializeParserResult(Closure $toSerializedAndBack): void + public function testSerializeParserResultForQueryWithSqlWalker(Closure $toSerializedAndBack): void { $query = $this->_em ->createQuery('SELECT u FROM Doctrine\Tests\Models\Company\CompanyEmployee u WHERE u.name = :name'); + // Use the (legacy) SqlWalker which directly puts an SqlExecutor instance into the parser result + $query->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, Query\SqlWalker::class); + $parserResult = self::parseQuery($query); $unserialized = $toSerializedAndBack($parserResult); $this->assertInstanceOf(ParserResult::class, $unserialized); $this->assertInstanceOf(ResultSetMapping::class, $unserialized->getResultSetMapping()); $this->assertEquals(['name' => [0]], $unserialized->getParameterMappings()); - $this->assertInstanceOf(SingleSelectExecutor::class, $unserialized->getSqlExecutor()); + $this->assertNotNull($unserialized->prepareSqlExecutor($query)); + } + + /** + * @param Closure(ParserResult): ParserResult $toSerializedAndBack + * + * @dataProvider provideToSerializedAndBack + */ + public function testSerializeParserResultForQueryWithSqlOutputWalker(Closure $toSerializedAndBack): void + { + $query = $this->_em + ->createQuery('SELECT u FROM Doctrine\Tests\Models\Company\CompanyEmployee u WHERE u.name = :name'); + + $parserResult = self::parseQuery($query); + $unserialized = $toSerializedAndBack($parserResult); + + $this->assertInstanceOf(ParserResult::class, $unserialized); + $this->assertInstanceOf(ResultSetMapping::class, $unserialized->getResultSetMapping()); + $this->assertEquals(['name' => [0]], $unserialized->getParameterMappings()); + $this->assertNotNull($unserialized->prepareSqlExecutor($query)); } /** @return Generator */ @@ -75,6 +99,9 @@ public function testItSerializesParserResultWithAForwardCompatibleFormat(): void $query = $this->_em ->createQuery('SELECT u FROM Doctrine\Tests\Models\Company\CompanyEmployee u WHERE u.name = :name'); + // Use the (legacy) SqlWalker which directly puts an SqlExecutor instance into the parser result + $query->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, Query\SqlWalker::class); + $parserResult = self::parseQuery($query); $serialized = serialize($parserResult); $this->assertStringNotContainsString( @@ -118,11 +145,12 @@ public static function provideSerializedSingleSelectResults(): Generator public function testSymfony44ProvidedData(): void { - $sqlExecutor = $this->createMock(SingleSelectExecutor::class); + $sqlExecutor = new FinalizedSelectExecutor('test'); + $sqlFinalizer = new PreparedExecutorFinalizer($sqlExecutor); $resultSetMapping = $this->createMock(ResultSetMapping::class); $parserResult = new ParserResult(); - $parserResult->setSqlExecutor($sqlExecutor); + $parserResult->setSqlFinalizer($sqlFinalizer); $parserResult->setResultSetMapping($resultSetMapping); $parserResult->addParameterMapping('name', 0); @@ -132,7 +160,7 @@ public function testSymfony44ProvidedData(): void $this->assertInstanceOf(ParserResult::class, $unserialized); $this->assertInstanceOf(ResultSetMapping::class, $unserialized->getResultSetMapping()); $this->assertEquals(['name' => [0]], $unserialized->getParameterMappings()); - $this->assertInstanceOf(SingleSelectExecutor::class, $unserialized->getSqlExecutor()); + $this->assertEquals($sqlExecutor, $unserialized->prepareSqlExecutor($this->createMock(Query::class))); } private static function parseQuery(Query $query): ParserResult diff --git a/tests/Tests/ORM/Functional/QueryCacheTest.php b/tests/Tests/ORM/Functional/QueryCacheTest.php index 4935c3d78ee..3894ecef697 100644 --- a/tests/Tests/ORM/Functional/QueryCacheTest.php +++ b/tests/Tests/ORM/Functional/QueryCacheTest.php @@ -43,7 +43,7 @@ public function testQueryCacheDependsOnHints(): array } /** @depends testQueryCacheDependsOnHints */ - public function testQueryCacheDependsOnFirstResult(array $previous): void + public function testQueryCacheDoesNotDependOnFirstResultForDefaultOutputWalker(array $previous): void { [$query, $cache] = $previous; assert($query instanceof Query); @@ -55,11 +55,11 @@ public function testQueryCacheDependsOnFirstResult(array $previous): void $query->setMaxResults(9999); $query->getResult(); - self::assertCount($cacheCount + 1, $cache->getValues()); + self::assertCount($cacheCount, $cache->getValues()); } /** @depends testQueryCacheDependsOnHints */ - public function testQueryCacheDependsOnMaxResults(array $previous): void + public function testQueryCacheDoesNotDependOnMaxResultsForDefaultOutputWalker(array $previous): void { [$query, $cache] = $previous; assert($query instanceof Query); @@ -70,7 +70,7 @@ public function testQueryCacheDependsOnMaxResults(array $previous): void $query->setMaxResults(10); $query->getResult(); - self::assertCount($cacheCount + 1, $cache->getValues()); + self::assertCount($cacheCount, $cache->getValues()); } /** @depends testQueryCacheDependsOnHints */ diff --git a/tests/Tests/ORM/Functional/Ticket/GH11112Test.php b/tests/Tests/ORM/Functional/Ticket/GH11112Test.php new file mode 100644 index 00000000000..3dbdb533b33 --- /dev/null +++ b/tests/Tests/ORM/Functional/Ticket/GH11112Test.php @@ -0,0 +1,79 @@ +useModelSet('cms'); + self::$queryCache = new ArrayAdapter(); + + parent::setUp(); + } + + public function testSimpleQueryHasLimitAndOffsetApplied(): void + { + $platform = $this->_em->getConnection()->getDatabasePlatform(); + $query = $this->_em->createQuery('SELECT u FROM ' . CmsUser::class . ' u'); + $originalSql = $query->getSQL(); + + $query->setMaxResults(10); + $query->setFirstResult(20); + $sqlMax10First20 = $query->getSQL(); + + $query->setMaxResults(30); + $query->setFirstResult(40); + $sqlMax30First40 = $query->getSQL(); + + // The SQL is platform specific and may even be something with outer SELECTS being added. So, + // derive the expected value at runtime through the platform. + self::assertSame($platform->modifyLimitQuery($originalSql, 10, 20), $sqlMax10First20); + self::assertSame($platform->modifyLimitQuery($originalSql, 30, 40), $sqlMax30First40); + + $cacheEntries = self::$queryCache->getValues(); + self::assertCount(1, $cacheEntries); + } + + public function testSubqueryLimitAndOffsetAreIgnored(): void + { + // Not sure what to do about this test. Basically, I want to make sure that + // firstResult/maxResult for subqueries are not relevant, they do not make it + // into the final query at all. That would give us the guarantee that the + // "sql finalizer" step is sufficient for the final, "outer" query and we + // do not need to run finalizers for the subqueries. + + // This DQL/query makes no sense, it's just about creating a subquery in the first place + $queryBuilder = $this->_em->createQueryBuilder(); + $queryBuilder + ->select('o') + ->from(CmsUser::class, 'o') + ->where($queryBuilder->expr()->exists( + $this->_em->createQueryBuilder() + ->select('u') + ->from(CmsUser::class, 'u') + ->setFirstResult(10) + ->setMaxResults(20) + )); + + $query = $queryBuilder->getQuery(); + $originalSql = $query->getSQL(); + + $clone = clone $query; + $clone->setFirstResult(24); + $clone->setMaxResults(42); + $limitedSql = $clone->getSQL(); + + $platform = $this->_em->getConnection()->getDatabasePlatform(); + + // The SQL is platform specific and may even be something with outer SELECTS being added. So, + // derive the expected value at runtime through the platform. + self::assertSame($platform->modifyLimitQuery($originalSql, 42, 24), $limitedSql); + } +} diff --git a/tests/Tests/ORM/Query/CustomTreeWalkersTest.php b/tests/Tests/ORM/Query/CustomTreeWalkersTest.php index b2267078ff5..eb27b6113a3 100644 --- a/tests/Tests/ORM/Query/CustomTreeWalkersTest.php +++ b/tests/Tests/ORM/Query/CustomTreeWalkersTest.php @@ -6,6 +6,7 @@ use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\Query; +use Doctrine\ORM\Query\AST; use Doctrine\ORM\Query\QueryException; use Doctrine\ORM\Query\SqlWalker; use Doctrine\ORM\Query\TreeWalker; @@ -110,13 +111,13 @@ public function testSupportsSeveralHintsQueries(): void } } -class AddUnknownQueryComponentWalker extends Query\SqlWalker +class AddUnknownQueryComponentWalker extends Query\SqlOutputWalker { - public function walkSelectStatement(Query\AST\SelectStatement $selectStatement): void + protected function createSqlForFinalizer(AST\SelectStatement $AST): string { - parent::walkSelectStatement($selectStatement); - $this->setQueryComponent('x', []); + + return parent::createSqlForFinalizer($AST); } } diff --git a/tests/Tests/ORM/Tools/Pagination/LimitSubqueryOutputWalkerTest.php b/tests/Tests/ORM/Tools/Pagination/LimitSubqueryOutputWalkerTest.php index cb69a3a5674..95d2047d593 100644 --- a/tests/Tests/ORM/Tools/Pagination/LimitSubqueryOutputWalkerTest.php +++ b/tests/Tests/ORM/Tools/Pagination/LimitSubqueryOutputWalkerTest.php @@ -10,6 +10,7 @@ use Doctrine\DBAL\Platforms\PostgreSQLPlatform; use Doctrine\ORM\Query; use Doctrine\ORM\Tools\Pagination\LimitSubqueryOutputWalker; +use Symfony\Component\Cache\Adapter\ArrayAdapter; use function class_exists; @@ -385,6 +386,28 @@ public function testLimitSubqueryOrderBySubSelectOrderByExpressionOracle(): void ); } + public function testParsingQueryWithDifferentLimitOffsetValuesTakesOnlyOneCacheEntry(): void + { + $queryCache = new ArrayAdapter(); + $this->entityManager->getConfiguration()->setQueryCache($queryCache); + + $query = $this->createQuery('SELECT p, c, a FROM Doctrine\Tests\ORM\Tools\Pagination\MyBlogPost p JOIN p.category c JOIN p.author a'); + + self::assertSame( + 'SELECT DISTINCT id_0 FROM (SELECT m0_.id AS id_0, m0_.title AS title_1, c1_.id AS id_2, a2_.id AS id_3, a2_.name AS name_4, m0_.author_id AS author_id_5, m0_.category_id AS category_id_6 FROM MyBlogPost m0_ INNER JOIN Category c1_ ON m0_.category_id = c1_.id INNER JOIN Author a2_ ON m0_.author_id = a2_.id) dctrn_result LIMIT 20 OFFSET 10', + $query->getSQL() + ); + + $query->setFirstResult(30)->setMaxResults(40); + + self::assertSame( + 'SELECT DISTINCT id_0 FROM (SELECT m0_.id AS id_0, m0_.title AS title_1, c1_.id AS id_2, a2_.id AS id_3, a2_.name AS name_4, m0_.author_id AS author_id_5, m0_.category_id AS category_id_6 FROM MyBlogPost m0_ INNER JOIN Category c1_ ON m0_.category_id = c1_.id INNER JOIN Author a2_ ON m0_.author_id = a2_.id) dctrn_result LIMIT 40 OFFSET 30', + $query->getSQL() + ); + + self::assertCount(1, $queryCache->getValues()); + } + private function createQuery(string $dql): Query { $query = $this->entityManager->createQuery($dql); diff --git a/tests/Tests/ORM/Tools/Pagination/PaginatorTest.php b/tests/Tests/ORM/Tools/Pagination/PaginatorTest.php index 1c8b47ac0cf..23ff3b90be4 100644 --- a/tests/Tests/ORM/Tools/Pagination/PaginatorTest.php +++ b/tests/Tests/ORM/Tools/Pagination/PaginatorTest.php @@ -87,7 +87,8 @@ public function testExtraParametersAreStrippedWhenWalkerRemovingOriginalSelectEl public function testPaginatorNotCaringAboutExtraParametersWithoutOutputWalkers(): void { - $this->connection->expects(self::exactly(3))->method('executeQuery'); + $result = $this->getMockBuilder(Result::class)->disableOriginalConstructor()->getMock(); + $this->connection->expects(self::exactly(3))->method('executeQuery')->willReturn($result); $this->createPaginatorWithExtraParametersWithoutOutputWalkers([])->count(); $this->createPaginatorWithExtraParametersWithoutOutputWalkers([[10]])->count(); @@ -96,7 +97,8 @@ public function testPaginatorNotCaringAboutExtraParametersWithoutOutputWalkers() public function testgetIteratorDoesCareAboutExtraParametersWithoutOutputWalkersWhenResultIsNotEmpty(): void { - $this->connection->expects(self::exactly(1))->method('executeQuery'); + $result = $this->getMockBuilder(Result::class)->disableOriginalConstructor()->getMock(); + $this->connection->expects(self::exactly(1))->method('executeQuery')->willReturn($result); $this->expectException(Query\QueryException::class); $this->expectExceptionMessage('Too many parameters: the query defines 1 parameters and you bound 2'); diff --git a/tests/Tests/ORM/Tools/Pagination/WhereInWalkerTest.php b/tests/Tests/ORM/Tools/Pagination/WhereInWalkerTest.php index 1f1710d2cf1..6f5c9c6dd22 100644 --- a/tests/Tests/ORM/Tools/Pagination/WhereInWalkerTest.php +++ b/tests/Tests/ORM/Tools/Pagination/WhereInWalkerTest.php @@ -21,9 +21,10 @@ public function testDqlQueryTransformation(string $dql, string $expectedSql): vo $query->setHint(Query::HINT_CUSTOM_TREE_WALKERS, [WhereInWalker::class]); $query->setHint(WhereInWalker::HINT_PAGINATOR_HAS_IDS, true); - $result = (new Parser($query))->parse(); + $result = (new Parser($query))->parse(); + $executor = $result->prepareSqlExecutor($query); - self::assertEquals($expectedSql, $result->getSqlExecutor()->getSqlStatements()); + self::assertEquals($expectedSql, $executor->getSqlStatements()); self::assertEquals([0], $result->getSqlParameterPositions(WhereInWalker::PAGINATOR_ID_ALIAS)); } From ee0d7197dd2be959bb07bf25dee442ccf9d7aa8c Mon Sep 17 00:00:00 2001 From: Simon Podlipsky Date: Fri, 11 Oct 2024 13:00:52 +0200 Subject: [PATCH 7/7] test: cover all transactional methods in `EntityManagerTest::testItPreservesTheOriginalExceptionOnRollbackFailure()` --- tests/Tests/ORM/EntityManagerTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Tests/ORM/EntityManagerTest.php b/tests/Tests/ORM/EntityManagerTest.php index 88ff5ed924f..202560602cd 100644 --- a/tests/Tests/ORM/EntityManagerTest.php +++ b/tests/Tests/ORM/EntityManagerTest.php @@ -399,7 +399,7 @@ public function rollBack(): bool }); try { - $entityManager->transactional(static function (): void { + $entityManager->$methodName(static function (): void { throw new Exception('Original exception'); }); self::fail('Exception expected');