Make serialized SQL executors forward compatible

The idea here is that instead of having a backward compatibility layer in the next major branch, we can have a forward compatibility layer in this branch.
2026-03-24 06:52:09 +01:00 · 2023-10-26 00:01:12 +02:00
parent 16028e4fd3
commit 2a9390d426
9 changed files with 107 additions and 29 deletions
--- a/UPGRADE.md
+++ b/UPGRADE.md
@@ -1,5 +1,9 @@
 # Upgrade to 2.17

+## Deprecate `Doctrine\ORM\Query\Exec\AbstractSqlExecutor::_sqlStatements`
+
+Use `Doctrine\ORM\Query\Exec\AbstractSqlExecutor::sqlStatements` instead.
+
 ## Undeprecate `Doctrine\ORM\Proxy\Autoloader`

 It will be a full-fledged class, no longer extending
--- a/lib/Doctrine/ORM/Query/Exec/AbstractSqlExecutor.php
+++ b/lib/Doctrine/ORM/Query/Exec/AbstractSqlExecutor.php
@@ -9,6 +9,10 @@ use Doctrine\DBAL\Connection;
 use Doctrine\DBAL\Result;
 use Doctrine\DBAL\Types\Type;

+use function array_diff;
+use function array_keys;
+use function array_values;
+
 /**
 * Base class for SQL statement executors.
 *
@@ -18,12 +22,24 @@ use Doctrine\DBAL\Types\Type;
 */
 abstract class AbstractSqlExecutor
 {
-    /** @var list<string>|string */
+    /**
+     * @deprecated use $sqlStatements instead
+     *
+     * @var list<string>|string
+     */
    protected $_sqlStatements;

+    /** @var list<string>|string */
+    protected $sqlStatements;
+
    /** @var QueryCacheProfile */
    protected $queryCacheProfile;

+    public function __construct()
+    {
+        $this->_sqlStatements = &$this->sqlStatements;
+    }
+
    /**
     * Gets the SQL statements that are executed by the executor.
     *
@@ -31,21 +47,18 @@ abstract class AbstractSqlExecutor
     */
    public function getSqlStatements()
    {
-        return $this->_sqlStatements;
+        return $this->sqlStatements;
    }

-    /** @return void */
-    public function setQueryCacheProfile(QueryCacheProfile $qcp)
+    public function setQueryCacheProfile(QueryCacheProfile $qcp): void
    {
        $this->queryCacheProfile = $qcp;
    }

    /**
     * Do not use query cache
-     *
-     * @return void
     */
-    public function removeQueryCacheProfile()
+    public function removeQueryCacheProfile(): void
    {
        $this->queryCacheProfile = null;
    }
@@ -60,4 +73,22 @@ abstract class AbstractSqlExecutor
     * @return Result|int
     */
    abstract public function execute(Connection $conn, array $params, array $types);
+
+    /** @return list<string> */
+    public function __sleep(): array
+    {
+        /* Two reasons for this:
+           - we do not need to serialize the deprecated property, we can
+             rebuild the reference to the new property in __wakeup()
+           - not having the legacy property in the serialized data means the
+             serialized representation becomes compatible with 3.0.x, meaning
+             there will not be a deprecation warning about a missing property
+             when unserializing data */
+        return array_values(array_diff(array_keys((array) $this), ["\0*\0_sqlStatements"]));
+    }
+
+    public function __wakeup(): void
+    {
+        $this->_sqlStatements = &$this->sqlStatements;
+    }
 }
