ENH Align rendering of image genercated by ImageShortCodeProvider wit…

…h generic Image generation (silverstripe#596)

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;

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;
-    }
 }

templates/DBFile_download.ss

@@ -1 +1 @@
-<a href="$URL.ATT" title="$Title" <% if $Basename %>download="$Basename.ATT"<% else %>download<% end_if %>>$Title</a>
+<% include SilverStripe\Assets\Storage\DBFile %>

templates/DBFile_image.ss

@@ -1 +1 @@
-<img $AttributesHTML />
+<% include SilverStripe\Assets\Storage\DBFile_Image %>

templates/SilverStripe/Assets/Shortcodes/ImageShortcodeProvider_Image.ss

@@ -0,0 +1 @@
+<% include SilverStripe\Assets\Storage\DBFile_Image %>

templates/SilverStripe/Assets/Storage/DBFile.ss

@@ -0,0 +1 @@
+<a href="$URL.ATT" title="$Title" <% if $Basename %>download="$Basename.ATT"<% else %>download<% end_if %>>$Title</a>

templates/SilverStripe/Assets/Storage/DBFile_Image.ss

@@ -0,0 +1 @@
+<img $AttributesHTML />

tests/php/Shortcodes/FileLinkTrackingTest.php

@@ -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(
@@ -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(
@@ -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()
             );
         });
@@ -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()
             );
         });
@@ -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()
             );
         });
@@ -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()
             );
         });
@@ -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()
         );
 
@@ -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()
             );
         });