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

Multiple image transform query strings in asset url #336

Open
ch-turnstyle opened this issue Dec 10, 2024 · 11 comments
Open

Multiple image transform query strings in asset url #336

ch-turnstyle opened this issue Dec 10, 2024 · 11 comments
Labels

Comments

@ch-turnstyle
Copy link

Description

I've enabled images with transforms for use within a rich text field. The available transforms are defined in Craft's asset settings. The image displays fine within the control panel, but when viewing on the frontend there are two query param strings appended to the asset's url which returns a 500. If I remove the second query param string, the image is found so it seems my host Servd is correctly transforming the image. Wondering why a second transform string is being appended and how to resolve.

baseImageUrl.jpg?w=200&h=150&q=90&auto=format&fit=crop&dm=1732631447&s=c5d824714c4852eb8495f30cbe6d7d62?w=200&q=90&auto=format&fit=crop&dm=1732631447&s=b143f59c284e705261b7fe6957ffe904

// first transform query string
?w=200&h=150&q=90&auto=format&fit=crop&dm=1732631447&s=c5d824714c4852eb8495f30cbe6d7d62

// second transform query string
?w=200&q=90&auto=format&fit=crop&dm=1732631447&s=b143f59c284e705261b7fe6957ffe904

Steps to reproduce

  1. Add asset in a rich text field, apply a defined transform.
  2. View entry on the frontend and the image is broken.

Additional info

  • Craft version: Craft Pro 5.5.5
  • PHP version: 8.2
  • Database driver & version: MySQL 28.1
  • Plugins & versions: CKEditor 4.4.0
  • Deployed on Servd
@brimby
Copy link

brimby commented Dec 11, 2024

I'm also getting this where the query string is appended twice. (For example https://mywebsite/images/image.jpg?w=500&h=500&auto=compress%2Cformat&fit=crop&dm=1728060295&s=134d1fad5d99eea8beebf6e0e6c6beaf?w=500&h=500&q=90&auto=format&fit=clip&dm=1728060295&s=98a5db076ee1b9b6f95fbd316963e2c3)

I do use Servd as well, but I see the issue even on my local so I don't think they're to blame, although I will admit that I import DBs from staging sometimes, so I can't say for certain that it's impossible something is coming over from that side.

Something interesting is that I use the image alignment configs (see below) and if I save the entry but then go back and change the alignment and save again, the bug goes away.

  "image": {
      "toolbar": [
        "imageStyle:alignLeft",
        "imageStyle:alignRight"
      ]
    }

I think I'll write some js that will remove the second query param, but I must say this is a pretty gnarly bug and I don't even know where to start looking for the issue in the vendor code.

Edit: Oh also I'm still on Craft 4, so it's not just a C5 thing

@brimby
Copy link

brimby commented Dec 11, 2024

Here's that js if it's helpful to you at all @ch-turnstyle . Might want to wrap it in either a document.ready function if you have jquery, or for vanilla, wrap it with a 'DOMContentLoaded' event listener. I use that one in the code below.

// remove any second query string from img src
document.addEventListener('DOMContentLoaded', () => {
  const imgs = [...document.getElementsByTagName('img')]
  imgs.forEach(img => {
    const urlSplit = img.src ? img.src.split('?') : []
    if (urlSplit.length > 2) img.src = urlSplit[0] + '?' + urlSplit[1]
  })
})

@i-just
Copy link
Contributor

i-just commented Dec 11, 2024

Hi, thanks for getting in touch!

Those query strings come from the “Servd Assets and Helpers” plugin. It looks like this issue was raised in that plugin’s repo: servdhost/craft-asset-storage#80. Can you please follow up there?

@mattgrayisok
Copy link

Hey @i-just

I've just been looking into this within the Servd plugin and I think I've tracked down the root cause to the way that the asset URLs are being stored in the database when saved from a CKEditor field. I am having trouble finding the code within the CKEditor plugin which actually formats the way image URLs are stored so I'm hoping you can help me to debug further.

Servd generates image URLs using the Imgix query param format, E.G:

https://mywebsite/images/image.jpg?w=500&h=500&auto=compress%2Cformat&fit=crop&dm=1728060295&s=134d1fad5d99eea8beebf6e0e6c6beaf

When these are rendered within the CKEditor field in the Craft control panel, they are showing up fine, and I can see that uses a pretty basic controller function to output that correct URL.

However when the images are saved to the database, they are being stored in the elements_sites json blob like this:

<figure class="image">
  <img style="aspect-ratio:500/375;" src="{asset:2631:transform:medium||https://mywebsite/images/image.jpg}?w=500&h=500&auto=compress%2Cformat&fit=crop&dm=1728060295&s=134d1fad5d99eea8beebf6e0e6c6beaf" width="500" height="500" alt="" />
