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

Image variant names aren't expressive enough / can't flush resampled images #109

Open
jonom opened this issue Jan 13, 2018 · 25 comments
Open

Comments

@jonom
Copy link

jonom commented Jan 13, 2018

In SS3 if you visited a page and did a ?flush on that URL, the images for that page would be regenerated. That doesn't seem to work in SS4. There was also a task to remove all resampled manipulations and I think a way to regenerate them too, plus you could call deleteFormattedImages() on an Image object.

I've been digging but can't find any way to clear out old resampled images. Am I missing something?

@flamerohr
Copy link
Contributor

Hi @jonom, you're right there isn't a way to remove resampled images

@tractorcow had talked with me about why that was missing, I think it was to do with the flysystem and how that works - perhaps he could help explain.

@tractorcow
Copy link
Contributor

@jonom you can remove resampled images but it's not exposed as a public API.

Look at FlysystemAssetStore::findVariants for the search method. You can then call ->delete($fileID) on the public / protected filesystem backends to remove the variant.

Can I ask why you are regenerating the images?

@jonom
Copy link
Author

jonom commented Jan 16, 2018

Sweet, thanks @tractorcow I'll take a look.

Can I ask why you are regenerating the images?

I have a module called FocusPoint that crops images from a point you select instead of the centre. I was looking for a way to override SilverStripe's built in cropping methods (such as Fill()) rather than needing to add new methods (like FocusFill()) for the updated behaviour. That way devs wouldn't have to use alternate methods and anywhere an image is cropped (like thumbnails in the CMS) it would take the focus point in to account automatically.

I've explored a few different ideas and gotten super close by creating a subclass of InterventionBackend and overriding the crop method. The problem is that I need to invalidate the cache for each cropped image if the selected focus point changes. Ideally I would include a hash of the focus point coordinates in the cache key (or filesystem name in this case) but I can't work out how to do that. (Do you have any ideas?) The other option I'm considering is deleting any resampled images for an image when the focus point changes. The downside to that option is that fully or partially cached page responses may display broken image links for a time which I'm not too keen on.

I tried in the SS3 version to subclass Image, and use the Injector to use that class instead. But it resulted in ORM weirdness like this so I don't consider it a stable approach. Not subclassing Image wasn't appealing in SS3 as it would mean replicating a ton of methods and having to manually patch in updates.

The most direct route would be to replace the ImageManipulation trait with my own version but I don't know how to do that. Is that possible with the injector or composer auto loading some how? I don't want to manually replicate all the code from the original trait though so I would essentially want to replace Trait A with Trait B which uses Trait A. That seems like it goes round in circles :P

@tractorcow
Copy link
Contributor

I have a module called FocusPoint that crops images from a point you select instead of the centre. I was looking for a way to override SilverStripe's built in cropping methods (such as Fill()) rather than needing to add new methods (like FocusFill()) for the updated behaviour. That way devs wouldn't have to use alternate methods and anywhere an image is cropped (like thumbnails in the CMS) it would take the focus point in to account automatically.

My suggestion in this case is to override those methods, but rename the variant so that your crop-point is included in the variant string. That way it's impossible for two different variants (same action but different crop points) to overlap.

And I guess, you'll have to subclass Image. I know I normally don't recommend that... but you may have to in this case.

I guess we could shift image manipulation to a service instead of a trait?

@jonom
Copy link
Author

jonom commented Jan 17, 2018

My suggestion in this case is to override those methods, but rename the variant so that your crop-point is included in the variant string. That way it's impossible for two different variants (same action but different crop points) to overlap.

Yep, that was exactly my goal. I was doing pretty much that in SS3 but the injector usage caused some funniness, I'm assuming because I was subclassing a DataObject and then trying to replace the original with a subclass. Also DBFile uses the ImageManipulation trait too so I guess I would have to swap that out as well?

I think I thought of a simple solution - what about adding an extension point to the ImageManipulation::variantName($format, $arg = null) method that gets passed the $format and array of $args? Then I can look at the function name and add an argument for the focus point coords if it's a cropping method.

@jonom
Copy link
Author

jonom commented Jan 18, 2018

@tractorcow if I opened a PR to add an extension hook to ImageManipulation as described above do you think it would be likely to be accepted?

@tractorcow
Copy link
Contributor

tractorcow commented Jan 22, 2018

It may not be... I understand the way you are trying to solve it, and I would impress that you shouldn't be doing it at the variant name level. I would much rather apply DI above this level so you could inject another image manipulator. A PR to allow this would be preferred.

@jonom
Copy link
Author

jonom commented Jan 22, 2018

I would much rather apply DI above this level

You lost me... what is DI?

@tractorcow
Copy link
Contributor

Dependency injection :D sorry!

@jonom
Copy link
Author

jonom commented Jan 22, 2018

Oh haha. Okay, if/when I get some time I'll look at doing a PR for that. I guess I'll close this issue since not being able to delete resampled images isn't a bug.

@jonom jonom closed this as completed Jan 22, 2018
@tractorcow
Copy link
Contributor

What I would imagine is switching out all the Pad / Resize / etc... methods in the ImageManipulation trait into an injected service, one that you could decorate or extend.

@blueo
Copy link

blueo commented Apr 11, 2018

