Updating the focal point position does not update the cached image for anonymous users

I believe issues like this are the whole reason for a itok query string correct?

The real reason for the itok is to prevent a denial of service attack caused by someone nefarious bringing down your server by requesting images...

I think you need to configure your server to not use cache-control for images, no?

Disabling image caching by CDN/Varnish is not reasonable for many sites. Image caching is one of the most basic benefits of a CDN. This came up before at #2590061: Change focal point position doesn't effect CDN/Varnish cached images. I can work on enhancing the various Purge modules to react to a focal point change once we figure out what event to listen for.

I'm a bit confused. I thought that by flushing the image derivatives the (relevant) internal drupal caches would be invalided and that would propagate all the way out to the headers that a CDN typically uses, no? Based on this it sounds like I've been making a wrong assumption all this time ... maybe?

That said...

I can work on enhancing the various Purge modules to react to a focal point change once we figure out what event to listen for.

The triggering event should be an update to the crop entity that stores the focal point position. I'm happy to work with you on getting images to be invalidated properly but this is definitely not my area of expertise.

Yeah, I think that assumption was incorrect. Nothing reaches out to Varnish/CDN to tell them that a derivative has been flushed in Drupal. The would be the job of a Purge plugin ... Thanks for highlighting the crop entity update.

I poked around crop module earlier this morning and found #2658268: Cache persistence after derivative creation. That bypasses the problem entirely by changing a derivative's url when a crop entity updates. Once that patch is merged, we won't need to do any purging. I 'll post support there after using the patch for a bit.

All in all, I think this issue can be closed as duplicate of above issue.

Sounds good

The solution in #2658268 has been merged, and focal point image styles still exhibit this issue.

The problem is that #2658268 is specific based on crop module's settings.
The key bit is here:

    foreach ($image_style->getEffects() as $uuid => $effect) {
      $data_effect = $image_style->getEffect($uuid)->getConfiguration()['data'];
      if (isset($data_effect['crop_type'])) {
        $crop_type = $data_effect['crop_type'];
        break;
      }
    }

Focal point image styles don't have any "crop_type" data config, so this test fails and the cache-busting url query string param doesn't get added to the derivative's URL.

I can think of a couple ways to move this forward:
1. Take advantage of crop module's solution by adding a dummy "crop_type" index to focal point's config data.
2. Re-implement a similar hook_file_url_alter for focal point

I'm gonna work on a patch for #1

Adds a default "crop_type" configuration to FocalPointScaleAndCropImageEffect to take advantage of crop_file_url_alter()'s cache invalidation strategy.

now with correct file path.
derp.

one more try tonight... oy

Can you give me exact steps to reproduce? I've never seen a situation where a crop entity created by focal point is missing a crop type (though I haven't ever been specifically looking).

If it turns out that your solution is correct it needs a better comment, but the code seems sound.

it's not that the focal point doesn't have an associated crop, it's that the focal point image effect doesn't define a "crop_type" key in its configuration. so, when crop_file_url_alter fires, crop doesn't interpret the focal point image effect as a cropping effect.

CropEffect uses a "crop_type" to track the admin-configurable crop type assigned to an image effect.
FocalPointScaleAndCropImageEffect doesn't *need* a crop_type, because it always uses the "focal_point" crop that comes packaged with this module.
We're adding it only to take advantage of the crop_file_url_alter cache-invalidation mechanism.

I've altered the patch slightly, so that it replicates CropEffect::defaultConfiguration

In terms of reproducing, we're dealing with edge caching, which makes it tricky to see the caching action, but it's easy to observe the conditions that lead to it:
1. create a focal-point cropped image
2. load the image, and note the image URL
3. change the focal point, and re-save the image
4. reload the image, and note the image URL again
if the URLs from 2 and 4 are the same, then the edge cache will see them as the same image.

#15 is working great for me and fixed a problem that was a bit a of a headache for a while. I assume the patch in #17 is just as effective.

I've tested the patch n #17 and it's working reliably for me.

Did the three of you really test this independently or just look at one person's screen? :D

Ill look into the steps you mentioned in #17 and see what I can see

if the URLs from 2 and 4 are the same, then the edge cache will see them as the same image.

You're referring to the itok value, right?

Neither the query string (including itok value) nor the path changes when a focal point is updated for an image.
The entire URL is identical.

The solution in crop_file_url_alter is to add an additional url param, which gets updated based on crop->anchor() and crop->position().
From crop.module:

    // In case the image style uses "crop_type" effect, load the crop entity.
    if ($crop_type && $crop = Crop::findCrop($file_uri, $crop_type)) {
      // Found a crop for this image, append a hash of it to the URL,
      // so that browsers reload the image and CDNs and proxies can be bypassed.
      $shortened_hash = substr(md5(implode($crop->position()) . implode($crop->anchor())), 0, 8);
      $uri .= '&h=' . $shortened_hash;
    }

