Do not break proper LIMIT/OFFSET caching; mention limitations

Author: janedbal

README.md

@@ -51,6 +51,11 @@ class MaxExecutionTimeSqlWalker extends HintHandler
 
 SqlNode is an enum of all `walkXxx` methods in Doctrine's SqlWalker, so you are able to intercept any part of AST processing the SqlWalker does.
 
+### Limitations
+- Please note that since [doctrine/orm 3.3.0](https://github.com/doctrine/orm/pull/11188), the produced SQL gets finalized with `LIMIT` / `OFFSET` / `FOR UPDATE` after `SqlWalker` processing is done.
+  - Thus, implementors should be aware that those SQL parts can be appended to the SQL after `HintHandler` processing.
+  - This means that e.g. placing a comment at the end of the SQL breaks LIMIT functionality completely
+
 ### Implementors
 - [shipmonk/doctrine-mysql-optimizer-hints](https://github.com/shipmonk-rnd/doctrine-mysql-optimizer-hints) (since v2)
 - [shipmonk/doctrine-mysql-index-hints](https://github.com/shipmonk-rnd/doctrine-mysql-index-hints) (since v3)

composer.json

@@ -19,7 +19,8 @@
         "phpunit/phpunit": "10.5.36",
         "shipmonk/composer-dependency-analyser": "1.7.0",
         "shipmonk/phpstan-rules": "3.2.1",
-        "slevomat/coding-standard": "8.15.0"
+        "slevomat/coding-standard": "8.15.0",
+        "symfony/cache": "^6.4"
     },
     "autoload": {
         "psr-4": {

src/HintDrivenSqlWalker.php

@@ -53,18 +53,15 @@
 use Doctrine\ORM\Query\AST\UpdateItem;
 use Doctrine\ORM\Query\AST\UpdateStatement;
 use Doctrine\ORM\Query\AST\WhereClause;
-use Doctrine\ORM\Query\Exec\PreparedExecutorFinalizer;
-use Doctrine\ORM\Query\Exec\SqlFinalizer;
-use Doctrine\ORM\Query\OutputWalker;
 use Doctrine\ORM\Query\Parser;
-use Doctrine\ORM\Query\SqlWalker;
+use Doctrine\ORM\Query\SqlOutputWalker;
 use LogicException;
 use function is_a;
 
 /**
  * @psalm-import-type QueryComponent from Parser
  */
-class HintDrivenSqlWalker extends SqlWalker implements OutputWalker
+class HintDrivenSqlWalker extends SqlOutputWalker
 {
 
     /**
@@ -94,20 +91,10 @@ public function __construct(
         }
     }
 
-    public function getFinalizer(DeleteStatement|UpdateStatement|SelectStatement $AST): SqlFinalizer
+    protected function createSqlForFinalizer(SelectStatement $selectStatement): string
     {
-        switch (true) {
-            case $AST instanceof SelectStatement:
-                return new NoopSqlFinalizer($this->walkSelectStatement($AST));
-
-            case $AST instanceof UpdateStatement:
-                return new PreparedExecutorFinalizer($this->createUpdateStatementExecutor($AST));
-
-            case $AST instanceof DeleteStatement: // @phpstan-ignore instanceof.alwaysTrue (keep it readable)
-                return new PreparedExecutorFinalizer($this->createDeleteStatementExecutor($AST));
-        }
-
-        throw new LogicException('Unexpected AST node type');
+        $selectStatementSql = parent::createSqlForFinalizer($selectStatement);
+        return $this->callWalkers(SqlNode::SelectStatement, $selectStatementSql);
     }
 
     public function walkSelectStatement(SelectStatement $AST): string

tests/HintDrivenSqlWalkerTest.php

@@ -14,6 +14,7 @@
 use PHPUnit\Framework\TestCase;
 use ShipMonk\Doctrine\Walker\Handlers\CommentWholeSqlHintHandler;
 use ShipMonk\Doctrine\Walker\Handlers\LowercaseSelectHintHandler;
+use Symfony\Component\Cache\Adapter\ArrayAdapter;
 use function sprintf;
 
 class HintDrivenSqlWalkerTest extends TestCase
@@ -40,6 +41,37 @@ public function testWalker(
         self::assertSame($expectedSql, $producedSql);
     }
 
+    public function testPagination(): void
+    {
+        $pageSize = 10;
+        $entityManagerMock = $this->createEntityManagerMock();
+
+        self::assertNotNull(
+            $entityManagerMock->getConfiguration()->getQueryCache(),
+            'QueryCache needed. The purpose of this test is to ensure that we do not break the pagination by using a cache',
+        );
+
+        $expectedSqls = [
+            'select d0_.id AS id_0 FROM dummy_entity d0_ LIMIT 10',
+            'select d0_.id AS id_0 FROM dummy_entity d0_ LIMIT 10 OFFSET 10',
+            'select d0_.id AS id_0 FROM dummy_entity d0_ LIMIT 10 OFFSET 20',
+        ];
+
+        foreach ([0, 1, 2] as $page) {
+            $query = $entityManagerMock->createQueryBuilder()
+                ->select('w')
+                ->from(DummyEntity::class, 'w')
+                ->getQuery()
+                ->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, HintDrivenSqlWalker::class)
+                ->setHint(LowercaseSelectHintHandler::class, null)
+                ->setFirstResult($page * $pageSize)
+                ->setMaxResults($pageSize);
+            $producedSql = $query->getSQL();
+
+            self::assertSame($expectedSqls[$page], $producedSql, 'Page ' . $page . ' failed:');
+        }
+    }
+
     /**
      * @return Generator<string, array{callable(EntityManager):Query, class-string<HintHandler>, mixed, string}>
      */
@@ -85,7 +117,7 @@ static function (EntityManager $entityManager): Query {
             },
             CommentWholeSqlHintHandler::class,
             'custom comment',
-            'SELECT d0_.id AS id_0 FROM dummy_entity d0_ LIMIT 1 -- custom comment',
+            'SELECT d0_.id AS id_0 FROM dummy_entity d0_ -- custom comment LIMIT 1', // see readme limitations
         ];
     }
 
@@ -94,6 +126,7 @@ private function createEntityManagerMock(): EntityManager
         $config = new Configuration();
         $config->setProxyNamespace('Tmp\Doctrine\Tests\Proxies');
         $config->setProxyDir('/tmp/doctrine');
+        $config->setQueryCache(new ArrayAdapter());
         $config->setAutoGenerateProxyClasses(false);
         $config->setSecondLevelCacheEnabled(false);
         $config->setMetadataDriverImpl(new AttributeDriver([__DIR__]));