This seems like the right place to ask... we have a need to delete generated images. We have a deployed site that has generated images, we change the way our manipulation function works but the arguments remain the same so the variant name remains the same. In this case, we never get a variant from the new function as it doesn't get re-generated. It would be good to be able remove the variants and force a regeneration. The other case for deleting variants is when data changes. Eg when a focal point property changes, our manipulation function creates new variants but the old ones are now redundant. It would be good to be able to clean these up.

I can't access the assetstore functions as they're protected. Is the way to do this to inject a new asset store with access to find/delete a variant?

@tractorcow
Copy link
Contributor

@blueo if you are implementing a custom generation, you can apply a salt to the variant to force a different key.

E.g.

-$variant = $this->variantName(__FUNCTION__, $width);
+$variant = 'b' . $this->variantName(__FUNCTION__, $width)

@blueo
Copy link

blueo commented Apr 15, 2018

@tractorcow that's a good point and will work for changing the manipulation, thanks!

For cleaning up previously generated images I'm planning to inject a new AssetStore with some public methods.

@tractorcow
Copy link
Contributor

That's a good idea. :) Sorry we don't have anything yet in core.

@mlewis-everley
Copy link

@tractorcow This seems an appropriate place to ask this (even though it is closed). If I wanted to write a build task to clear cached images (similar to the one is SS3), would that still be possible in 4?

Even at a basic level it would be fine to just loop through all images, then loop through all variants and try and simply remove the generated file. But I cannot see a way you can actually do this?

@tractorcow
Copy link
Contributor

It's possible but not easy.

Basically any file with a double underscore __ is a variant, and can be deleted. You can use a recursive iterator operating directly on the internal flysystem adapter and search for these items to deterministically delete them.

Does that help you @mlewis-everley ?

@mlewis-everley
Copy link

Hmm, kind of, so basically I would need to start off with a list of all images (say via Image::get()), then from that I can find every variant (and remove it)?

Or are you saying it is better to work off the filesystem (through the flysystem adapter somehow) and not touch the database?

If the latter I will need to do a bit more digging on flysystem, we have had to do some work with it recently and it does make my head hurt a bit!

What do you think @tractorcow ?

@tractorcow
Copy link
Contributor

Hmm, kind of, so basically I would need to start off with a list of all images (say via Image::get()), then from that I can find every variant (and remove it)?

I'd just do it at the filesystem level, since you can have DBFile attached to non-Image dataobjects, as well as historic dataobject versions that won't come up in Image::get().

@wernerkrauss
Copy link
Contributor

I'd just do it at the filesystem level

Just deleted pretty all resampled images in assets/ using

find . -regextype posix-extended -regex '.*__(Fit|Fill|ResizedImage|Scale|Resampled).*\.(jpg|png|JPG|jpeg)' -exec rm {} \;

Of course it can be extended to flush cropped images etc... depending on the project. It seems all images are generated then, in HTMLField if it's wrapped in an image shortcode. So beware with old data. There is a migration task available for migrating old content to use image shortcodes.

With find . -name '*__*.jpg'

you can e.g. find all modified .jpg images to see what manipulations are used in your project

@blueo
Copy link

blueo commented Nov 21, 2018

FWIW i've got a super hacky way to delete variants via a task here: https://gist.github.com/blueo/6598bc349b406cf678f9a8f009587a95

@tractorcow
Copy link
Contributor

I addressed it in another thread, but there is a "get all variants" function in FlysystemAssetStore we should probably just make public.

@christopherdarling
Copy link
Contributor

Just wanted to chip in here with a different UC to jonom as have just been searching for this after switching our drive from GD to Imagick as we were experiencing some issues with the output from GD. We ended up resorting to the script @wernerkrauss kindly shared to clear previously generated ones with the old driver.

@jonom jonom changed the title No way to flush resampled images? Image variant names aren't expressive enough / can't flush resampled images Jul 15, 2020
@jonom
Copy link
Author

jonom commented Jul 15, 2020

I feel like there's enough interest in this to try to do something about it in core. Seems to me that the common pain point everyone here shares is that sites are serving images that are out of date (i.e. if you regenerated them they would have the same filename but be different contents).

Two possible solutions have been presented:

  1. Task to allow deleting all resampled images
  2. Make the variant name (aka cache key) more expressive

I would find option 1 handy on occasion but I consider it an incomplete solution. Performance-minded sites will have images aggressively cached in the browser and possibly on an edge CDN. This means that despite deleting your resampled images on the server and generating new ones, visitors might continue to see your old images for months.

I think instead the variant name should capture all of the variables that we know affect image output. That should include things like the driver (GD / Imagick) and Quality setting, and ideally allow for a hook so that modules like FocusPoint can add their own parameters.

It looks like the variant name currently includes all args to the function but converted to be filesystem safe. This renders the arguments in the filename unreadable anyway, so instead of doing that could we just use a hash instead? That way we could feed in as many params as we like and it wouldn't result in a longer and longer filename.

By the way, I came back to this issue today because I changed the quality setting for regenerated images on a site and realised that to have it take effect I'd have to manually delete all the existing resampled images. I don't think that's a good developer experience.

@tractorcow
Copy link
Contributor

My suggestion in this case is to override those methods, but rename the variant so that your crop-point is included in the variant string. That way it's impossible for two different variants (same action but different crop points) to overlap.

By the way, this was done in my PR to focuspoint (pending merge).

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

7 participants