aelvan / Imager-Craft

This plugin has been DEPRECATED. Check out Imager X instead.
MIT License
342 stars 69 forks source link

Incorrect AWS path when using symlink in imagerSystemPath #289

Closed SanderVanLeeuwen closed 4 years ago

SanderVanLeeuwen commented 4 years ago

In our setup, the imagerSystemPath is set to @storage/transforms for local caching, and a S3 bucket with CloudFront as external storage.

The @storage/transforms folder itself is a symlink to the real folder. So for example, /var/www/vhosts/craft/current/storage/transforms points to the real path /var/www/vhosts/craft/storage/transforms. The CraftTransformer::store() method strips the underlying path from the key, so we're left with something like transforms/images/1234/example.jpg. So far so good.

However, when generating a set of transformed images, the first image gets a different key prefix, where the path hasn't been stripped from the key. For example,

{% set transformedImages = craft.imager.transformImage(img,
    [
        { width: xsWidth },
        { width: smWidth },
        { width: mdWidth },
        { width: lgWidth },
    ],
    ...
) %}

generates 4 images which are uploaded to S3 with keys:

var/www/vhosts/craft/storage/transforms/images/1234/example_a.jpg
transforms/images/1234/example_b.jpg
transforms/images/1234/example_c.jpg
transforms/images/1234/example_d.jpg

Obviously, the first key is incorrect. Dumping $path, $uri and $config->imagerSystemPath in CraftTransformer::store() reveals that for the first transform of each image on a page $config->imagerSystemPath contains the symlink (/var/www/vhosts/craft/current/storage/transforms) and $path contains the real path. The first can't be stripped from the latter and thus the key includes the whole path.

From here on it's a mystery to me why the second iteration of this transform suddenly uses the right path. I'm not acquainted enough with Craft internals to figure out what's going on here, but hope someone else is.

In the meantime, hardcoding imagerSystemPath instead of using the @storage alias seems to mitigate this issue, but that's not a very pretty solution.

aelvan commented 4 years ago

That's a weird one indeed. I'm not able to reproduce, but maybe I didn't understand exactly how you had everything set up. Would you be able to test something for me? In src/models/Settings.php, could you try to change this line:

$this->imagerSystemPath = FileHelper::normalizePath(Yii::getAlias($this->imagerSystemPath));

to:

$this->imagerSystemPath = realpath(FileHelper::normalizePath(Yii::getAlias($this->imagerSystemPath)));

I think that should resolve any symbolic links in the system path. Let me know if you have a chance to test.

SanderVanLeeuwen commented 4 years ago

Thanks for pointing me in that direction. I have to confess my initial message is not entirely correct. While $this->imagerSystemPath seems to be incorrect (pointing to the symlink instead of real path), it is $path which varies between the first and subsequent transforms in a set.

It looks like getTransformedImage() is the culprit. When the target directory doesn't exist, it's created and $targetModel->path is set to the realpath(). In the next iteration, the directory exists so this piece of code isn't executed. Moving the assignment out of the ifstatement gives consistent results where $path is always the realpath.

Additionally, $config->imagerSystemPath doesn't contain the realpath but the symlinked path (for all images in a set). Adding the realpath() call in the location you suggested doesn't change anything. Directly wrapping it around $config->imagerSystemPath in store() does work though, but I'm not sure if that's the correct location.

The symlinked setup should not be too hard to recreate. The directory tree looks something like this:

- /var/www/vhosts/craft/current <- root dir
- /var/www/vhosts/craft/current/storage <- default craft storage dir
- /var/www/vhosts/craft/shared <- dir where we keep stuff which is shared between deployments
- /var/www/vhosts/craft/shared/transforms <- image transforms go here
- /var/www/vhosts/craft/current/storage/transforms <- symlink to /var/www/vhosts/craft/shared/transforms
SanderVanLeeuwen commented 4 years ago

Thanks for the fix! It took a while to find time to deploy the update(s), but we just did and confirmed that 2.3.1 indeed fixes the issue :)