-
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
Align rendering of image genercated by ImageShortCodeProvider with generic Image generation #596
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1145,8 +1145,8 @@ protected function getDefaultAttributes(): array | |
$attributes = []; | ||
if ($this->getIsImage()) { | ||
$attributes = [ | ||
'width' => $this->getWidth(), | ||
'height' => $this->getHeight(), | ||
'width' => $this->getWidth() ?: null, | ||
'height' => $this->getHeight() ?: null, | ||
'alt' => $this->getTitle(), | ||
'src' => $this->getURL(false) | ||
]; | ||
|
@@ -1171,11 +1171,19 @@ public function getAttributes() | |
|
||
$attributes = array_merge($defaultAttributes, $this->attributes); | ||
|
||
// We need to suppress the `loading="eager"` attribute after we merge the default attributes | ||
if (isset($attributes['loading']) && $attributes['loading'] === 'eager') { | ||
unset($attributes['loading']); | ||
if (isset($attributes['loading'])) { | ||
// Check if the dimensions for the image have been set | ||
$dimensionsUnset = ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The shortcode had some logic to make sure that if no size was provided, the image would be eager loaded. This logic made more sense here in Image manipulation than in the original one. -- The shortcode also had some logic to handle the scenario where only the height or only the width was provided. In that scenario, the missing attribute would be omitted. Unfortunately, ImageManipulation really wants to read the size from the actual Image. This would lead to the aspect ratio of the image being changed. So I updated ImageShortcodeProvider to explicitely set the missing value to |
||
empty($attributes['width']) || | ||
empty($attributes['height']) | ||
); | ||
|
||
// We need to suppress the `loading="eager"` attribute after we merge the default attributes | ||
// or if dimensions are not set | ||
if ($attributes['loading'] === 'eager' || $dimensionsUnset) { | ||
unset($attributes['loading']); | ||
} | ||
} | ||
|
||
$this->extend('updateAttributes', $attributes); | ||
|
||
return $attributes; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
use SilverStripe\Assets\Image; | ||
use SilverStripe\Core\Flushable; | ||
use SilverStripe\Core\Injector\Injector; | ||
use SilverStripe\Dev\Deprecation; | ||
use SilverStripe\View\Parsers\ShortcodeHandler; | ||
use SilverStripe\View\Parsers\ShortcodeParser; | ||
|
||
|
@@ -73,26 +74,43 @@ public static function handle_shortcode($args, $content, $parser, $shortcode, $e | |
return null; // There were no suitable matches at all. | ||
} | ||
|
||
// Grant access to file if necessary | ||
if (static::getGrant($record)) { | ||
$record->grantFile(); | ||
} | ||
|
||
// Check if a resize is required | ||
$manipulatedRecord = $record; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The old approach was basically building the I'm using standard Image manipulation to assign the correct size and attribute to a manipulated image instead. |
||
$width = null; | ||
$height = null; | ||
$grant = static::getGrant($record); | ||
$src = $record->getURL($grant); | ||
maxime-rainville marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if ($record instanceof Image) { | ||
$width = isset($args['width']) ? (int) $args['width'] : null; | ||
$height = isset($args['height']) ? (int) $args['height'] : null; | ||
|
||
// Resize the image if custom dimensions are provided | ||
$hasCustomDimensions = ($width && $height); | ||
if ($hasCustomDimensions && (($width != $record->getWidth()) || ($height != $record->getHeight()))) { | ||
$resized = $record->ResizedImage($width, $height); | ||
$resized = $manipulatedRecord->ResizedImage($width, $height); | ||
// Make sure that the resized image actually returns an image | ||
if ($resized) { | ||
$src = $resized->getURL($grant); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The grant isn't needed here. Session grants apply to the original file and all its variant. |
||
$manipulatedRecord = $resized; | ||
} | ||
} | ||
|
||
// If only one of width or height is provided, explicitly unset the other | ||
if ($width && !$height) { | ||
$args['height'] = false; | ||
} elseif (!$width && $height) { | ||
$args['width'] = false; | ||
} | ||
} | ||
|
||
// Determine whether loading="lazy" is set | ||
$args = ImageShortcodeProvider::updateLoadingValue($args, $width, $height); | ||
// Set lazy loading attribute | ||
if (!empty($args['loading'])) { | ||
$loading = strtolower($args['loading']); | ||
unset($args['loading']); | ||
$manipulatedRecord = $manipulatedRecord->LazyLoad($loading !== 'eager'); | ||
} | ||
|
||
// Build the HTML tag | ||
$attrs = array_merge( | ||
|
@@ -101,7 +119,7 @@ public static function handle_shortcode($args, $content, $parser, $shortcode, $e | |
// Use all other shortcode arguments | ||
$args, | ||
// But enforce some values | ||
['id' => '', 'src' => $src] | ||
['id' => '', 'src' => ''] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
); | ||
|
||
// If file was not found then use the Title value from static::find_error_record() for the alt attr | ||
|
@@ -111,11 +129,14 @@ public static function handle_shortcode($args, $content, $parser, $shortcode, $e | |
|
||
// Clean out any empty attributes (aside from alt) and anything not whitelisted | ||
$whitelist = static::config()->get('attribute_whitelist'); | ||
$attrs = array_filter($attrs ?? [], function ($v, $k) use ($whitelist) { | ||
return in_array($k, $whitelist) && (strlen(trim($v ?? '')) || $k === 'alt'); | ||
}, ARRAY_FILTER_USE_BOTH); | ||
foreach ($attrs as $key => $value) { | ||
if (in_array($key, $whitelist) && (strlen(trim($value ?? '')) || in_array($key, ['alt', 'width', 'height']))) { | ||
$manipulatedRecord = $manipulatedRecord->setAttribute($key, html_entity_decode($value)); | ||
} | ||
} | ||
|
||
$markup = ImageShortcodeProvider::createImageTag($attrs); | ||
// We're calling renderWith() with an explicit template in case someone wants to use a custom template | ||
$markup = $manipulatedRecord->renderWith(self::class . '_Image'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're using a |
||
|
||
// cache it for future reference | ||
if ($fileFound) { | ||
|
@@ -131,9 +152,12 @@ public static function handle_shortcode($args, $content, $parser, $shortcode, $e | |
|
||
/** | ||
* Construct and return HTML image tag. | ||
* | ||
* @deprecated 2.3.0 | ||
*/ | ||
public static function createImageTag(array $attributes) : string | ||
{ | ||
Deprecation::notice('2.3.0', 'Will be removed without equivalent functionality to replace it.'); | ||
$preparedAttributes = ''; | ||
foreach ($attributes as $attributeKey => $attributeValue) { | ||
if (strlen($attributeValue ?? '') > 0 || $attributeKey === 'alt') { | ||
|
@@ -209,29 +233,4 @@ protected static function find_error_record($errorCode) | |
'Title' => _t(__CLASS__ . '.IMAGENOTFOUND', 'Image not found'), | ||
]); | ||
} | ||
|
||
/** | ||
* Updated the loading attribute which is used to either lazy-load or eager-load images | ||
* Eager-load is the default browser behaviour so when eager loading is specified, the | ||
* loading attribute is omitted | ||
* | ||
* @param array $args | ||
* @param int|null $width | ||
* @param int|null $height | ||
* @return array | ||
*/ | ||
private static function updateLoadingValue(array $args, ?int $width, ?int $height): array | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is private so no need to deprecate the method. We can just nuke it. |
||
{ | ||
if (!Image::getLazyLoadingEnabled()) { | ||
return $args; | ||
} | ||
if (isset($args['loading']) && $args['loading'] == 'eager') { | ||
// per image override - unset the loading attribute unset to eager load (default browser behaviour) | ||
unset($args['loading']); | ||
} elseif ($width && $height) { | ||
// width and height must be present to prevent content shifting | ||
$args['loading'] = 'lazy'; | ||
} | ||
return $args; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
<a href="$URL.ATT" title="$Title" <% if $Basename %>download="$Basename.ATT"<% else %>download<% end_if %>>$Title</a> | ||
<% include SilverStripe\Assets\Storage\DBFile %> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
<img $AttributesHTML /> | ||
<% include SilverStripe\Assets\Storage\DBFile_Image %> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
<% include SilverStripe\Assets\Storage\DBFile_Image %> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
<a href="$URL.ATT" title="$Title" <% if $Basename %>download="$Basename.ATT"<% else %>download<% end_if %>>$Title</a> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
<img $AttributesHTML /> |
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.
When the short code was rendering an non-existent image,
getWidth
andgetHeight
would return 0. This would lead to awidth="0"
attribute in the HTML.Defaulting to null here fixes this.