See this excerpt from \Drupal\image\Plugin\Field\FieldFormatter\ImageFormatter::viewElements():
$cache_tags = array();
...
foreach ($files as $delta => $file) {
...
$cache_tags = Cache::mergeTags($cache_tags, $file->getCacheTags());
}
Cache tags from files that are later in the $files incorrectly get the cache tags from the earlier ones.
Change the variable name outside of the foreach loop to $base_cache_tags or (preferably) to something more semantic.
Write test to show that this is a problem.
Create patch that fixes the problem.
None.
None.
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | 2743463-imageformatter-cache-tags-7.patch | 4.63 KB | tstoeckler |
Comments
Comment #2
pcambraJust confirmed the error, I've added a test to verify it, not sure if where I've placed it is the best fit
Comment #4
pcambraComment #5
mondrakeLooks good to me, has tests. Not sure do we need to clear caches?
Comment #6
tstoecklerSorry, X-post.
Yay, thanks for picking this up.
Sorry, for being a bit picky, but I think it would be nicer to just merge those lines. and do
directly.
For the cache contexts it makes sense to declare the variable separately, because the context is only added conditionally, but this is not the case for the tags.
Also I think we should find a better place for the test.
ImageItemTestactually tests the field functionality itself, and I think it would be not super nice to mix tests for the formatter in with it.Sadly there's no
ImageFormatterTestyet, but maybe we can create it (as another kernel test). The test code itself looks pretty great, in my opinion. The only quibble I have is that we could be a bit more explicit and actually instantiate the formatter and call theviewElements()method on it (or whichever it is).Comment #7
pcambraLet's see if this looks better :)
Comment #8
tstoecklerLooks great, thanks a lot for the quick follow-up. Uploading a tests only patch just to see if it still fails as expected, but absolutely RTBC if so.
Comment #9
tstoecklerComment #10
tstoecklerAhh, the testbot still can't run files which are uploaded as "hidden"...
Comment #12
tstoecklerAwesome! Re-uploading #7 and marking RTBC.
Comment #13
catchCommitted/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!