Merge pull request #4048 from morozov/cache-no-close-cursor

Make caching layer not rely on closeCursor()

docs/en/reference/caching.rst

@@ -35,15 +35,13 @@ the default cache instance:
     new QueryCacheProfile(0, "some key", $cache);
 
 In order for the data to actually be cached its necessary to ensure that the entire
-result set is read (the easiest way to ensure this is to use ``fetchAll``) and the statement
-object is closed:
+result set is read (the easiest way to ensure this is to use one of the ``fetchAll*()`` methods):
 
 ::
 
     <?php
     $stmt = $conn->executeCacheQuery($query, $params, $types, new QueryCacheProfile(0, "some key"));
     $data = $stmt->fetchAllAssociative();
-    $stmt->closeCursor(); // at this point the result is cached
 
 .. warning::
 

lib/Doctrine/DBAL/Cache/ResultCacheStatement.php

@@ -50,14 +50,7 @@ class ResultCacheStatement implements IteratorAggregate, ResultStatement, Forwar
     /** @var ResultStatement */
     private $statement;
 
-    /**
-     * Did we reach the end of the statement?
-     *
-     * @var bool
-     */
-    private $emptied = false;
-
-    /** @var array<int,array<string,mixed>> */
+    /** @var array<int,array<string,mixed>>|null */
     private $data;
 
     /** @var int */
@@ -83,19 +76,8 @@ class ResultCacheStatement implements IteratorAggregate, ResultStatement, Forwar
     public function closeCursor()
     {
         $this->statement->closeCursor();
-        if (! $this->emptied || $this->data === null) {
-            return true;
-        }
-
-        $data = $this->resultCache->fetch($this->cacheKey);
-        if (! $data) {
-            $data = [];
-        }
 
-        $data[$this->realKey] = $this->data;
-
-        $this->resultCache->save($this->cacheKey, $data, $this->lifetime);
-        unset($this->data);
+        $this->data = null;
 
         return true;
     }
@@ -169,7 +151,7 @@ class ResultCacheStatement implements IteratorAggregate, ResultStatement, Forwar
             throw new InvalidArgumentException('Invalid fetch-style given for caching result.');
         }
 
-        $this->emptied = true;
+        $this->saveToCache();
 
         return false;
     }
@@ -183,8 +165,9 @@ class ResultCacheStatement implements IteratorAggregate, ResultStatement, Forwar
     {
         $data = $this->statement->fetchAll(FetchMode::ASSOCIATIVE, $fetchArgument, $ctorArgs);
 
-        $this->data    = $data;
-        $this->emptied = true;
+        $this->data = $data;
+
+        $this->saveToCache();
 
         if ($fetchMode === FetchMode::NUMERIC) {
             foreach ($data as $i => $row) {
@@ -327,7 +310,7 @@ class ResultCacheStatement implements IteratorAggregate, ResultStatement, Forwar
             return $row;
         }
 
-        $this->emptied = true;
+        $this->saveToCache();
 
         return false;
     }
@@ -337,7 +320,22 @@ class ResultCacheStatement implements IteratorAggregate, ResultStatement, Forwar
      */
     private function store(array $data): void
     {
-        $this->data    = $data;
-        $this->emptied = true;
+        $this->data = $data;
+    }
+
+    private function saveToCache(): void
+    {
+        if ($this->data === null) {
+            return;
+        }
+
+        $data = $this->resultCache->fetch($this->cacheKey);
+        if (! $data) {
+            $data = [];
+        }
+
+        $data[$this->realKey] = $this->data;
+
+        $this->resultCache->save($this->cacheKey, $data, $this->lifetime);
     }
 }

tests/Doctrine/Tests/DBAL/Functional/ResultCacheTest.php

@@ -135,7 +135,7 @@ class ResultCacheTest extends DbalFunctionalTestCase
         self::assertEquals($data, $dataIterator);
     }
 
-    public function testDontCloseNoCache(): void
+    public function testFetchAndFinishSavesCache(): void
     {
         $stmt = $this->connection->executeQuery('SELECT * FROM caching ORDER BY test_int ASC', [], [], new QueryCacheProfile(0, 'testcachekey'));
 
@@ -153,7 +153,7 @@ class ResultCacheTest extends DbalFunctionalTestCase
             $data[] = $row;
         }
 
-        self::assertCount(2, $this->sqlLogger->queries);
+        self::assertCount(1, $this->sqlLogger->queries);
     }
 
     public function testDontFinishNoCache(): void
@@ -161,7 +161,6 @@ class ResultCacheTest extends DbalFunctionalTestCase
         $stmt = $this->connection->executeQuery('SELECT * FROM caching ORDER BY test_int ASC', [], [], new QueryCacheProfile(0, 'testcachekey'));
 
         $stmt->fetch(FetchMode::ASSOCIATIVE);
-        $stmt->closeCursor();
 
         $stmt = $this->connection->executeQuery('SELECT * FROM caching ORDER BY test_int ASC', [], [], new QueryCacheProfile(0, 'testcachekey'));
 
@@ -170,12 +169,11 @@ class ResultCacheTest extends DbalFunctionalTestCase
         self::assertCount(2, $this->sqlLogger->queries);
     }
 
-    public function testFetchAllAndFinishSavesCache(): void
+    public function testFetchAllSavesCache(): void
     {
         $layerCache = new ArrayCache();
         $stmt       = $this->connection->executeQuery('SELECT * FROM caching WHERE test_int > 500', [], [], new QueryCacheProfile(0, 'testcachekey', $layerCache));
         $stmt->fetchAll();
-        $stmt->closeCursor();
 
         self::assertCount(1, $layerCache->fetch('testcachekey'));
     }
@@ -189,7 +187,6 @@ class ResultCacheTest extends DbalFunctionalTestCase
 
         $stmt = $this->connection->executeCacheQuery($query, [], [], $qcp);
         $stmt->fetchAll(FetchMode::COLUMN);
-        $stmt->closeCursor();
 
         $stmt = $this->connection->executeCacheQuery($query, [], [], $qcp);
 
@@ -251,8 +248,6 @@ class ResultCacheTest extends DbalFunctionalTestCase
             $data[] = is_array($row) ? array_change_key_case($row, CASE_LOWER) : $row;
         }
 
-        $stmt->closeCursor();
-
         return $data;
     }
 
@@ -267,8 +262,6 @@ class ResultCacheTest extends DbalFunctionalTestCase
             $data[] = is_array($row) ? array_change_key_case($row, CASE_LOWER) : $row;
         }
 
-        $stmt->closeCursor();
-
         return $data;
     }
 }