Deprecate old output walker types

UPGRADE.md

@@ -1,3 +1,18 @@
+# Upgrade to xxx
+
+## Changes for custom output walkers
+
+To implement a custom output walker, extend the `SqlOutputWalker` class. Either override
+the `getFinalizer()` method to return an instance of `SqlFinalizer`, or override 
+`createSqlForFinalizer()` and return your SQL string from it. 
+
+When extending `SqlOutputWalker`, the query cache speeding up the DQL->SQL transformation
+will no longer take the `Query::setFirstResult()` and `Query::setMaxResults()` values
+into consideration, so your output walker must not use or depend on these values. If
+it does, move this part to a custom `SqlFinalizer` class.
+
+https://github.com/doctrine/orm/pull/11188 gives more background.
+
 # Upgrade to 2.17
 
 ## Deprecate annotations classes for named queries

psalm-baseline.xml

@@ -1445,12 +1445,6 @@
     <InvalidArgument>
       <code>$sqlParams</code>
     </InvalidArgument>
-    <InvalidNullableReturnType>
-      <code>AbstractSqlExecutor</code>
-    </InvalidNullableReturnType>
-    <NullableReturnStatement>
-      <code><![CDATA[$this->sqlExecutor]]></code>
-    </NullableReturnStatement>
     <PossiblyNullArgument>
       <code><![CDATA[$this->getDQL()]]></code>
     </PossiblyNullArgument>
@@ -1866,12 +1860,6 @@
       <code><![CDATA[$this->_sqlStatements = &$this->sqlStatements]]></code>
     </UnsupportedPropertyReferenceUsage>
   </file>
-  <file src="src/Query/Exec/FinalizedSelectExecutor.php">
-    <PropertyNotSetInConstructor>
-      <code>FinalizedSelectExecutor</code>
-      <code>FinalizedSelectExecutor</code>
-    </PropertyNotSetInConstructor>
-  </file>
   <file src="src/Query/Exec/MultiTableDeleteExecutor.php">
     <InvalidReturnStatement>
       <code>$numDeleted</code>
@@ -2061,6 +2049,11 @@
       <code>$token === Lexer::T_IDENTIFIER</code>
     </RedundantConditionGivenDocblockType>
   </file>
+  <file src="src/Query/ParserResult.php">
+    <PropertyNotSetInConstructor>
+      <code>$sqlExecutor</code>
+    </PropertyNotSetInConstructor>
+  </file>
   <file src="src/Query/QueryExpressionVisitor.php">
     <InvalidReturnStatement>
       <code><![CDATA[new ArrayCollection($this->parameters)]]></code>

src/Query.php

@@ -18,6 +18,7 @@
 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;
@@ -294,6 +295,15 @@ private function initializeSqlExecutor(): void
         if ($this->parserResult->hasSqlFinalizer()) {
             $this->sqlExecutor = $this->parserResult->getSqlFinalizer()->createExecutor($this);
         } else {
+            Deprecation::trigger(
+                'doctrine/orm',
+                'https://github.com/doctrine/orm/pull/xxx',
+                'In Doctrine ORM 3.0, instances of %s must provide an instance of %s. Check the %s output walker that created this parser result.',
+                ParserResult::class,
+                SqlFinalizer::class,
+                $this->getHint(self::HINT_CUSTOM_OUTPUT_WALKER)
+            );
+
             $this->sqlExecutor = $this->parserResult->getSqlExecutor();
         }
     }
