From e79cc360938ec5f011c34a1e90f6a2cdaae03afa Mon Sep 17 00:00:00 2001 From: Lukas Bestle Date: Mon, 2 Dec 2024 20:42:07 +0100 Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20cache=20changes=20versions=20in?= =?UTF-8?q?=20pages=20cache?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Updating a changes version does not flush the pages cache (would be pretty wasteful), so rendered changes versions should also never be cached. Extra logic is needed because one can now manually pass the changes version to render, even if no request param is set (which would also prevent caching with existing logic). --- src/Cms/Page.php | 14 ++++-- tests/Cms/Pages/PageRenderTest.php | 74 +++++++++++------------------- 2 files changed, 38 insertions(+), 50 deletions(-) diff --git a/src/Cms/Page.php b/src/Cms/Page.php index 45572684bc..54eba6f0ee 100644 --- a/src/Cms/Page.php +++ b/src/Cms/Page.php @@ -263,7 +263,7 @@ public function blueprints(string|null $inSection = null): array /** * Builds the cache id for the page */ - protected function cacheId(string $contentType): string + protected function cacheId(string $contentType, VersionId $versionId): string { $cacheId = [$this->id()]; @@ -271,6 +271,7 @@ protected function cacheId(string $contentType): string $cacheId[] = $this->kirby()->language()->code(); } + $cacheId[] = $versionId->value(); $cacheId[] = $contentType; return implode('.', $cacheId); @@ -551,7 +552,7 @@ public function isAncestorOf(Page $child): bool * pages cache. This will also check if one * of the ignore rules from the config kick in. */ - public function isCacheable(): bool + public function isCacheable(VersionId|null $versionId = null): bool { $kirby = $this->kirby(); $cache = $kirby->cache('pages'); @@ -563,6 +564,11 @@ public function isCacheable(): bool return false; } + // updating the changes version does not flush the pages cache + if ($versionId?->is('changes') === true) { + return false; + } + // inspect the current request $request = $kirby->request(); @@ -962,9 +968,9 @@ public function render( $versionId = VersionId::from($versionId); // try to get the page from cache - if ($data === [] && $this->isCacheable() === true) { + if ($data === [] && $this->isCacheable($versionId) === true) { $cache = $kirby->cache('pages'); - $cacheId = $this->cacheId($contentType); + $cacheId = $this->cacheId($contentType, $versionId); $result = $cache->get($cacheId); $html = $result['html'] ?? null; $response = $result['response'] ?? []; diff --git a/tests/Cms/Pages/PageRenderTest.php b/tests/Cms/Pages/PageRenderTest.php index 3e926eacf0..2e2d3779fd 100644 --- a/tests/Cms/Pages/PageRenderTest.php +++ b/tests/Cms/Pages/PageRenderTest.php @@ -147,6 +147,8 @@ public function testIsCacheableRequestMethod($method, $expected) ]); $this->assertSame($expected, $app->page('default')->isCacheable()); + $this->assertSame($expected, $app->page('default')->isCacheable(VersionId::latest())); + $this->assertFalse($app->page('default')->isCacheable(VersionId::changes())); } /** @@ -195,7 +197,11 @@ public function testIsCacheableIgnoreId() ]); $this->assertTrue($app->page('default')->isCacheable()); + $this->assertTrue($app->page('default')->isCacheable(VersionId::latest())); + $this->assertFalse($app->page('default')->isCacheable(VersionId::changes())); $this->assertFalse($app->page('data')->isCacheable()); + $this->assertFalse($app->page('data')->isCacheable(VersionId::latest())); + $this->assertFalse($app->page('data')->isCacheable(VersionId::changes())); } /** @@ -212,7 +218,11 @@ public function testIsCacheableIgnoreCallback() ]); $this->assertFalse($app->page('default')->isCacheable()); + $this->assertFalse($app->page('default')->isCacheable(VersionId::latest())); + $this->assertFalse($app->page('default')->isCacheable(VersionId::changes())); $this->assertTrue($app->page('data')->isCacheable()); + $this->assertTrue($app->page('data')->isCacheable(VersionId::latest())); + $this->assertFalse($app->page('data')->isCacheable(VersionId::changes())); } /** @@ -250,12 +260,12 @@ public function testRenderCache() $cache = $this->app->cache('pages'); $page = $this->app->page('default'); - $this->assertNull($cache->retrieve('default.html')); + $this->assertNull($cache->retrieve('default.latest.html')); $html1 = $page->render(); $this->assertStringStartsWith('This is a test:', $html1); - $value = $cache->retrieve('default.html'); + $value = $cache->retrieve('default.latest.html'); $this->assertInstanceOf(Value::class, $value); $this->assertSame($html1, $value->value()['html']); $this->assertNull($value->expires()); @@ -273,11 +283,11 @@ public function testRenderCacheCustomExpiry() $cache = $this->app->cache('pages'); $page = $this->app->page('expiry'); - $this->assertNull($cache->retrieve('expiry.html')); + $this->assertNull($cache->retrieve('expiry.latest.html')); $time = $page->render(); - $value = $cache->retrieve('expiry.html'); + $value = $cache->retrieve('expiry.latest.html'); $this->assertInstanceOf(Value::class, $value); $this->assertSame($time, $value->value()['html']); $this->assertSame((int)$time, $value->expires()); @@ -292,7 +302,7 @@ public function testRenderCacheMetadata() $cache = $this->app->cache('pages'); $page = $this->app->page('metadata'); - $this->assertNull($cache->retrieve('metadata.html')); + $this->assertNull($cache->retrieve('metadata.latest.html')); $html1 = $page->render(); $this->assertStringStartsWith('This is a test:', $html1); @@ -323,12 +333,12 @@ public function testRenderCacheDisabled() $cache = $this->app->cache('pages'); $page = $this->app->page('disabled'); - $this->assertNull($cache->retrieve('disabled.html')); + $this->assertNull($cache->retrieve('disabled.latest.html')); $html1 = $page->render(); $this->assertStringStartsWith('This is a test:', $html1); - $this->assertNull($cache->retrieve('disabled.html')); + $this->assertNull($cache->retrieve('disabled.latest.html')); $html2 = $page->render(); $this->assertStringStartsWith('This is a test:', $html2); @@ -355,12 +365,12 @@ public function testRenderCacheDynamicNonActive(string $slug, array $dynamicElem $cache = $this->app->cache('pages'); $page = $this->app->page($slug); - $this->assertNull($cache->retrieve($slug . '.html')); + $this->assertNull($cache->retrieve($slug . '.latest.html')); $html1 = $page->render(); $this->assertStringStartsWith('This is a test:', $html1); - $cacheValue = $cache->retrieve($slug . '.html'); + $cacheValue = $cache->retrieve($slug . '.latest.html'); $this->assertNotNull($cacheValue); $this->assertSame(in_array('auth', $dynamicElements), $cacheValue->value()['usesAuth']); if (in_array('cookie', $dynamicElements)) { @@ -394,12 +404,12 @@ public function testRenderCacheDynamicActiveOnFirstRender(string $slug, array $d $cache = $this->app->cache('pages'); $page = $this->app->page($slug); - $this->assertNull($cache->retrieve($slug . '.html')); + $this->assertNull($cache->retrieve($slug . '.latest.html')); $html1 = $page->render(); $this->assertStringStartsWith('This is a test:', $html1); - $cacheValue = $cache->retrieve($slug . '.html'); + $cacheValue = $cache->retrieve($slug . '.latest.html'); $this->assertNull($cacheValue); // reset the Kirby Responder object @@ -419,12 +429,12 @@ public function testRenderCacheDynamicActiveOnSecondRender(string $slug, array $ $cache = $this->app->cache('pages'); $page = $this->app->page($slug); - $this->assertNull($cache->retrieve($slug . '.html')); + $this->assertNull($cache->retrieve($slug . '.latest.html')); $html1 = $page->render(); $this->assertStringStartsWith('This is a test:', $html1); - $cacheValue = $cache->retrieve($slug . '.html'); + $cacheValue = $cache->retrieve($slug . '.latest.html'); $this->assertNotNull($cacheValue); $this->assertSame(in_array('auth', $dynamicElements), $cacheValue->value()['usesAuth']); if (in_array('cookie', $dynamicElements)) { @@ -454,12 +464,12 @@ public function testRenderCacheDataInitial() $cache = $this->app->cache('pages'); $page = $this->app->page('data'); - $this->assertNull($cache->retrieve('data.html')); + $this->assertNull($cache->retrieve('data.latest.html')); $html = $page->render(['test' => 'custom test']); $this->assertStringStartsWith('This is a custom test:', $html); - $this->assertNull($cache->retrieve('data.html')); + $this->assertNull($cache->retrieve('data.latest.html')); } /** @@ -471,12 +481,12 @@ public function testRenderCacheDataPreCached() $cache = $this->app->cache('pages'); $page = $this->app->page('data'); - $this->assertNull($cache->retrieve('data.html')); + $this->assertNull($cache->retrieve('data.latest.html')); $html1 = $page->render(); $this->assertStringStartsWith('This is a test:', $html1); - $value = $cache->retrieve('data.html'); + $value = $cache->retrieve('data.latest.html'); $this->assertInstanceOf(Value::class, $value); $this->assertSame($html1, $value->value()['html']); $this->assertNull($value->expires()); @@ -485,7 +495,7 @@ public function testRenderCacheDataPreCached() $this->assertStringStartsWith('This is a custom test:', $html2); // cache still stores the non-custom result - $value = $cache->retrieve('data.html'); + $value = $cache->retrieve('data.latest.html'); $this->assertInstanceOf(Value::class, $value); $this->assertSame($html1, $value->value()['html']); $this->assertNull($value->expires()); @@ -595,13 +605,6 @@ public function testRenderHookAfter() */ public function testRenderVersionDetectedFromRequest() { - // TODO: To be removed in the next PR when caching respects versions - $this->app = $this->app->clone([ - 'options' => [ - 'cache.pages' => false - ] - ]); - $page = $this->app->page('version'); $page->version('latest')->save(['title' => 'Latest Title']); $page->version('changes')->save(['title' => 'Changes Title']); @@ -623,13 +626,6 @@ public function testRenderVersionDetectedFromRequest() */ public function testRenderVersionDetectedRecursive() { - // TODO: To be removed in the next PR when caching respects versions - $this->app = $this->app->clone([ - 'options' => [ - 'cache.pages' => false - ] - ]); - $versionPage = $this->app->page('version'); $versionPage->version('latest')->save(['title' => 'Latest Title']); $versionPage->version('changes')->save(['title' => 'Changes Title']); @@ -658,13 +654,6 @@ public function testRenderVersionDetectedRecursive() */ public function testRenderVersionManual() { - // TODO: To be removed in the next PR when caching respects versions - $this->app = $this->app->clone([ - 'options' => [ - 'cache.pages' => false - ] - ]); - $page = $this->app->page('version'); $page->version('latest')->save(['title' => 'Latest Title']); $page->version('changes')->save(['title' => 'Changes Title']); @@ -683,13 +672,6 @@ public function testRenderVersionManual() */ public function testRenderVersionException() { - // TODO: To be removed in the next PR when caching respects versions - $this->app = $this->app->clone([ - 'options' => [ - 'cache.pages' => false - ] - ]); - $page = $this->app->page('version-exception'); try {