Problem/Motivation

This was extracted from #2765959: Make 4xx REST responses cacheable by (Dynamic) Page Cache + comprehensive cacheability test coverage (this is a critical subset of that issue, which is blocking #2765959).

In \Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber::on4xx(), we are returning a \Symfony\Component\HttpFoundation\Response object. Which does not implement \Drupal\Core\Cache\CacheableResponseInterface, and hence prevents cacheability metadata (tags, contexts, max-age) from being added to the response.

Consequently, page_cache and dynamic_page_cache will not cache these 4xx responses.

This is for 4xx responses though, so why do we care?

Turns out there are use cases where we want cached 4xx responses — for example: some 4xx responses are requested many times (and may also be very expensive).

Proposed resolution

When we are faced with a client error, we should return a \Drupal\Core\Cache\CacheableResponse. Then \Drupal\Core\EventSubscriber\ClientErrorResponseSubscriber will automatically add the 4xx-response cache tag, just like for HTML responses.

But in order for error responses to be CacheableResponse and not just Response, we of course need to know how to cache an error response. Symfony's \Symfony\Component\HttpKernel\Exception\HttpException(Interface), which we use to do throw new \Symfony\Component\HttpKernel\Exception\NotFoundHttpException($message), of course does not carry/provide cacheability information at all.
This issue proposes to solve that by introducing \Drupal\Core\Http\Exception\CacheableAccessDeniedHttpException and friends, which provide "cacheability-capable" subclasses of all Symfony exception classes, like this:

class CacheableAccessDeniedHttpException extends AccessDeniedHttpException implements CacheableDependencyInterface {
  use CacheableDependencyTrait;
}

In other words: provide "richer" versions of the existing exceptions. Code that actually throws those richer exceptions can then actually have cacheable error responses. All other error responses remain not cacheable. Therefore there is no BC break; this is purely opt-in.

Remaining tasks

None.

User interface changes

None.

API changes

API addition: CacheableHttpExceptionInterface. Exceptions of this type are cacheable, and will therefore generate cacheable 4xx responses that can be cached by Page Cache & Dynamic Page Cache.

Data model changes

None.

CommentFileSizeAuthor
#25 2920001-25.patch35.93 KBWim Leers
#25 interdiff.txt5.85 KBWim Leers
#23 2920001-23.patch32.94 KBeffulgentsia
#19 2920001-19.patch32.72 KBWim Leers
#19 interdiff.txt979 bytesWim Leers
#16 2920001-16.patch32.74 KBWim Leers
#16 interdiff.txt2.98 KBWim Leers
#7 2920001-7.patch31.8 KBWim Leers
#7 interdiff.txt1.94 KBWim Leers
#6 2920001-6.patch29.93 KBWim Leers
#6 interdiff.txt1.97 KBWim Leers
#5 2920001-5.patch28.02 KBWim Leers
#5 interdiff.txt2.82 KBWim Leers
#4 2920001-4.patch25.24 KBWim Leers
#4 interdiff.txt3.25 KBWim Leers
#3 2920001-3.patch22.05 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue summary: View changes

First, let's import the relevant parts of the IS from the parent issue.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Active » Needs review
FileSize
2920001-3.patch22.05 KB

First: the foundations!

  1. CacheableAccessDeniedHttpException + CacheableBadRequestHttpException.php and so on
  2. CacheableDependencyTrait extracted from RefinableCacheableDependencyTrait, which the above can use
  3. updated DefaultExceptionHtmlSubscriber and ExceptionJsonSubscriber to respect cacheability metadata and bubble it up to the responses they generate

Imported straight from the parent issue's patch.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
interdiff.txt3.25 KB
2920001-4.patch25.24 KB

Next: the first use case!

This updates \Drupal\basic_auth\Authentication\Provider\BasicAuth::challengeException() to throw a cacheable HTtP exception rather than an uncacheable one.

Imported straight from the parent issue's patch.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
interdiff.txt2.82 KB
2920001-5.patch28.02 KB

Next: explicit test coverage for the first use case.

This provides explicit functional test coverage for the changes in #4, by adding \Drupal\Tests\basic_auth\Functional\BasicAuthTest::testCacheabilityOf401Response().

This was not imported from the parent issue's patch.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
interdiff.txt1.97 KB
2920001-6.patch29.93 KB

Next: fixing the failing tests.

We're getting a 401 where a 403 is expected now. Turns out that the "REST test" authentication provider simply was buggy: it did not yet support Page Cache correctly. This simply went unnoticed until now because the REST test coverage doesn't test Page Cache support yet for every request it makes, and it should. (But doing that is in the scope of the parent issue, not this issue.)

Imported straight from the parent issue's patch.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: Wim Leers » Unassigned
FileSize
interdiff.txt1.94 KB
2920001-7.patch31.8 KB

Next: fixing the remaining failing tests.

#6 fixed some of the failing tests: we're able to get further along in those test cases, but we're still failing. This is because Dynamic Page Cache is now caching more than only a 200 response when testing with Basic Auth: it's also caching a 4xx response. We simply need to update the expectations.

This should be green again.

Imported straight from the parent issue's patch.

The last submitted patch, 4: 2920001-4.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 5: 2920001-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 6: 2920001-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Yay, green, as predicted :)

Note we should credit @davidwbarratt and @dawehner — they've been huge influences in this patch, in the parent issue.

Also, the parent issue is now officially postponed on this issue: see #2765959-230: Make 4xx REST responses cacheable by (Dynamic) Page Cache + comprehensive cacheability test coverage.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and at Dazzle commented
Status: Needs review » Reviewed & tested by the community

