From 194f8ff18760fd00294551a944c134fc91c41487 Mon Sep 17 00:00:00 2001 From: Alexandre Alapetite Date: Sun, 29 Sep 2024 12:47:39 +0200 Subject: [PATCH] Set cache_expiration_time when storing data to cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we switched to PSR-16 inspired `DataCache` in bb38d740c12a953dce4e2722a8d26d7a3ed2be3f, we introduced a regression due to impedance mismatch between PSR-16 and HTTP Caching mechanisms: - HTTP caching uses two step invalidation – if a cache entry is accessed after its TTL passes, a request is made with a stored HTTP caching header (e.g. ETag) and the cache is considered invalid only if the server returns something else than 304 Not Modified response. - On the other hand, PSR-16 cache does not allow access to entries past its TTL, forcing us to fetch the content every time, even when the remote server could tell us it is still fine. To work around this, bb38d740c12a953dce4e2722a8d26d7a3ed2be3f decided to store `cache_expiration_time` field in the cache entry so that we could bypass the PSR-16’s strict TTL and handle expiration ourselves. But there were few places where we forgot to set it. This patch fixes that. Unfortunately, because we also pass the same value as `cache_expiration_time` to `DataCache::set_data` as TTL, the early eviction still happens. We will also need to set the TTL to `null` (unlimited) so that the `DataCache` does not evict the entries behind our back. Partly fixes https://github.com/simplepie/simplepie/issues/830 ([BUG] Cache constantly updating) --- src/SimplePie.php | 4 +++- tests/Integration/CachingTest.php | 6 +++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/SimplePie.php b/src/SimplePie.php index 8651d2f25..22d381174 100644 --- a/src/SimplePie.php +++ b/src/SimplePie.php @@ -1900,7 +1900,7 @@ protected function fetch_data(&$cache) $this->data = []; } // Check if the cache has been updated - elseif (isset($this->data['cache_expiration_time']) && $this->data['cache_expiration_time'] > time()) { + elseif (!isset($this->data['cache_expiration_time']) || $this->data['cache_expiration_time'] < time()) { // Want to know if we tried to send last-modified and/or etag headers // when requesting this file. (Note that it's up to the file to // support this, but we don't always send the headers either.) @@ -1924,6 +1924,7 @@ protected function fetch_data(&$cache) $this->status_code = 0; if ($this->force_cache_fallback) { + $this->data['cache_expiration_time'] = $this->cache_duration + time(); $cache->set_data($cacheKey, $this->data, $this->cache_duration); return true; @@ -1936,6 +1937,7 @@ protected function fetch_data(&$cache) // Set raw_data to false here too, to signify that the cache // is still valid. $this->raw_data = false; + $this->data['cache_expiration_time'] = $this->cache_duration + time(); $cache->set_data($cacheKey, $this->data, $this->cache_duration); return true; diff --git a/tests/Integration/CachingTest.php b/tests/Integration/CachingTest.php index 350410061..e01ef95a7 100644 --- a/tests/Integration/CachingTest.php +++ b/tests/Integration/CachingTest.php @@ -120,7 +120,7 @@ public function testInitWithDifferentCacheStateCallsCacheCorrectly( */ public static function provideSavedCacheData(): array { - $defaultMtime = time(); + $defaultMtime = time() - 1; // -1 to account for tests running within the same second $defaultExpirationTime = $defaultMtime + 3600; $expectDefaultDataWritten = [ @@ -246,10 +246,10 @@ public static function provideSavedCacheData(): array [CacheInterface::class, $currentlyCachedDataWithNonFeedUrl, $expectDataWithNewFeedUrl, $defaultMtime], // Check if the cache has been updated [Base::class, $currentlyCachedDataIsUpdated, $expectDefaultDataWritten, $defaultMtime], - [CacheInterface::class, $currentlyCachedDataIsUpdated, $expectDefaultDataWritten, $defaultMtime], + [CacheInterface::class, $currentlyCachedDataIsUpdated, $expectNoDataWritten, $defaultMtime], // If the cache is still valid, just return true [Base::class, $currentlyCachedDataIsValid, $expectDefaultDataWritten, $defaultMtime], - [CacheInterface::class, $currentlyCachedDataIsValid, $expectNoDataWritten, $defaultMtime], + [CacheInterface::class, $currentlyCachedDataIsValid, $expectDefaultDataWritten, $defaultMtime], ]; } }