From cf787b008619996e39160bb4ac4f6221ec25f636 Mon Sep 17 00:00:00 2001 From: Maxime Rainville Date: Mon, 1 Jul 2024 10:36:27 +1200 Subject: [PATCH] ENH Align rendering of image genercated by ImageShortCodeProvider with generic Image generation (#596) --- src/ImageManipulation.php | 20 +- src/Shortcodes/ImageShortcodeProvider.php | 71 ++-- templates/DBFile_download.ss | 2 +- templates/DBFile_image.ss | 2 +- .../ImageShortcodeProvider_Image.ss | 1 + .../SilverStripe/Assets/Storage/DBFile.ss | 1 + .../Assets/Storage/DBFile_Image.ss | 1 + tests/php/Shortcodes/FileLinkTrackingTest.php | 22 +- .../Shortcodes/ImageShortcodeProviderTest.php | 387 +++++++++++++----- 9 files changed, 339 insertions(+), 168 deletions(-) create mode 100644 templates/SilverStripe/Assets/Shortcodes/ImageShortcodeProvider_Image.ss create mode 100644 templates/SilverStripe/Assets/Storage/DBFile.ss create mode 100644 templates/SilverStripe/Assets/Storage/DBFile_Image.ss diff --git a/src/ImageManipulation.php b/src/ImageManipulation.php index d07027e5..a2827fc2 100644 --- a/src/ImageManipulation.php +++ b/src/ImageManipulation.php @@ -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 = ( + 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; diff --git a/src/Shortcodes/ImageShortcodeProvider.php b/src/Shortcodes/ImageShortcodeProvider.php index 11ce70b3..0830cfcd 100644 --- a/src/Shortcodes/ImageShortcodeProvider.php +++ b/src/Shortcodes/ImageShortcodeProvider.php @@ -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; $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( @@ -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 @@ -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'); // 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 - { - 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; - } } diff --git a/templates/DBFile_download.ss b/templates/DBFile_download.ss index 05197af8..7eacf0c7 100644 --- a/templates/DBFile_download.ss +++ b/templates/DBFile_download.ss @@ -1 +1 @@ -download="$Basename.ATT"<% else %>download<% end_if %>>$Title +<% include SilverStripe\Assets\Storage\DBFile %> diff --git a/templates/DBFile_image.ss b/templates/DBFile_image.ss index c6a20a2a..5c46dddf 100644 --- a/templates/DBFile_image.ss +++ b/templates/DBFile_image.ss @@ -1 +1 @@ - +<% include SilverStripe\Assets\Storage\DBFile_Image %> \ No newline at end of file diff --git a/templates/SilverStripe/Assets/Shortcodes/ImageShortcodeProvider_Image.ss b/templates/SilverStripe/Assets/Shortcodes/ImageShortcodeProvider_Image.ss new file mode 100644 index 00000000..8d25a333 --- /dev/null +++ b/templates/SilverStripe/Assets/Shortcodes/ImageShortcodeProvider_Image.ss @@ -0,0 +1 @@ +<% include SilverStripe\Assets\Storage\DBFile_Image %> diff --git a/templates/SilverStripe/Assets/Storage/DBFile.ss b/templates/SilverStripe/Assets/Storage/DBFile.ss new file mode 100644 index 00000000..05197af8 --- /dev/null +++ b/templates/SilverStripe/Assets/Storage/DBFile.ss @@ -0,0 +1 @@ +download="$Basename.ATT"<% else %>download<% end_if %>>$Title diff --git a/templates/SilverStripe/Assets/Storage/DBFile_Image.ss b/templates/SilverStripe/Assets/Storage/DBFile_Image.ss new file mode 100644 index 00000000..c6a20a2a --- /dev/null +++ b/templates/SilverStripe/Assets/Storage/DBFile_Image.ss @@ -0,0 +1 @@ + diff --git a/tests/php/Shortcodes/FileLinkTrackingTest.php b/tests/php/Shortcodes/FileLinkTrackingTest.php index b65a7def..3e81bf71 100644 --- a/tests/php/Shortcodes/FileLinkTrackingTest.php +++ b/tests/php/Shortcodes/FileLinkTrackingTest.php @@ -78,7 +78,7 @@ public function testFileRenameUpdatesDraftAndPublishedPages() // Live and stage pages both have link to public file $this->assertStringContainsString( - 'dbObject('Content')->forTemplate() ); $this->assertStringContainsString( @@ -91,7 +91,7 @@ public function testFileRenameUpdatesDraftAndPublishedPages() /** @var EditableObject $pageLive */ $pageLive = EditableObject::get()->byID($page->ID); $this->assertStringContainsString( - 'dbObject('Content')->forTemplate() ); $this->assertStringContainsString( @@ -120,14 +120,14 @@ public function testFileRenameUpdatesDraftAndPublishedPages() // the mocked test location disappears for secure files. $page = EditableObject::get()->byID($page->ID); $this->assertStringContainsString( - 'dbObject('Content')->forTemplate() ); Versioned::withVersionedMode(function () use ($page) { Versioned::set_stage(Versioned::LIVE); $pageLive = EditableObject::get()->byID($page->ID); $this->assertStringContainsString( - 'dbObject('Content')->forTemplate() ); }); @@ -137,14 +137,14 @@ public function testFileRenameUpdatesDraftAndPublishedPages() $image1->publishRecursive(); $page = EditableObject::get()->byID($page->ID); $this->assertStringContainsString( - 'dbObject('Content')->forTemplate() ); Versioned::withVersionedMode(function () use ($page) { Versioned::set_stage(Versioned::LIVE); $pageLive = EditableObject::get()->byID($page->ID); $this->assertStringContainsString( - 'dbObject('Content')->forTemplate() ); }); @@ -153,14 +153,14 @@ public function testFileRenameUpdatesDraftAndPublishedPages() $page->publishRecursive(); $page = EditableObject::get()->byID($page->ID); $this->assertStringContainsString( - 'dbObject('Content')->forTemplate() ); Versioned::withVersionedMode(function () use ($page) { Versioned::set_stage(Versioned::LIVE); $pageLive = EditableObject::get()->byID($page->ID); $this->assertStringContainsString( - 'dbObject('Content')->forTemplate() ); }); @@ -194,7 +194,7 @@ public function testTwoFileRenamesInARowWork() Versioned::set_stage(Versioned::LIVE); $livePage = EditableObject::get()->byID($page->ID); $this->assertStringContainsString( - 'dbObject('Content')->forTemplate() ); }); @@ -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( - 'dbObject('Content')->forTemplate() ); @@ -223,7 +223,7 @@ public function testTwoFileRenamesInARowWork() Versioned::set_stage(Versioned::LIVE); $pageLive = EditableObject::get()->byID($page->ID); $this->assertStringContainsString( - 'dbObject('Content')->forTemplate() ); }); diff --git a/tests/php/Shortcodes/ImageShortcodeProviderTest.php b/tests/php/Shortcodes/ImageShortcodeProviderTest.php index b3109a1a..35e6102e 100644 --- a/tests/php/Shortcodes/ImageShortcodeProviderTest.php +++ b/tests/php/Shortcodes/ImageShortcodeProviderTest.php @@ -15,6 +15,7 @@ use SilverStripe\Core\Injector\Injector; use SilverStripe\Security\InheritedPermissions; use SilverStripe\Security\Member; +use DOMDocument; class ImageShortcodeProviderTest extends SapphireTest { @@ -46,33 +47,46 @@ protected function tearDown(): void public function testShortcodeHandlerDoesNotFallBackToFileProperties() { $image = $this->objFromFixture(Image::class, 'imageWithTitle'); - $parser = new ShortcodeParser(); - $parser->register('image', [ImageShortcodeProvider::class, 'handle_shortcode']); - - $this->assertEquals( - sprintf( - '', - $image->Link() - ), - $parser->parse(sprintf('[image id=%d]', $image->ID)) + $this->assertImageTag( + [ + 'src' => $image->Link(), + 'alt' => '', + 'width' => '300', + 'height' => '300', + 'title' => false, + 'id' => false, + ], + sprintf('[image id=%d]', $image->ID), + 'Shortcode handler does not fall back to file properties to generate alt attribute' ); } public function testShortcodeHandlerUsesShortcodeProperties() { $image = $this->objFromFixture(Image::class, 'imageWithTitle'); - $parser = new ShortcodeParser(); - $parser->register('image', [ImageShortcodeProvider::class, 'handle_shortcode']); + $this->assertImageTag( + [ + 'src' => $image->Link(), + 'alt' => 'Alt content', + 'title' => 'Title content', + ], + sprintf('[image id="%d" alt="Alt content" title="Title content"]', $image->ID), + 'Shortcode handler uses properties from shortcode' + ); + } - $this->assertEquals( - sprintf( - 'Alt content', - $image->Link() - ), - $parser->parse(sprintf( - '[image id="%d" alt="Alt content" title="Title content"]', - $image->ID - )) + public function testShortcodeHandlerHandlesEntities() + { + $image = $this->objFromFixture(Image::class, 'imageWithTitle'); + // Because we are using a DOMDocument to read the values, we need to compare to non-escaped value + $this->assertImageTag( + [ + 'src' => $image->Link(), + 'alt' => 'Alt & content', + 'title' => '" Hockey > Rugby "', + ], + sprintf('[image id="%d" alt="Alt & content" title="" Hockey > Rugby ""]', $image->ID), + 'Shortcode handler can handle HTML entities' ); } @@ -101,10 +115,12 @@ public function testShorcodeRegenrator() $image->ID, $image->Link() ), - $parser->parse(sprintf( - '[image id="%d" alt="" title=""]', - $image->ID - )), + $parser->parse( + sprintf( + '[image id="%d" alt="" title=""]', + $image->ID + ) + ), "Shortcode regeneration properly handles empty attributes" ); } @@ -112,54 +128,65 @@ public function testShorcodeRegenrator() public function testShortcodeHandlerAddsDefaultAttributes() { $image = $this->objFromFixture(Image::class, 'imageWithoutTitle'); - $parser = new ShortcodeParser(); - $parser->register('image', [ImageShortcodeProvider::class, 'handle_shortcode']); - - $this->assertEquals( - sprintf( - '', - $image->Link() - ), - $parser->parse(sprintf( - '[image id="%d"]', - $image->ID - )) + $this->assertImageTag( + [ + 'src' => $image->Link(), + 'alt' => '', + 'width' => '300', + 'height' => '300', + 'title' => false, + 'id' => false, + ], + sprintf('[image id=%d]', $image->ID), + 'Shortcode handler adds default attributes' ); } public function testShortcodeHandlerDoesNotResampleToNonIntegerImagesSizes() { $image = $this->objFromFixture(Image::class, 'imageWithoutTitle'); - $parser = new ShortcodeParser(); - $parser->register('image', [ImageShortcodeProvider::class, 'handle_shortcode']); - - $this->assertEquals( - sprintf( - '', - $image->Link() - ), - $parser->parse(sprintf( - '[image id="%d" alt="" width="50%%" height="auto"]', - $image->ID - )) + $this->assertImageTag( + [ + 'src' => $image->Link(), + 'alt' => '', + 'width' => '50%', + 'title' => false, + 'id' => false, + ], + sprintf('[image id="%d" alt="" width="50%%"]', $image->ID), + 'Shortcode handler does resample to non-integer image sizes' ); } public function testShortcodeHandlerFailsGracefully() { - $parser = new ShortcodeParser(); - $parser->register('image', [ImageShortcodeProvider::class, 'handle_shortcode']); - $nonExistentImageID = File::get()->max('ID') + 1; - $expected = 'Image not found'; - $shortcodes = [ + + $this->assertImageTag( + [ + 'src' => false, + 'alt' => 'Image not found', + 'width' => false, + 'height' => false, + 'title' => false, + 'id' => false, + ], '[image id="' . $nonExistentImageID . '"]', + 'Shortcode handler fails gracefully when image does not exist' + ); + + $this->assertImageTag( + [ + 'src' => false, + 'alt' => 'Image not found', + 'width' => false, + 'height' => false, + 'title' => false, + 'id' => false, + ], '[image id="' . $nonExistentImageID . '" alt="my-alt-attr"]', - ]; - foreach ($shortcodes as $shortcode) { - $actual = $parser->parse($shortcode); - $this->assertEquals($expected, $actual); - } + 'Shortcode handler fails gracefully when image does not exist asd overrides alt attribute' + ); } public function testMissingImageDoesNotCache() @@ -189,39 +216,119 @@ public function testMissingImageDoesNotCache() public function testLazyLoading() { - $parser = new ShortcodeParser(); - $parser->register('image', [ImageShortcodeProvider::class, 'handle_shortcode']); + $image = $this->objFromFixture(Image::class, 'imageWithoutTitle'); + $id = $image->ID; + + // regular shortcode + $this->assertImageTag( + [ + 'src' => $image->ResizedImage(300, 200)->Link(), + 'alt' => '', + 'width' => '300', + 'height' => '200', + 'title' => false, + 'id' => false, + 'loading' => 'lazy', + ], + '[image id="' . $id . '" width="300" height="200"]', + 'Lazy loading is enabled by default' + ); - $id = $this->objFromFixture(Image::class, 'imageWithTitle')->ID; // regular shortcode - $shortcode = '[image id="' . $id . '" width="300" height="200"]'; - $this->assertStringContainsString('loading="lazy"', $parser->parse($shortcode)); + $this->assertImageTag( + [ + 'src' => $image->Link(), + 'alt' => '', + 'width' => '300', + 'height' => '300', + 'title' => false, + 'id' => false, + 'loading' => 'lazy', + ], + '[image id="' . $id . '"]', + 'Lazy loading still works if width and height are not provided. Dimensions are read from the file instead.' + ); // missing width - $shortcode = '[image id="' . $id . '" height="200"]'; - $this->assertStringNotContainsString('loading="lazy"', $parser->parse($shortcode)); + $this->assertImageTag( + [ + 'src' => $image->Link(), + 'alt' => '', + 'height' => '200', + 'title' => false, + 'id' => false, + 'loading' => false, + ], + '[image id="' . $id . '" height="200"]', + 'If the width of the image can not be determined, lazy loading is not applied' + ); // missing height - $shortcode = '[image id="' . $id . '" width="300"]'; - $this->assertStringNotContainsString('loading="lazy"', $parser->parse($shortcode)); + $this->assertImageTag( + [ + 'src' => $image->Link(), + 'alt' => '', + 'width' => '300', + 'title' => false, + 'id' => false, + 'loading' => false, + ], + '[image id="' . $id . '" width="300"]', + 'If the height of the image can not be determined, lazy loading is not applied' + ); // loading="eager" - $shortcode = '[image id="' . $id . '" width="300" height="200" loading="eager"]'; - $this->assertStringNotContainsString('loading="lazy"', $parser->parse($shortcode)); + $this->assertImageTag( + [ + 'src' => $image->ResizedImage(300, 200)->Link(), + 'alt' => '', + 'height' => '200', + 'width' => '300', + 'title' => false, + 'id' => false, + 'loading' => false, + ], + '[image id="' . $id . '" width="300" height="200" loading="eager"]', + 'Shortcode can force eager loading' + ); // loading="nonsense" - $shortcode = '[image id="' . $id . '" width="300" height="200" loading="nonsense"]'; - $this->assertStringContainsString('loading="lazy"', $parser->parse($shortcode)); + $this->assertImageTag( + [ + 'src' => $image->ResizedImage(300, 200)->Link(), + 'alt' => '', + 'height' => '200', + 'width' => '300', + 'title' => false, + 'id' => false, + 'loading' => 'lazy', + ], + '[image id="' . $id . '" width="300" height="200" loading="nonsense"]', + 'Invalid loading value in short code are ignored and the image is lazy loaded' + ); // globally disabled - Config::withConfig(function () use ($id, $parser) { - Config::modify()->set(Image::class, 'lazy_loading_enabled', false); - // clear-provider-cache is so that we don't get a cached result from the 'regular shortcode' - // assertion earlier in this function from ImageShortCodeProvider::handle_shortcode() - $shortcode = '[image id="' . $id . '" width="300" height="200" clear-provider-cache="1"]'; - $this->assertStringNotContainsString('loading="lazy"', $parser->parse($shortcode)); - }); + Config::withConfig( + function () use ($id, $image) { + Config::modify()->set(Image::class, 'lazy_loading_enabled', false); + // clear-provider-cache is so that we don't get a cached result from the 'regular shortcode' + // assertion earlier in this function from ImageShortCodeProvider::handle_shortcode() + $this->assertImageTag( + [ + 'src' => $image->ResizedImage(300, 200)->Link(), + 'alt' => '', + 'height' => '200', + 'width' => '300', + 'title' => false, + 'id' => false, + 'loading' => false, + ], + '[image id="' . $id . '" width="300" height="200" clear-provider-cache="1"]', + 'If lazy loading is disabled globally the image is not lazy loaded' + ); + } + ); } public function testRegenerateShortcode() @@ -245,10 +352,13 @@ public function testRegenerateShortcode() 'class' => 'leftAlone ss-htmleditorfield-file image', ]; $shortHash = substr($image->getHash(), 0, 10); - $expected = implode(' ', [ + $expected = implode( + ' ', + [ '[image id="' . $image->ID . '" src="/assets/folder/' . $shortHash . '/test-image.png" width="550"', 'height="366" class="leftAlone ss-htmleditorfield-file image"]' - ]); + ] + ); $parsedFileID = new ParsedFileID($image->getFilename(), $image->getHash()); $html = ImageShortcodeProvider::regenerate_shortcode($args, '', '', 'image'); $this->assertSame($expected, $html); @@ -271,77 +381,93 @@ public function testRegenerateShortcode() public function testEmptyAttributesAreRemoved() { $image = $this->objFromFixture(Image::class, 'imageWithTitle'); - $parser = new ShortcodeParser(); - $parser->register('image', [ImageShortcodeProvider::class, 'handle_shortcode']); - // Note that alt is allowed to be empty - $this->assertEquals( - sprintf( - '', - $image->Link() - ), - $parser->parse(sprintf('[image id=%d alt="" class=""]', $image->ID)) + $this->assertImageTag( + [ + 'src' => $image->Link(), + 'alt' => '', + 'width' => '300', + 'height' => '300', + 'title' => false, + 'id' => false, + 'loading' => 'lazy', + 'class' => false, + ], + sprintf('[image id=%d alt="" class=""]', $image->ID), + 'Image shortcode does not render empty attributes' ); } public function testOnlyWhitelistedAttributesAllowed() { $image = $this->objFromFixture(Image::class, 'imageWithoutTitle'); - $parser = new ShortcodeParser(); - $parser->register('image', [ImageShortcodeProvider::class, 'handle_shortcode']); $whitelist = ImageShortcodeProvider::config()->get('attribute_whitelist'); - $attributes = ''; + $attributeString = ''; + $attributes = []; foreach ($whitelist as $attrName) { if ($attrName === 'src') { continue; } - $attributes .= $attrName . '="' . $attrName . '" '; + $attributeString .= $attrName . '="' . $attrName . '" '; + $attributes[$attrName] = $attrName; } - $this->assertEquals( - sprintf( - '', - $image->Link(), - trim($attributes) + $this->assertImageTag( + array_merge( + $attributes, + [ + 'src' => $image->Link(), + 'id' => false, + 'loading' => 'lazy', + 'style' => false, + 'data-some-value' => false + ] ), - $parser->parse(sprintf( + sprintf( '[image id="%d" %s style="width:100px" data-some-value="my-data"]', $image->ID, - trim($attributes) - )) + trim($attributeString) + ), + 'Image shortcode does not render attributes not in whitelist' ); } public function testWhiteIsConfigurable() { $image = $this->objFromFixture(Image::class, 'imageWithoutTitle'); - $parser = new ShortcodeParser(); - $parser->register('image', [ImageShortcodeProvider::class, 'handle_shortcode']); $whitelist = ImageShortcodeProvider::config()->get('attribute_whitelist'); - $attributes = ''; + $attributeString = ''; + $attributes = []; foreach ($whitelist as $attrName) { if ($attrName === 'src') { continue; } - $attributes .= $attrName . '="' . $attrName . '" '; + $attributeString .= $attrName . '="' . $attrName . '" '; + $attributes[$attrName] = $attrName; } // Allow new whitelisted attribute Config::modify()->merge(ImageShortcodeProvider::class, 'attribute_whitelist', ['data-some-value']); - $this->assertEquals( - sprintf( - '', - $image->Link(), - trim($attributes) . ' data-some-value="my-data"' + $this->assertImageTag( + array_merge( + $attributes, + [ + 'src' => $image->Link(), + 'id' => false, + 'loading' => 'lazy', + 'style' => false, + 'data-some-value' => 'my-data' + ] ), - $parser->parse(sprintf( + sprintf( '[image id="%d" %s style="width:100px" data-some-value="my-data"]', $image->ID, - trim($attributes) - )) + trim($attributeString) + ), + 'Image shortcode does not render attributes not in whitelist' ); } @@ -380,4 +506,39 @@ public function testCreateImageTag(string $expected, array $attributes) { $this->assertEquals($expected, ImageShortcodeProvider::createImageTag($attributes)); } + + /** + * This method will assert that the $tag will contain an image with specific attributes and values + * + * @param array $attrs Key pair values of attributes and values. + * If the value is FALSE we assume that it is not present + */ + private function assertImageTag(array $attrs, string $shortcode, $message = ""): void + { + $parser = new ShortcodeParser(); + $parser->register('image', [ImageShortcodeProvider::class, 'handle_shortcode']); + $tag = $parser->parse($shortcode); + + $doc = new DOMDocument(); + $doc->loadHTML($tag); + $node = $doc->getElementsByTagName('img')->item(0); + $nodeAttrs = $node->attributes; + + foreach ($attrs as $key => $value) { + $nodeAttr = $nodeAttrs->getNamedItem($key); + if ($value === false) { + if ($nodeAttr !== null) { + $this->fail("$message\nImage should not contain attribute '$key'\n$tag"); + } + } else { + if ($nodeAttr === null) { + $this->fail("$message\nImage should contain attribute '$key'\n$tag"); + } + if ($nodeAttr->nodeValue !== $value) { + $this->fail("$message\nImage attribute '$key' should have value '$value'\n$tag"); + } + } + $this->assertTrue(true, 'Suppress warning about not having any assertion'); + } + } }