Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow DI on ImageTransform #15646

Draft
wants to merge 2 commits into
base: 5.x
Choose a base branch
from
Draft

Allow DI on ImageTransform #15646

wants to merge 2 commits into from

Conversation

timkelty
Copy link
Contributor

@timkelty timkelty commented Sep 3, 2024

Description

The goal is to allow developers to write an ImageTransformer that builds URLs using properties other than those included in \craft\models\ImageTransform, eg blur.

This PR does 2 things:

  • Replaces new instance creation with createObject so \craft\models\ImageTransform can be customized through DI.
  • Calls \craft\models\ImageTransform::toArray without any args, so that all properties are returned.

Related issues

#15062

Example usage

Module/Plugin

<?php

namespace modules\testmodule;

use Craft;
use craft\elements\Asset;
use craft\events\DefineAssetUrlEvent;
use craft\helpers\UrlHelper;
use craft\models\ImageTransform;
use yii\base\Event;
use yii\base\Module as BaseModule;

/**
 * @method static Module getInstance()
 */
class Module extends BaseModule
{
    public function init(): void
    {
        parent::init();

        // Defer most setup tasks until Craft is fully initialized
        Craft::$app->onInit(function() {
            Craft::$container->set(ImageTransform::class, [
                'class' => \modules\testmodule\ImageTransform::class,
            ]);


            Event::on(
                Asset::class,
                Asset::EVENT_DEFINE_URL,
                function (DefineAssetUrlEvent $event) {
                    $transform = $event->transform;

                    if (!$transform) {
                        return;
                    }

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

                    $url = UrlHelper::urlWithParams(
                        "https://foo.com/$asset->path",
                        $transform,
                    );

                    $event->url = $url;
                });
        });
    }
}
<?php

namespace modules\testmodule;

class ImageTransform extends \craft\models\ImageTransform
{
    public int $blur = 0;
}

Template

{% set transform = {
    mode: 'crop',
    width: 100,
    height: 100,
    quality: 75,
    blur: 40,
} %}
{% set asset = craft.assets().one() %}

{{ asset.getUrl(transform) }}
{{ asset.getImg(transform, ['1.5x', '2.0']) }}

'quality',
'width',
'fill',
]) : [];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting all attributes instead of specifying, so we pick up any defined by a different class.

Alternatively, we could introduce something like \craft\models\ImageTransform::getImageAttributes that returned the attribute names involved in the actual image manipulation.

@leevigraham
Copy link
Contributor

@timkelty Finally got round to testing this and it's perfecto!

I forked the 5.x branch and rebased your changes on top. Everything worked fine.

This looks like a simple enough change it could be merged into Craft 4 as well?

image

@leevigraham
Copy link
Contributor

@timkelty What's needed to get this merged :)

@leevigraham
Copy link
Contributor

@timkelty I had another play with this PR and I can reduce it to just removing the filtering on the ->toArray() methods and the $whitelist.

I can pass through the behaviours custom property by using the EVENT_DEFINE_FIELDS event.

Event::on(
    ImageTransform::class,
    Model::EVENT_DEFINE_FIELDS,
    function (DefineFieldsEvent $event) {
        $event->fields['imgixParams'] = 'imgixParams';
    }
);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants