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
| Comment | File | Size | Author |
|---|
Issue fork drupal-3304772
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:
- 3304772-computed-field-cacheable-metadata
changes, plain diff MR !6355
Comments
Comment #2
murzI'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.
Comment #3
murzComment #4
murzComment #7
kksandr commentedI 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.
Comment #10
tstoecklerI 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.
Comment #11
catch#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.
Comment #12
smustgrave commentedSo looking at the change and reading issue summary change makes sense so +1. But leaving in NR for additional eyes.
Comment #13
smustgrave commentedCircled 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.
Comment #14
tstoecklerFilled in the proposed resolution and added a draft change notice for this, so back to needs review.
Comment #15
smustgrave commentedThanks! CR reads well. Everything still looks fine to me so won't stall.
Comment #19
catchCommitted/pushed to 11.x and cherry-picked to 10.3.x and 10.2.x, thanks!
Comment #20
wim leersWOW 😳 Can't believe this was wrong all this time 🙈