I was ready to RTBC the other issue, and this one is smaller - all the nitpicks I had were resolved in the other issue.

aheimlich’s picture

aheimlich
he/him
Primary language English
Location Chicago area
CreditAttribution: aheimlich as a volunteer commented 
+++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
@@ -170,6 +172,11 @@ protected function makeSubrequest(GetResponseForExceptionEvent $event, $url, $st
+      // Persist the exception's cacheability metadata, if any.
+      if ($response instanceof CacheableResponseInterface && $exception instanceof CacheableHttpExceptionInterface) {
+        $response->addCacheableDependency($exception->getCacheableMetadata());
+      }
+

I've seen Drupal\Core\Cache\Exception\CacheableHttpExceptionInterface mentioned several times, yet I don't see any code for it. None of the exceptions added in this patch are implementing it, either. Am I missing something?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

It would be nice to have an exception extending HttpException itself.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: Unassigned » Wim Leers
Status: Reviewed & tested by the community » Needs work

#12: ❤️

#13: that's a good catch: that's indeed no longer relevant, it's CacheableDependencyInterface that matters now!

#14: Sure, I can add that too.

#13 + #14 are excellent nits, thanks!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.98 KB
2920001-16.patch32.74 KB

Done!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Thank you @Wim Leers!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 2920001-16.patch, failed testing. View results

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: Wim Leers » Unassigned
Status: Needs work » Reviewed & tested by the community
FileSize
interdiff.txt979 bytes
2920001-19.patch32.72 KB
effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented

Ticking credit boxes for all the reviewers. Thank you.

effulgentsia credited davidwbarratt.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented

Also crediting @davidwbarratt per #11.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
FileSize
2920001-23.patch32.94 KB

Just a reroll due to changes committed in #2874938: AdminRouteSubscriber must only mark HTML routes as administrative.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
Status: Reviewed & tested by the community » Needs work

This patch looks great to me. Some relatively minor nits/questions:

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
@@ -170,6 +172,11 @@ protected function makeSubrequest(GetResponseForExceptionEvent $event, $url, $st
+      // Persist the exception's cacheability metadata, if any.
+      if ($response instanceof CacheableResponseInterface && $exception instanceof CacheableDependencyInterface) {
+        $response->addCacheableDependency($exception);
+      }

    What about if $exception is not a CacheableDependencyInterface. Do we need code somewhere to make $response not cacheable in such a situation? 

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
@@ -35,7 +37,16 @@ protected static function getPriority() {
-    $response = new JsonResponse(['message' => $event->getException()->getMessage()], $exception->getStatusCode(), $exception->getHeaders());
+
+    // If the exception is cacheable, generate a cacheable response.
+    if ($exception instanceof CacheableDependencyInterface) {
+      $response = new CacheableJsonResponse(['message' => $event->getException()->getMessage()], $exception->getStatusCode(), $exception->getHeaders());
+      $response->addCacheableDependency($exception);
+    }
+    else {
+      $response = new JsonResponse(['message' => $event->getException()->getMessage()], $exception->getStatusCode(), $exception->getHeaders());
+    }
+

    This looks great. Shouldn't we also add a test for it? 

  3. +++ b/core/modules/rest/tests/modules/rest_test/src/PageCache/RequestPolicy/DenyTestAuthRequests.php
@@ -0,0 +1,26 @@
+ * Cache policy for pages requested with REST Test Auth.
+ *
+ * @see \Drupal\rest_test\Authentication\Provider\TestAuth
+ * @see \Drupal\rest_test\Authentication\Provider\TestAuthGlobal
+ * @see \Drupal\basic_auth\PageCache\DisallowBasicAuthRequests

    Our other cache policies (e.g., DisallowBasicAuthRequests) include the reasoning for them in the PHPDoc. Even though this is just a test class, can we still add the reason for having this policy in its docs?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community
FileSize
interdiff.txt5.85 KB
2920001-25.patch35.93 KB

This patch looks great to me.

🎉

  1. Good question. I think that's kind of out of scope.
    Note that unlike in ExceptionJsonSubscriber, we're not generating a response! We're enriching a response that we receive from a subrequest: we receive the response for one of the \Drupal\system\Controller\Http4xxController() routes.
    However, I think you're right: when we deal with a cacheable response for an exception that itself is not cacheable, we should also set it to become uncacheable, by setting max-age = 0. Fortunately, doing that is absolutely trivial thanks to the existing cacheability-related infrastructure. So I guess it does make sense to do here after all :)
  2. 👍 Done. (In doing so, I found a small oversight in CacheableMethodNotAllowedException, which we could easily have fixed later,
    but still, good to have it fixed now!)
  3. 👍 Done.
effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented

Thanks!

Unticking my own credit box, since #23 was just a straight reroll.

  • effulgentsia committed b83782a on 8.5.x 
    Issue #2920001 by Wim Leers, dawehner, borisson_, aheimlich,...
effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
Status: Reviewed & tested by the community » Fixed

Pushed to 8.5.x.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

🎉🎉🍾

#2765959 is now fully unblocked: #2765959-232: Make 4xx REST responses cacheable by (Dynamic) Page Cache + comprehensive cacheability test coverage!

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2929428: [>=8.5] Convert "throw new *HttpException" into "throw new Cacheable*HttpException" where possible

#2765959: Make 4xx REST responses cacheable by (Dynamic) Page Cache + comprehensive cacheability test coverage took advantage of the APIs introduced here to bring that to core's REST module. #2929428: [>=8.5] Convert "throw new *HttpException" into "throw new Cacheable*HttpException" where possible aims to do the same for the contrib JSON API module.