-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIX getCachedMarkup returns NULL instead of string if file doesn't exist #562
FIX getCachedMarkup returns NULL instead of string if file doesn't exist #562
Conversation
$url = $url ?? ''; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested without this and it doesn't seem to change anything... seems to not be needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File::getURL()
returns null
silverstripe-assets/src/File.php
Line 896 in 7605cda
public function getURL($grant = false) |
And then we set this value in
$markup
in cache. After we return $item['markup'] in getCachedMarkup
method.So probably, there are some cases where we can get null from
getCachedMarkup
I'll add checking in the
getCachedMarkup
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
@@ -139,11 +141,12 @@ public static function handle_shortcode($arguments, $content, $parser, $shortcod | |||
protected static function getCachedMarkup($cache, $cacheKey, $arguments): string | |||
{ | |||
$item = $cache->get($cacheKey); | |||
if ($item && !empty($item['filename'])) { | |||
$assetStore = Injector::inst()->get(AssetStore::class); | |||
if ($item && !empty($item['filename']) && $assetStore->exists($item['filename'], $item['hash'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ($item && !empty($item['filename']) && $assetStore->exists($item['filename'], $item['hash'])) { | |
if ($item && !empty($item['filename'])) { |
Move this below (see #562 (comment)) - we shouldn't hit the filesystem if we're not even allowing the grant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
@@ -139,11 +141,12 @@ public static function handle_shortcode($arguments, $content, $parser, $shortcod | |||
protected static function getCachedMarkup($cache, $cacheKey, $arguments): string | |||
{ | |||
$item = $cache->get($cacheKey); | |||
if ($item && !empty($item['filename'])) { | |||
$assetStore = Injector::inst()->get(AssetStore::class); | |||
if ($item && !empty($item['filename']) && $assetStore->exists($item['filename'], $item['hash'])) { | |||
// Initiate a protected asset grant if necessary | |||
$allowSessionGrant = static::getGrant(null, $arguments); | |||
if ($allowSessionGrant) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ($allowSessionGrant) { | |
if ($allowSessionGrant && $assetStore->exists($item['filename'], $item['hash'])) { |
Moved as per #562 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
7e2cebf
to
bd9ebb7
Compare
Description
getCachedMarkup
method.handle_shortcode
should return string.Test steps
FileShortcodeProvider::allow_session_grant = true
;Insert Link to a file
in Content field;Parent issue