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

Imgix - folder (or filenames) with umlaut / special chars result in broken images (404) #395

Closed
mandrasch opened this issue Jan 9, 2024 · 11 comments
Labels

Comments

@mandrasch
Copy link

mandrasch commented Jan 9, 2024

Describe the bug

Hi,

we're using this awesome plugin with imgix!

Our customers (from Austria and Germany) would love to use umlauts (ä,ö,ü) in the folder names, e.g.

https://example.com/uploads/Österreich/foto1.png

This results in

https://XXX.imgix.net/uploads/%25C3%2596sterreich/foto1.png?auto=format&crop=focalpoint&domain=XXXX.imgix.net&fit=crop&fp-x=0.5&fp-y=0.5&h=957&ixlib=php-3.3.1&q=82&w=640

which returns 404 in the browser and a broken image.

For filenames we enabled convertFilenamesToAscii to avoid the problem

https://craftcms.com/docs/4.x/config/general.html#convertfilenamestoascii

Imgix docs state to avoid - or encode - special characters

Avoid special characters in folder and file names. They require encoding to resolve and if not handled carefully, can result in 404 errors for your assets. List of characters to avoid

https://docs.imgix.com/setup/asset-storage

To reproduce

Steps to reproduce the behaviour:

  1. Create folder like "Österreich"
  2. Put image in it
  3. Activate imgix integration
  4. Add image to page
  5. View page

Expected behaviour

If possible - it would be cool if the encoding could be fixed to support those folder names. ChatGPT suggested, the encoding was maybe done twice?

encodeURI in JS returns this

encodeURI("Österreich")
'%C3%96sterreich'

While URL had %25C3%2596 in it.

Screenshots

Versions

  • Plugin version: 4.0.5
  • Craft version: 4.5.11.1

Thanks very much in advance!

@mandrasch mandrasch added the bug label Jan 9, 2024
@mandrasch mandrasch changed the title Imgix - folder (or filenames) with umlaut / special chars result in broken images (404= Imgix - folder (or filenames) with umlaut / special chars result in broken images (404) Jan 10, 2024
@mandrasch
Copy link
Author

@aloco
Copy link

aloco commented Jan 17, 2024

Hi, I also tested this and it seems like the Image Optimize Variant urls are double encoded, the "original" imgix url seems right (encoded once) and are working. Not sure if this is an issue in imgix-php or in Image Optimize .. https://github.com/imgix/imgix-php/blob/main/src/UrlHelper.php#L73

Thank you for your help!

@khalwat
Copy link
Contributor

khalwat commented Mar 25, 2024

Are your volumes remote volumes (such as in an S3 bucket) or are they local volumes?

@khalwat
Copy link
Contributor

khalwat commented May 9, 2024

Will re-open if additional information is provided

@khalwat khalwat closed this as completed May 9, 2024
@mandrasch
Copy link
Author

mandrasch commented May 9, 2024

Hi @khalwat, sorry for delayed response! The project were it occurred just used local volumes, no S3 or similar involved.

@khalwat khalwat reopened this May 9, 2024
@khalwat
Copy link
Contributor

khalwat commented May 9, 2024

Re-reading the initial issue, it sounds like the right thing to do here is turn on convertFilenamesToAscii in Craft CMS.

Is this an acceptable solution, or would you want some kind of special encoding handling in ImageOptimize?

@mandrasch
Copy link
Author

mandrasch commented May 9, 2024

We activated convertFilenamesToAscii after discovering the problem as a quick fix.

And we tried to convert existing image filenames with craft utils/ascii-filenames, but that did not convert folder names iirc. We needed to manually rename all folders.

From a point of authoring experience, just my personal opinion:

It would be best if customer can use Umlauts like ä,ö,ü in files and folders. The convertion to ascii characters is currently not very satisfying for german clients. Even WordPress supports it, so there is not really room for arguments like "it's technically not possible".

It is also the default setting in craft to not convert to ascii.

And imgix generally supports it (as we have demonstrated in our tests, docs: https://docs.imgix.com/best-practices/ensuring-origin-deliverability#foreign-characters-and-character-encoding)

We also created this issue imgix/imgix-php#87, in which you already participated. Thanks!

So it would be awesome if Umlauts would be possible in future. I haven't had time to really try out if this works differently in Imager-X for example (which also relies on imgix/imgix-php).

@mandrasch
Copy link
Author

mandrasch commented May 9, 2024

@khalwat
Copy link
Contributor

khalwat commented May 9, 2024

Okay so I've been over the code a few times, and here's what I don't understand @mandrasch

Here is where we get the asset's URI:

https://github.com/nystudio107/craft-imageoptimize-imgix/blob/develop-v5/src/imagetransforms/ImgixImageTransform.php#L313

        $fs = $volume->getFs();
        if ($fs instanceof Local) {
            $assetUrl = AssetsHelper::generateUrl($asset);

            return parse_url(rawurldecode($assetUrl), PHP_URL_PATH);
        }

And in your case, you said it was just a local file system, so it will hit that condition... and it's true that AssetsHelper::generateUrl() does call rawurlencode (via array_map()) to encode the URL:

https://github.com/craftcms/cms/blob/5.x/src/helpers/Assets.php#L90

        $path = implode('/', array_map('rawurlencode', $pathParts));

...however we then decode it before returning it:

return parse_url(rawurldecode($assetUrl), PHP_URL_PATH);

I ran some test strings through these methods, and they appear to be encoding things correctly. Is there any way for you to have a look at what you're seeing on your end in terms of the code path it's taking, and the results that are going in/out of the aforementioned functions?

That or we could do a screen share:

https://savvycal.com/nystudio107/chat

@mandrasch
Copy link
Author

@khalwat Thanks very much for investigating! I'll check again / go through xdebug locally in the next days and get back to you here with some results.

@mandrasch
Copy link
Author

Okay, so... I couldn't reproduce the issue anymore. Encoding is fine, as you already stated. We updated craft from 4.5.11.1 to 4.5.13 in the meantime, Image Optimize is still on 4.0.5. Couldn't really pinpoint it. Don't know if imgix changed something on their end, maybe the most likely explaination? 🤔

But I guess the good news is that this is now working, thanks very much for checking and support! 👍

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

3 participants