DEP Upgrade to intervention/image 3 #621

GuySartorelli · 2024-07-09T23:01:32Z

Also add strict typing to Image_Backend and InterventionBackend.

Most changes are documented to some extent in the Upgrade Guide.
Changes to exceptions aren't documented, but it basically boils down to:
NotReadableException => DecoderException
NotSupportedException => EncoderException
NotWritableException => EncoderException
ImageException => RuntimeException

Issue

Upgrade to intervention 3 #619

GuySartorelli · 2024-07-10T02:05:00Z

src/InterventionBackend.php

-        //skip the `getImageResource` method because we don't want to load the resource just to destroy it
-        if ($this->image) {
-            $this->image->destroy();
-        }


Intervention/image#1273 That method is no longer available and there's no replacement.

GuySartorelli · 2024-07-10T02:05:24Z

composer.json

@@ -23,7 +23,7 @@
        "silverstripe/framework": "^6",
        "silverstripe/vendor-plugin": "^2",
        "symfony/filesystem": "^6.1",
-        "intervention/image": "^2.7.2",
+        "intervention/image": "^3.7",


Had to go with the latest version in the end because it introduces some API that we need.

GuySartorelli · 2024-07-10T02:06:03Z

src/Conversion/InterventionImageFileConverter.php

-        return $this->supportedByIntervention($fromExtension, $backend) && $this->supportedByIntervention($toExtension, $backend);
+        /** @var InterventionBackend $backend */
+        $driver = $backend->getImageManager()->driver();
+        return $driver->supports($fromExtension) && $driver->supports($toExtension);


This is built in now! 🎉 No need for our hardcoded nonsense lol

GuySartorelli · 2024-07-10T02:07:02Z

src/Image_Backend.php

All changes in this interface are mostly just adding strict typing to reduce the chance of developer error or things accidentally working and later being regressed.

Any changes not related to strict typing will be explicitly commented on.

GuySartorelli · 2024-07-10T02:08:48Z

src/InterventionBackend.php

