From 41378dc6054c04ff6a09a1d191b1aab5c4d17920 Mon Sep 17 00:00:00 2001
From: Maxime Rainville <maxime@silverstripe.com>
Date: Tue, 30 Apr 2024 13:51:22 +1200
Subject: [PATCH] FIX Make sure the cache shortcode value is used in
 FileShortcodeProvider (#598)

---
 src/Shortcodes/FileShortcodeProvider.php      | 31 ++++++++++++++-----
 src/Shortcodes/ImageShortcodeProvider.php     |  1 -
 .../Shortcodes/FileShortcodeProviderTest.php  | 28 +++++++++++++++++
 3 files changed, 51 insertions(+), 9 deletions(-)

diff --git a/src/Shortcodes/FileShortcodeProvider.php b/src/Shortcodes/FileShortcodeProvider.php
index 4f4a4459..311abf42 100644
--- a/src/Shortcodes/FileShortcodeProvider.php
+++ b/src/Shortcodes/FileShortcodeProvider.php
@@ -136,17 +136,32 @@ 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)) {
+            // This is a cache miss
+            return '';
+        }
+
+        // Destructure our cache entry
+        $markup = $item['markup'] ?? '';
+        $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'];
-            }
+
+        // Validate the cache entry
+        if (empty($markup) || empty($filename) || empty($hash) || !$assetStore->exists($filename, $hash)) {
+            // Cache entry is wrong, delete it
+            $cache->delete($cacheKey);
+            return '';
         }
-        return '';
+
+        // Shortcode can be configured to allow session grant to files even if the file is protected
+        if (static::getGrant(null, $arguments)) {
+            $assetStore->grant($filename, $hash);
+        }
+
+        return $markup;
     }
 
     /**
diff --git a/src/Shortcodes/ImageShortcodeProvider.php b/src/Shortcodes/ImageShortcodeProvider.php
index 9b8cd419..2e5c7b9c 100644
--- 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
index 16c7c4ec..9c079ffc 100644
--- 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 nonsense 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.'
+        );
+    }
 }