Merge pull request #11027 from greg0ire/fw-compat-serialization

Make serialized SQL executors forward compatible

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

lib/Doctrine/ORM/Query/Exec/AbstractSqlExecutor.php

@@ -9,6 +9,10 @@
 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,34 +22,43 @@
  */
 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.
      *
      * @return mixed[]|string  All the SQL update statements.
      */
     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 @@ public function removeQueryCacheProfile()
      * @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;
+    }
 }

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 @@ public function __construct(AST\Node $AST, $sqlWalker)
         // 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 @@ public function execute(Connection $conn, array $params, array $types)
             $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) {

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 @@ public function __construct(AST\Node $AST, $sqlWalker)
             }
 
             if ($affected) {
-                $this->_sqlStatements[$i] = $updateSql . ' WHERE (' . $idColumnList . ') IN (' . $idSubselect . ')';
+                $this->sqlStatements[$i] = $updateSql . ' WHERE (' . $idColumnList . ') IN (' . $idSubselect . ')';
             }
         }
 
@@ -163,7 +165,7 @@ public function execute(Connection $conn, array $params, array $types)
             );
 
             // Execute UPDATE statements
-            foreach ($this->_sqlStatements as $key => $statement) {
+            foreach ($this->sqlStatements as $key => $statement) {
                 $paramValues = [];
                 $paramTypes  = [];
 

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 @@ public function __construct(SelectStatement $AST, SqlWalker $sqlWalker)
      */
     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);
     }
 }

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 @@ public function execute(Connection $conn, array $params, array $types)
             $conn->ensureConnectedToPrimary();
         }
 
-        return $conn->executeStatement($this->_sqlStatements, $params, $types);
+        return $conn->executeStatement($this->sqlStatements, $params, $types);
     }
 }

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">

tests/Doctrine/Tests/ORM/Functional/ParserResultSerializationTest.php

@@ -11,6 +11,7 @@
 use Doctrine\Tests\OrmFunctionalTestCase;
 use Generator;
 use ReflectionMethod;
+use ReflectionProperty;
 
 use function file_get_contents;
 use function rtrim;
@@ -41,6 +42,30 @@ public function testSerializeParserResult(): void
         $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 @@ public static function provideSerializedSingleSelectResults(): Generator
     {
         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