--- a/lib/Doctrine/ORM/Query/Exec/MultiTableDeleteExecutor.php
+++ b/lib/Doctrine/ORM/Query/Exec/MultiTableDeleteExecutor.php
@@ -45,6 +45,8 @@ class MultiTableDeleteExecutor extends AbstractSqlExecutor
     */
    public function __construct(AST\Node $AST, $sqlWalker)
    {
+        parent::__construct();
+
        $em            = $sqlWalker->getEntityManager();
        $conn          = $em->getConnection();
        $platform      = $conn->getDatabasePlatform();
@@ -83,8 +85,8 @@ class MultiTableDeleteExecutor extends AbstractSqlExecutor
        // 3. Create and store DELETE statements
        $classNames = array_merge($primaryClass->parentClasses, [$primaryClass->name], $primaryClass->subClasses);
        foreach (array_reverse($classNames) as $className) {
-            $tableName              = $quoteStrategy->getTableName($em->getClassMetadata($className), $platform);
-            $this->_sqlStatements[] = 'DELETE FROM ' . $tableName
+            $tableName             = $quoteStrategy->getTableName($em->getClassMetadata($className), $platform);
+            $this->sqlStatements[] = 'DELETE FROM ' . $tableName
                    . ' WHERE (' . $idColumnList . ') IN (' . $idSubselect . ')';
        }

@@ -117,7 +119,7 @@ class MultiTableDeleteExecutor extends AbstractSqlExecutor
            $numDeleted = $conn->executeStatement($this->insertSql, $params, $types);

            // Execute DELETE statements
-            foreach ($this->_sqlStatements as $sql) {
+            foreach ($this->sqlStatements as $sql) {
                $conn->executeStatement($sql);
            }
        } catch (Throwable $exception) {
--- a/lib/Doctrine/ORM/Query/Exec/MultiTableUpdateExecutor.php
+++ b/lib/Doctrine/ORM/Query/Exec/MultiTableUpdateExecutor.php
@@ -50,6 +50,8 @@ class MultiTableUpdateExecutor extends AbstractSqlExecutor
     */
    public function __construct(AST\Node $AST, $sqlWalker)
    {
+        parent::__construct();
+
        $em            = $sqlWalker->getEntityManager();
        $conn          = $em->getConnection();
        $platform      = $conn->getDatabasePlatform();
@@ -119,7 +121,7 @@ class MultiTableUpdateExecutor extends AbstractSqlExecutor
            }

            if ($affected) {
-                $this->_sqlStatements[$i] = $updateSql . ' WHERE (' . $idColumnList . ') IN (' . $idSubselect . ')';
+                $this->sqlStatements[$i] = $updateSql . ' WHERE (' . $idColumnList . ') IN (' . $idSubselect . ')';
            }
        }

@@ -163,7 +165,7 @@ class MultiTableUpdateExecutor extends AbstractSqlExecutor
            );

            // Execute UPDATE statements
-            foreach ($this->_sqlStatements as $key => $statement) {
+            foreach ($this->sqlStatements as $key => $statement) {
                $paramValues = [];
                $paramTypes  = [];

--- a/lib/Doctrine/ORM/Query/Exec/SingleSelectExecutor.php
+++ b/lib/Doctrine/ORM/Query/Exec/SingleSelectExecutor.php
@@ -18,7 +18,9 @@ class SingleSelectExecutor extends AbstractSqlExecutor
 {
    public function __construct(SelectStatement $AST, SqlWalker $sqlWalker)
    {
-        $this->_sqlStatements = $sqlWalker->walkSelectStatement($AST);
+        parent::__construct();
+
+        $this->sqlStatements = $sqlWalker->walkSelectStatement($AST);
    }

    /**
@@ -28,6 +30,6 @@ class SingleSelectExecutor extends AbstractSqlExecutor
     */
    public function execute(Connection $conn, array $params, array $types)
    {
-        return $conn->executeQuery($this->_sqlStatements, $params, $types, $this->queryCacheProfile);
+        return $conn->executeQuery($this->sqlStatements, $params, $types, $this->queryCacheProfile);
    }
 }
--- a/lib/Doctrine/ORM/Query/Exec/SingleTableDeleteUpdateExecutor.php
+++ b/lib/Doctrine/ORM/Query/Exec/SingleTableDeleteUpdateExecutor.php
@@ -22,10 +22,12 @@ class SingleTableDeleteUpdateExecutor extends AbstractSqlExecutor
    /** @param SqlWalker $sqlWalker */
    public function __construct(AST\Node $AST, $sqlWalker)
    {
+        parent::__construct();
+
        if ($AST instanceof AST\UpdateStatement) {
-            $this->_sqlStatements = $sqlWalker->walkUpdateStatement($AST);
+            $this->sqlStatements = $sqlWalker->walkUpdateStatement($AST);
        } elseif ($AST instanceof AST\DeleteStatement) {
-            $this->_sqlStatements = $sqlWalker->walkDeleteStatement($AST);
+            $this->sqlStatements = $sqlWalker->walkDeleteStatement($AST);
        }
    }

@@ -40,6 +42,6 @@ class SingleTableDeleteUpdateExecutor extends AbstractSqlExecutor
            $conn->ensureConnectedToPrimary();
        }

-        return $conn->executeStatement($this->_sqlStatements, $params, $types);
+        return $conn->executeStatement($this->sqlStatements, $params, $types);
    }
 }
