hook_image_style_flush doesn't get the $path passed to Drupal\image\Entity\ImageStyle::flush() method

This happens partially because the hooks are not invoked when $path is set in Drupal\image\Entity\ImageStyle::flush():

/**
 * {@inheritdoc}
 */
public function flush($path = NULL) {
  // A specific image path has been provided. Flush only that derivative.
  if (isset($path)) {
    $derivative_uri = $this->buildUri($path);
    if (file_exists($derivative_uri)) {
      file_unmanaged_delete($derivative_uri);
    }
    return $this;
  }

  // Delete the style directory in each registered wrapper.
  $wrappers = $this->getStreamWrapperManager()->getWrappers(StreamWrapperInterface::WRITE_VISIBLE);
  foreach ($wrappers as $wrapper => $wrapper_data) {
    if (file_exists($directory = $wrapper . '://styles/' . $this->id())) {
      file_unmanaged_delete_recursive($directory);
    }
  }

  // Let other modules update as necessary on flush.
  $module_handler = \Drupal::moduleHandler();
  $module_handler->invokeAll('image_style_flush', [$this]);

  // Clear caches so that formatters may be added for this style.
  drupal_theme_rebuild();

  Cache::invalidateTags($this->getCacheTagsToInvalidate());

  return $this;
}

The attached patch triggers things appropriately. The next step is to add $path as an additional argument for the hook.

Attached is an updated patch that passes $path as an additional argument for the hook.

+++ b/core/modules/image/src/Entity/ImageStyle.php
@@ -256,20 +256,20 @@ public function flush($path = NULL) {
-    $module_handler->invokeAll('image_style_flush', [$this]);
+    $module_handler->invokeAll('image_style_flush', [$this, $path]);

This looks like it's an API break? There isn't anything on the BC policy page about alter hook arguments, so I'm not sure.

Re-rolled the patch from #6 as it didn't apply cleanly for me (8.7.3)

+++ b/core/modules/image/image.api.php
@@ -32,8 +32,11 @@ function hook_image_effect_info_alter(&$effects) {
  * @param \Drupal\image\ImageStyleInterface $style
  *   The image style object that is being flushed.
+ *
+ * @param $path
+ *   A string containing a file path or (streamwrapper) URI.

there must be no empty line betwen the params and the param needs to document the type.

I think BC is OK, the only theoretical scenario is that someone would call the hook from a different place and not pass the second argument. We can fix that by documenting it as optional and defaulting to NULL.

I have re-rolled the patch against Drupal 8.9.x-dev and made changes that mentioned in #11.

Attached patch implements the following changes:

1. Update the @param description for $path so it matches the description of \Drupal\image\ImageStyleInterface::flush($path = NULL)
2. Fixed coding standards

Just confirming that the patch from #17 still works perfectly fine against D9.4.7

Hi, i'll review it.

Just to confirm: the patch from #17 still applies to Drupal core 9.4.9. I also needed this patch to get https://www.drupal.org/project/imageapi_optimize_webp/issues/3213379 working (WebP image styles not being updated using focal_point module)

Does this solution needs a test, and so a status Needs work? Or can we RTBC this?

I see many +1 in the #3213379: WebP images are not regenerated when using image_widget_crop ticket.

Hi can you add steps to reproduce this issue ? Please.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Think this could use a test case to show the issue.

Also since a new parameter is being added to the hook it will need a change record

thanks

Added a test. Adding a test-only for $path being passed is non-sensical since we wouldn't expect that to pass currently, but we can test the change where if you pass a $path to flush(), currently the hook is never called at all.
So added a test for that.

Test only is the interdiff.

Test coverage looks solid.

Added an attempt at a CR, https://www.drupal.org/node/3373248

Would be great to get confirmation from the release managers that the behavior change here is acceptable for a minor release.

I think this is sensible for a minor release - more likely it fixes a bug for someone than breaks it, and easy mitigation if there's a problem.

Updated the IS, added a release note

Test failure was unrelated to this change, and tests are back to green, so reverting to RTBC.

These seems a surprising change for me. Currently the hook is only invoked when a whole style is being flushed and not a single path. Therefore all existing implementations are going to assume that the everything is being flushed and not just a single path and therefore act accordingly. That said I can only find 2 instances of this hook in contrib and one already requires this patch so I think I agree with @catch here - okay for a minor release.

Committed 996fb53 and pushed to 11.x. Thanks!

Did a couple of improvements on commit...

Ensured that $path is the expected value in the test

diff --git a/core/modules/image/tests/modules/image_module_test/image_module_test.module b/core/modules/image/tests/modules/image_module_test/image_module_test.module
index 7ff821971f1..3955e44b69e 100644
--- a/core/modules/image/tests/modules/image_module_test/image_module_test.module
+++ b/core/modules/image/tests/modules/image_module_test/image_module_test.module
@@ -44,5 +44,5 @@ function image_module_test_image_style_presave(ImageStyleInterface $style) {
  */
 function image_module_test_image_style_flush($style, $path = NULL) {
   $state = \Drupal::state();
-  $state->set('image_module_test_image_style_flush.called', TRUE);
+  $state->set('image_module_test_image_style_flush.called', $path);
 }
diff --git a/core/modules/image/tests/src/Functional/ImageStyleFlushTest.php b/core/modules/image/tests/src/Functional/ImageStyleFlushTest.php
index 337f3827cc6..13334291953 100644
--- a/core/modules/image/tests/src/Functional/ImageStyleFlushTest.php
+++ b/core/modules/image/tests/src/Functional/ImageStyleFlushTest.php
@@ -129,10 +129,9 @@ public function testFlush() {
     $state = \Drupal::state();
     $state->set('image_module_test_image_style_flush.called', FALSE);
     $style->flush();
-    $this->assertTrue($state->get('image_module_test_image_style_flush.called'));
-    $state->set('image_module_test_image_style_flush.called', FALSE);
+    $this->assertNull($state->get('image_module_test_image_style_flush.called'));
     $style->flush('/made/up/path');
-    $this->assertTrue($state->get('image_module_test_image_style_flush.called'));
+    $this->assertSame('/made/up/path', $state->get('image_module_test_image_style_flush.called'));
   }
 
 }

Added correct typehinit on commit to hook docs.

diff --git a/core/modules/image/image.api.php b/core/modules/image/image.api.php
index 83fde6a69c0..2f786fc318c 100644
--- a/core/modules/image/image.api.php
+++ b/core/modules/image/image.api.php
@@ -32,7 +32,7 @@ function hook_image_effect_info_alter(&$effects) {
  *
  * @param \Drupal\image\ImageStyleInterface $style
  *   The image style object that is being flushed.
- * @param $path
+ * @param string|null $path
  *   (optional) The original image path or URI. If it's supplied, only this
  *   image derivative will be flushed.
  */

Ran the tests locally prior to commit.

It seems this caused some regressions:

• #3439981: Uploading a file to media library flushes theme registry
• #3439990: With 10.2 image styles might get wrongly flushed