Uploading a file to media library flushes theme registry

Traced it down to be caused by 🐛 hook_image_style_flush doesn't get the $path passed to Drupal\image\Entity\ImageStyle::flush() method Fixed - before this change it was returning earlier in the method, thus never clearing theme registry when $path is set

After some more research I found out the change was introduced by 🐛 hook_image_style_flush doesn't get the $path passed to Drupal\image\Entity\ImageStyle::flush() method Fixed in Drupal 10.2 - a new optional $path parameter was added to image style flush hook. While doing so the flush() method was re-factory wrongly such that the theme registry + cache tags are cleared also when $path is given.

So both should be fixed. We've reproduced that addressing this and the related bug 🐛 With 10.2 image styles might get wrongly flushed Active solves the issues and brings those slow file upload requests down from ~25seconds to normal <2seconds.

Related change record: https://www.drupal.org/node/3373248 →

Here is a patch to prevent flushing theme registry + cache tags if the path is provided.

Converted the patch into a MR so the pipeline runs and slightly improved the comment.

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

This should be rebased against 11.x first, also we don't need to include usernames in the MR titles. Hiding the patch.

And of course, we need tests :)

Not sure what a test would do here? Maybe a unit test making sure the theme-registry service is not invoked when we call $imageStyle->flush($path);

@fago yeah having a look at what the theme registry reset does, I guess you could check that those specific cids still exist in cache. Ideally we'd have a test cache tag invalidator that could record the tags invalidated and we could assert things weren't invalidated but I can't see anything like that in core atm.

ericgsmith → changed the visibility of the branch 3439981-uploading-a-file to hidden.

Added a new test method to the existing Image Style unit test to check that the registry and cache invalidation is not called if flush is called with a path. Checking the number of invocations seemed simpler than trying to dig through cache ids.

Hid the original MR - I see now according to https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... → I should have pinged you to update it the MR - anyway, no open discussions on the original so hopefully all good.

Looks good to me, thanks!

, I guess you could check that those specific cids still exist in cache. Ideally we'd have a test cache tag invalidator that could record the tags invalidated and we could assert things weren't invalidated but I can't see anything like that in core atm.

It's possible to assert the number of calls to cache tag invalidation in performance tests - would that be enough to produce a test that fails without the fix?

I think the test coverage we have here is good enough for a pretty important bug fix. Sorry about committing the issue that caused the regression.

Committed and pushed 7b24d54842 to 11.x and 7546eb2a72 to 10.3.x and 02b8bc5462 to 10.2.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.