Problem/Motivation

If a computed field introduces some extra cache metadata, that metadata isn't used unless the field is displayed using a lazy builder.

Steps to reproduce

This can be demonstrated by the test_current_user plugin (enable the test_computed_field_plugins module). You also need two users with identical roles/permissions.

1. Add the "computed field as normal, create a node etc.
2. View the node as user A. Cache miss. Works as expected: user A's name is shown. Output is cached.
3. View the node as user B. Cache hit. User A's name is shown.

Although that plugin declares the user cache context in its getCacheability() function, this metadata doesn't make its way up to the node.

Proposed resolution

Apply the cache metadata when the computed field is displayed. Or, if this can't be done for some reason, we should document that.

Remaining tasks

TBC

Issue fork computed_field-3487785

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:

About issue forks

Comments

erik.erskine created an issue. See original summary.

erik.erskine’s picture

erik.erskine commented

Did some digging, and found this in FieldFormatterBase::view():

    // Field item lists, in particular for computed fields, may carry cacheable
    // metadata which must be bubbled.
    if ($items instanceof CacheableDependencyInterface) {
      (new CacheableMetadata())
        ->addCacheableDependency($items)
        ->applyTo($elements);
    }

So it looks as though ComputedFieldClass needs to implement CacheableDependencyInterface and pass on the cacheable metadata from the plugin. Patch coming up.

erik.erskine opened merge request !16

erik.erskine’s picture

erik.erskine commented
Status: Active » Needs review

First attempt at seeing how we might solve this. Needs work for sure, but wanted to get feedback on the approach.

joachim’s picture

joachim commented
Status: Needs review » Needs work

> Did some digging, and found this in FieldFormatterBase::view():

Nice find!

I've fixed the broken tests -- could you rebase this please so the tests can run on it again?

erik.erskine’s picture

erik.erskine commented
Status: Needs work » Needs review

Rebased!

joachim’s picture

joachim commented
Status: Needs review » Needs work

Thanks!

I started looking at this, and figured I'd add test coverage. I've got something working (in branch 3487785-cacheability-tests) -- but I need to do a bit of clean-up on it as I had to try several approaches to get caching to kick in!

joachim’s picture

joachim commented

Hmm on second thoughts, I can't get it to work :/
Pushed everything to the branch.

joachim’s picture

joachim commented
Status: Needs work » Fixed

I figured out the test.

Committed, and going to make a 1.0.0 release.

Thanks for your help on this one!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • joachim committed e3cbded3 on 4.0.x 
    feat: #3487785 Cacheability is not respected when not using a lazy...

Status: Fixed » Closed (fixed)

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