@@ -830,6 +840,9 @@ protected function getQueryCacheId(): string
     {
         ksort($this->_hints);
 
+        // TODO: Once \Doctrine\ORM\Query\Parser::parse accepts only instances of OutputWalker,
+        // this branching can be removed and we never need to consider first/max results.
+
         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 = '';

src/Query/Exec/SingleSelectExecutor.php

@@ -6,18 +6,28 @@
 
 use Doctrine\DBAL\Connection;
 use Doctrine\DBAL\Result;
+use Doctrine\Deprecations\Deprecation;
 use Doctrine\ORM\Query\AST\SelectStatement;
 use Doctrine\ORM\Query\SqlWalker;
 
 /**
  * Executor that executes the SQL statement for simple DQL SELECT statements.
  *
+ * @deprecated This class will be removed in 3.0
+ *
  * @link        www.doctrine-project.org
  */
 class SingleSelectExecutor extends AbstractSqlExecutor
 {
     public function __construct(SelectStatement $AST, SqlWalker $sqlWalker)
     {
+        Deprecation::trigger(
+            'doctrine/orm',
+            'https://github.com/doctrine/orm/pull/xxx',
+            'The %s class will be removed in Doctrine ORM 3.0',
+            self::class
+        );
+
         parent::__construct();
 
         $this->sqlStatements = $sqlWalker->walkSelectStatement($AST);

src/Query/Parser.php

@@ -391,13 +391,22 @@ public function parse()
             $this->queryComponents = $treeWalkerChain->getQueryComponents();
         }
 
+        // TODO: In 3.0, add a runtime check that the class implements OutputWalker?
         $outputWalkerClass = $this->customOutputWalker ?: SqlOutputWalker::class;
         $outputWalker      = new $outputWalkerClass($this->query, $this->parserResult, $this->queryComponents);
 
         if ($outputWalker instanceof OutputWalker) {
             $finalizer = $outputWalker->getFinalizer($AST);
             $this->parserResult->setSqlFinalizer($finalizer);
         } else {
+            // TODO once this path is removed, update \Doctrine\ORM\Query::getQueryCacheId as well
+            Deprecation::trigger(
+                'doctrine/orm',
+                'https://github.com/doctrine/orm/pull/xxx',
+                'In Doctrine ORM 3.0, output walkers will need to implement the %s interface',
+                OutputWalker::class
+            );
+
             $executor = $outputWalker->getExecutor($AST);
             $this->parserResult->setSqlExecutor($executor);
         }

src/Query/ParserResult.php

@@ -4,6 +4,7 @@
 
 namespace Doctrine\ORM\Query;
 
+use Doctrine\Deprecations\Deprecation;
 use Doctrine\ORM\Query\Exec\AbstractSqlExecutor;
 use Doctrine\ORM\Query\Exec\SqlFinalizer;
 use RuntimeException;
@@ -91,6 +92,15 @@ public function setResultSetMapping(ResultSetMapping $rsm)
      */
     public function setSqlExecutor($executor)
     {
+        Deprecation::triggerIfCalledFromOutside(
+            'doctrine/orm',
+            'https://github.com/doctrine/orm/pull/xxx',
+            'In Doctrine ORM 3.0, the %s class will no longer contain an sqlExecutor. The %s::%s method will be removed.',
+            self::class,
+            self::class,
+            __METHOD__
+        );
+
         $this->sqlExecutor = $executor;
     }
 
@@ -101,6 +111,15 @@ public function setSqlExecutor($executor)
      */
     public function getSqlExecutor()
     {
+        Deprecation::triggerIfCalledFromOutside(
+            'doctrine/orm',
+            'https://github.com/doctrine/orm/pull/xxx',
+            'In Doctrine ORM 3.0, the %s class will no longer contain an sqlExecutor. The %s::%s method will be removed.',
+            self::class,
+            self::class,
+            __METHOD__
+        );
+
         return $this->sqlExecutor;
     }
 
@@ -120,6 +139,7 @@ public function getSqlFinalizer(): SqlFinalizer
 
     /**
      * @internal
+     * @deprecated will be removed in 3.0, SqlFinalizers will have to be present in every ParserResult
      *
      * @psalm-internal Doctrine\ORM\Query
      * @psalm-assert-if-true !null $this->sqlFinalizer

src/Query/SqlWalker.php

@@ -179,6 +179,15 @@ class SqlWalker implements TreeWalker
      */
     public function __construct($query, $parserResult, array $queryComponents)
     {
+        if (! $this instanceof SqlOutputWalker) {
+            Deprecation::trigger(
+                'doctrine/orm',
+                'https://github.com/doctrine/orm/pull/xxx',
+                'The %s class may be removed in Doctrine ORM 3.0. To implement an output walker, inherit from %s and override the getFinalizer() or createSqlForFinalizer() methods instead.',
+                self::class,
+                SqlOutputWalker::class
+            );
+        }
         $this->query           = $query;
         $this->parserResult    = $parserResult;
         $this->queryComponents = $queryComponents;
@@ -275,12 +284,23 @@ public function setQueryComponent($dqlAlias, array $queryComponent)
     /**
      * Gets an executor that can be used to execute the result of this walker.
      *
+     * @deprecated This method will be replaced by the getFinalizer() method in 3.0
+     *
      * @param AST\DeleteStatement|AST\UpdateStatement|AST\SelectStatement $AST
      *
      * @return Exec\AbstractSqlExecutor
      */
     public function getExecutor($AST)
     {
+        Deprecation::triggerIfCalledFromOutside(
+            'doctrine/orm',
+            'https://github.com/doctrine/orm/pull/xxx',
+            'In Doctrine ORM 3.0, output walkers need to implement the %s interface and no longer provide a getExecutor() method. Thus, %s::%s will be removed.',
+            OutputWalker::class,
+            self::class,
+            __METHOD__
+        );
+
         switch (true) {
             case $AST instanceof AST\DeleteStatement:
                 $primaryClass = $this->em->getClassMetadata($AST->deleteClause->abstractSchemaName);

tests/Tests/ORM/Functional/PaginationTest.php

@@ -635,8 +635,6 @@ public function testCountQueryStripsParametersInSelect(): void
         self::assertCount(2, $getCountQuery->invoke($paginator)->getParameters());
         self::assertCount(9, $paginator);
 
-        $query->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, Query\SqlWalker::class);
-
         $paginator = new Paginator($query);
 
         // if select part of query is replaced with count(...) paginator should remove

tests/Tests/ORM/Query/CustomTreeWalkersTest.php

@@ -97,7 +97,7 @@ public function testSetUnknownQueryComponentThrowsException(): void
         $this->generateSql(
             'select u from Doctrine\Tests\Models\CMS\CmsUser u',
             [],
-            AddUnknownQueryComponentWalker::class
+            AddUnknownQueryComponentOutputWalker::class
         );
     }
 
@@ -111,7 +111,7 @@ public function testSupportsSeveralHintsQueries(): void
     }
 }
 
-class AddUnknownQueryComponentWalker extends Query\SqlOutputWalker
+class AddUnknownQueryComponentOutputWalker extends Query\SqlOutputWalker
 {
     protected function createSqlForFinalizer(AST\SelectStatement $AST): string
     {

tests/Tests/ORM/Query/ParserResultTest.php

@@ -4,13 +4,16 @@
 
 namespace Doctrine\Tests\ORM\Query;
 
+use Doctrine\Deprecations\PHPUnit\VerifyDeprecations;
 use Doctrine\ORM\Query\Exec\AbstractSqlExecutor;
 use Doctrine\ORM\Query\ParserResult;
 use Doctrine\ORM\Query\ResultSetMapping;
 use PHPUnit\Framework\TestCase;
 
 class ParserResultTest extends TestCase
 {
+    use VerifyDeprecations;
+
     /** @var ParserResult */
     public $parserResult;
 
@@ -26,6 +29,7 @@ public function testGetRsm(): void
 
     public function testSetGetSqlExecutor(): void
     {
+        self::expectDeprecationWithIdentifier('https://github.com/doctrine/orm/pull/xxx');
         self::assertNull($this->parserResult->getSqlExecutor());
 
         $executor = $this->getMockForAbstractClass(AbstractSqlExecutor::class);

tests/Tests/ORM/Query/SqlWalkerTest.php → tests/Tests/ORM/Query/SqlOutputWalkerTest.php

@@ -6,22 +6,23 @@
 
 use Doctrine\ORM\Query;
 use Doctrine\ORM\Query\ParserResult;
-use Doctrine\ORM\Query\SqlWalker;
+use Doctrine\ORM\Query\SqlOutputWalker;
 use Doctrine\Tests\OrmTestCase;
 
 /**
- * Tests for {@see \Doctrine\ORM\Query\SqlWalker}
+ * Tests for {@see \Doctrine\ORM\Query\SqlOutputWalker}
  *
+ * @covers \Doctrine\ORM\Query\SqlOutputWalker
  * @covers \Doctrine\ORM\Query\SqlWalker
  */
-class SqlWalkerTest extends OrmTestCase
+class SqlOutputWalkerTest extends OrmTestCase
 {
-    /** @var SqlWalker */
+    /** @var SqlOutputWalker */
     private $sqlWalker;
 
     protected function setUp(): void
     {
-        $this->sqlWalker = new SqlWalker(new Query($this->getTestEntityManager()), new ParserResult(), []);
+        $this->sqlWalker = new SqlOutputWalker(new Query($this->getTestEntityManager()), new ParserResult(), []);
     }
 
     /** @dataProvider getColumnNamesAndSqlAliases */