- πΊπΈUnited States generalredneck Texas, USA πΊπΈ
This is a simple and creative fix.
The only situation where this "might" fail is removing the
image_path_flush($this->uri->value);
on instantiation. Looking through the commit history, there is no mention of why they did it on entity creation as well, but it may happen if a crop is added to a field where an existing file entity is stored. This may happen in the case where someone is using a module to update a file with Media Entity File Replace or IMCE β , or if the file entity is programmatically changed somehow (migration?). In these cases a particular style may already have a derivative, but I would guess it would be up to that module to clean them up? I'm grasping at straws honestly, but I want us to test those usecases.As it is, using with focal point and none of these situations, this works amazingly well and is a vast improvment performance wise, ESPECIALLY on S3 and Pantheon Filesystems.
- πΊπΈUnited States generalredneck Texas, USA πΊπΈ
FYI I would have post a new relic stack trace, except that the improvements were so fast, new relic didn't keep any of the trace data. We went from 30+ seconds down to ~800ms with this fix on media creation with around 150 image styles and focal point installed.
- π¨π΄Colombia jedihe
I can confirm this patch improves the time to update a media entity (incl. manual crops, with image_widget_crop) vs. not using the patch.
- π¨π΄Colombia jedihe
I found that triggering the flush only for crop entity updates allows for an outdated image derivative to be served for this scenario:
- Site using Crop API + Image Widget Crop.
- An image style ImgSt configured to use only one effect (Manual Crop), using crop type CropA.
- The entity with the image field is saved, but no crop CropA is defined (widget set to not require it)
- The derivative for ImgSt is viewed/loaded (saves file)
- The entity is saved again, this time applying a crop for CropA. This creates a crop entity for CropA + this image.
- Loading the image derivative does not reflect the just-applied crop for CropA.
- Re-save the entity (make sure to do any kind of change to crop CropA). This updates the crop entity for CropA + this image.
- Loading the image derivative finally reflects the just-applied crop for CropA.
This effectively means, requiring $update == TRUE forces the user/editor to save a new crop twice (first creates it, second updates it) in order to see already-generated derivatives properly updated.
I'm attaching a patch that triggers the flush both for create and update scenarios of the crop entity. I just removed " && $update".
Note: I tried to reproduce the above scenario in a simplytest.me site, but couldn't. Since I don't have a way to verify that simplytest.me actually applied patch #1 (I definitely added it to the build), I decided to trust what I see in the site I'm working on which is consistent with what I read in the code.
- First commit to issue fork.
- πΊπΈUnited States pingevt
I was running down a performance issue which this solves for the most part. But found another check that could be added in. Flushing image styles I'm pretty sure makes the assumption we are working with a uri that has a streamwrapper Drupal can work with. In a custom module, I have been making crops for images from a DAMS and have set the uri to an external URL (which i'm pretty sure is still valid as a uri) since I'm not actually working with a file in Drupal. When this get flushed it creates some weird paths to check and getting some errors from Drupal to to unlink, etc. So, I would propose checking for a valid stream wrapper, before flushing as well. It seemed appropriate to piggy back off of these patches, but if this should be a separate issue, let me know.