File/Image field formatters don't add a cache tag for the file they display

Thanks, great catch. Should be easy to fix :)

GoZ is working on this at Drupal Dev Days.

I add cache tags for image formatter with link.
I also change tests but can't test for the moment, they need to be reviewed and tested.

Progress, yay! :) Thank you!

1. +++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
   @@ -153,8 +154,8 @@ function _testImageFieldFormatters($scheme) {
   +    $file = file_load($node->{$field_name}->target_id);
   
   @@ -186,8 +187,9 @@ function _testImageFieldFormatters($scheme) {
   +    $image_style = entity_load('image_style', 'thumbnail');
   

   Let's use File::load() and ImageStyle::load() instead :)

2. +++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
   @@ -332,8 +334,8 @@ function testImageFieldDefaultImage() {
   +    $file = file_load($node->{$field_name}->target_id);
   
   @@ -361,8 +363,8 @@ function testImageFieldDefaultImage() {
   +    $file = file_load($node->{$field_name}->target_id);
   
   @@ -383,9 +385,8 @@ function testImageFieldDefaultImage() {
   +    $file = file_load($node->{$field_name}->target_id);
   
   @@ -430,9 +431,8 @@ function testImageFieldDefaultImage() {
   +    $file = file_load($node->{$field_name}->target_id);
   

   Do we really want to load the file every single time?

I fix some things with your recommendations.

I was disagree with previous tests, so new one aren't same as before.

Still one issue with tests, if i'm not wrong, cache tag should be found at line 159, but test fail because tag is not present (and should be). I think test is good, but point on a case where cache tag is missing.

Set needs review status to see l159 case. But still needs work

Forget this case, i was wrong with test, cache tag should not be found since we search for file cache tag and do not display link.

@GoZ could you please provide an interdiff with new patches? Thanks!

Here is interdiff.

Patch can't apply since d8 has new commits.

Looking great! :) Test coverage is already sufficient, no need for more tests. Below, only nitpicks. The main missing thing is that file formatters still aren't setting File entity cache tags: this patch only deals with the Image formatter right now.

1. +++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
   @@ -94,7 +95,7 @@ function _testImageFieldFormatters($scheme) {
   +    $image_uri = File::load($node->{$field_name}->target_id)->getFileUri();
   
   @@ -123,8 +124,9 @@ function _testImageFieldFormatters($scheme) {
   +    $file = File::load($node->{$field_name}->target_id);
   

   Why not move the $file = line to come first, and then do $image_uri = $file->getFileUri()?

2. +++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
   @@ -123,8 +124,9 @@ function _testImageFieldFormatters($scheme) {
   -    $cache_tags_header = $this->drupalGetHeader('X-Drupal-Cache-Tags');
   -    $this->assertTrue(!preg_match('/ image_style\:/', $cache_tags_header), 'No image style cache tag found.');
   
   @@ -154,8 +156,7 @@ function _testImageFieldFormatters($scheme) {
   -    $cache_tags_header = $this->drupalGetHeader('X-Drupal-Cache-Tags');
   -    $this->assertTrue(!preg_match('/ image_style\:/', $cache_tags_header), 'No image style cache tag found.');
   
   @@ -362,8 +365,7 @@ function testImageFieldDefaultImage() {
   -    $cache_tags_header = $this->drupalGetHeader('X-Drupal-Cache-Tags');
   -    $this->assertTrue(!preg_match('/ image_style\:/', $cache_tags_header), 'No image style cache tag found.');
   
   @@ -375,17 +377,17 @@ function testImageFieldDefaultImage() {
   -    $cache_tags_header = $this->drupalGetHeader('X-Drupal-Cache-Tags');
   -    $this->assertTrue(!preg_match('/ image_style\:/', $cache_tags_header), 'No image style cache tag found.');
   
   @@ -431,8 +433,7 @@ function testImageFieldDefaultImage() {
   -    $cache_tags_header = $this->drupalGetHeader('X-Drupal-Cache-Tags');
   -    $this->assertTrue(!preg_match('/ image_style\:/', $cache_tags_header), 'No image style cache tag found.');
   

   I think we still want these assertions, don't we?

   These assert that there are no image style cache tags at all. Just checking that there is no file cache tag either is good, but I don't see why we want to remove the existing assertions?

3. +++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
   @@ -187,8 +188,9 @@ function _testImageFieldFormatters($scheme) {
   -    $cache_tags = explode(' ', $this->drupalGetHeader('X-Drupal-Cache-Tags'));
   -    $this->assertTrue(in_array('config:image.style.thumbnail', $cache_tags));
   +
   +    $image_style = ImageStyle::load('thumbnail');
   +    $this->assertCacheTag($image_style->getCacheTags()[0]);
   

   This change is correct, it's simple clean-up :) Can you remove the added newline though?

4. +++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
   @@ -247,9 +249,10 @@ function testImageFieldSettings() {
   +    $file = File::load($node->{$field_name}->target_id);
   ...
   -      '#uri' => file_load($node->{$field_name}->target_id)->getFileUri(),
   +      '#uri' => $file->getFileUri(),
   
   @@ -260,7 +263,7 @@ function testImageFieldSettings() {
   -      '#uri' => file_load($node->{$field_name}->target_id)->getFileUri(),
   +      '#uri' => $file->getFileUri(),
   

   Yes, better! :)

is this issue specific to image formatters ? Wouldn't any entity_ref formatter need to do something similar ?

@Yched : This issue is only for file and image formatters. For some others, better is to have an issue by formatter to check.

Here is corrections based on Wim #19 comments plus file formatter.

--- a/core/modules/file/src/Plugin/Field/FieldFormatter/BaseFieldFileFormatterBase.php
+++ b/core/modules/file/src/Plugin/Field/FieldFormatter/BaseFieldFileFormatterBase.php

The name is misleading, but that one is actually not a formatter for fields of type "file" (i.e fields referencing a File entiy), it's a helper base class for formatters of the base fields living *on* the File entity itself.

So this is not part of the issue here, and this hunk should be reverted :-)

Here is patch without adding cache tags and tests for BaseFieldFileFormatterBase

Awesome, thank you! :)

+++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
@@ -123,6 +125,8 @@ function _testImageFieldFormatters($scheme) {
+

Committer: please remove this single line of unnecessary whitespace :)

Sorry, hadn't spotted that earlier :

+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
@@ -210,6 +211,7 @@ public function viewElements(FieldItemListInterface $items) {
       if (isset($link_file)) {
         $image_uri = $file->getFileUri();
         $url = Url::fromUri(file_create_url($image_uri));
+        $cache_tags = Cache::mergeTags($cache_tags, $file->getCacheTags());
       }

This doesn't look correct : we should be adding the $file's tags unconditionally, not only within this "if (we're wrapping the output in a link)"
So that line should move just outside if the if branch (or it could also be directly inlined in the line that does '#tags' => ...)

Other than that, RTBC++ :-)

NW for that tiny bit. So very close!

Line has moved outside, and tests should now see cache tag each time file is displayed.

Those assertions also make 200% more sense. Can't believe I missed that before.

@yched++
@GoZ++

RTBC +1, thanks @GoZ :-)

#30: ooh, nice!

Let's do that too.

Here patch for #30.

Great, thanks!

Facepalm. I was like "nice, this removes a file_load() call", but didn't look further. @Berdir++

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 9def612 and pushed to 8.0.x. Thanks!