-            // Fix image orientation
-            try {
-                $resource->orientate();
-            } catch (NotSupportedException $e) {
-                // noop - we can't orientate, don't worry about it


orientate() was replaced with orient() - but that happens automatically by default now, so we don't have to do it manually here.

GuySartorelli · 2024-07-10T02:12:17Z

src/InterventionBackend.php

-                return $resource->resize(
-                    $width,
-                    $height,
-                    function (Constraint $constraint) use ($useAsMinimum) {
-                        $constraint->aspectRatio();
-                        if (!$useAsMinimum) {
-                            $constraint->upsize();
-                        }
-                    }
-                );
+                if ($useAsMinimum) {
+                    return $resource->scale($width, $height);
+                }
+                return $resource->scaleDown($width, $height);


If $useAsMinimum is true, upscaling is allowed. If false, scaleDown() is used to restrict scaling to only allow reducing the image size.

GuySartorelli · 2024-07-10T02:14:38Z

src/Image_Backend.php

-    public function paddedResize($width, $height, $backgroundColor = "FFFFFF", $transparencyPercent = 0);
+    public function paddedResize(string $width, string $height, string $backgroundColour = 'FFFFFF', int $transparencyPercent = 0): static;


Change color to colour - these are abstracted by the ImageManipulation trait anyway so most people proably won't be calling it directly, much less with named arguments.

GuySartorelli · 2024-07-10T02:15:00Z

src/Image_Backend.php

-    public function croppedResize($width, $height);
+    public function croppedResize(int $width, int $height, string $position): static;


Added $position argument for advanced control, should it be needed.

GuySartorelli · 2024-07-10T02:15:12Z

src/Image_Backend.php

-    public function crop($top, $left, $width, $height);
+    public function crop(int $top, int $left, int $width, int $height, string $position, string $backgroundColour = 'FFFFFF'): static;


Added $position and $backgroundColour arguments for advanced control, should it be needed.

GuySartorelli · 2024-07-10T02:16:17Z

src/InterventionBackend.php

-        // caclulate the background colour
-        $background = $resource->getDriver()->parseColor($backgroundColor)->format('array');
-        // convert transparancy % to alpha
-        $background[3] = 1 - round(min(100, max(0, $transparencyPercent)) / 100, 2);
+        if ($transparencyPercent < 0 || $transparencyPercent > 100) {
+            throw new InvalidArgumentException('$transparencyPercent must be between 0 and 100. Got ' . $transparencyPercent);
+        }
+
+        $bgColour = Color::create($backgroundColour);
+        // The Color class is immutable, so we have to instantiate a new one to set the alpha channel.
+        // No need to do that if both the $backgroundColor and $transparencyPercent are 0.
+        if ($bgColour->channel(Alpha::class)->value() !== 0 && $transparencyPercent !== 0) {
+            $channels = $bgColour->channels();
+            $alpha = (int) round(255 * (1 - ($transparencyPercent * 0.01)));
+            $bgColour = new Color($channels[0]->value(), $channels[1]->value(), $channels[2]->value(), $alpha);
+        }


As per the upgrade guide:

It is no longer possible to pass color values as array

Passing as a Color object instead. Alpha now has to be between 0 and 255, not 0 and 1.

GuySartorelli · 2024-07-10T02:49:17Z

_config/image.yml

gd used to be used because it was defined as the default in the Intervention\Image\ImageManager class.

Now we have to define one ourselves - so it'll be imagick if the extension is installed, or gd as a fallback.

GuySartorelli · 2024-07-10T02:50:04Z

src/Conversion/InterventionImageFileConverter.php

-        } catch (ImageException $e) {
+        } catch (RuntimeException $e) {


Note that this is Intervention's special exception, not the base one.... I can add an alias if you want to avoid confusion.

GuySartorelli · 2024-07-10T02:50:57Z

src/Image_Backend.php

+    /**
+     * Get the current quality (between 0 and 100)
+     */
+    public function getQuality(): int;


This method is in InterventionBackend and we use it in tests without checking if the object is InterventionBackend or some other class using this interface. Seems sensible to just add it as a required method.

GuySartorelli · 2024-07-10T02:53:11Z

src/InterventionBackend.php

-     * @var AssetContainer
-     */
-    private $container;
+    private ?AssetContainer $container = null;


This (and some of the other properties below) need to have an explicit default value set to avoid the following error:

Typed property must not be accessed before initialization

This error doesn't happen when it's not strictly typed. PHP is weird.

GuySartorelli · 2024-07-10T03:59:11Z

tests/php/Shortcodes/FileLinkTrackingTest.php

-            $content = $file->Filename . ' ' . str_repeat('x', 1000000);
-            $file->setFromString($content, $file->Filename);
+            $sourcePath = __DIR__ . '/FileLinkTrackingTest/' . $file->Name;
+            $file->setFromLocalFile($sourcePath, $file->Filename);


Had to have a real image to avoid the following error:

ImagickException: negative or zero image size `/var/www/html/silverstripe-cache/user/interventionimage_rcPBnB.jpg' @ error/image.c/CloneImage/794

GuySartorelli · 2024-07-10T03:59:23Z

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 alt="" src="/assets/FileLinkTrackingTest/testscript-test-file.jpg"',
+            '<img width="300" height="300" alt="" src="/assets/FileLinkTrackingTest/testscript-test-file.jpg"',


Now that we have a real image, it has a height and a width.

GuySartorelli · 2024-07-10T23:10:35Z

src/InterventionBackend.php

-    private $quality;
+    private int $quality = AbstractEncoder::DEFAULT_QUALITY;


We need to set this default value because a: we can't pass a null in anymore, and b: there are unit tests that fail if we set it to arbitrary values like 0 or 100.

emteknetnz

I'm getting warning when trying to install both PRs locally:

If this hangs for a long time, then you are probably debugging in phpstorm or vscode
PHP Fatal error: Declaration of SilverStripe\Assets\ImageBackendFactory::create($service, array $params = []) must be compatible with SilverStripe\Core\Injector\Factory::create(string $service, array $params = []): ?object in /var/www/vendor/silverstripe/assets/src/ImageBackendFactory.php on line 38

GuySartorelli · 2024-07-17T22:20:53Z

I'm getting warning when trying to install both PRs locally

Rebased both PRs - it was because of the new strict typing on Factory which was done after these PRs were created.

emteknetnz

I don't know where the fish image came from and future people won't either, so just change it some generic silverstripe logo test image e.g. silverstripe/admin/tests/behat/features/files/file1.jpg - it's fine if you just duplicate the image

The performance of large animated gifs under both Imagick and GD seems to be atrocious, things go veeery slow in the file manager on my local after uploading an large animated gif.

I validated that I my configuration to switch between GD and Imagick was correct via debugging get_class($this->getImageManager()->driver()) in silverstripe/assets InterventionBackend.php

I tried uploading https://i.pinimg.com/originals/60/5f/6a/605f6ab7beccce2cb2083f97d013445a.gif and the file manager was unresponsive for quite a while while it uploaded. After it was uploaded the file manager was much slower simply because this animated gif was in the list, even when viewing the files of a different file in the same folder. Archiving the animated gif resolved the performance issues

Given how bad this is, unless this is some sort of bug on our end, I can't help thinking we should actually disable animated gif support, otherwise people are going to inadvertently kill their file manager with a new feature we're promoting.

GuySartorelli · 2024-07-18T01:44:15Z

I don't know where the fish image came from and future people won't etierh

It was already being used for another test in this module, I just copied it for use here.
I don't want to replace the existing usage of it because there are different qualities of the image that I don't know how to reproduce.

GuySartorelli · 2024-07-18T01:46:24Z

What is the "file manager"? If you mean asset admin, that's why thumbnails don't generate animated GIFs by default (see the asset-admin PR) - so we're providing sane default behaviour already. Given that.... I'm not sure what the problem is?

emteknetnz · 2024-07-18T03:01:31Z

Yes asset-admin

Here's a video of what I mean by performance is atrocious

https://www.youtube.com/watch?v=C5iu1w6010o

GuySartorelli · 2024-07-18T03:40:32Z

Huh. Something about that gif sucks lol, the sample gifs I was using didn't have that problem but I can absolutely reproduce that problem using the gif you used. I'll look into it.

GuySartorelli · 2024-07-21T23:50:51Z

Turns out checking if an image is animated requires fully reading the image, which is slow for animated images.
I've resolved this by making the driver not be a singleton (which matches up with pre-upgrade behaviour anyway) and setting decodeAnimation to false temporarily in the driver when creating a thumbnail.
I've updated the kitchen sink CI run linked in the issue description as well to ensure these changes are okay when both PRs are included.

This will also be reflected in the docs, along with the option to disable it globally via yaml if people want to just avoid it altogether. I reckon leave that up to developers but leave it enabled by default, since in cases where they do want the animated result, it only needs to take the extra time the first time the variant is generated.

GIFs are uncommon enough in the kind of websites that tend to be built in Silverstripe CMS that unintended animated gif manipulations aren't likely to be a major problem, and the times where they are uploaded the content authors probably expect the animation to be retained. So disabling it globally imo should be an intentional decision by project developers.

GuySartorelli · 2024-07-21T23:37:36Z

src/Image_Backend.php

+    /**
+     * Set whether this image backend is allowed to output animated images as a result of manipulations.
+     */
+    public function setAllowsAnimationInManipulations(bool $allow): static;
+
+    /**
+     * Get whether this image backend is allowed to output animated images as a result of manipulations.
+     */
+    public function getAllowsAnimationInManipulations(): bool;
+
+    /**
+     * Check if the image is animated (e.g. an animated GIF).
+     */
+    public function getIsAnimated(): bool;
+
+    /**
+     * Discards all animation frames of the current image instance except the one at the given position. Turns an animated image into a static one.
+     *
+     * @param integer|string $position Which frame to use as the still image.
+     * If an integer is passed, it represents the exact frame number to use (starting at 0). If that frame doesn't exist, an exception is thrown.
+     * If a string is passed, it must be in the form of a percentage (e.g. '0%' or '50%'). The frame to use is then determined based
+     * on this percentage (e.g. if '50%' is passed, a frame halfway through the animation is used).
     */
-    public function crop($top, $left, $width, $height);
+    public function removeAnimation(int|string $position): ?static;


Note that there are two different but related concepts here.

On one hand, if animation is not allowed then all variants will result in a still frame regardless of whether the original image was animated or not.

On the other hand, if animation is allowed, developers can create a variant which is a still frame of the original animated image using removeAnimation. This could be useful for getting a specific frame (not just the first frame like when animated variants aren't allowed), or for scenarios where you're generating a series of variants and want some to be animated, and some to be still.

emteknetnz

This something still not right with the animation

It's still veeeery slow to do the initial processing, though I guess this is at least documented

However it's not actually fully processing the animation - for example the large animated gif that I provided as a sample https://i.pinimg.com/originals/60/5f/6a/605f6ab7beccce2cb2083f97d013445a.gif - a couple of variants I created only provide a portion of the animation

The variant used in asset admin in the right hand panel on inital upload
A variant used on the front end when I go $MyImage.ScaleWidth(300), where MyImage is a has_one on Page

Here's a video of what I mean https://youtu.be/nqMl5vD-sRU

GuySartorelli · 2024-07-23T01:49:59Z

I don't know that I agree it's "veeeery" slow, but yes it is slower than a non-animated image. That's because it's still generating an animated preview as you noted

The variant used in asset admin in the right hand panel on inital upload

I can disable that by default along with the thumnail generation if you like.

it's not actually fully processing the animation

That's a bug with intervention/image itself. Can reproduce directly using the following:

composer.json

{
    "name": "gsartorelli/pulls-621",
    "require": {
        "intervention/image": "^3.7"
    }
}

run.php

<?php

$autoload = 'vendor/autoload.php';
if (!file_exists($autoload)) {
    echo 'autoload file is missing - make sure you ran `composer install`.' . PHP_EOL;
    exit(1);
}
include_once $autoload;

use Intervention\Image\ImageManager;
use Intervention\Image\Drivers\Imagick\Driver;

// create new manager instance with desired driver
$manager = new ImageManager(new Driver());

// read image from filesystem
$image = $manager->read('605f6ab7beccce2cb2083f97d013445a.gif');

// scale and save result
$image->scale(300)->save('resizedimage.gif');

Other gifs I've tested with don't have this problem, but I'll open a bug report with intervention/image for this specific gif as it may happen with other gifs as well. (see Intervention/image#1380)

emteknetnz · 2024-07-23T02:14:06Z

I can disable that by default along with the thumnail generation if you like.

Yeah it should be disabled - the docs PR already says that it should be disabled

Because of this, the ThumbnailGenerator will provide still images as thumbnails for animated gifs by default.

GuySartorelli · 2024-07-23T02:33:23Z

That's talking about ThumbnailGenerator specifically, which is why it says that :p It doesn't say "for all images anywhere in the CMS".

Also add strict typing to Image_Backend and InterventionBackend.

GuySartorelli · 2024-07-23T03:12:20Z

Added a new Image.allow_animated_preview configuration property which devs can use to allow image previews to be animated.
This preview is used in asset admin, and anywhere you can "use existing" files (e.g. UploadField and the WYSIWYG image modal).

I'll update the docs accordingly.

It's worth noting that with exception of that one busted gif, I didn't find any of my test gifs were causing a bad lag when generating that preview (there's a slight one but the result of the animated preview outweighs that short wait IMO)

You might want to try a few other gifs as well with this new config set to true to determine whether it's really warranted. I think it's most likely that the bug with that specific gif is what's causing the slow down.

emteknetnz · 2024-07-23T03:35:27Z

I think it's most likely that the bug with that specific gif is what's causing the slow down.

The problem is that any "busted" gif makes for a really bad UX, and end-users (content authors) will think that there's simply something wrong with the CMS. We have no way of knowing how many animated gifs that get uploaded will be of the "busted" variety. We basically need to handle the worst case scenario at the expense of "normal" animated gifs

GuySartorelli marked this pull request as draft July 9, 2024 23:01

GuySartorelli mentioned this pull request Jul 9, 2024

Upgrade to intervention 3 #619

Closed

5 tasks

GuySartorelli force-pushed the pulls/3/intervention-3 branch from 4f8a096 to 26d7a95 Compare July 10, 2024 02:04

GuySartorelli commented Jul 10, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/3/intervention-3 branch from 26d7a95 to 7bcafca Compare July 10, 2024 02:47

GuySartorelli commented Jul 10, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/3/intervention-3 branch 3 times, most recently from 6c42cdb to c786bef Compare July 10, 2024 03:58

GuySartorelli commented Jul 10, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/3/intervention-3 branch 5 times, most recently from 49c56fa to 549c349 Compare July 10, 2024 23:09

GuySartorelli commented Jul 10, 2024

View reviewed changes

GuySartorelli marked this pull request as ready for review July 10, 2024 23:35

GuySartorelli force-pushed the pulls/3/intervention-3 branch 2 times, most recently from b48adae to 8fd36a7 Compare July 11, 2024 01:54

GuySartorelli mentioned this pull request Jul 11, 2024

ENH Remove animation for thumbnails by default. silverstripe/silverstripe-asset-admin#1475

Merged

GuySartorelli force-pushed the pulls/3/intervention-3 branch from 8fd36a7 to b24ab93 Compare July 11, 2024 23:03

emteknetnz requested changes Jul 15, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/3/intervention-3 branch 2 times, most recently from de44096 to fc77d38 Compare July 17, 2024 22:18

emteknetnz requested changes Jul 18, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/3/intervention-3 branch 3 times, most recently from bb5b70e to f716212 Compare July 21, 2024 23:50

GuySartorelli commented Jul 21, 2024

View reviewed changes

emteknetnz requested changes Jul 23, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/3/intervention-3 branch from f716212 to 86a32c7 Compare July 23, 2024 03:10

DEP Upgrade to intervention/image 3

04e0016

Also add strict typing to Image_Backend and InterventionBackend.

GuySartorelli force-pushed the pulls/3/intervention-3 branch from 86a32c7 to 04e0016 Compare July 23, 2024 03:11

emteknetnz approved these changes Jul 23, 2024

View reviewed changes

emteknetnz merged commit 16444e4 into silverstripe:3 Jul 23, 2024
9 checks passed

emteknetnz deleted the pulls/3/intervention-3 branch July 23, 2024 03:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEP Upgrade to intervention/image 3 #621

DEP Upgrade to intervention/image 3 #621

GuySartorelli commented Jul 9, 2024 •

edited

Loading

GuySartorelli Jul 10, 2024

GuySartorelli Jul 10, 2024

GuySartorelli Jul 10, 2024

GuySartorelli Jul 10, 2024

GuySartorelli Jul 10, 2024

GuySartorelli Jul 10, 2024

GuySartorelli Jul 10, 2024

GuySartorelli Jul 10, 2024

GuySartorelli Jul 10, 2024

GuySartorelli Jul 10, 2024

GuySartorelli Jul 10, 2024

GuySartorelli Jul 10, 2024

GuySartorelli Jul 10, 2024

GuySartorelli Jul 10, 2024 •

edited

Loading

GuySartorelli Jul 10, 2024

GuySartorelli Jul 10, 2024

GuySartorelli Jul 10, 2024

emteknetnz left a comment

GuySartorelli commented Jul 17, 2024

emteknetnz left a comment •

edited

Loading

GuySartorelli commented Jul 18, 2024 •

edited

Loading

GuySartorelli commented Jul 18, 2024 •

edited

Loading

emteknetnz commented Jul 18, 2024

GuySartorelli commented Jul 18, 2024

GuySartorelli commented Jul 21, 2024 •

edited

Loading

GuySartorelli Jul 21, 2024

emteknetnz left a comment

GuySartorelli commented Jul 23, 2024 •

edited

Loading

emteknetnz commented Jul 23, 2024 •

edited

Loading

GuySartorelli commented Jul 23, 2024

GuySartorelli commented Jul 23, 2024 •

edited

Loading

emteknetnz commented Jul 23, 2024

		public function paddedResize($width, $height, $backgroundColor = "FFFFFF", $transparencyPercent = 0);
		public function paddedResize(string $width, string $height, string $backgroundColour = 'FFFFFF', int $transparencyPercent = 0): static;

		public function croppedResize($width, $height);
		public function croppedResize(int $width, int $height, string $position): static;

		public function crop($top, $left, $width, $height);
		public function crop(int $top, int $left, int $width, int $height, string $position, string $backgroundColour = 'FFFFFF'): static;

		} catch (ImageException $e) {
		} catch (RuntimeException $e) {

		private $quality;
		private int $quality = AbstractEncoder::DEFAULT_QUALITY;

DEP Upgrade to intervention/image 3 #621

DEP Upgrade to intervention/image 3 #621

Conversation

GuySartorelli commented Jul 9, 2024 • edited Loading

Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz left a comment

Choose a reason for hiding this comment

GuySartorelli commented Jul 17, 2024

emteknetnz left a comment • edited Loading

Choose a reason for hiding this comment

GuySartorelli commented Jul 18, 2024 • edited Loading

GuySartorelli commented Jul 18, 2024 • edited Loading

emteknetnz commented Jul 18, 2024

GuySartorelli commented Jul 18, 2024

GuySartorelli commented Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

emteknetnz left a comment

Choose a reason for hiding this comment

GuySartorelli commented Jul 23, 2024 • edited Loading

emteknetnz commented Jul 23, 2024 • edited Loading

GuySartorelli commented Jul 23, 2024

GuySartorelli commented Jul 23, 2024 • edited Loading

emteknetnz commented Jul 23, 2024

GuySartorelli commented Jul 9, 2024 •

edited

Loading

GuySartorelli Jul 10, 2024 •

edited

Loading

emteknetnz left a comment •

edited

Loading

GuySartorelli commented Jul 18, 2024 •

edited

Loading

GuySartorelli commented Jul 18, 2024 •

edited

Loading

GuySartorelli commented Jul 21, 2024 •

edited

Loading

GuySartorelli commented Jul 23, 2024 •

edited

Loading

emteknetnz commented Jul 23, 2024 •

edited

Loading

GuySartorelli commented Jul 23, 2024 •

edited

Loading