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

feat: optionally disable path encoding #87

Open
luqven opened this issue Sep 26, 2022 · 3 comments
Open

feat: optionally disable path encoding #87

luqven opened this issue Sep 26, 2022 · 3 comments

Comments

@luqven
Copy link
Contributor

luqven commented Sep 26, 2022

Description

Related PR: imgix/js-core#314

Add disablePathEncoding functionality to the URL building logic in formatPath. This way, users can decide to opt-out of path encoding.

Probably a good way of handling this would be to add a new option, disablePathEncoding, in the constructor that can be referenced in any of the instance methods. Specifically, createURL, createSrcSet., createSrcSetPairs, createDPRSrcSet all need to pass this option down to the URLHelper.

@luqven luqven added enhancement hacktoberfest Good first issue for hacktober contributors labels Sep 26, 2022
@luqven luqven added good first issue Good first issue for new contributors and removed hacktoberfest Good first issue for hacktober contributors labels Nov 10, 2022
@luqven luqven added help wanted and removed good first issue Good first issue for new contributors labels Jan 30, 2023
@aloco
Copy link

aloco commented Jan 17, 2024

We are using Craft CMS and the Plugin ImageOptimize which supports integration with imgix. However, it seems like under specific circumstances, urls get encoded twice which leads to problems with german umlauts and leads to 404 for images with umlauts in the path. The related issue can be found here: nystudio107/craft-imageoptimize#395 I am not entirely sure if this is an issue in the plugin or in imgix-php - we are currently looking for a quick solution, or workaround (besides renaming the path) since we are facing this issue in a client project.

Thank you for your help

@luqven
Copy link
Contributor Author

luqven commented Jan 19, 2024

Hey @aloco , thanks for opening this issue.

This seems to come from the craft-imageoptimize-imgix dependency encoding the path before passing it to imgix-php, which will also encode it. I could be missing something, but I believe if they didn't encode before sending URLs to our library then this would resolve the issue.

They could directly parse the URL without encoding it in getAssetUri().

// in src/imagetransforms/ImgixImageTransform.php

    public function getAssetUri(Asset $asset): ?string
    {
        // ...
        $fs = $volume->getFs();
        if ($fs instanceof Local) {
            $assetUrl = AssetsHelper::generateUrl($fs, $asset);

            // Directly parse the URL to get the path component without encoding
            return parse_url($assetUrl, PHP_URL_PATH);
        }

        return parent::getAssetUri($asset);
    }

So that it doesn't get double encoded in getTransformUrl().

    public function getTransformUrl(Asset $asset, CraftImageTransformModel|string|array|null $transform): ?string
    {
        // ...
        $assetUri = $this->getAssetUri($asset); // un-encoded
        $url = $builder->createURL($assetUri, $params);
        Craft::debug(
            'Imgix transform created for: ' . $assetUri . ' - Params: ' . print_r($params, true) . ' - URL: ' . $url,
            __METHOD__
        );

        return $url;
    }

That being said, path encoding on createURL() would be an easier and less impactful change for maintainers and it is on our radar. We hope to add it in a future release.

@khalwat
Copy link

khalwat commented Mar 25, 2024

@luqven developer of said craft-imageoptimize-imgix here -- we're not actually encoding the URL though, we're calling rawurldecode() before parsing the URL.

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

No branches or pull requests

3 participants