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

Double slash in rc path #722

Closed
ox160d05d opened this issue Mar 31, 2016 · 4 comments
Closed

Double slash in rc path #722

ox160d05d opened this issue Mar 31, 2016 · 4 comments
Labels
Attn: Good First Issue This issue or PR is a great candidate for first time contributors to resolve. Level: Bug 🐞 This item involves a legitimate regression (bug) to existing functionality.

Comments

@ox160d05d
Copy link

In getRuntimePath (/liip/imagine-bundle/Imagine/Cache/CacheManager.php, line 155) we have:

    public function getRuntimePath($path, array $runtimeConfig)
    {
        return 'rc/'.$this->signer->sign($path, $runtimeConfig).'/'.$path;
    }

Thus output link (notice the //):
.../media/cache/preview/rc/Hbw6cV1x**//**uploads/2016/03/22/d1c56da4ee3d96e985863b89c78bf3c4.jpeg

Should it be replaced by something like:
return 'rc/'.$this->signer->sign($path, $runtimeConfig).'/'.ltrim($path, '/');

@trsteel88
Copy link
Contributor

trsteel88 commented Mar 31, 2016

This is probably because the path you are giving the imagine filter begins
with a slash. I don't have this problem as I don't use a slash at the
beginning of the path.

@makasim
Copy link
Collaborator

makasim commented Mar 31, 2016

The sign and getRuntimePath has to be consistent. We have to either remove ltrim from sign or add it to getRuntimePath. I prefer the second one.

@michellesanver
Copy link
Contributor

I also prefer the second one, so what needs to be done: Add ltrim to getRuntimePath. I consider this a good first issue.

@michellesanver michellesanver added the Attn: Good First Issue This issue or PR is a great candidate for first time contributors to resolve. label Oct 4, 2019
@dbu dbu added the Level: Bug 🐞 This item involves a legitimate regression (bug) to existing functionality. label Oct 6, 2021
@dbu
Copy link
Member

dbu commented Oct 28, 2021

fixed in 81d7799 (accidentally pushed to 2.x branch directly).

i am a bit worried by all the places where we ltrim the path. and i wonder if this does not break if a loader expects absolute paths for some reason... but now at least generateUrl and getRuntimePath are consistent in what they do.

@dbu dbu closed this as completed Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Attn: Good First Issue This issue or PR is a great candidate for first time contributors to resolve. Level: Bug 🐞 This item involves a legitimate regression (bug) to existing functionality.
Projects
None yet
Development

No branches or pull requests

5 participants