BUG Make sure the cache shortcode value is used in FileShortcodeProvider

silverstripe · Apr 29, 2024 · bdc1270 · bdc1270
1 parent 0cee400
commit bdc1270
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 9 deletions.
diff --git a/src/Shortcodes/FileShortcodeProvider.php b/src/Shortcodes/FileShortcodeProvider.php
@@ -136,17 +136,30 @@ public static function handle_shortcode($arguments, $content, $parser, $shortcod
      */
     protected static function getCachedMarkup($cache, $cacheKey, $arguments): string
     {
+        // Retrieve the cache markup
         $item = $cache->get($cacheKey);
+        if (empty($item) || empty($item['markup'])) {
+            // This is a cache miss
+            return '';
+        }
+
+        // Check the file exists
+        $filename = $item['filename'];
+        $hash = $item['hash'];
         $assetStore = Injector::inst()->get(AssetStore::class);
-        if ($item && $item['markup'] && !empty($item['filename'])) {
-            // Initiate a protected asset grant if necessary
-            $allowSessionGrant = static::getGrant(null, $arguments);
-            if ($allowSessionGrant && $assetStore->exists($item['filename'], $item['hash'])) {
-                $assetStore->grant($item['filename'], $item['hash']);
-                return $item['markup'];
-            }
+        if (empty($filename) || empty($hash) || !$assetStore->exists($filename, $hash)) {
+            // Cache entry is wrong, delete it
+            $cache->delete($cacheKey);
+            return '';
         }
-        return '';
+
+        // Shortcode can be configured to session grant access to files even if the file is protected
+        $allowSessionGrant = static::getGrant(null, $arguments);
+        if ($allowSessionGrant) {
+            $assetStore->grant($filename, $hash);
+        }
+
+        return $item['markup'];
     }
 
     /**

diff --git a/src/Shortcodes/ImageShortcodeProvider.php b/src/Shortcodes/ImageShortcodeProvider.php
@@ -7,7 +7,6 @@
 use SilverStripe\Assets\Image;
 use SilverStripe\Core\Flushable;
 use SilverStripe\Core\Injector\Injector;
-use SilverStripe\View\HTML;
 use SilverStripe\View\Parsers\ShortcodeHandler;
 use SilverStripe\View\Parsers\ShortcodeParser;
 

diff --git a/tests/php/Shortcodes/FileShortcodeProviderTest.php b/tests/php/Shortcodes/FileShortcodeProviderTest.php
@@ -44,6 +44,8 @@ protected function setUp(): void
             Config::inst()->set(SiteTree::class, 'create_default_pages', true);
             ErrorPage::singleton()->requireDefaultRecords();
         }
+
+        FileShortcodeProvider::getCache()->clear();
     }
 
     protected function tearDown(): void
@@ -179,4 +181,30 @@ public function testMarkupHasStringValue()
             'Test that shortcode with invalid file ID is not parsed.'
         );
     }
+
+    public function testCacheHit()
+    {
+        $testFile = $this->objFromFixture(File::class, 'asdf');
+        $testFileID = $testFile->ID;
+        $tuple = $testFile->File->getValue();
+
+        // Prewarm cache with non-sense value
+        $markup = '<img src="cacheHit">';
+        $key = FileShortcodeProvider::getCacheKey(['id' => (string)$testFileID], '');
+        FileShortcodeProvider::getCache()->set($key, [
+            'markup' => $markup,
+            'filename' => $tuple['Filename'],
+            'hash' => $tuple['Hash']
+        ]);
+
+        // Try to retrieve short code ... which should hit the cache
+        $parser = new ShortcodeParser();
+        $parser->register('file_link', [FileShortcodeProvider::class, 'handle_shortcode']);
+        $fileShortcode = sprintf('[file_link,id=%d]', $testFileID);
+        $this->assertEquals(
+            $markup,
+            $parser->parse(sprintf('[file_link,id=%d]', $testFileID)),
+            'Value Stored in the FileShortcodeProvider cache is returned by the shortcode.'
+        );
+    }
 }