Let Dynamic Page Cache warn developers when a controller's render array does not have #cache set

Let's see how many fails we have

#2 will come back at least mostly green. It's in the wrong place.

I think it's a great idea to nudge/force people to think about cacheability, always.

OTOH, this approach means that even on _admin_route: TRUE routes, no_cache: TRUE routes and POST requests, where Dynamic Page Cache doesn't do anything, you'll still be forced to add cacheability metadata. Personally, I think that's great, but that means we'll be forcing people to think about cacheability metadata even in cases where it doesn't make sense to do caching.

+++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
@@ -223,6 +223,9 @@ protected function prepare(array $main_content, Request $request, RouteMatchInte
+          if (!isset($main_content['#cache'])) {
+            throw new \LogicException('Must have cacheability metadata.');
+          }

Do we allow to return an empty array?

I assume yes, just asking.

#6:

Can we check the $request->data('smart_cache_cacheable') attribute, e.g. the request policies?

This moves the exception to Dynamic Page Cache, so it is only thrown when Dynamic Page Cache actually may cache the response. This means all admin routes and no_cache routes will work as expected again.

(The do-not-test patch is the actual patch, the other one includes #2579847: /node/add is lacking cacheability metadata, causes problems when cached by Dynamic Page Cache and "Use admin theme when editing or creating content" is turned off, just like #5.)

Looks good. Lets have the exception also link to the relevant handbook or API page.

Overall I am +1 to making this explicit; however some caveats:

We decided in the smart cache / dynamic_page_cache policy issue that we wanted to make this enabled by default for everything for now and just fix critical core fails with a simple no_cache: TRUE.

It is quite astonishing that we only found one real critical problem so far - which is also a real edge case as that should have been marked no_cache: TRUE already since a while if there were not this strange dynamic adding of admin_route.

So lets not get into a panic situation, where all the world is blowing up. We knew there would be some criticals following from smart cache / dynamic_page_cache and we knew we could simply mark the routes as uncacheable and be done.

I think throwing an Exception is a good developer tool, however it also means a HUGE BC break as a developer now will have to decide:

- opt-in to smart-cache
- opt-out of smart-cache

explicitly.

It means that whoever is returning a render array will either opt-in to dynamic_page_cache or opt-out of dynamic_page_cache or needs to disable dynamic_page_cache locally.

So the previous version of not doing anything is no longer allowed and maybe that is a good thing.

Berdir told me anecdotically that they have not yet enabled smart_cache for the existing production sites, which limits the BC break for existing sites to contrib authors.

I like the idea of checking for #cache, it is what I would have originally wanted to see happen - but did not know how.

To the patch itself:

#11: The event view subscriber should mainly set a flag on the request that the render array was not having '#cache', then the check should be done in smart cache itself after the real response policy check.

The fake response policy check will not work and again lead to false positives.

Per #2556889: [policy, no patch] Decide if SmartCache is still in scope for 8.0 and whether remaining risks require additional mitigation:

The "delay release" risk can be mitigated by adding the "no_cache: TRUE" option to any route whose controller's cacheability metadata is found to be incorrect.

This issue is then about

1. warning the developer that a certain controller is missing cacheability metadata — this hugely helps with pushing contrib in the right direction
2. adding no_cache: TRUE to all routes in core that aren't yet providing correct cacheability metadata; core can gradually add the necessary cacheability metadata to those routes over time, so that by e.g. Drupal 8.1.0 or 8.2.0, all of the core routes that can be cached, also are, and provide the necessary cacheability metadata.

Working on doing point 2. Some of the test controllers that are super trivial to fix, I will just fix.

Heh, I cross-posted with Fabianx. Similar thoughts.

The fake response policy check will not work and again lead to false positives.

Without that, we will still end up complaining on _admin_route: TRUE and no_cache: TRUE routes, because those are in response policies. Yes, we should actually have "route policies" too, besides just request & response policies.

Working on doing point 2. Some of the test controllers that are super trivial to fix, I will just fix.

Here's a first (small) round. It's getting late here, and it's been a long day. I will continue in the morning.

#18: Yes, but what I mean is change the code to do:

+++ b/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php
@@ -149,6 +152,44 @@ public function onRouteMatch(GetResponseEvent $event) {
+  public function onViewRenderArray(GetResponseForControllerResultEvent $event) {
+    $request = $event->getRequest();
+    $result = $event->getControllerResult();
...
+    // Check if MainContentViewSubscriber would render this render array, i.e.
+    // if this render array will in fact be converted into a response.
+    if (is_array($result) && ($request->query->has(MainContentViewSubscriber::WRAPPER_FORMAT) || $request->getRequestFormat() == 'html')) {
+      // If #cache is not set, complain.

Just here do:

   // If #cache is not set, complain later.
  $request->attributes->set(self::ATTRIBUTE_CONTROLLER_RESPONSE_HAS_CACHE, isset($result['#cache']);

Then when actually having passed the Response and Request policies in DPC onResponse listener do:

  if ($request->attributes->has(self::ATTRIBUTE_CONTROLLER_RESPONSE_HAS_CACHE) && $request->attributes->get(self::ATTRIBUTE_CONTROLLER_RESPONSE_HAS_CACHE) === FALSE) {
    throw new Exception('...');
}

Just when we are about to cache the response we can decide properly on the data that the controller provided no cacheability metadata.

No exception please. trigger_error() should do.

I am fine with assert() or trigger_error() also makes the BC break less.

@catch Which should it be: trigger_error() or assert()? I'll leave the choice to you.

This updates the 4XX controller's render arrays. This should also significantly reduce the number of failures.

Implemented #20, which is indeed much better.

Ah this is much better

Just some nits:

+++ b/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php
@@ -241,6 +232,13 @@ public function onResponse(FilterResponseEvent $event) {
+    if ($request->attributes->get(self::ATTRIBUTE_CONTROLLER_RESULT_RENDER_ARRAY_HAS_CACHEABILITY) === FALSE) {

I think adding the ->has() check is better / cleaner as a controller can just return a CacheableResponse and then this is NULL.

We also use ->has() for some other attributes - in the rest of the code.

Let's provide more useful information to developers. And let's add test coverage.

For now, I started using trigger_error(). Easy enough to change.

#2579847: /node/add is lacking cacheability metadata, causes problems when cached by Dynamic Page Cache and "Use admin theme when editing or creating content" is turned off was just committed, that's why #32 failed.

Uploading #32's do-not-test patch.

+++ b/core/modules/dynamic_page_cache/src/Tests/DynamicPageCacheIntegrationTest.php
@@ -127,6 +127,12 @@ public function testDynamicPageCache() {
+    $this->assertResponse(200);
+    $this->assertRaw("The controller for the route 'dynamic_page_cache_test.html_without_cacheability' returned a render array without cacheability metadata. See https://www.drupal.org/developing/api/8/render/arrays/cacheability.");
   }

Are you sure assertRaw is the right way to check that? Don't we have a error handler which detects those errors on test time?

More accurate title.

#37: I'm certain this is not 100% correct :) That test still fails because that error is triggered; I don't know how to test properly. I know I've fought this in the past too, and there are at most a handful of WebTestBase tests dealing with this. Pointers (or reroll) welcome.

Does this really meet the definition of critical (release blocking)?

Let's try to be objective. This issue certainly hardens security by making it harder for people to specify non cacheablity information at all.
On the other hand this issue also doesn't solve any actual existing critical security issues, given which places had to be covered. I kinda assume that giving people
more information about filter formats isn't a security issue given that we check access to them on forms.

If we can get this in as major, sure fine with me ...

But it is a little big behavior change.

This will fix another whole bunch.

It should be E_DEPRECATED then it can go in a minor I think.

We don't know that routes without #cache have actual cacheability problems, they might be fine. Also ones that have it might not be.

So it is not an error to not have it, we just prefer you to have it or set no_cache. Deprecation seems about right.

We don't know that routes without #cache have actual cacheability problems, they might be fine. Also ones that have it might not be.

Exactly! This is what I'd been pondering yesterday night too: it's not a perfect signal, and so we shouldn't actually hard force this. It should only be a nudge towards developers.

Do you think this can go in a future RC release? AFAICT it could.

If we think it is actual security hardening, then it could probably go in during RC - the patch itself looks low risk.

Whether it's actually helping anything to have empty #cache in controllers to suppress whatever error we add (like the patch adds) is a different question though.

This is hopefully everything.

Added an issue for the same idea, but for blocks: #2584951: Let Block plugins require to set #cache on their render arrays

Some less

More than a year after this was filed, the fear has fortunately not become reality. I think it's safe to close this.

Not sure what the best status is.