[>=8.5] Convert "throw new *HttpException" into "throw new Cacheable*HttpException" where possible

Attached is a patch that doesn't work (it's not yet passing the necessary cacheability metadata), but these are examples of places where I think JSON API would benefit. It's not everything for sure.

Wait to do this until JSON API requires Drupal 8.5

That makes this a v2-only issue.

👍 Great, thank you! :)

#2986987: Convert EntityAccessDeniedHttpException into cacheable exception was committed last week 🎉

Just got back from vacation — you posted that the day after I left for vacation. Queued a test! Thanks so much for working on this!

+++ b/src/EventSubscriber/DefaultExceptionSubscriber.php
@@ -36,8 +36,8 @@ class DefaultExceptionSubscriber extends SerializationDefaultExceptionSubscriber
-    if (($exception = $event->getException()) && !$exception instanceof HttpException) {
-      $exception = new HttpException(500, $exception->getMessage(), $exception);
+    if (($exception = $event->getException()) && !$exception instanceof CacheableHttpException) {
+      $exception = new CacheableHttpException(500, $exception->getMessage(), $exception);

This is probably the only place that you cannot convert to be a cacheable HTTP exception!

Reverting this change.

+++ b/src/Normalizer/HttpExceptionNormalizer.php
@@ -25,7 +25,7 @@ class HttpExceptionNormalizer extends NormalizerBase {
    */
-  protected $supportedInterfaceOrClass = HttpException::class;
+  protected $supportedInterfaceOrClass = CacheableHttpException::class;
 
   /**
    * The current user making the request.
@@ -67,13 +67,13 @@ class HttpExceptionNormalizer extends NormalizerBase {

@@ -67,13 +67,13 @@ class HttpExceptionNormalizer extends NormalizerBase {
   /**
    * Builds the normalized JSON API error objects for the response.
    *
-   * @param \Symfony\Component\HttpKernel\Exception\HttpException $exception
+   * @param \Drupal\Core\Http\Exception\CacheableHttpException $exception
    *   The Exception.
    *
    * @return array
    *   The error objects to include in the response.
    */
-  protected function buildErrorObjects(HttpException $exception) {
+  protected function buildErrorObjects(CacheableHttpException $exception) {

Actually, this is another place. Reverting these changes too (plus those in its subclasses).

Here is an example of a concrete change. In this particular instance, we're throwing an exception due to URL query args that are invalid. So this exception depends on the url.query_args cache context. (See https://www.drupal.org/docs/8/api/cache-api/cache-contexts.)

This makes WorkflowTest pass! So we should see a lower number of test failures now :)

Back to you now!

From >100 to 35! 🎉

I'll do just one more. TermTest::testRelationships() is failing, and the fatal error points to RequestHandler.php on line 137. On that line, cacheability is missing, just like in #16. But in this particular case, a cacheable exception does not even make sense, because it only ever occurs for POST and PATCH requests, and responses for those HTTP methods are never cacheable anyway, because the HTTP spec forbids anybody from caching responses to such requests!
So in this case, we simply can make the exception not cacheable.

Remember, the issue summary says: Inspect the JSON API codebase and convert where this makes sense. — this is a case where it doesn't make sense.

After fixing that, a similar problem comes up in \Drupal\jsonapi\Normalizer\RelationshipNormalizer::massageRelationshipInput(), that code is also only called during denormalization and hence during unsafe requests.

Now, truly, back to you!

Taking this on.

First step: rebase, because #19 no longer applied.

Recent changes (a.o. #2949807: Spec Compliance: Error responses are missing the `jsonapi` top-level member) have made error responses cacheable (yay!), and the changes here result in more accurate cacheability (more yay!).

This demands some assertion updates. This interdiff for example makes ::getIndividual() pass tests.

This removes most if not all "cacheable" exception conversions from unsafe HTTP method controllers in EntityResource. This should fix many failures!

Green? :)

Oops.

Apparently there's quite a bit more here.

Fix CS violations.

1. +++ b/src/Context/FieldResolver.php
   @@ -238,7 +238,7 @@ class FieldResolver {
   -    $cacheability = (new CacheableMetadata())->addCacheContexts(['url.query_args']);
   +    $cacheability = (new CacheableMetadata())->addCacheContexts(['url.query_args:filter', 'url.query_args:sort']);
   
   +++ b/tests/src/Functional/UserTest.php
   @@ -462,7 +462,7 @@ class UserTest extends ResourceTestBase {
   +    $this->assertResourceErrorResponse(400, "Filtering on config entities is not supported by Drupal's entity API. You tried to filter on a Role config entity.", $collection_url, $response, FALSE, ['4xx-response', 'http_response'], ['url.path', 'url.query_args:filter'], FALSE, 'MISS');
   

   Nice!

2. +++ b/src/Context/FieldResolver.php
   @@ -525,7 +525,8 @@ class FieldResolver {
   -      throw new CacheableBadRequestHttpException($message);
   +      $cacheability = (new CacheableMetadata())->addCacheContexts(['url.query_args:filter', 'url.query_args:sort']);
   +      throw new CacheableBadRequestHttpException($cacheability, $message);
   
   +++ b/src/Controller/EntityResource.php
   @@ -354,7 +354,8 @@ class EntityResource {
   -        throw new CacheableBadRequestHttpException(new CacheableMetadata(), sprintf("Filtering on config entities is not supported by Drupal's entity API. You tried to filter on a %s config entity.", $config_entity_type_id));
   +        $cacheability = (new CacheableMetadata())->addCacheContexts(['url.path', 'url.query_args:filter']);
   +        throw new CacheableBadRequestHttpException($cacheability, sprintf("Filtering on config entities is not supported by Drupal's entity API. You tried to filter on a %s config entity.", $config_entity_type_id));
   

   Good catch — I missed these!

I went ahead and made all those changes. RTBC from my perspective.

RTBC from mine too!

@axle_foley00 Don't apologize! :) You got this started, that's what matters most!