</figure>

Notice that the url query params sit outside the {asset:} segment.

When this is rendered on the front end, the {asset:} segment is expanded back out to a full URL with the transform applied, but the extra query params following the {asset:} segment are added onto the end again, leading to a doubling up of query params.

So I think the CKEditor plugin might need adjusting to bring the query params for an asset URL inside the {asset:} segment. And I assume there's some code somewhere which is parsing the asset URL which is currently ignoring the query params causing this.

I believe this would impact any images which use query params, including Imgix urls etc, so not exclusive to Servd.

I also believe this only impacts Craft 5 as I haven't been able to replicate on any Craft 4 projects.

Any thoughts very welcome!

Matt

@i-just
Copy link
Contributor

i-just commented Jan 3, 2025

Thanks for sharing your findings!

The refs are parsed in the HtmlField package, which is required by the CKEditor plugin.

I’ve set up a basic plugin (for Craft 5) that adds some hard-coded query params to the asset’s URL (similar to what I can see happening here) and in my case, the query param stays inside the curly braces <img style="aspect-ratio:200/200;" src="{asset:55:transform:w200||https://5.ddev.site/assets/local/150C.png?w=200&h=200&q=80}" alt="" width="200" height="200" />.
The query string can be stored outside if this condition is not met.

@mattgrayisok
Copy link

If I'm understanding that correctly, I think that condition would fail if the URL contains any character sequences that are changed by Html::decode(), which under the hood runs htmlspecialchars_decode over the query string, but that should only react to &amp; &quot; which shouldn't be present in the URL strings (unless something else is changing the ampersand characters to &amp; before we get to this point).

So I think we'll need to debug this function using a URL which is triggering the bug, to determine how the URL is being parsed as it's possible the nearby regex might also be parsing incorrectly.

I'll see if I can get some more info.

@mattgrayisok
Copy link

Debugging has led me to a probable cause for this. CKEditor generates its embedded image URLs by called the generate-transform action directly using an ajax request. This does not seem to honour any pre and post image url generation hooks, so the way the Servd plugin is currently inserting transform URLs (using event hooks) is either ignored or doesn't work as expected. In CKEditor this results in different versions of the URL being generated via different means and then compared, which don't match, causing the bug.

I think that replacing the image URL event hooks we currently use, with a full ImageTransformer implementation should fix the issue, however as there's no docs on ImageTransformers I'm not sure how they are supposed to be set up. Implementing the class itself is likely straight forward, however it looks as though ImageTransformers are chosen based on the transformer associated with a provided ImageTransform. However, I don't see any way to actually set a custom transformer on a transform other than specifying a transformer param on a dynamically created transform object.

Also, I can see Craft cloud gets around this by simply forcing the use of Craft's transformer for everything:

https://github.com/craftcms/cloud-extension-yii2/blob/04af6766aa395879d54cdbbdc676f718096fce55/src/Module.php#L75

So it looks like:

  • CKEditor image URLs ignore image url generation hooks
  • The only way to work around this is with a custom ImageTransformer
  • ImageTransformers can only be set as a global or via dynamically created transform objects
  • This prevents us from being able to use Volume/Filesystem specific image transformers, as would be required to mix Servd asset platform stored images and images stored elsewhere (Craft Cloud just forces the use of Craft Cloud storage for everything to work around this problem).

Given all of that, I don't think there's currently a way for us to fix this. Do you agree with my findings @i-just ? Let me know if I've missed anything that might help.

@i-just
Copy link
Contributor

i-just commented Jan 13, 2025

Thanks for this, much appreciated! We’ll have a chat about this and see what we can do. I’ll post an update once we know more!

@timkelty
Copy link
Contributor

timkelty commented Jan 21, 2025

Thanks to everyone for the very useful debugging!

@mattgrayisok @i-just I think I got to the bottom of this: craftcms/cms#16464

craft-asset-storage is affected because it is falling back to a default transform when it is passed an empty transform – and that transform may not match the transform it should be passed (e.g. if the named transform has only width or height). Once the query strings don't match, the ref parsing from HtmlField has a bad day.

@mattgrayisok You're also right that a custom ImageTransformer is a more straightforward way to handle this, but as you've discovered, they can't yet be applied to a particular FS type. Making this easier to deal with is planned for Craft 6, but I'm going to see if I can get something non-breaking into 5 so that an FS can define a default transformer.

@timkelty
Copy link
Contributor

craftcms/cms:4.14.0 and craftcms/cms:5.6.0 are now released with this fix.

Please let me know if anyone else is able to confirm the fix!

@ch-turnstyle
Copy link
Author

Finally been able to test this, and transforms are now displaying as expected.

Thank you @timkelty, @mattgrayisok and @i-just for all your efforts here!

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

No branches or pull requests

5 participants