?_format=hal_json error responses are application/json, yet should be application/hal+json

Oh, wait, I forgot one important bit!

?_format=hal_json

Response:

Content-Type: application/hal+json
No authentication credentials provided.

?_format=json

Response:

Content-Type: application/json
{"message":"A fatal error occurred: No authentication credentials provided."}

The first is wrong because of the reasons I first explained in the issue summary. But the ?_format=json case is also wrong, because it mentions A fatal error occurred:, i.e. somehow this is a PHP fatal error message that ends up in the response, which should definitely not happen.

Since both are wrong, moving to the serialization.module component.

This is a blocker for JSON API, see #2829977-11: SerializableHttpException and Drupal\jsonapi\Error\* are not necessary.

@dawehner had a suspicion in the right direction in #3. The root cause is not the HAL module. It's not even the serialization module. It's the terribleness that is \Drupal\Core\EventSubscriber\HttpExceptionSubscriberBase and the effect it has on classes building on that base class.

1. The case (401) where a ?_format=hal_json request results in a text/plain response, is one that nor \Drupal\Core\EventSubscriber\ExceptionHalJsonSubscriber nor \Drupal\Core\EventSubscriber\ExceptionJsonSubscriber nor \Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber() implement, but that gets handled by \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber
2. The case where a ?_format=json request results in a application/json response is one that is implemented by \Drupal\Core\EventSubscriber\ExceptionJsonSubscriber (which \Drupal\Core\EventSubscriber\ExceptionHalJsonSubscriber extends and hence inherits)

While working on #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method I was under the false impression that it was intentional that HAL+JSON request's error responses always used application/json. But no, that itself is a bug too! It's simply because ExceptionHalJsonSubscriber extends ExceptionJsonSubscriber which returns JsonResponse objects. This was done in #2472323: Move modal / dialog to query parameters without actual consideration, probably because we had close to zero test coverage for REST (and definitely zero test coverage to verify consistent behavior across different formats).

But since #2472323: Move modal / dialog to query parameters, we've introduced \Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber. That class automatically handles all serialization formats. json and hal_json are serialization formats. Therefore we can simply remove ExceptionHalJsonSubscriber.

P.S.: this also made me discover #2833469: Remove 'exception.custom_page_json' service: unused, dead, even with wrong arguments!.

It looks like #2739617: Make it easier to write on4xx() exception subscribers is already solving this! Postponing on that issue.

As of #2739617-83: Make it easier to write on4xx() exception subscribers, it's clear that it won't solve everything that this issue aims to solve, but it will definitely pave the path; allowing this patch to be significantly smaller.

Here's a patch that's relative to #2739617, it will apply once it's committed.

Also note that this issue makes the need for \Drupal\Tests\rest\Functional\ResourceTestBase::$expectedErrorMimeType go away entirely. So the attached patch can be simplified even further. On top of that, ResourceTestBase::assertResourceErrorResponse() can be removed, just using ResourceTestBase::assertResourceResponse() in all cases will be sufficient. In other words: a good amount of simplification is possible thanks to the bugfix in this issue :)

Oops, I forgot two hunks.

This also blocks #2813853: RequestHandler has its own error handling rather than leaving it to KernelEvents::EXCEPTION event subscribers, so tagging blocker.

Turns out this is also blocking #2827084-65: EntityNormalizer::denormalize should not throw UnexpectedValueException, but \Symfony\Component\HttpKernel\Exception\UnprocessableEntityHttpException, so we get a 422 response instead of 400. blocker++

#2739617: Make it easier to write on4xx() exception subscribers landed, which means this is now unblocked!

Reuploading #11 so it can be tested now.

I'll look at this again early next week.

I missed one spot!

#21 will fail because there's still one case where the response will have Content-Type: application/json instead of Content-Type: application/hal+json: where there's a fatal PHP error.

This results in a call to \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber::onException(), which determines the format at \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber::getFormat(), which does this:

    // Make an educated guess that any Accept header type that includes "json"
    // can probably handle a generic JSON response for errors. As above, for
    // any format this doesn't catch or that wants custom handling should
    // register its own exception listener.
    foreach ($request->getAcceptableContentTypes() as $mime) {
      if (strpos($mime, 'html') === FALSE && strpos($mime, 'json') !== FALSE) {
        $format = 'json';
      }
    }

