Problem/Motivation

Currently, the ViewsBlockBase class does not fully leverage the cache metadata (contexts, tags, max-age) provided by the associated view's display handler. This can lead to inconsistencies in cache behavior when using Views blocks, especially when the display handler defines specific cache settings.

For example:

  • Cache contexts from the view are not merged into the block's cache contexts.
  • Cache tags provided by the view's display handler are ignored in the block context.
  • Cache max-age of the block does not reflect the display handler's cache max-age.

This limits the ability to align the block's caching behavior with that of the View, potentially resulting in outdated content being served or inefficient cache invalidation.

Steps to reproduce

1. Create a View with specific cache metadata in its display handler (e.g., cache contexts or tags).

2. Add the View as a block on a page.

3. Observe that the cache behavior of the block does not reflect the settings in the display handler.

Proposed resolution

Introduce overrides in the ViewsBlockBase class for the following methods:

  • getCacheContexts()
  • getCacheTags()
  • getCacheMaxAge()

These methods will merge the cache metadata from the parent block class with the cache metadata from the view's display handler, ensuring consistent and efficient caching.

Remaining tasks

  • Write unit tests to ensure the cache metadata (contexts, tags, max-age) is correctly merged between the block and the view's display handler. Done
  • Test scenarios where the display handler provides:
    • Additional cache contexts. Done
    • Specific cache tags. Done
    • A finite or infinite cache max-age. Done
  • Kernel tests. Necessary?
  • Verify that cache invalidation works as expected when display handler cache metadata changes. Necessary?

User interface changes

None.

Introduced terminology

None.

API changes

No API changes. The modifications enhance the existing methods without altering their signatures or introducing breaking changes.

Data model changes

None.

Release notes snippet

The ViewsBlockBase class now merges cache metadata from the associated view's display handler for contexts, tags, and max-age. This ensures consistent cache behavior between Views blocks and their display handlers.

Issue fork drupal-3493858

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

xavier.masson created an issue. See original summary.

xavier.masson’s picture

StatusFileSize
new1.28 KB
lendude’s picture

quietone’s picture

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

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs tests

@xavier.masson can you see if the issue mentioned by @lendude in #3 is the same?

If not the change here should be in an MR, with test coverage showing the issue.

Thanks!

xavier.masson’s picture

@smustgrave , after reviewing the issue mentioned in comment #3, it seems to be completely different.

I attempted to create a merge request, but it seems I don’t have the necessary permissions. Regarding the tests, I’ll try to provide them when i'll have some free time.

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

vidorado’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests

Added a unit test that I believe is enough to test the funcionality.

In the "Remaining tasks" section there were kernel test and cache invalidation test suggestions, but I think they are unnecessary.

vidorado changed the visibility of the branch 11.x to hidden.

smustgrave’s picture

Status: Needs review » Needs work

Comments in MR.

vidorado’s picture

Thanks for the quick review!

I've replied to some comments. Please, let me know if this additional comment on the issue queue was not necessary.

vidorado’s picture

Status: Needs work » Needs review

All threads resolved! :)

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

All feedback on this one appears to be resolved.

quietone’s picture

I read the IS, comments and the MR. I didn't find any unanswered questions or other work to do.

  • longwave committed 09158e66 on 10.4.x
    Issue #3493858 by vidorado, xavier.masson, smustgrave: Extend...

  • longwave committed 766f3263 on 10.5.x
    Issue #3493858 by vidorado, xavier.masson, smustgrave: Extend...

  • longwave committed 1a8a6184 on 11.1.x
    Issue #3493858 by vidorado, xavier.masson, smustgrave: Extend...

  • longwave committed 1ad501aa on 11.x
    Issue #3493858 by vidorado, xavier.masson, smustgrave: Extend...

longwave’s picture

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

This is a nice self contained fix, backported down to 10.4.x as an eligible bug fix.

Committed and pushed 1ad501aafe7 to 11.x and 1a8a6184101 to 11.1.x and 766f32639c9 to 10.5.x and 09158e668be to 10.4.x. Thanks!

wim leers’s picture

Issue tags: +D8 cacheability

WOW! How did we miss this before 8.0.0 shipped?! And especially: I'm shocked that this did not become a priority years ago 😮

Very nice — I bet this fixes many subtle confusing problems on many sites, and it does so in a backwards compatible way! Impressive 👏

Status: Fixed » Closed (fixed)

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

mxr576’s picture

Bubbling up cacheabilty information from the display handler may not be enough, the cacheability information from the View entity should be also incorporated - at least cache tags, that includes the View config entity's own cache tag - more details in #2887144-44: Views exposed form block options are not updated immediately when adding additional sorts, filters, etc (Caching?)

Let me also deeplink the failing test which should prove a cache invalidation issue still exists: https://git.drupalcode.org/issue/drupal-2887144/-/pipelines/500971/test_...)