[4.x]: Asset::EVENT_DEFINE_URL doesn't give plugins a chance to define the URL

[khalwat]

What happened?

Description

ImageOptimize, and potentially other plugins hook into Craft via Asset::EVENT_DEFINE_URL to potentially define the URL for an asset coming from an external source.

In the past, this worked fine... but some recent change (Craft 4.4 perhaps?) caused this to no longer work as expected, or at least now have unfortunate side-effects because what happens is this:

https://github.com/craftcms/cms/blob/4.4/src/elements/Asset.php#L1824

        $url = $this->_url($transform, $immediately);

        // Give plugins/modules a chance to customize it
        if ($this->hasEventHandlers(self::EVENT_DEFINE_URL)) {
            $event = new DefineAssetUrlEvent([
                'url' => $url,
                'transform' => $transform,
                'asset' => $this,
            ]);
            $this->trigger(self::EVENT_DEFINE_URL, $event);
            // If DefineAssetUrlEvent::$url is set to null, only respect that if $handled is true
            if ($event->url !== null || $event->handled) {
                $url = $event->url;
            }
        }

The call to $url = $this->_url($transform, $immediately); ends up causing Craft to start doing its thing, transforming the image either immediately or via Queue job, even though the plugin itself is taking care of everything that needs to be done to define the URL.

Contrast this with the Asset::_url() method which does this:

            if ($this->hasEventHandlers(self::EVENT_BEFORE_GENERATE_TRANSFORM)) {
                $event = new GenerateTransformEvent([
                    'asset' => $this,
                    'transform' => $transform,
                ]);

                $this->trigger(self::EVENT_BEFORE_GENERATE_TRANSFORM, $event);

                // If a plugin set the url, we'll just use that.
                if ($event->url !== null) {
                    return Html::encodeSpaces($event->url);
                }
            }

Which causes the operation to immediately short-circuit and return the URL that the plugin defined via the Asset::EVENT_BEFORE_GENERATE_TRANSFORM event.

While I could refactor the ImageOptimize code to use Asset::EVENT_BEFORE_GENERATE_TRANSFORM instead, it feels like this might be a mistake? The current Asset::EVENT_DEFINE_URL event never allows a plugin or module to override whatever Craft is going to do.

I think the code should be refactored like this:

    public function getUrl(mixed $transform = null, ?bool $immediately = null): ?string
    {
        if ($this->isFolder) {
            return null;
        }

        // Give plugins/modules a chance to customize it
        if ($this->hasEventHandlers(self::EVENT_DEFINE_URL)) {
            $event = new DefineAssetUrlEvent([
                'url' => $url,
                'transform' => $transform,
                'asset' => $this,
            ]);
            $this->trigger(self::EVENT_DEFINE_URL, $event);
            // If DefineAssetUrlEvent::$url is set to null, only respect that if $handled is true
            if ($event->url !== null || $event->handled) {
                return Html::encodeSpaces($url);
            }
        }
        $url = $this->_url($transform, $immediately);

        return $url !== null ? Html::encodeSpaces($url) : $url;
    }

...so that it returns immediately if a plugin/module set $event->url so the plugin can short-circuit the Asset::EVENT_DEFINE_URL operation properly as before.

Related issues:

nystudio107/craft-imageoptimize#373

Craft CMS version

4.4.5

PHP version

n/a

Operating system and version

n/a

Database type and version

n/a

Image driver and version

n/a

Installed plugins and versions

ImageOptimize 4.0.4

[croxton]

+1. My plugin Imgixer also uses this event.

[brandonkelly]

The event was moved to after the built-in URL generation in 4.3 via #12168, to be consistent with how craft\base\Element::getUrl() calls the event, and to allow the event to double as a way to modify the Craft-defined element URLs.

The current Asset::EVENT_DEFINE_URL event never allows a plugin or module to override whatever Craft is going to do.

Not true – getUrl() will replace $url with the $event->url value here:

cms/src/elements/Asset.php

Lines 1834 to 1837 in 4adcbee

	// If DefineAssetUrlEvent::$url is set to null, only respect that if $handled is true
	if ($event->url !== null || $event->handled) {
	$url = $event->url;
	}

(That condition was even added to work around an ImageOptimize issue introduced in 4.3.0 – 3f00b8d.)

Is the issue here just that there’s doubled efforts, or is something actually broken in 4.4?

[brandonkelly]

Just saw nystudio107/craft-imageoptimize#373. I suspect that is related to 303e5dd, where Craft is more assertive about generating control panel thumbnails, even for volumes that don’t support transforms directly.

