(Responsive) Image styles do not add correct cache tags, nor do they invalidate cache tags upon flushing

Makes sense.

Was trying to think why we'd ever need this in the first place because it wasn't immediately obvious. And why it wasn't just using cache_clear_all() too, then we'd have seen the problem sooner.

The only situation that cache clear is valid I can think of is the following:

1. The final dimensions of the image derivatives change as a result of the update.

2. Since we specify width/height in markup, those would be wrong in cached HTML.

If that's correct, then rather than trying to clear the cache directly, we should be adding cache tags when an image derivative gets rendered, so that affected HTML gets invalidated. That will then work for both the page and block caches as well as anything else.

I understand that, but the underlying bug here is that image styles have been trying to clear the block cache because of the content, not the plugins, and never got updated to clearing the render cache.

This would have been caught if it'd been a cache_clear_all() call, or if there was test coverage for outdated image style dimensions in cached HTML from blocks, but yeah neither of those...

To fix the plugin performance issue, we could fix this by changing the code here to clear the 'content' cache tag, then open a critical follow-up as a sub-issue of #2124957: Replace 'content' cache tag with 'rendered' and use it sparingly to add image style cache tags instead of using that.

And agreed this should be in cache_bootstrap or cache, not cache_block.

#1 was already accidentally committed in http://drupalcode.org/project/drupal.git/commitdiff/af0f562

The attached patch clears the 'content' cache tag as suggested in #5.

OK that works, I left a note on the other issue. Also whoopsie on the commit.

Why would we add a new content cache tag? Our goal is to remove it! See #2124957: Replace 'content' cache tag with 'rendered' and use it sparingly.

Why not just add proper cache tags to Image Styles and add the necessary test coverage?

Yeah we could also just do that here.

I wanted to let a quick fix in to stop the block plugin rebuilding, but inadvertently did that already.

Great! :)

This should wait for #2188565: Test coverage for Comment, Custom Block, Node, Taxonomy Term and User entity cache tags, at least in terms of test coverage, because that issue provides a handy base class.

To solve the general problem, I've opened #2217749: Entity base class should provide standardized cache tags and built-in invalidation.

There still is a part of the solution that is specific to this issue though, so I've kept this. This is now blocked on #2217749: Entity base class should provide standardized cache tags and built-in invalidation.

#2217749: Entity base class should provide standardized cache tags and built-in invalidation landed, now we can continue here!

Here is a (cleaner) reroll of #12. I've expanded the scope to also handle the Responsive Image Formatter, since it's so similar (and because otherwise ImageStyle::flush() still wouldn't have the desired effect, so it's actually within scope on second thought).

As commented in the views bubble up issue, this is one of the cases where I'm wondering if it's really a good idea to add 3,4, 10+ cache tags to pages just because you change them twice a year? And when you do, then it will be as part of a config sync on any serious sites, which does a full cache clear anyway.

It really might not be a stupid idea to retain the content cache tag and use it for extremely rare cases like this because it's going to be much more efficient in the end to do a huge cache deletion once a month or possibly even once every few days instead of having to check 10+ additional cache tags on every single page request...

#14: I thought the idea was to first add cache tags to everything and then see which ones make sense to remove and/or combine for performance reasons. "Premature optimization is the root of all evil" and all that.

Ok fine (although one could argue how premature this is :)), but then I think we should have a critical task or something to profile cache tags usage, possibly tag with revisit before release or something if we don't have that already anyway?

Should config sync do a full cache clear if we have cache tags working properly? Not sure it has to except for module enable/disable.

Agreed on a critical task for rationalising the tags once we have them. I think it's better to use them consistently then optimise them away afterwards. In the back of my mind I'm thinking about implementations that can do direct deletion and then won't have a runtime hit fetching the tags at all, but since both memcache and db backend will have the performance hit we can't rely on that.

#16: we already have that, see #2241377: [meta] Profile/rationalise cache tags :)

So, with that aspect out of the way, can I has RTBC? :) (The patch still applies cleanly.)

I reviewed the code and it looks like a solid improvement.

Committed/pushed to 8.x, we might rip this out again when we do the cache tag audit but this unblocks other stuff.

+++ b/core/modules/image/lib/Drupal/image/Plugin/Field/FieldFormatter/ImageFormatter.php
@@ -113,6 +113,14 @@ public function viewElements(FieldItemListInterface $items) {
+    // Collect cache tags to be added for each item in the field.
+    $cache_tags = array();
...
+      $cache_tags = $image_style->getCacheTag();

It seems this should be

$cache_tags[] = $image_style->getCacheTag();

right? That would also be consistent with how it is used below.

#22: No, because Entity::getCacheTag() looks like this:

  public function getCacheTag() {
    return array($this->entityTypeId => array($this->id()));
  }

i.e. it already returns an array.

In other words, the name is a bit misleading. But I think that's a fine cost to pay, because it allows us to things like:

$cache_tags = NestedArray::mergeDeep(
  $entity->getCacheTag(),
  $entity->getListCacheTags(),
  $other_entity->getCacheTag(),
  array('and_something_custom' => TRUE)
);

i.e. it makes it super trivial to gather cache tags from all sorts of places and merge them together in a single cache tags array.

If you disagree with this and would like to change it, please open a new issue :)

+++ b/core/modules/responsive_image/lib/Drupal/responsive_image/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
@@ -168,6 +169,23 @@ public function viewElements(FieldItemListInterface $items) {
+    $all_cache_tags = array();
...
+      $all_cache_tags[] = $responsive_image_mapping->getCacheTag();
...
+          $all_cache_tags[] = $image_style->getCacheTag();
...
+    $cache_tags = NestedArray::mergeDeepArray($all_cache_tags);

Ahh... thanks very much for the explanation. The [] part of the code above threw me off and lead me to believe it was just a single tag. I missed the NestedArray::mergeDeepArray().

I do think the method name should be changed but that is definitely not part of this issue.

Thanks very much and sorry for the noise!

You're very welcome and no worries! :)