Cache persistence after derivative creation

Note that with NP8, we integrated with fastly and it can perfectly flush, even cache tag based.
Also you need to be careful when applying a min cache time since browsers can locally cache it without reasking for a change and your explicit flushes will never reach the browser.

IMHO the itok should change or some other thing in the URL should change if a crop changes (which doesn't).
ImageStyleDownloadController::deliver() finally uses getPathToken() and it seems that getPathToken() is not supporting anything to vary itok.
So we end up on the same problem as #2617818: Support multiple crop variants per URI and crop type

Like you said, it is a general problem: All crop modules are affected. Drupal providers with Varnish (like Pantheon) take extra meassures to purge the image from Varnish, see: https://www.drupal.org/node/2148619

To make the proposed solution from #9 works fine. For responsive images I had to use similar code in template_preprocess_responsive_image().

Lets just try to convert #9 into a patch then, not sure what this is postponed. I don't think this is something that core can solve for us.

There are some interesting cases where not even having working cache invalidation (file name based, not cache tags, as files have no cache tags) helps. For example when using file entity and entity browser, and editing/croping a file when clicking on the edit button and expecting it to refresh when saving. Even when updating the HTML, which we do know, the browser dosn't reload the picture without a full page refresh.

Here is the patch based on #9 placed in crop module.

+++ b/crop.module
@@ -108,3 +110,26 @@ function crop_file_delete(FileInterface $file) {
+    preg_match("/http:\/\/.*\/styles\/(.*?)\/public\/(.*?)\?/", $uri, $match);
+    $style = $match[1];
+    $file_uri = "public://" . $match[2];

we should not hardcode public but parse it from the string, just like we do with the image style.

lets also add a comment to explain what we are doing.

.

I agree too. :) #13 was actually #9 as a patch file.

Tried to improve things with a new patch.

+++ b/crop.module
@@ -108,3 +110,26 @@ function crop_file_delete(FileInterface $file) {
+    $crop = Crop::findCrop($file_uri, $image_widget_manager->getEffectData($style, 'crop_type'));

I'm a bit confused with this line. In the case of editing entity with entity browser (explained in #12), this means we are looking for (the last) "manual crop" effect added in the image style that's used to preview the image on the edit form?

Let's say global image widget configuration (admin/config/media/crop-widget) uses an image style (with no manual crop) for crop preview and all crop types are selected.

In case the cropping happens using the same cropping type used in mentioned preview-image-style, the image will be updated. In any other case, it won't.

If this is not the expected behaviour, it looks to me that missing information in this function is (chosen) crop type. Then, we could use that value to load the proper crop entity.

Finding the crop type has nothing to do with entity browser. It's based on the image style, and it will render the entity and based on the image style that is displayed, pick the right crop type. So please don't mention browser in the comments :)

+++ b/crop.module
@@ -108,3 +111,37 @@ function crop_file_delete(FileInterface $file) {
+      $query_string = implode($crop->position(), '-') . '-' . implode($crop->anchor(), '-');
+      $uri .= '&v=' . $query_string;

I think i would prefer to see a (shortened?) hash here than raw values. It's not about the numbers. It's just about uniqueness.

We are now depending on the crop record to determine changed values.
Alternatively we could say updating a crop should touch / update the file change timestamp. That's what we need to guarantee.
Thus, the only thing we would need to lookup and hash is the file change timestamp.

And then we automatically cover replacing a file and other kinds of potential alters, too.
But then also not sure if Crop API should provide the URL alter. We could easily apply this to all managed files.

Discussed with miro. Relying on changed of the file entity (not the physical file) sounds like an interesting idea. using image styles on something else is possible but rare I'd say.

This will simplify this (although loading a file entity based on uri needs a query and will actually be slower) and it has the advantage that we could even suggest that core ads that.

For that to work, we'll also need to resave the file entity when saving a crop.

Is this the idea of #22?

In discussion with @Berdir and @miro_dietiker, it seems that #18 is acceptable patch at this point.

Created a Core followup: #2800731: Add file change timestamp to the file URI related to #20 and #21.

Edit:
Created a Crop followup: #2800739: Improve findCrop() performance

Yes, lets go with #18, but please update the comment.

Comment improvement by @Berdir.

The interdiff is against #18.

I think this is ready now.

#18 looks ok to me. Noticed two things:

1. +    /** @var \Drupal\image_widget_crop\ImageWidgetCropManager $image_widget_manager */
   +    $image_widget_manager = \Drupal::service('image_widget_crop.manager');
   +
   +    /** @var \Drupal\image\Entity\ImageStyle $image_style */
   +    $image_style = ImageStyle::load($image_style);
   +
   +    // In case the image style uses "crop_type" effect, load the crop entity.
   +    if ($crop = Crop::findCrop($file_uri, $image_widget_manager->getEffectData($image_style, 'crop_type'))) {
   
   

   If this is going to live in Crop API we can't depend on Image widget crop.

2. +++ b/crop.module
   @@ -108,3 +111,37 @@ function crop_file_delete(FileInterface $file) {
   +      $query_string = implode($crop->position(), '-') . '-' . implode($crop->anchor(), '-');
   +      $uri .= '&v=' . $query_string;
   

   I like the idea of using a shortened hash here.

EDIT: Commented on wrong hunk.

Removed image widget crop manager and copied almost the same code to crop_file_url_alter().

This looks good now I think. Since the hash is predictable, I think it actually shouldn't be too hard to extend e.g. \Drupal\Tests\crop\Kernel\CropCRUDTest::testCropSave() to generate an image style url for the test image and check that it has crop hash appended?

Here are the tests. Didn't find any other way to test this except the one that's added here.

Also, seems there was a bug with regular expressions...

+++ b/crop.module
@@ -108,3 +111,45 @@ function crop_file_delete(FileInterface $file) {
+  // Parse given URI.
+  $uri_info = parse_url($uri);
+  // Use schema information to skip relative URIs.
+  $protocol = isset($uri_info['scheme']) ? in_array($uri_info['scheme'], ['http', 'https']) : FALSE;
+
+  // Process only files that are stored in "styles" directory.
+  if ($protocol && strpos($uri, PublicStream::basePath() . '/styles/') !== FALSE) {

I honestly don't know what the http/https check is for. Lets remove that, the test shows that it doesn't really work (manually calling the alter hook is cheating :))

This is the test code that I would except to return the attached hash (based on crop that's created for the original file).

    // Build an image style derivative for the file URI.
    $image_style_uri = $this->testStyle->buildUri($file->getFileUri());
    // Creating the image style URL.
    $image_style_url = file_create_url($image_style_uri);

As file_create_url() triggers crop_file_url_alter():
- The processed URI (in crop_file_alter()) is: public://styles/test/public/sarajevo.png
- While the public stream is something like this: vfs://root/sites/simpletest/78359729/files

Does this mismatch mean we need to call file_create_url() twice: the first time to get absolute URL and the second time to assert the hash?
Or it means PublicStream::basePath() in crop_file_url_alter() is not necessary?

Switched to the web test with some support for rare cases.

Still not sure about PublicStream::basePath() in the first condition...

Uploaded a "test" patch that has changed URI in the crop entity.

1. +++ b/src/Tests/CropFunctionalTest.php
   @@ -130,4 +140,44 @@ class CropFunctionalTest extends WebTestBase {
    
   +  /**
   +   * Asserts a shortened has is added to the file URI.
   +   *
   

   hash.

2. +++ b/src/Tests/CropFunctionalTest.php
   @@ -130,4 +140,44 @@ class CropFunctionalTest extends WebTestBase {
   +    $shortened_hash = substr(md5(implode($crop->position()) . implode($crop->anchor())), 0, 8);
   +    $this->assertTrue(strpos($altered_image_style_url, $shortened_hash) !== FALSE, 'The image style URL contains a shortened hash.');
   

   I think the actual hash isn't even that important. What would be nice is to change the crop now and make sure we get a different hash than the first one.

3. +++ b/src/Tests/CropFunctionalTest.php
   @@ -130,4 +140,44 @@ class CropFunctionalTest extends WebTestBase {
   +    // Delete the file and the crop entity.
   +    $file->delete();
   +    $crop->delete();
   

   what's this for?

Added a new test to assert the hash has changed.

#39.3: The UI tests executed after tries to delete the crop type via UI and fails because there are some entities using it.

I was excited to try out this patch, but it seems to not work for my site. We are using the 'FocalPointScaleAndCropImageEffect' effect from Focal Point module. This effect has a CropStorage object, so definitely builds uno crop module. Yet the patch checks for if (isset($data_effect['crop_type'])) { and that check fails for this effect. It would be good if it did not. Any advice? Should this be fixed in Focal Point module?

@moshe: This module currently only works with the manual crop effect added by this, if focal point has its own effect then it either has to at least match the configuration structure, then it might work or duplicate the logic here.

Anyway, this is not working very well and definitely broken for responsive images, where it doesn't even get that far: #2868339: Public folder check in crop_file_url_alter() is incorrect, does not work for responsive images