ENH Align rendering of image genercated by ImageShortCodeProvider wit…

…h generic Image generation
silverstripe · Jun 5, 2024 · 1e88f22 · 1e88f22
1 parent c4081b9
commit 1e88f22
Show file tree

Hide file tree

Showing 9 changed files with 319 additions and 166 deletions.
diff --git 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,25 @@ 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']) ||
+                $attributes['width'] === 'auto' ||
+                $attributes['height'] === 'auto'
+            );
+
+            // 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
@@ -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, set the other to auto
+            if ($width && !$height) {
+                $args['height'] = 'auto';
+            } elseif (!$width && $height) {
+                $args['width'] = 'auto';
+            }
         }
 
-        // Determine whether loading="lazy" is set
-        $args = self::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 ?? '')) || $key === 'alt')) {
+                $manipulatedRecord = $manipulatedRecord->setAttribute($key, $value);
+            }
+        }
 
-        $markup = self::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
@@ -1 +1 @@
-<a href="$URL.ATT" title="$Title" <% if $Basename %>download="$Basename.ATT"<% else %>download<% end_if %>>$Title</a>
+<% include SilverStripe\Assets\Storage\DBFile %>
diff --git a/templates/DBFile_image.ss b/templates/DBFile_image.ss
@@ -1 +1 @@
-<img $AttributesHTML />
+<% include SilverStripe\Assets\Storage\DBFile_Image %>
diff --git a/templates/SilverStripe/Assets/Shortcodes/ImageShortcodeProvider_Image.ss 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
@@ -0,0 +1 @@
+<a href="$URL.ATT" title="$Title" <% if $Basename %>download="$Basename.ATT"<% else %>download<% end_if %>>$Title</a>
diff --git a/templates/SilverStripe/Assets/Storage/DBFile_Image.ss b/templates/SilverStripe/Assets/Storage/DBFile_Image.ss
@@ -0,0 +1 @@
+<img $AttributesHTML />
diff --git 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(
-            '<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()
             );
         });