CachedConfigCollection is written to disk on every page load (#89)

* only write if changed * use hash to reduce memory * avoid uninitialized property * test cacheOnDestruct * Update src/Collections/CachedConfigCollection.php Co-authored-by: Steve Boyd <[email protected]> * Update tests/Collections/CachedConfigCollectionTest.php Co-authored-by: Steve Boyd <[email protected]> * Update tests/Collections/CachedConfigCollectionTest.php Co-authored-by: Steve Boyd <[email protected]> * Update src/Collections/CachedConfigCollection.php Co-authored-by: Steve Boyd <[email protected]> * split --------- Co-authored-by: Steve Boyd <[email protected]>
silverstripe · Jul 18, 2023 · 44769a6 · 44769a6
1 parent 67de47d
commit 44769a6
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 3 deletions.
diff --git a/src/Collections/CachedConfigCollection.php b/src/Collections/CachedConfigCollection.php
@@ -27,6 +27,11 @@ class CachedConfigCollection implements ConfigCollectionInterface
      */
     protected $collection;
 
+    /**
+     * Stores a hash to allow comparison
+     */
+    private ?string $collectionHash = null;
+
     /**
      * @var callable
      */
@@ -108,6 +113,11 @@ public function getHistory()
         return $this->getCollection()->getHistory();
     }
 
+    private function getHash(): string
+    {
+        return md5(serialize($this->collection));
+    }
+
     /**
      * Get or build collection
      *
@@ -124,6 +134,7 @@ public function getCollection()
         if (!$this->flush) {
             $this->collection = $this->cache->get(self::CACHE_KEY);
             if ($this->collection) {
+                $this->collectionHash = $this->getHash();
                 return $this->collection;
             }
         }
@@ -142,8 +153,9 @@ public function getCollection()
         }
 
         // Save immediately.
-        // Note additional deferred save will occur in _destruct()
+        // Note additional deferred save can occur in _destruct()
         $this->cache->set(self::CACHE_KEY, $this->collection);
+        $this->collectionHash = $this->getHash();
         return $this->collection;
     }
 
@@ -153,11 +165,14 @@ public function getCollection()
     public function __destruct()
     {
         // Ensure back-end cache is updated
-        if ($this->collection) {
-            $this->cache->set(self::CACHE_KEY, $this->collection);
+        if ($this->collection && $this->collectionHash) {
+            if ($this->getHash() !== $this->collectionHash) {
+                $this->cache->set(self::CACHE_KEY, $this->collection);
+            }
 
             // Prevent double-destruct
             $this->collection = null;
+            $this->collectionHash = null;
         }
     }
 

diff --git a/tests/Collections/CachedConfigCollectionTest.php b/tests/Collections/CachedConfigCollectionTest.php
@@ -56,6 +56,62 @@ public function testCacheHit()
         $collection->__destruct();
     }
 
+    public function testCacheNotSavedOnDestruct()
+    {
+        $mockCache = $this->getMockBuilder(CacheInterface::class)->getMock();
+
+        $mockCollection = $this->getMockBuilder(ConfigCollectionInterface::class)->getMock();
+
+        // Cache will be generated, saved, and NOT saved again on __destruct()
+        $mockCache
+            ->expects($this->exactly(1))
+            ->method('set')
+            ->with(CachedConfigCollection::CACHE_KEY, $mockCollection);
+
+        $collection = new CachedConfigCollection();
+        $collection->setCollectionCreator(function () use ($mockCollection) {
+            return $mockCollection;
+        });
+        $collection->setCache($mockCache);
+
+        // Triggers cache set
+        $collection->getCollection();
+        // Call again to validate that cache set() is only called once
+        $collection->getCollection();
+
+        // Trigger __destruct() to validate that cache set() is not called again since there were no changes
+        $collection->__destruct();
+    }
+
+    public function testCacheSavedOnDestruct()
+    {
+        $mockCache = $this->getMockBuilder(CacheInterface::class)->getMock();
+
+        $mockCollection = $this->getMockBuilder(ConfigCollectionInterface::class)->getMock();
+        $mockCollection
+            ->expects($this->once())
+            ->method('get')
+            ->with('test', 'name', 0)
+            ->willReturn('value');
+
+        // Cache will be generated, saved, and saved again on __destruct()
+        $mockCache
+           ->expects($this->exactly(2))
+           ->method('set')
+           ->with(CachedConfigCollection::CACHE_KEY, $mockCollection);
+
+        $collection = new CachedConfigCollection();
+        $collection->setCollectionCreator(function () use ($mockCollection) {
+            return $mockCollection;
+        });
+        $collection->setCache($mockCache);
+
+        // Making a get call will update the callCache
+        $this->assertEquals('value', $collection->get('test', 'name'));
+
+        $collection->__destruct();
+    }
+
     public function testCacheMiss()
     {
         $mockCache = $this->getMockBuilder(CacheInterface::class)->getMock();