ie. a unique URL for every possible unique crop.

1. +++ b/src/Plugin/ImageEffect/FocalPointScaleAndCropImageEffect.php
   @@ -43,4 +43,17 @@ class FocalPointScaleAndCropImageEffect extends FocalPointEffectBase {
   +    // effect looks more like a Crop effect.
   ...
   +    return parent::defaultConfiguration() + [
   

   Can we change this to "Include a `crop_type` so that the crop object can act on images generated using this effect."

2. +++ b/src/Plugin/ImageEffect/FocalPointScaleAndCropImageEffect.php
   @@ -43,4 +43,17 @@ class FocalPointScaleAndCropImageEffect extends FocalPointEffectBase {
   +    // See https://www.drupal.org/node/2842260
   

   @see https://www.drupal.org/node/2842260

Update comment block to:

    // Include a `crop_type` so that the crop module can act on images
    // generated using this effect.
    // @see crop_file_url_alter()
    // @see https://www.drupal.org/node/2842260

I added this patch to my local environment and started getting new itoks for the images after moving the focal point and saving the images. BUT the images do not get recreated until I then select the preview button. I created https://www.drupal.org/node/2906631 for this and stupidly forgot to add a comment here.

@badrange ... I'm confused that you are getting new itoks. They should not be changing. Do you have any other image and/or caching related modules installed?

Also, just to be clear can you post your exact steps to reproduce so we can try to get to the bottom of this

Here is a video showing what I'm doing:
https://photos.google.com/share/AF1QipOOkbmC1bVhppAfhUInT9SS7994a1wO0JqL...

Basically:

1. Upload image, set focal point in bottom right corner
2. Save node, see how it looks, be unhappy about result
3. Edit node, edit image entity, set focal point in top right corner
4. Save image entity - verify that focal point is moved by opening the media entity in a new tab
5. Save node (or don't save node, makes no difference)
6. See that the image stays the same. Refresh the page. Nothing changes. Refresh again. Nothing.
7. Edit image entity, click preview
8. Open node again, see that image has ben updated, finally

Project uses crop 1.2, latest git revision of focal_point with the patch in this issue. My Drupal instance runs in a Ubuntu Vagrant box with nginx, php-fpm, php 7.0, image library is gd (of all things) and we have set up redis as a cache backend.

Sounds like your issue is similar but not related, badrange.
This issue is about cache expiration for anonymous users or for front-end proxy cache like varnish.

That's why I created https://www.drupal.org/node/2906631 - I just changed it's status from being duplicate to active again. [Note: Comment edited to point to the correct issue]

tested and committed #24

Good news, Thank you!

And I just realised that I referenced the wrong issue in my comment above; this is the one I meant to refer to:
https://www.drupal.org/node/2906631

Is there a reason this was added to FocalPointScaleAndCropImageEffect but not FocalPointCropImageEffect or FocalPointEffectBase? Seems like it should be on all of them, or at least the base, right?

And, just to confirm based on what I read in the comments from these issues, the solution was not to change the itok but rather to change Drupal's response from 304 to 200, causing CDNs to update? Is that right?

This adds a second argument to the image style URL, a hash based on the crop configuration, so if that changes, it will generate a new URL, and it will be cached separately by the CDN. See crop_file_url_alter().

But yes, that it is not on all relelvant classes might be a bug, I suggest you open a new issue.

@RaisinBranCrunch i think you're right, FocalPointCropImageEffect should have the same default config, but actually not FocalPointEffectBase.
Conceivably, FocalPointEffectBase could be extended with a non-cropping image effect, so shouldn't make that assumption.

Agree with Berdir, please open a new issue and link it here.

Okay, I created and linked the issue. This feature we're using was added in Crop API version 8.x-1.3, right? That's what it looks like. We're still using 1.2, which is another reason I was confused.

The Crop API patch was committed on March 22, and Crop 1.2 was released in April, so it should be in there.
https://www.drupal.org/commitlog/commit/72175/0b9a1051934032cb4f799615c6...
https://www.drupal.org/project/crop/releases/8.x-1.2

Oh, I see.

PublicStream::basePath() evaluates to sites/default/files... so the crop module (1.2) is looking for sites/default/files/styles/ in the $uri, when the $uri is coming in as the string:

public:://styles/thumbnail/public/image.jpg

And, also, we're using S3FS so urls will never come in as sites/default/files. It looks like 1.3 crop will fix this for us, though.

Thanks for the info.

FYI, if you had focal_point-cache_buster-2842260-24.patch setup in your composer.json as a patch, be aware that this patch will still apply cleanly when you update focal point. This will result in a fatal error and tear down your website.

Fatal error: Cannot redeclare Drupal\\focal_point\\Plugin\\ImageEffect\\FocalPointScaleAndCropImageEffect::defaultConfiguration()