--- a/psalm-baseline.xml
+++ b/psalm-baseline.xml
@@ -1847,6 +1847,18 @@
    <PossiblyNullPropertyAssignmentValue>
      <code>null</code>
    </PossiblyNullPropertyAssignmentValue>
+    <PropertyNotSetInConstructor>
+      <code>$_sqlStatements</code>
+      <code>$queryCacheProfile</code>
+      <code>$sqlStatements</code>
+    </PropertyNotSetInConstructor>
+    <UninitializedProperty>
+      <code><![CDATA[$this->sqlStatements]]></code>
+    </UninitializedProperty>
+    <UnsupportedPropertyReferenceUsage>
+      <code><![CDATA[$this->_sqlStatements = &$this->sqlStatements]]></code>
+      <code><![CDATA[$this->_sqlStatements = &$this->sqlStatements]]></code>
+    </UnsupportedPropertyReferenceUsage>
  </file>
  <file src="lib/Doctrine/ORM/Query/Exec/MultiTableDeleteExecutor.php">
    <InvalidReturnStatement>
@@ -1856,15 +1868,12 @@
      <code>int</code>
    </InvalidReturnType>
    <PossiblyInvalidIterator>
-      <code><![CDATA[$this->_sqlStatements]]></code>
+      <code><![CDATA[$this->sqlStatements]]></code>
    </PossiblyInvalidIterator>
    <PropertyNotSetInConstructor>
      <code>MultiTableDeleteExecutor</code>
      <code>MultiTableDeleteExecutor</code>
    </PropertyNotSetInConstructor>
-    <UninitializedProperty>
-      <code><![CDATA[$this->_sqlStatements]]></code>
-    </UninitializedProperty>
  </file>
  <file src="lib/Doctrine/ORM/Query/Exec/MultiTableUpdateExecutor.php">
    <InvalidReturnStatement>
@@ -1874,40 +1883,40 @@
      <code>int</code>
    </InvalidReturnType>
    <PossiblyInvalidIterator>
-      <code><![CDATA[$this->_sqlStatements]]></code>
+      <code><![CDATA[$this->sqlStatements]]></code>
    </PossiblyInvalidIterator>
    <PropertyNotSetInConstructor>
      <code>MultiTableUpdateExecutor</code>
      <code>MultiTableUpdateExecutor</code>
+      <code>MultiTableUpdateExecutor</code>
    </PropertyNotSetInConstructor>
    <PropertyTypeCoercion>
-      <code><![CDATA[$this->_sqlStatements]]></code>
+      <code><![CDATA[$this->sqlStatements]]></code>
    </PropertyTypeCoercion>
-    <UninitializedProperty>
-      <code><![CDATA[$this->_sqlStatements]]></code>
-    </UninitializedProperty>
  </file>
  <file src="lib/Doctrine/ORM/Query/Exec/SingleSelectExecutor.php">
    <PossiblyInvalidArgument>
