cms icon indicating copy to clipboard operation
cms copied to clipboard

[WIP] Add extraOptions property to transforms

Open leevigraham opened this issue 1 year ago • 10 comments

Description

This PR adds a new extraOptions property to ImageTransforms.

Why is this needed?

While creating a custom Imgix module I wanted to pass imgix parameters into the transform. To achieve this I added a new behaviour to the ImageTransform

<?php

namespace Newism\Imgix\behaviors;

use yii\base\Behavior;

class ImageTransformBehavior extends Behavior
{
    public array $imgixParams = [];
}

I then added an EVENT_BEFORE_GENERATE_TRANSFORM event listener that created an Imgix url.

Event::on(
            Asset::class,
            Asset::EVENT_BEFORE_GENERATE_TRANSFORM,
            function (GenerateTransformEvent $event) {

                /** @var Asset $asset */
                $asset = $event->asset;

                /** @var ImageTransform|ImageTransformBehavior $transform */
                $transform = $event->transform;

                // Pull the imgix params from the behaviour
                $imgixParams = $transform->imgixParams;

                // Do a bunch of stuff here to create the URL
                $url = '…'

                $event->url = $url;
            }
        );

This worked fine until I added srcset behaviour.

The current Asset::getUrlsBySize() normalises the current image transform, creates a new ImageTransform by copying the properties and generating the URL.

The issue form me was the imgixParams value defined by the custom behaviour was not passed to the new sizeTransforms.

Solution

This PR adds a new extraOptions property to ImageTransforms. This property is passed down to the sizeTransforms and therefore can be accessed in the Asset::EVENT_BEFORE_GENERATE_TRANSFORM event or anywhere else the transform is accessed.

TODO

  • [ ] Add extraOptions to the GraphQL code / implementation
  • [ ] Add extraOptions to ImageTransforms:createTransformFromString and ImageTransforms:parseTransformString … I assume these are used in filenames, db columns… probably need to base64 the serialised array content.

leevigraham avatar May 24 '24 01:05 leevigraham

I’m also looking to add extra properties to transforms in my Cloudinary plugin that won't be removed during normalization. Ideally, I’d like to achieve this without using an extraOptions property to bundle non-native properties, by simply ensuring that these extra properties are not deleted from the transforms.

thomasvantuycom avatar Aug 06 '24 15:08 thomasvantuycom

@thomasvantuycom I'm not sure it can be done without extraOptions with the current code. The way the transforms are cloned explicitly copies the properties across.

leevigraham avatar Aug 07 '24 02:08 leevigraham

@brandonkelly Any thoughts on this?

leevigraham avatar Aug 16 '24 05:08 leevigraham

@leevigraham to make sure we're on the same page, can you briefly describe your use-case and exactly what you're trying to do, outside of implementation?

Am I correct in saying that you're trying to have transforms that include non-native transform params when building the URL?

// renders something like: https://static-b.imgix.net/apples.jpg?blur=200&width=100
{{ asset.getUrl({ width: 100, blur: 200 }) }}

timkelty avatar Aug 29 '24 15:08 timkelty

@timkelty basically yes that's the outcome.

The thing I'm trying to do at a more conceptual level is add extra properties to the transform so they can be picked up later in the before transform event. Currently they are lost in the Asset::getUrlsBySize() method because they are not known to Craft so Craft doesn't add them to the new transform.

The idea was that adding a single extraOptions property allows third parties to dump what ever they want in there and they will be passed along.

leevigraham avatar Aug 30 '24 04:08 leevigraham

@leevigraham this is definitely something we want to refactor for Craft 6. Something like extraOptions can get the job done for now, but I'm not sure we want to go that route.

An alternative idea is to use create your own ImageTransform class, and use DI to swap it. I made some tweaks here to make that possible. Want to have a look and see if an approach like that might work for you instead?

timkelty avatar Sep 03 '24 18:09 timkelty

The proposed solution works really well!

thomasvantuycom avatar Sep 08 '24 16:09 thomasvantuycom

@timkelty I'll take a look at this today / tomorrow. I like the solution using a custom image transform class and getting the attributes.

leevigraham avatar Sep 08 '24 23:09 leevigraham

@timkelty I'll take a look at this today / tomorrow. I like the solution using a custom image transform class and getting the attributes.

Sounds good! Its just a proof of concept as is, but probably pretty close to what you'd need. Let me know how it goes.

timkelty avatar Sep 08 '24 23:09 timkelty

Additionally, if a refactor is planned for Craft 6, it might be worth considering a broader approach by generalizing the concept of image transforms to asset transforms. This would allow for more flexibility, as services like Cloudinary offer transformations for videos, PDFs, and various other file types.

thomasvantuycom avatar Sep 09 '24 07:09 thomasvantuycom

Closed in favour of https://github.com/craftcms/cms/pull/15646

leevigraham avatar Nov 07 '24 03:11 leevigraham