I don’t want to revert that, and I don’t want to swap the order of the event/core URL generation for reasons stated. We could add a new EVENT_BEFORE_DEFINE_URL, which would have the same short-circuiting behavior as EVENT_BEFORE_GENERATE_TRANSFORM.

ImageOptimize and Imgixer could then start doing this, without needing to bump their craftcms/cms requirements:

use craft\elements\Asset;
use craft\events\DefineUrlEvent;
use yii\base\Event;

$eventName = defined('craft\elements\Asset::EVENT_BEFORE_DEFINE_URL')
    ? Asset::EVENT_BEFORE_DEFINE_URL
    : Asset::EVENT_DEFINE_URL;

Event::on(Asset::class, $eventName, function(DefineUrlEvent $event) {
    $event->url = '...';
});

Seem reasonable?

[andersaloof]

I agree with you @brandonkelly that the event lets the plugin override $url with whatever it returns in $event-url, however, since the $url = $this->_url($transform, $immediately);is called before the event is dispatched, Craft ends up adding a transform event in the queue even when it's not needed (since the image is served from Imgix), which results in lots of unneccesary image transformations being performed (as reported in nystudio107/craft-imageoptimize#373).

Seems like the fix would be for the plugin to also listen to the EVENT_BEFORE_GENERATE_TRANSFORM, as that seems to short-circuit before the transform event is added to the queue?

[brandonkelly]

@andersaloof Yeah got it. I think adding the new EVENT_BOFERE_DEFINE_URL event would probably be ideal as it gives plugins a way to fully take over URL generation with minimal code change.

[croxton]

EVENT_BEFORE_DEFINE_URL would work for me, thank you.

[brandonkelly]

Craft 4.4.6 is out now with that event 🎉

[khalwat]

So unfortunately, I just now was able to sit down and look at this... and it's not going to work for what ImageOptimize needs.

You're doing this for the BEFORE event:

        $event = new DefineUrlEvent();
        $this->trigger(self::EVENT_BEFORE_DEFINE_URL, $event);
        $url = $event->url;

...while doing this for the existing event:

            $event = new DefineAssetUrlEvent([
                'url' => $url,
                'transform' => $transform,
                'asset' => $this,
            ]);
            $this->trigger(self::EVENT_DEFINE_URL, $event);

So EVENT_BEFORE_DEFINE_URL only receives a DefineUrlEvent object, while EVENT_DEFINE_URL receives a DefineAssetUrlEvent object, which has the transform and asset that ImageOptimize needs.

I suspect the same will be the case for @croxton 's plugin as well.

[brandonkelly]

Sorry, overlooked that. Adjusted for the next release (2c5291f) – as of 4.4.7, craft\elements\Asset::EVENT_BEFORE_DEFINE_URL will start sending a craft\events\DefineAssetUrlEvent instead of DefineUrlEvent. (The former extends the latter, so this won’t break existing listeners.)

If your code is relying on DefineAssetUrlEvent, here’s the code you can use to choose the proper event constant:

use craft\elements\Asset;
use craft\events\DefineAssetUrlEvent;
use yii\base\Event;

if (
    defined('craft\elements\Asset::EVENT_BEFORE_DEFINE_URL') &&
    (new ReflectionClassConstant(Asset::class, 'EVENT_BEFORE_DEFINE_URL'))->getDeclaringClass()->name === Asset::class
) {
    $eventName = Asset::EVENT_BEFORE_DEFINE_URL;
} else {
    $eventName = Asset::EVENT_DEFINE_URL;
}

Event::on(Asset::class, $eventName, function(DefineAssetUrlEvent $event) {
    $event->url = '...';
});

We have to use reflection because defined('craft\elements\Asset::EVENT_BEFORE_DEFINE_URL') will return true regardless of whether the event is defined by Asset actually declares the class itself.

[khalwat]

Here's what I ended up with, for anyone who stumbles across this:

        // Use Asset::EVENT_BEFORE_DEFINE_URL if it's available
        // ref: https://github.com/craftcms/cms/issues/13018
        try {
            $ref = new \ReflectionClassConstant(Asset::class, 'EVENT_BEFORE_DEFINE_URL');
        } /** @noinspection PhpRedundantCatchClauseInspection */ catch (\ReflectionException) {
            $ref = null;
        }
        $eventName = $ref?->getDeclaringClass()->name === Asset::class
            ? Asset::EVENT_BEFORE_DEFINE_URL
            : Asset::EVENT_DEFINE_URL;
        // Handler: Assets::EVENT_DEFINE_URL
        Event::on(
            Asset::class,
            $eventName,
            static function (DefineAssetUrlEvent $event): void {
...

nystudio107/craft-imageoptimize@bef35ad

[brandonkelly]

Alright, 4.4.7 is out now with the additional event changes.