-
Notifications
You must be signed in to change notification settings - Fork 25
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
Comments
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.
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 |
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.
|
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? |
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: 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
Notice that the url query params sit outside the When this is rendered on the front end, the So I think the CKEditor plugin might need adjusting to bring the query params for an asset URL inside the 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 |
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 |
If I'm understanding that correctly, I think that condition would fail if the URL contains any character sequences that are changed by 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. |
Debugging has led me to a probable cause for this. CKEditor generates its embedded image URLs by called the 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 Also, I can see Craft cloud gets around this by simply forcing the use of Craft's transformer for everything: So it looks like:
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. |
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! |
Thanks to everyone for the very useful debugging! @mattgrayisok @i-just I think I got to the bottom of this: craftcms/cms#16464
@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. |
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! |
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! |
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.
Steps to reproduce
Additional info
The text was updated successfully, but these errors were encountered: