SerializableHttpException and Drupal\jsonapi\Error\* are not necessary

+++ b/src/ResourceResponse.php
@@ -52,4 +52,10 @@ class ResourceResponse extends Response implements CacheableResponseInterface, R
+  // @todo needs proper solution
+  public function __sleep() {
+    $this->responseData = NULL;
+    return array_keys(get_object_vars($this));
+  }

This is not the proper solution.

The proper solution basically requires duplicating what [#2807501 did. But that's very wasteful.

OTOH, reusing the exact same solution means reusing ResourceResponseInterface and the rest.resource_response.subscriber service.

What this issue is pointing out, is that that service and interface (plus default implementation) are useful outside of the scope of the REST module. It seems we should move all of that to core/lib/Drupal/Core/Serialization? GraphQL should be able to benefit from this too.

Thoughts?

HEAD changed 9 hours ago, when #2821312-20: [BUGFIX] Catch non HTTP exceptions landed. That's the third commit to that issue. The patch in #2 removes all the complexity.

Rebased, so no interdiff.

And now with updated unit test expectations.

Just one failing test now — I'll leave that to e0ipso.

#11: that sounds like #2805281: ?_format=hal_json error responses are application/json, yet should be application/hal+json.

AFAICT, all code in Drupal\jsonapi\Error is related. That should also not be necessary. (If I'm mistaken and it's not related, please create a separate issue for that.)

#2805281: ?_format=hal_json error responses are application/json, yet should be application/hal+json landed!

I've been working on this today. Big update on the way.

First line in the IS:

The only reason why \Drupal\jsonapi\EventSubscriber\DefaultExceptionSubscriber exists, is because \Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber does not know to handle \Drupal\jsonapi\Error\SerializableHttpException

Once #2841027: JSON API format should only be used on JSON API routes, but can currently also be used for REST resources is fixed, this statement will be incorrect. Because the Serialization and REST modules will no longer be able to handle the JSON API format, because that format is not meant to be used generically: it's only meant to be used on the JSON API routes!

Also, we'll need a custom exception subscriber anyway, for the changes made in #2821045: [BUGFIX] Conform to the error spec.

Therefore this is also no longer true:

If we can remove the need for SerializableHttpException, we can also remove that additional exception event subscriber, and just rely on the serialization module's handling of it.

IS almost completely rewritten.

First, let's rebase this patch.

(This patch is rolled on top of #2841027: JSON API format should only be used on JSON API routes, but can currently also be used for REST resources.)

#20:

So probably we can just get rid of the SerializableHttpException and keep the DefaultExeptionSubscriber.

This indeed allows me to create a patch that passes all tests.

Patch attached that's hopefully green :)

See interdiff: this does exactly what @dagmar wrote, plus some additional changes to code that was added after @dagmar worked on it.

But does this patch also make sense?

The current patch does:

1. removes SerializableHttpException
2. adds ResourceResponse::__sleep() (to prevent that we attempt to cache and hence serialize the entities stored in a JSON API response object)

Point 1 is fine.

But point 2 is a problem. We had the same problem in the REST module, and we fixed that in #2807501: ResourceResponse::$responseData is serialized, but is never used: unserializing it on every Page Cache hit is a huge performance penalty. We want the exact same solution here. Then there is no need for a __sleep() method. Yes, that means we're duplicating code from the REST module. But better a duplication than an incorrect abstraction. If it turns out that it's the right abstraction, we can move the code for this over to the Serialization module. So that still leaves open the door for future simplification. For now, what matters is that we follow the exact same pattern.

In other words: we should revert the changes made by #2818221: [BUGFIX] Restore serialization with HttpException inline responses and instead introducing the same solution that is used in core's rest module: #2807501: ResourceResponse::$responseData is serialized, but is never used: unserializing it on every Page Cache hit is a huge performance penalty. Opened #2855693: Remove \Drupal\jsonapi\Controller\RequestHandler::renderJsonApiResponse(), add \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber for that, because tacking that onto this issue will make this issue/patch very very difficult to review.

A bit of clean-up.

That still leaves

Furthermore, everything in the Drupal\jsonapi\Error namespace would no longer be necessary.

in the IS.

Well, simplifying that seems out of scope for this issue. That class was introduced in #2738269-15: [FEATURE] Implement expected error format for JSON API. But it turns out that the simplifications introduced in this patch allow me to simply remove that altogether!

This also allows #2855693: Remove \Drupal\jsonapi\Controller\RequestHandler::renderJsonApiResponse(), add \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber to be simpler!

Updating issue title. Adding maintainability tag. I think this is so important for maintainability that this deserves to be major. This issue brings us to a place where JSON API does NOT have any special error/exception handling anymore. It does have normalizers for exceptions, but that's it.

The IS and patch now also clearly state that ResourceResponse::__sleep() is only a temporary work-around, and it will be removed in #2855693: Remove \Drupal\jsonapi\Controller\RequestHandler::renderJsonApiResponse(), add \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber.

It [JSON API] does have normalizers for exceptions, but that's it.

This made me think about ensuring that this normalizer is only applied to the JSON API format. Turns out it's not! It's doing it for any format!

So, effectively, the current \Drupal\jsonapi\Normalizer\HttpExceptionNormalizer is exactly the code that one would propose for #2818525: Generate error responses for HttpException by using a Normalizer instead of a KernelEvents::EXCEPTION event subscriber.

However… that's not without problems too, because:

  public function normalize($object, $format = NULL, array $context = []) {
    $errors = $this->buildErrorObjects($object);
    …
  }

and

  protected function buildErrorObjects(HttpException $exception) {
    $error = [];
    $status_code = $exception->getStatusCode();
    if (!empty(Response::$statusTexts[$status_code])) {
      $error['title'] = Response::$statusTexts[$status_code];
    }
    $error += [
      'status' => $status_code,
      'detail' => $exception->getMessage(),
      'links' => [
        'info' => $this->getInfoUrl($status_code),
      ],
      'code' => $exception->getCode(),
    ];
    if ($this->currentUser->hasPermission('access site reports')) {
      // The following information may contain sensitive information. Only show
      // it to authorized users.
      $error['source'] = [
        'file' => $exception->getFile(),
        'line' => $exception->getLine(),
      ];
      $error['meta'] = [
        'exception' => (string) $exception,
        'trace' => $exception->getTrace(),
      ];
    }

    return [ $error ];
  }

All those keys are JSON API-specific: they comply with http://jsonapi.org/format/#error-objects.

So, really, this normalizer is highly specific to the JSON API format. And that also means that doing #2818525: Generate error responses for HttpException by using a Normalizer instead of a KernelEvents::EXCEPTION event subscriber is fairly pointless, because JSON API is the only module that would benefit from that, but it'd still have to A) provide a custom HttpExceptionNormalizer, B) still have to provide a custom exception subscriber, because it needs to pass ['data_wrapper' => 'errors'] as the serialization context when calling the serializer.

So, made \Drupal\jsonapi\Normalizer\HttpExceptionNormalizer only support the 'api_json' format.

#2841027: JSON API format should only be used on JSON API routes, but can currently also be used for REST resources landed, this is unblocked now!

#30:

We are losing the ability to serialize PHP errors. Before, a syntax error would be turned into a JSON API response. Without the error handler we cannot really do that. I'm not sure how useful is that feature, I'm open for discussion.

1. But surely that was not the only reason this was added? If it were, then we shouldn't have replaced all the existing BadRequestHttpException etc instances with SerializableHttpException.
2. If one of the key features was to convert PHP errors, then why is there no test coverage for it?
3. Besides, we still have this:
   

       if (!$exception instanceof HttpException) {
         $exception = new SerializableHttpException(500, $exception->getMessage(), $exception);
         $event->setException($exception);
       }
   

#31:

I'd argue that at least on production this really doesn't matter. You don't want to serialize PHP errors and expose them to the public. For dev sites IMHO not rendering PHP exceptions not within a nice JSONAPI output is totally acceptable. You don't want to handle the output anymore in your Client JS, I assume, but rather just look at the PHP error for example via postman.

I could not agree more. If there's a PHP error, serializing it into an otherwise valid JSON API response is not at all helpful. The client can't fix it, because it's a server-side error. It's a hard failure. Making the client still kinda work is not helpful. In fact, I'd argue it's harmful… because it might result in the client pretending everything's okay, even though the server response cannot be trusted, precisely because it's a PHP error.

YAY!

This really is quite a big simplification. Also in diff stats:

27 files changed, 78 insertions(+), 138 deletions(-)

… but more so in abstraction complexity/concept complexity. JSON API now no longer has these two concepts that make it much more difficult to understand and hence maintain.

One of my few virtues is that I know how to listen for arguments, and change my opinion.

I think you have more virtues ;) But yes, that's definitely an important one in the world of software!

Hehe, is it the linking to https://gist.github.com/e0ipso/efcc4e96ca2aed58e32948e4f70c2460? :)