servdhost / craft-asset-storage

MIT License
10 stars 1 forks source link

link issue with 2.1.16 #21

Closed dlindberg closed 3 years ago

dlindberg commented 3 years ago

It looks like 2.1.16 introduces a regression, where in previous versions the globe icon in the assets control panel would provide the vanity link to the asset, in 2.1.16 it is providing the direct CDN link.

mattgrayisok commented 3 years ago

Hey. Do you have any other plugins installed which might be trying to make changes to asset URLs?

The change in 2.1.16 allows other plugins to change the URL for an asset (which was required for a couple of plugins which parse asset contents) so the most likely cause of this is another plugin which is setting the asset transform URL to some default; maybe because it looked at the asset and decided not to do anything with it, so set its url to a default instead of just leaving it blank for other plugins to deal with.

dlindberg commented 3 years ago

I do have ImageOptimize which is the only one that seems like a candidate for messing with asset urls (famous last words probably), but that should only be applying to a separate volume for images. I’ll do some additional testing and see what I come up with.

mattgrayisok commented 3 years ago

Do you have Image Optimize's transform method set to 'Servd'?

If it isn't set to Servd I think you're seeing the behaviour I'd expect - IO would catch the asset url event and then use its own transform method to figure out an appropriate URL and give that back to Craft to use.

If it is set to Servd I'd expect IO to call out to the Servd transform adapter which should then send back the optimize2. URL instead which IO would then pass back to Craft to use.

dlindberg commented 3 years ago

Seeing now in the release notes the issue in craft embedded assets #https://github.com/spicywebau/craft-embedded-assets/issues/158

I just confirmed it is an interaction with ImageOptimize (method is set to Servd) there are no ImageOptimize fields set for the volume though so maybe that is the edge case? Toggling ImageOptimized being enabled does toggle the expected links on the icon. If the Servd transformer isn't getting passed the URL to deal with though which kind of seems right on a volume that isn't images I'm not quite sure what the path forward is because everything is doing what should be the right then but the net result is an unexpected result.

mattgrayisok commented 3 years ago

I've had a look over the ImageOptimize code and I think it's this line which is causing the issue:

https://github.com/nystudio107/craft-imageoptimize/blob/a077b4c3ee7cb8ca69eaf83119b4e750f8bb6839/src/services/Optimize.php#L159

If IO decides not to handle the transform because the file isn't an image it returns a URL generated by Craft's AssetsHelper rather than returning null and allowing other plugins to then have a go instead.

I'll create an issue for AW to have a look at and see if he agrees.

mattgrayisok commented 3 years ago

@dlindberg Link to associated IO issue above ^

dlindberg commented 3 years ago

Looks good, I like your explanation of the issue. Your note on event handlers in general led me to take a look at the Craft Docs on this to see if they said anything about "best practices" on this front. Unfortunately, they seem to give effectively no guidance on working with EVENT_GET_ASSET_URL so it seems like this issue could be a somewhat persistent challenge. Do you think it would be worth submitting an issue on the docs so that hopefully this issue comes up slightly less often?

dlindberg commented 3 years ago

Also, that was fast from Andrew... I'm good with closing this out too unless you can think of a reason not to.

mattgrayisok commented 3 years ago

I'll close it as it isn't really an issue with the Servd plugin, but an ecosystem problem with plugins in general.

Not sure it's worth opening an issue with Craft because the problem would likely need addressing within Yii2. It has some basic support for ordering of event handlers:

https://www.yiiframework.com/doc/guide/2.0/en/concept-events#event-handler-order

But the larger problem will be getting plugin developers to all agree on who's handler is the most important.