Problem/Motivation

I need to add custom cache tags to my custom Computed field. I do this exactly like in entity_test/src/Plugin/Field/ComputedTestCacheableStringItemList.php but the Entity View page does not get tags from the Computed field.

So I dig in and reproduce this issue with the system/tests/modules/entity_test itself, and seems that this is a bug in Drupal Core.

Steps to reproduce

1. Enable the system/tests/modules/entity_test module.
To enable it you must allow showing test modules via this line in settings.php:

$settings['extension_discovery_scan_tests'] = TRUE;

2. Extend the baseFieldDefinitions() in the entity_test/src/Entity/EntityTestComputedField.php to show the field value on Entity View page:

    $fields['computed_test_cacheable_string_field'] = BaseFieldDefinition::create('computed_test_cacheable_string_item')
      ->setLabel(new TranslatableMarkup('Computed Cacheable String Field Test'))
      ->setComputed(TRUE)
      ->setClass(ComputedTestCacheableStringItemList::class)
      ->setReadOnly(FALSE)
      ->setInternal(FALSE)
      ->setDisplayOptions('view', [
        'label' => 'inline',
        'type' => 'string',
      ]);

3. Create a custom controller, where try to create the entity_test_computed_field entity, build a render array for it, and check the tags in it, something like this:

$entity = \Drupal::entityTypeManager()
  ->getStorage('entity_test_computed_field')
  ->create();
$view_builder = \Drupal::entityTypeManager()->getViewBuilder('entity_test_computed_field');
$pre_render = $view_builder->view($entity);
$build = $view_builder->build($pre_render);
dump($build['#cache']['tags']);

You will see only one tag in the list:

entity_test_computed_field_view

But class ComputedTestCacheableStringItemList also adds the field:computed_test_cacheable_string_field custom tag to the field value via this code:

  protected function computeValue() {
    /** @var \Drupal\entity_test\Plugin\Field\FieldType\ComputedTestCacheableStringItem $item */
    $item = $this->createItem(0, 'computed test cacheable string field');
    $cacheability = (new CacheableMetadata())
      ->setCacheContexts(['url.query_args:computed_test_cacheable_string_field'])
      ->setCacheTags(['field:computed_test_cacheable_string_field'])
      ->setCacheMaxAge(800);
    $item->get('value')->addCacheableDependency($cacheability);
    $this->list[0] = $item;
  }

So the issue is that tag is missing in render array. But, as I understand, it should be there, right? If I check the presence of the tag from the computed field incorrectly, please point me to the right way.

Also you can render the full HTML page and check that the field:computed_test_cacheable_string_field cache tag is missing in x-drupal-cache-tags string too.

Proposed resolution

Check if a field item list implements CacheableDependencyInterface in when rendering a field in FormatterBase and, if so, bubble the cacheable metadata to the rendered output.

Remaining tasks

Update issue summary

User interface changes

NA

API changes

No API change, but (arguably) an API addition, see the change notice: https://www.drupal.org/node/3423720

Data model changes

NA

Release notes snippet

NA

CommentFileSizeAuthor
#7 3304772-7.patch1.46 KBkksandr

Issue fork drupal-3304772

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

Murz created an issue. See original summary.

murz’s picture

I've found the change record Computed fields now bubble cacheability metadata to serializer that says that the issue is fixed in 9.3.0, but I test this on 9.5.x branch and the issue is still here, so seems this is another issue.

Also I've found some related issues:
- #3247945: Computed field is always cached on a JSON:API response
- #3252278: Complete JSON:API's handling of field (including computed & empty fields) cache metadata

but they don't help to solve the issue.

murz’s picture

Issue summary: View changes
murz’s picture

Issue summary: View changes

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

kksandr’s picture

StatusFileSize
new1.46 KB

I haven't found any code that treats field elements as cache dependencies. EntityViewDisplay should do this, but it only adds access checking as a cache dependency.

That is, now you can add cache metadata to the rendering array only through an access check. But I believe that if the list is doing value calculation, EntityViewDisplay should check if the list implements CacheacbleDependencyInterface.

With this approach, a list of elements will be able to provide metadata from its elements that implement CacheableDependencyInterface.

tstoeckler made their first commit to this issue’s fork.

tstoeckler’s picture

I encountered this as well, in the context of #3252278: Complete JSON:API's handling of field (including computed & empty fields) cache metadata.

I went with a slightly different approach than in #7 by checking this in the field formatter rather than the entity view display. It shouldn't make much of a difference practically, but there may be cases were a formatter is used without a view display (at least theoretically).

Also added some test coverage.

catch’s picture

#10 looks like a good approach. I wondered about computed fields being used without a formatter in hook_entity_view_alter() and similar places, but then people can add their the cache metadata themselves in that case.

smustgrave’s picture

So looking at the change and reading issue summary change makes sense so +1. But leaving in NR for additional eyes.

smustgrave’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Circled back to this one.

Re-verified test coverage exists here https://git.drupalcode.org/issue/drupal-3304772/-/jobs/704515

Issue summary is missing some sections, left TBD in sections I wasn't sure of as not as familiar with issue.

tstoeckler’s picture

Issue summary: View changes
Status: Needs work » Needs review

Filled in the proposed resolution and added a draft change notice for this, so back to needs review.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! CR reads well. Everything still looks fine to me so won't stall.

  • catch committed 82923eaf on 10.2.x
    Issue #3304772 by tstoeckler, kksandr, Murz, smustgrave: Cache tags from...

  • catch committed 555c098f on 10.3.x
    Issue #3304772 by tstoeckler, kksandr, Murz, smustgrave: Cache tags from...

  • catch committed 7ec35d9a on 11.x
    Issue #3304772 by tstoeckler, kksandr, Murz, smustgrave: Cache tags from...
catch’s picture

Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked to 10.3.x and 10.2.x, thanks!

wim leers’s picture

WOW 😳 Can't believe this was wrong all this time 🙈

Status: Fixed » Closed (fixed)

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