-      <code><![CDATA[$this->_sqlStatements]]></code>
+      <code><![CDATA[$this->sqlStatements]]></code>
    </PossiblyInvalidArgument>
    <PropertyNotSetInConstructor>
      <code>SingleSelectExecutor</code>
+      <code>SingleSelectExecutor</code>
    </PropertyNotSetInConstructor>
  </file>
  <file src="lib/Doctrine/ORM/Query/Exec/SingleTableDeleteUpdateExecutor.php">
    <InvalidReturnStatement>
-      <code><![CDATA[$conn->executeStatement($this->_sqlStatements, $params, $types)]]></code>
+      <code><![CDATA[$conn->executeStatement($this->sqlStatements, $params, $types)]]></code>
    </InvalidReturnStatement>
    <InvalidReturnType>
      <code>int</code>
    </InvalidReturnType>
    <PossiblyInvalidArgument>
-      <code><![CDATA[$this->_sqlStatements]]></code>
+      <code><![CDATA[$this->sqlStatements]]></code>
    </PossiblyInvalidArgument>
    <PropertyNotSetInConstructor>
      <code>SingleTableDeleteUpdateExecutor</code>
      <code>SingleTableDeleteUpdateExecutor</code>
+      <code>SingleTableDeleteUpdateExecutor</code>
    </PropertyNotSetInConstructor>
  </file>
  <file src="lib/Doctrine/ORM/Query/Expr.php">
--- a/tests/Doctrine/Tests/ORM/Functional/ParserResultSerializationTest.php
+++ b/tests/Doctrine/Tests/ORM/Functional/ParserResultSerializationTest.php
@@ -11,6 +11,7 @@ use Doctrine\ORM\Query\ResultSetMapping;
 use Doctrine\Tests\OrmFunctionalTestCase;
 use Generator;
 use ReflectionMethod;
+use ReflectionProperty;

 use function file_get_contents;
 use function rtrim;
@@ -41,6 +42,30 @@ class ParserResultSerializationTest extends OrmFunctionalTestCase
        $this->assertInstanceOf(SingleSelectExecutor::class, $unserialized->getSqlExecutor());
    }

+    public function testItSerializesParserResultWithAForwardCompatibleFormat(): void
+    {
+        $query = $this->_em
+            ->createQuery('SELECT u FROM Doctrine\Tests\Models\Company\CompanyEmployee u WHERE u.name = :name');
+
+        $parserResult = self::parseQuery($query);
+        $serialized   = serialize($parserResult);
+        $this->assertStringNotContainsString(
+            '_sqlStatements',
+            $serialized,
+            'ParserResult should not contain any reference to _sqlStatements, which is a legacy property.'
+        );
+        $unserialized = unserialize($serialized);
+
+        $r = new ReflectionProperty($unserialized->getSqlExecutor(), '_sqlStatements');
+        $r->setAccessible(true);
+
+        $this->assertSame(
+            $r->getValue($unserialized->getSqlExecutor()),
+            $unserialized->getSqlExecutor()->getSqlStatements(),
+            'The legacy property should be populated with the same value as the new one.'
+        );
+    }
+
    /**
     * @dataProvider provideSerializedSingleSelectResults
     */
@@ -59,6 +84,7 @@ class ParserResultSerializationTest extends OrmFunctionalTestCase
    {
        yield '2.14.3' => [rtrim(file_get_contents(__DIR__ . '/ParserResults/single_select_2_14_3.txt'), "\n")];
        yield '2.15.0' => [rtrim(file_get_contents(__DIR__ . '/ParserResults/single_select_2_15_0.txt'), "\n")];
+        yield '2.17.0' => [rtrim(file_get_contents(__DIR__ . '/ParserResults/single_select_2_17_0.txt'), "\n")];
    }

    private static function parseQuery(Query $query): ParserResult
--- a/tests/Doctrine/Tests/ORM/Functional/ParserResults/single_select_2_17_0.txt
+++ b/tests/Doctrine/Tests/ORM/Functional/ParserResults/single_select_2_17_0.txt