Problem/Motivation

Right now the only part of views which adds arguments to views cache keys is "DisplayPluginBase::buildBasicRenderable".

My specific use case involved programmatically rendering a block with different arguments. 'url' was the only cache context that appeared and given two of these blocks were programatically rendered on the same page, the markup was the same regardless of the arguments. Using the static method fixed this.

Proposed resolution

Views should add these to all displays, not making developers rely on the static to add this for them. I can't see a downside with making the views arguments part of the cache keys for all scenarios.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#22 2823914--22.patch5.59 KBdanflanagan8
#20 2823914--20--FAIL.patch4.59 KBdanflanagan8
#18 2823914--18--FAIL.patch4.59 KBdanflanagan8
#7 views-build-renderable-cache-args-2823914-7.patch1 KBberdir

Comments

Sam152 created an issue. See original summary.

sam152’s picture

sam152 commented
Issue summary: View changes

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Just found this as well, this is fun.

I suspect we're not seing that very often because in 99% (rough estimate ;) of the cases, the arguments are coming from the URL.

Added a workaround for now in my project, but will try to provide a patch, should be easy enough.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +D8 cacheability, +VDC
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new views-build-renderable-cache-args-2823914-7.patch1 KB

Strange, buildBasicRenderable() is used by Rest/Feed/Page controllers/displays and lots of tests, don't really understand why we have those two different approaches.

I just copied the existing logic from that up into buildRenderable(), lets see if any tests are failing because of this.

Status: Needs review » Needs work

The last submitted patch, 7: views-build-renderable-cache-args-2823914-7.patch, failed testing. View results

sam152’s picture

sam152 commented
Title: Always add views arguments to #cache keys to help protect against erroneous cache entires » Render caching in DisplayPluginInterface::buildRenderable is broken when arguments are provided.

I'm not sure why both methods exist either, but fwiw the patch looks good to me. There is already a flag to allow you to opt out of render caching, so I suppose that's useful if the arguments are a high cardinality?

I think this title is probably a bit more discoverable for others who might be having this issue.

dawehner’s picture

dawehner
Primary language German
commented

Strange, buildBasicRenderable() is used by Rest/Feed/Page controllers/displays and lots of tests, don't really understand why we have those two different approaches.

I don't remember it anymore either. I guess buildRenderable existed before we realized that we need something like buildBasicRenderable + a render element to support true render caching.
Reading the code I'm wondering whether we could replace buildRenderable by buildBasicRenderable

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

nvaken’s picture

nvaken
he/him
Primary language Dutch
commented

Bounced into this one as well. Cost me a few hours of debugging, finally found out that adding unique keys to the `keys` element in `#cache` fixed my issue. I'd be in for this patch.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
StatusFileSize
new 2823914--18--FAIL.patch4.59 KB

Here's a fail test (hopefully).

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs work » Needs review
danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
StatusFileSize
new 2823914--20--FAIL.patch4.59 KB

Try that again.

Status: Needs review » Needs work

The last submitted patch, 20: 2823914--20--FAIL.patch, failed testing. View results

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
StatusFileSize
new 2823914--22.patch5.59 KB

That failure was exactly what I wanted. Now I'll add the fix from #7 and we'll see if it passes.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs work » Needs review
sam152’s picture

sam152 commented
Status: Needs review » Reviewed & tested by the community

Elegant test and fix. Looks good to me.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Issue tags: +Bug Smash Initiative

Thanks for reviewing! Adding the Bug Smash tag because that's how I got here.

  • larowlan committed 10fc602 on 9.2.x 
    Issue #2823914 by danflanagan8: Render caching in DisplayPluginInterface...

  • larowlan committed 020a5fa on 9.1.x 
    Issue #2823914 by danflanagan8: Render caching in DisplayPluginInterface...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 10fc602 and pushed to 9.2.x. Thanks!

As the risk of disruption here is low, backported this to 9.1.x

Thanks all

Status: Fixed » Closed (fixed)

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