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

Asset transforms are not regenerated when missing #5956

Closed
msimpson opened this issue Apr 22, 2020 · 5 comments
Closed

Asset transforms are not regenerated when missing #5956

msimpson opened this issue Apr 22, 2020 · 5 comments
Labels
assets 📁 features related to asset management enhancement improvements to existing features
Milestone

Comments

@msimpson
Copy link

msimpson commented Apr 22, 2020

Description

If an automatically generated transformed image file or parent folder(s) are manually deleted they are never regenerated without manual intervention. Note, I understand the asset transform index can be cleared under clear caches but this feels like something which should be caught automatically.

Steps to reproduce

  1. Create an image transform in a template.
  2. View a page using said template.
  3. Manually delete the automatically generated transformed image file.
  4. Reload the same page and find that the image is now missing (resulting in a 404).

Additional info

  • Craft version: Craft Pro 3.4.16
  • PHP version: 7.4.3
  • Database driver & version: MySQL 5.5.5
@andris-sevcenko
Copy link
Contributor

@msimpson that's hard to catch without making it expensive. It's simple for locally stored transforms, but, if this is, on an AWS S3 Volume, this means that Craft has to make a request for each transform to figure make sure it still exists.

On a page with 20 transformed images, not only this adds time, but this also counts toward requests and can increase site hosting costs.

@brandonkelly
Copy link
Member

@andris-sevcenko Seems like locally-stored transforms are the main situation where this would be a concern to begin with. Maybe reconsider with the caveat that it will only affect local transforms?

Still adds extra I/O though, so not 100% advocating for it.

@msimpson
Copy link
Author

msimpson commented Apr 23, 2020

@brandonkelly Yes, I was speaking in regard to locally-stored transforms. @andris-sevcenko In my mind, the catch would occur in (or around) craft\web\Request when a request fails.

So, while it wouldn't be a preemptive measure that continually creates undue load, I can understand the complexity of parsing the failed path in an attempt to discover whether it's a transformed asset.

However, it would be easier if the transform URI contained some parameters to aid in that identification, e.g. https://www.example.com/assets/_1920x1080_crop_center-center_70_none/1/example.jpg?assettransformindexid=1.

@andris-sevcenko
Copy link
Contributor

@msimpson it's going to be a bit more simple and less ideal.

Here's how the transform thing works under the hood (so it doesn't seem like I'm just throwing words around):

  • Template outputs an asset URL with transform parameters (this step is skipped if accessing API directly with code)
  • Craft takes the asset and the transform parameters and figures out what the subfolder for that transform would be
  • Craft checks whether an entry exists for such a transform in the transform index. If it does not - Craft generates a new transform and creates an entry in the index for it.
  • Craft returns an URL based on the volume, asset, and the subfolder listed in the transform index.
  • Browser makes the request to the URL.

So, as you see, there's really nothing to wrap around when a request fails - that is entirely up to the browser to request that URL and get the file or the 404

So, there's no simple solution to figure out whether a transform needs to be regenerated after the URL has been generated, however, it's easy to check whether a file exists before generating the URL. It's just expensive for remote volumes.

@andris-sevcenko andris-sevcenko added this to the 3.5 milestone Apr 24, 2020
@brandonkelly brandonkelly added assets 📁 features related to asset management enhancement improvements to existing features labels Apr 24, 2020
@andris-sevcenko
Copy link
Contributor

I just pushed this change for 3.5. Note, that this will affect only local volumes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assets 📁 features related to asset management enhancement improvements to existing features
Projects
None yet
Development

No branches or pull requests

3 participants