Problem/Motivation

Those formatters should add a cache tag, so that in case those files change, the caches are properly cleared.

It's not common in core, but when you add file_entity/media, then it is possible that files are changed and have fields that might affect the output.

Proposed resolution

Add the cache tags to all file and image formatters like this:

          '#cache' => array(
            'tags' => $item->entity->getCacheTags(),
          ),

Remaining tasks

User interface changes

API changes

Comments

Wim Leers’s picture

Issue tags: +Needs tests

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

Wim Leers’s picture

Priority: Normal » Major
Wim Leers’s picture

GoZ is working on this at Drupal Dev Days.

GoZ’s picture

FileSize
7.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,588 pass(es), 3 fail(s), and 1 exception(s). View

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.

anavarre’s picture

Status: Active » Needs review
Wim Leers’s picture

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?

Status: Needs review » Needs work

The last submitted patch, 4: core-file-image-formatters-cache-tags-2388023-4.patch, failed testing.

GoZ’s picture

Status: Needs work » Needs review
FileSize
7.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core-file-image-formatters-cache-tags-2388023-8.patch. Unable to apply patch. See the log in the details link for more information. View

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

Status: Needs review » Needs work

The last submitted patch, 9: core-file-image-formatters-cache-tags-2388023-8.patch, failed testing.

GoZ’s picture

Status: Needs work » Needs review
FileSize
7.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core-file-image-formatters-cache-tags-2388023-11.patch. Unable to apply patch. See the log in the details link for more information. View

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.

Status: Needs review » Needs work

The last submitted patch, 11: core-file-image-formatters-cache-tags-2388023-11.patch, failed testing.

GoZ’s picture

FileSize
7.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core-file-image-formatters-cache-tags-2388023-13.patch. Unable to apply patch. See the log in the details link for more information. View
anavarre’s picture

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

GoZ’s picture

FileSize
4.13 KB

Here is interdiff.

anavarre’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 13: core-file-image-formatters-cache-tags-2388023-13.patch, failed testing.

GoZ’s picture

Status: Needs work » Needs review
FileSize
7.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,435 pass(es). View

Patch can't apply since d8 has new commits.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests +D8 Accelerate Dev Days

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! :)

yched’s picture

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

GoZ’s picture

Status: Needs work » Needs review
FileSize
10.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,444 pass(es). View
7.74 KB

@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.

yched’s picture

--- 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 :-)

GoZ’s picture

FileSize
9.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,478 pass(es). View
1.47 KB

Here is patch without adding cache tags and tests for BaseFieldFileFormatterBase

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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 :)

yched’s picture

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++ :-)

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

NW for that tiny bit. So very close!

GoZ’s picture

Status: Needs work » Needs review
FileSize
9.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,480 pass(es). View
3.75 KB

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

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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

@yched++
@GoZ++

yched’s picture

RTBC +1, thanks @GoZ :-)

Berdir’s picture

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

    You should actually be able to use $node->{$field_name}->entity here, if you're changing it anyway.

  2. +++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
    @@ -247,9 +251,10 @@ function testImageFieldSettings() {
         $node = $node_storage->load($nid);
    +    $file = File::load($node->{$field_name}->target_id);
         $image_style = array(
    

    same here.

  3. +++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
    @@ -375,15 +381,17 @@ function testImageFieldDefaultImage() {
         $node = $node_storage->load($nid);
    +    $file = File::load($node->{$field_name}->target_id);
    

    and here :)

But the changes look good to me as well. Next step would then be to update #2464475: Add cache tag test coverage for File content entity so that it uses a file reference instead of an entity reference and a file formatter and make sure it's still working (which it should now, with this fixed).

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll, +Novice

#30: ooh, nice!

Let's do that too.

GoZ’s picture

Status: Needs work » Needs review
FileSize
8.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,436 pass(es). View
1.58 KB

Here patch for #30.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll, -Novice

Great, thanks!

yched’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • alexpott committed 9def612 on
    Issue #2388023 by GoZ: File/Image field formatters don't add a cache tag...

Status: Fixed » Closed (fixed)

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