Skip to content

Commit

Permalink
ENH Align rendering of image genercated by ImageShortCodeProvider wit…
Browse files Browse the repository at this point in the history
…h generic Image generation (#596)
  • Loading branch information
Maxime Rainville authored Jun 30, 2024
1 parent 70530b1 commit cf787b0
Show file tree
Hide file tree
Showing 9 changed files with 339 additions and 168 deletions.
20 changes: 14 additions & 6 deletions src/ImageManipulation.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)
];
Expand All @@ -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 = (
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;
Expand Down
71 changes: 35 additions & 36 deletions src/Shortcodes/ImageShortcodeProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
$width = null;
$height = null;
$grant = static::getGrant($record);
$src = $record->getURL($grant);
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);
$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(
Expand All @@ -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' => '']
);

// If file was not found then use the Title value from static::find_error_record() for the alt attr
Expand All @@ -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');

Check failure on line 139 in src/Shortcodes/ImageShortcodeProvider.php

View workflow job for this annotation

GitHub Actions / CI / 8.1 mysql57 phplinting

Can't use keyword 'self'. Use 'ImageShortcodeProvider' instead.

// cache it for future reference
if ($fileFound) {
Expand All @@ -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') {
Expand Down Expand Up @@ -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
{
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;
}
}
2 changes: 1 addition & 1 deletion templates/DBFile_download.ss
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 %>
2 changes: 1 addition & 1 deletion templates/DBFile_image.ss
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 %>
1 change: 1 addition & 0 deletions templates/SilverStripe/Assets/Storage/DBFile.ss
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>
1 change: 1 addition & 0 deletions templates/SilverStripe/Assets/Storage/DBFile_Image.ss
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<img $AttributesHTML />
22 changes: 11 additions & 11 deletions tests/php/Shortcodes/FileLinkTrackingTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public function testFileRenameUpdatesDraftAndPublishedPages()

// Live and stage pages both have link to public file
$this->assertStringContainsString(
'<img src="/assets/FileLinkTrackingTest/testscript-test-file.jpg"',
'<img alt="" src="/assets/FileLinkTrackingTest/testscript-test-file.jpg"',
$page->dbObject('Content')->forTemplate()
);
$this->assertStringContainsString(
Expand All @@ -91,7 +91,7 @@ public function testFileRenameUpdatesDraftAndPublishedPages()
/** @var EditableObject $pageLive */
$pageLive = EditableObject::get()->byID($page->ID);
$this->assertStringContainsString(
'<img src="/assets/FileLinkTrackingTest/testscript-test-file.jpg"',
'<img alt="" src="/assets/FileLinkTrackingTest/testscript-test-file.jpg"',
$pageLive->dbObject('Content')->forTemplate()
);
$this->assertStringContainsString(
Expand Down Expand Up @@ -120,14 +120,14 @@ public function testFileRenameUpdatesDraftAndPublishedPages()
// the mocked test location disappears for secure files.
$page = EditableObject::get()->byID($page->ID);
$this->assertStringContainsString(
'<img src="/assets/5a5ee24e44/renamed-test-file.jpg"',
'<img alt="" src="/assets/5a5ee24e44/renamed-test-file.jpg"',
$page->dbObject('Content')->forTemplate()
);
Versioned::withVersionedMode(function () use ($page) {
Versioned::set_stage(Versioned::LIVE);
$pageLive = EditableObject::get()->byID($page->ID);
$this->assertStringContainsString(
'<img src="/assets/FileLinkTrackingTest/testscript-test-file.jpg"',
'<img alt="" src="/assets/FileLinkTrackingTest/testscript-test-file.jpg"',
$pageLive->dbObject('Content')->forTemplate()
);
});
Expand All @@ -137,14 +137,14 @@ public function testFileRenameUpdatesDraftAndPublishedPages()
$image1->publishRecursive();
$page = EditableObject::get()->byID($page->ID);
$this->assertStringContainsString(
'<img src="/assets/FileLinkTrackingTest/renamed-test-file.jpg"',
'<img alt="" src="/assets/FileLinkTrackingTest/renamed-test-file.jpg"',
$page->dbObject('Content')->forTemplate()
);
Versioned::withVersionedMode(function () use ($page) {
Versioned::set_stage(Versioned::LIVE);
$pageLive = EditableObject::get()->byID($page->ID);
$this->assertStringContainsString(
'<img src="/assets/FileLinkTrackingTest/renamed-test-file.jpg"',
'<img alt="" src="/assets/FileLinkTrackingTest/renamed-test-file.jpg"',
$pageLive->dbObject('Content')->forTemplate()
);
});
Expand All @@ -153,14 +153,14 @@ public function testFileRenameUpdatesDraftAndPublishedPages()
$page->publishRecursive();
$page = EditableObject::get()->byID($page->ID);
$this->assertStringContainsString(
'<img src="/assets/FileLinkTrackingTest/renamed-test-file.jpg"',
'<img alt="" src="/assets/FileLinkTrackingTest/renamed-test-file.jpg"',
$page->dbObject('Content')->forTemplate()
);
Versioned::withVersionedMode(function () use ($page) {
Versioned::set_stage(Versioned::LIVE);
$pageLive = EditableObject::get()->byID($page->ID);
$this->assertStringContainsString(
'<img src="/assets/FileLinkTrackingTest/renamed-test-file.jpg"',
'<img alt="" src="/assets/FileLinkTrackingTest/renamed-test-file.jpg"',
$pageLive->dbObject('Content')->forTemplate()
);
});
Expand Down Expand Up @@ -194,7 +194,7 @@ public function testTwoFileRenamesInARowWork()
Versioned::set_stage(Versioned::LIVE);
$livePage = EditableObject::get()->byID($page->ID);
$this->assertStringContainsString(
'<img src="/assets/FileLinkTrackingTest/testscript-test-file.jpg"',
'<img alt="" src="/assets/FileLinkTrackingTest/testscript-test-file.jpg"',
$livePage->dbObject('Content')->forTemplate()
);
});
Expand All @@ -213,7 +213,7 @@ public function testTwoFileRenamesInARowWork()
// Confirm that the correct image is shown in both the draft and live site
$page = EditableObject::get()->byID($page->ID);
$this->assertStringContainsString(
'<img src="/assets/FileLinkTrackingTest/renamed-test-file-second-time.jpg"',
'<img alt="" src="/assets/FileLinkTrackingTest/renamed-test-file-second-time.jpg"',
$page->dbObject('Content')->forTemplate()
);

Expand All @@ -223,7 +223,7 @@ public function testTwoFileRenamesInARowWork()
Versioned::set_stage(Versioned::LIVE);
$pageLive = EditableObject::get()->byID($page->ID);
$this->assertStringContainsString(
'<img src="/assets/FileLinkTrackingTest/renamed-test-file-second-time.jpg"',
'<img alt="" src="/assets/FileLinkTrackingTest/renamed-test-file-second-time.jpg"',
$pageLive->dbObject('Content')->forTemplate()
);
});
Expand Down
Loading

0 comments on commit cf787b0

Please sign in to comment.