Trigger an error when using \Drupal\Core\Cache\RefinableCacheableDependencyTrait::addCacheableDependency with a non CacheableDependencyInterface object

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

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

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 😅

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

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

Left a comment on the MR

@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.

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.

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

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

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 🤷

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

Rebased. Tests are green now ✅

Feedback does appear to be addressed and deprecation seems correct.

LGTM

One question on the MR.

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 → closed merge request !7362

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

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.

Feedback appears to be addressed.

Committed/pushed to 11.x, thanks!

catch → closed merge request !11466

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 📌 Add type hint to RefinableCacheableDependencyTrait::addCacheableDependency Postponed .

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

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