And so it sends application/json even though the request Accept header says application/hal+json . (And yes, that means we still use Accept header based negotiation here.

Fixing that is very much out of scope here: HttpExceptionSubscriberBase and its subclasses are only designed to handle \Symfony\Component\HttpKernel\Exception\HttpExceptionInterface exceptions, not anything else. Fatal PHP errors and how that is handled by the rest of the system is another problem, that deserves its own issue, so created that: #2842465: Behavior of fatal PHP errors/uncaught exceptions results is mostly undefined: \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber::onJson() is the de facto catch-all.

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
@@ -301,9 +303,10 @@ public function testPostDxWithoutCriticalBaseFields() {
-    // @todo Uncomment, remove next line in https://www.drupal.org/node/2820364.
-    $this->assertResourceErrorResponse(500, 'A fatal error occurred: Field  is unknown.', $response);
-    //$this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nfield_name: This value should not be null.\n", $response);
+    // @todo Uncomment, remove next 3 lines in https://www.drupal.org/node/2820364.
+    $this->assertSame(500, $response->getStatusCode());
+    $this->assertSame(['application/json'], $response->getHeader('Content-Type'));
+    $this->assertSame('{"message":"A fatal error occurred: Field  is unknown."}', (string) $response->getBody());

Oops I accidentally deleted the commented line! Restoring it.

I wrote this in #10:

Also note that this issue makes the need for \Drupal\Tests\rest\Functional\ResourceTestBase::$expectedErrorMimeType go away entirely. So the attached patch can be simplified even further. On top of that, ResourceTestBase::assertResourceErrorResponse() can be removed, just using ResourceTestBase::assertResourceResponse() in all cases will be sufficient. In other words: a good amount of simplification is possible thanks to the bugfix in this issue :)

This reroll handles the emphasized part. If committers would prefer to see this happen in another issue, that'd be fine too.

1. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Block/BlockHalJsonAnonTest.php
   @@ -27,9 +27,4 @@ class BlockHalJsonAnonTest extends BlockResourceTestBase {
   -  protected static $expectedErrorMimeType = 'application/hal+json';
   
   +++ b/core/modules/hal/tests/src/Functional/EntityResource/Block/BlockHalJsonBasicAuthTest.php
   @@ -30,11 +30,6 @@ class BlockHalJsonBasicAuthTest extends BlockResourceTestBase {
   -  protected static $expectedErrorMimeType = 'application/hal+json';
   

   This is done everywhere.

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -43,10 +43,9 @@
   - *    that specifies the exact @code $format @endcode, @code $mimeType @endcode,
   - *    @code $expectedErrorMimeType @endcode and @code $auth @endcode for this
   - *    concrete test. Usually that's all that's necessary: most concrete
   - *    subclasses will be very thin.
   + *    that specifies the exact @code $format @endcode, @code $mimeType @endcode
   + *    and @code $auth @endcode for this concrete test. Usually that's all that's
   + *    necessary: most concrete subclasses will be very thin.
   
   @@ -420,7 +419,7 @@ public function testGet() {
   -    $this->assertNotSame([static::$expectedErrorMimeType], $response->getHeader('Content-Type'));
   +    $this->assertNotSame([static::$mimeType], $response->getHeader('Content-Type'));
   
   +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
   @@ -47,17 +47,6 @@
      /**
   -   * The expected MIME type in case of 4xx error responses.
   -   *
   -   * (Can be different, when $mimeType for example encodes a particular
   -   * normalization, such as 'application/hal+json': its error response MIME
   -   * type is 'application/json'.)
   -   *
   -   * @var string
   -   */
   -  protected static $expectedErrorMimeType = 'application/json';
   
   @@ -318,7 +303,7 @@ protected function assertResourceResponse($expected_status_code, $expected_body,
   -      $this->assertSame([static::$expectedErrorMimeType], $response->getHeader('Content-Type'));
   +      $this->assertSame([static::$mimeType], $response->getHeader('Content-Type'));
   

   Thanks to this: we simply don't need $expectedErrorMimeType anymore! The only reason we needed it, was because of the bug that's fixed in this patch.

   So, we can simplify all REST tests thanks to this. :)

Issue summary completely rewritten. Now it accurately describes what the problem is and how it's being fixed. The scope has changed several times due to other REST, Serialization and request processing system issues being fixed!

Adjusting title for correctness too.

Test coverage is definitely present.

The key change is now in the hal module, so moving to that component.

This is now ready for final review, and can hopefully be RTBC'd.

#2813853: RequestHandler has its own error handling rather than leaving it to KernelEvents::EXCEPTION event subscribers and #2827084: EntityNormalizer::denormalize should not throw UnexpectedValueException, but \Symfony\Component\HttpKernel\Exception\UnprocessableEntityHttpException, so we get a 422 response instead of 400 are blocked by this, adding them to the related issues.

#31:

1. Indeed…
2. No, that's what I explained at length in #22: this must be hardcoded to application/json because \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber handles this (note that's the DefaultExceptionSubscriber in \Drupal\Core, not in \Drupal\serialization!). We've got #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 to fix that, as the accompanying code comment says:
   

       // @todo Update in https://www.drupal.org/node/2825347.
       $response = $this->request('GET', $url, $request_options);
       $this->assert406Response($response);
       $this->assertSame(['application/json'], $response->getHeader('Content-Type'));
   

HURRAY!!!!!!!

This unblocked both:

1. #2813853: RequestHandler has its own error handling rather than leaving it to KernelEvents::EXCEPTION event subscribers
2. #2827084: EntityNormalizer::denormalize should not throw UnexpectedValueException, but \Symfony\Component\HttpKernel\Exception\UnprocessableEntityHttpException, so we get a 422 response instead of 400