Problem/Motivation

If you call \Drupal\Core\Cache\RefinableCacheableDependencyTrait::addCacheableDependency with something that doesn't implement CacheableDependencyInterface, you end up with uncacheable pages.

Examples of bugs in core this has lead to include: #3227637: NodeController::revisionOverview is uncacheable and probably more.

I would argue that we should move towards deprecating calling this method with an object that doesn't implement CacheableDependencyInterface to prevent people from inadvertently introducing non-cacheable pages.

Steps to reproduce

Call \Drupal\Core\Cache\RefinableCacheableDependencyTrait::addCacheableDependency with \Drupal::currentUser or a render array - see your page becomes uncacheable

Proposed resolution

Trigger a deprecation error if called with an object that doesn't implement CacheableDependencyInterface
In the next major version remove that deprecation error and add a type-hint (hard break).

Remaining tasks

All of the above

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#2 3232018.patch796 byteslarowlan

Issue fork drupal-3232018

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

larowlan created an issue. See original summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Active » Needs review
StatusFileSize
new 3232018.patch796 bytes

This will fail on at least #3227637: NodeController::revisionOverview is uncacheable

acbramley’s picture

acbramley commented

Big +1 on this. Someone accidentally did this in a formatter on one of our projects and it destroyed performance on some views listing/search pages. Took a long time to figure out what was causing it too!

Status: Needs review » Needs work

The last submitted patch, 2: 3232018.patch, failed testing. View results

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Related issues: +#3313767: Version 3.2.0 makes all pages uncacheable with dynamic page cache

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.

abhaypai’s picture

abhaypai commented
Issue tags: +Bug Smash Initiative

Landed on this issue from https://www.drupal.org/project/drupal/issues/3227637 and https://lendude.gitlab.io/bug-smash-initiative/ and i am still able to replicate this in D11

leksat’s picture

leksat commented
Issue summary: View changes

Another +1. I spent quite some time with a debugger trying to find why a GraphQL query wasn't cached.

Leksat opened merge request !7362

leksat’s picture

leksat commented

Tried to fix LocalActionManagerTest::testGetActionsForRoute.

It fails because in the test we mock LocalActionInterface which does not extend CacheableDependencyInterface. And then it is passed to RefinableCacheableDependencyTrait::addCacheableDependency().

The actual class (LocalActionDefault) however does extend CacheableDependencyInterface.

I see two options:
1. Make LocalActionInterface extend CacheableDependencyInterface (would it be an API change?)
2. Mock LocalActionDefault instead (but AFAICS we always mock interfaces, not classes)

Does any of them look good? Are there other options? I have no clue 😅

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

I think making LocalActionInterface extend is reasonable - there's a lot of code in LocalActionDefault that would make it feasible that anything custom would extend it rather than replace it

leksat’s picture

leksat commented
Status: Needs work » Needs review

Tests are green, but there are open questions. I've put them into the MR. See todos in the code.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left a comment on the MR

leksat’s picture

leksat commented
Status: Needs work » Needs review

@smustgrave Changes are ready.

What about the two other questions pointed in TODOs? Should the maintainers of the corresponding modules answer them? There's one in field_ui and one in layout_builder.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs followup

For the todos may be out of scope to remove the cache there. So to keep this one moving lets open a follow up and update the comment with a @todo to the issue created. Then should be good here.

leksat’s picture

leksat commented
Status: Needs work » Needs review
Issue tags: -Needs followup

Created follow-ups
- #3446507: Adjust caching logic of FieldReuseAccessCheck::access()
- #3446509: Adjust caching logic of LayoutBuilderAccessCheck::access()
and linked them in TODOs ✅

Please note: The deprecation notice points to 10.3.0, but the MR still targets 11.x

leksat’s picture

leksat commented

The test failures seems to be caused by PDOException: SQLSTATE[HY000] [2002] Connection refused. So I guess they are random and not related.

It looks like I have no permissions to re-run them 🤷

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Would recommend rebasing the MR. Seems to be 300+ commits behind.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Trigger an error when using \Drupal\Core\Cache\RefinableCacheableDependencyTrait::addCacheableDependency with a non CacheableDependencyInterface object » Trigger a deprecation when using \Drupal\Core\Cache\RefinableCacheableDependencyTrait::addCacheableDependency with a non CacheableDependencyInterface object
leksat’s picture

leksat commented
Status: Needs work » Needs review

Rebased. Tests are green now ✅

smustgrave’s picture

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

Feedback does appear to be addressed and deprecation seems correct.

LGTM

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

One question on the MR.

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

prudloff’s picture

prudloff commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left some comments on the MR.

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

acbramley opened merge request !11466

acbramley closed merge request !7362

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

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

Addressed feedback and moved this to a new MR since the work was on the 11.x branch in the fork which is problematic for local development.

smustgrave’s picture

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

Feedback appears to be addressed.

  • catch committed 3137020e on 11.x 
    Issue #3232018 by leksat, prudloff, acbramley, smustgrave, larowlan:...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

catch closed merge request !11466

quietone’s picture

quietone commented

Asked in slack about how to remember to add the type hint in 12.0.0 when the trigger_error is deleted. @larowlan suggested a new issue. The issue is #3514554: Add type hint to RefinableCacheableDependencyTrait::addCacheableDependency.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Very nice to see we’ve come so far that we can deprecate that! 🥳

Status: Fixed » Closed (fixed)

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