Problem/Motivation

At the present moment, an error response is normalized in the exception subscriber Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber. However we want to allow other modules to choose how they serialize the exceptions to deliver errors to the consumers.

Proposed resolution

  1. Introduce HttpException normalizers for the core formats.
  2. Refactor the event subscriber to serialize the exception object directly. That way other formats can just provide the exception normalizers.
  3. Make sure the exception trace serializes correctly to put in the caches (PHP's serialize function). This is an issue if the exception object is added to the response data in ResourceResponse.

Remaining tasks

N/A

User interface changes

None.

API changes

None.

Data model changes

None.

Comments

e0ipso created an issue. See original summary.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented

This is a similar effort that we had to undertake in jsonapi. #2810293: [FEATURE] Use exception event for exception response.

Grayside’s picture

Grayside CreditAttribution: Grayside at Phase2 for Norwegian Cruise Line commented
Related issues: +#1916302: RFC 7807: "Problem Details for HTTP APIs" — serve REST error responses as application/problem+json, +#2811483: Adopt Problem Details RFC for REST Error Responses
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2813853: RequestHandler has its own error handling rather than leaving it to KernelEvents::EXCEPTION event subscribers, +#2805281: ?_format=hal_json error responses are application/json, yet should be application/hal+json
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Component: rest.module » serialization.module
Status: Active » Postponed (maintainer needs more info)
Issue tags: -REST +Needs issue summary update

It's not clear to me what this is trying to achieve. When/why is this necessary?

But it definitely belongs in the serialization.module component (just like Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber already is part of that), because the intent is for this to work for any implementation of web services, either "classical REST" (rest.module) or for example JSON API or GraphQL.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented
Status: Postponed (maintainer needs more info) » Active

It's not clear to me what this is trying to achieve. When/why is this necessary?

Core is doing inline exception normalization, which is only alterable by replacing the event subscriber. Instead, it should use the serialization system to deliver the information about the exception in the format it was negotiated.

See how http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/EventSubscri... normalizes the exception as:

$data = ['message' => sprintf('A fatal error occurred: %s', $message)]

For JSON API (and most probably other formats) we want the ability to control the contents of the response to conform to a specification. We want something close to:

$data = $this->serializer->normalize($exception, $format, …);

More examples of these inline normalizations are:

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2825347: 'Accept' header still is used for determining response format of error responses, ?_format= has no effect AND serialization module's exception subscriber does not support status code 500 error responses, +#2739617: Make it easier to write on4xx() exception subscribers

See how http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/EventSubscri... normalizes the exception as:

$data = ['message' => sprintf('A fatal error occurred: %s', $message)]

FYI: that specific code you linked to is being deleted in #2825347: 'Accept' header still is used for determining response format of error responses, ?_format= has no effect AND serialization module's exception subscriber does not support status code 500 error responses.

But that doesn't yet address your generic request: let a Normalizer generate the error response body for HttpExceptions, instead of letting KernelEvents::EXCEPTION subscribers do that. Which is usually going to be \Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber::on4xx() (since you wrote #6 and those links, we landed #2739617: Make it easier to write on4xx() exception subscribers, which made that a whole lot more consistent.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Create HttpException normalizers for non-HTML error handling » Generate error responses for HttpException by using a Normalizer instead of a KernelEvents::EXCEPTION event subscriber

d.o ate my issue title update :/

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2838949: HttpException Handling Does Not Pass Headers to Responses — results e.g. in missing Allow header for 405 responses

However, I do see one flaw in this proposal: #2838949: HttpException Handling Does Not Pass Headers to Responses — results e.g. in missing Allow header for 405 responses added the fix (and test coverage) to ensure that headers associated with an HttpException object actually do make it onto the error response too.

If the exception is normalized into a response body, then how are we going to get that header to make it onto the error response?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Active » Closed (works as designed)

And more importantly, I just discovered at #2829977-29: SerializableHttpException and Drupal\jsonapi\Error\* are not necessary that the only module and format that would benefit from this for now (JSON API) won't actually be able to benefit from this at all:

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.

I do think the idea proposed in this issue is sound, but for now, nothing would be able to benefit from this. Therefore, marking this works as designed for now. When the API-first module landscape in D8 is more mature, we should reconsider this!