'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

That would be wonderful :)

This is the relevant test coverage from:

\Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::testGet()

    // DX: 406 when requesting unsupported format.
    $response = $this->request('GET', $url, $request_options);
    $this->assert406Response($response);
    $this->assertNotSame([static::$mimeType], $response->getHeader('Content-Type'));


    $request_options[RequestOptions::HEADERS]['Accept'] = static::$mimeType;


    // DX: 406 when requesting unsupported format but specifying Accept header.
    // @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'));

(This is also where the @todo is that points to this very issue.)

\Drupal\Tests\rest\Functional\CookieResourceTestTrait::initAuthentication()

    $request_options[RequestOptions::HEADERS] = [
      'Accept' => 'application/json',
      'Content-Type' => 'application/json',
    ];
    $response = $this->request('POST', $user_login_url, $request_options);

\Drupal\Tests\rest\Functional\EntityResource\Comment\CommentResourceTestBase::testPostDxWithoutCriticalBaseFields()

    $request_options = [];
    $request_options[RequestOptions::HEADERS]['Accept'] = static::$mimeType;
    $request_options[RequestOptions::HEADERS]['Content-Type'] = static::$mimeType;
    $request_options = array_merge_recursive($request_options, $this->getAuthenticationRequestOptions('POST'));

    // DX: 422 when missing 'entity_type' field.
    $request_options[RequestOptions::BODY] = $this->serializer->encode(array_diff_key($this->getNormalizedPostEntity(), ['entity_type' => TRUE]), static::$format);
    $response = $this->request('POST', $url, $request_options);
    // @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'));

We should be able to remove all 3 of those Accept headers and tests should still pass.

Let's see!

Well, turns out that Symfony itself still does some basic content negotiation that causes Accept to work for error responses.

This does not explain the root cause.

The root cause is:

1. when the default Drupal core exception subscriber is used (\Drupal\Core\EventSubscriber\DefaultExceptionSubscriber), not to be confused with the default serialization exception subscriber (\Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber) nor the default HTML exception subscriber (\Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber), then…
2. … \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber::onException() is what handles the exception and
3. it does $format = $this->getFormat($event->getRequest()); to determine in which format to format the error response
4. this calls \Symfony\Component\HttpFoundation\Request::getAcceptableContentTypes(), which is the code that inspects the Accept subscriber

No: see the quoted code in EntityREsourceTestBase: this happens when you request a non-existing format. i.e.:

Without Accept

GET /node/1?_format=non_existing_format

Will return a text/plain error response.

With Accept

GET /node/1?_format=non_existing_format
Accept: application/json

Will return an application/json error response.

If we don't support the Accept header, we really should not support it. \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber should not read Request::getAcceptableContentTypes().

BTW, I noticed we can actually already remove the Accept header in CookieResourceTestTrait. That's the only one that's not actually related to this issue. See #2849474: \Drupal\Tests\rest\Functional\CookieResourceTestTrait::initAuthentication() should use the current test format, and should not send an Accept header for the patch that removes it.

#2849474: \Drupal\Tests\rest\Functional\CookieResourceTestTrait::initAuthentication() should use the current test format, and should not send an Accept header landed, yay!

That means one hunk from the patch can disappear. No interdiff, because this is a rebase.

So, actually, the thing is that we want to prove is that sending an Accept header has no effect. Then #3 and #9 are not what we want, but this is what we want.

i.e.:

GET /node/1?_format=non_existing_format

and

GET /node/1?_format=non_existing_format
Accept: application/json

should BOTH result in a text/plain response.

But that's not the case today.

This new patch modifies the test coverage to change it to the expectation described above. No interdiff, because different patch.

The solution is simple: modify \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber::getFormat() to not inspect the Accept request header.

Clearly, #11 fixed the fails in #10, but it caused different fails in CommentResourceTestBase::testPostDxWithoutCriticalBaseFields(). Working on that.

We simply also need to update \Drupal\Tests\rest\Functional\EntityResource\Comment\CommentResourceTestBase::testPostDxWithoutCriticalBaseFields() now.

Still 3 errors, but 3 other ones!

So now we have failures in Quick Edit tests. Because those are the only tests that are asserting 4xx AJAX responses (i.e. ?_wrapper_format=drupal_ajax). With the changes in #20, all exceptions are treated as fatal errors: that's what the code that I removed in #20 did:

-    // Display the message if the current error reporting level allows this type
-    // of message to be displayed,
-    $data = NULL;
-    if (error_displayable($error) && $message = $exception->getMessage()) {
-      $data = ['message' => sprintf('A fatal error occurred: %s', $message)];
-    }

… but many of those exceptions are not even remotely related to fatal errors! In fact, the recently committed #2808233: REST 403 responses don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out proved that: it added meaningful information about why access is denied (for 403 responses). There's nothing fatal about that. Yet because it's this exception subscriber that was generating the response, that's how it looked.

So the solution is fairly simple: make ExceptionJsonSubscriber not only handle json but also the 3 wrapper formats:

-    return ['json'];
+    return ['json', 'drupal_modal', 'drupal_dialog', 'drupal_ajax'];

Now we just need to adjust the expected error messages of the two tests failing in #20 to not have that A fatal error occurred: prefix, and BAM, tests are passing again!

#22 made me realize that we're actually running into a closely related bug: the fact that \Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber only handles 4XX error responses, and not 500 error responses.

For 500 error responses, we only have \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber, which only ever handles 500 error responses for json, not for anything else. Not for hal_json, not for xml, not for any other registered serialization format.

So, actually, we should revert the changes made in #20, because they might cause certain error responses to start working differently.

However, we need to do more than just add a on500() method. Because Those onSOMETHING methods are only called by \Drupal\Core\EventSubscriber\HttpExceptionSubscriberBase::onException() if the exception is a HttpExceptionInterface exception, which excludes exceptions due to fatal errors.

Then the end result is that we don't get text/plain error responses… but error responses serialized in the correct format! :)

(This still needs test coverage in \Drupal\Tests\serialization\Unit\EventSubscriber\DefaultExceptionSubscriberTest to be expanded though.)

Here's the expanded test coverage.

Simplification of the changes in \Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber. Far fewer changes now!

From
8 files changed, 152 insertions(+), 36 deletions(-)
to
8 files changed, 126 insertions(+), 31 deletions(-)

Most of this is expanded test coverage.

A round of self-review, that hopefully also explains all the parts of this patch. I think it explains it pretty clearly, because I have a clear explanation/justification for each change:

1. +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionSubscriber.php
   @@ -218,16 +218,6 @@ protected function getFormat(Request $request) {
   -    // 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';
   -      }
   -    }
   

   This is what the IS was originally about.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
   @@ -14,7 +14,7 @@ class ExceptionJsonSubscriber extends HttpExceptionSubscriberBase {
   -    return ['json'];
   +    return ['json', 'drupal_modal', 'drupal_dialog', 'drupal_ajax'];
   
   --- a/core/modules/editor/src/Tests/QuickEditIntegrationLoadingTest.php
   +++ b/core/modules/editor/src/Tests/QuickEditIntegrationLoadingTest.php
   
   --- a/core/modules/quickedit/src/Tests/QuickEditLoadingTest.php
   +++ b/core/modules/quickedit/src/Tests/QuickEditLoadingTest.php
   

   In working on this patch, I ran into fails for these tests, and this is the solution: not all exceptions are fatal errors. So the expected messages need to be updated to not have the A fatal error occurred: prefix.

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
   @@ -278,10 +278,8 @@ public function testPostDxWithoutCriticalBaseFields() {
   -    // @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: Internal Server Error"}', (string) $response->getBody());
   +    // @todo Uncomment, remove next line in https://www.drupal.org/node/2820364.
   +    $this->assertResourceErrorResponse(500, 'Internal Server Error', $response);
   
   @@ -303,10 +301,8 @@ public function testPostDxWithoutCriticalBaseFields() {
   -    // @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());
   +    // @todo Uncomment, remove next line in https://www.drupal.org/node/2820364.
   +    $this->assertResourceErrorResponse(500, 'A fatal error occurred: Field  is unknown.', $response);
   
   +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -501,17 +501,17 @@ public function testGet() {
   -    $this->assertNotSame([static::$mimeType], $response->getHeader('Content-Type'));
   +    $this->assertSame(['text/plain; charset=UTF-8'], $response->getHeader('Content-Type'));
   ...
   -    // DX: 406 when requesting unsupported format but specifying Accept header.
   -    // @todo Update in https://www.drupal.org/node/2825347.
   +    // DX: 406 when requesting unsupported format but specifying Accept header:
   +    // should still result in a response with the same Content-Type.
        $response = $this->request('GET', $url, $request_options);
        $this->assert406Response($response);
   -    $this->assertSame(['application/json'], $response->getHeader('Content-Type'));
   +    $this->assertSame(['text/plain; charset=UTF-8'], $response->getHeader('Content-Type'));
   

   This is the original mess we needed to fix.

   (And I see that that last comment is wrong, fixing that in my next reroll.)

4. +++ b/core/modules/serialization/src/EventSubscriber/DefaultExceptionSubscriber.php
   @@ -56,6 +59,26 @@ protected static function getPriority() {
   +  public function onException(GetResponseForExceptionEvent $event) {
   +    $exception = $event->getException();
   +    if (in_array($event->getRequest()->getRequestFormat(), $this->getHandledFormats(), TRUE)) {
   +      if (!$exception instanceof HttpExceptionInterface) {
   +        $error = Error::decodeException($exception);
   +        $message = error_displayable($error)
   +          ? sprintf('A fatal error occurred: %s', $exception->getMessage())
   +          : 'A fatal error occurred.';
   +        $event->setException(new HttpException(500, $message, $exception));
   +      }
   +    }
   +    parent::onException($event);
   +    if (!$event->isPropagationStopped()) {
   +      $event->setException($exception);
   +    }
   +  }
   +
   
   @@ -78,4 +101,14 @@ public function on4xx(GetResponseForExceptionEvent $event) {
   +  public function on500(GetResponseForExceptionEvent $event) {
   +    return $this->on4xx($event);
   +  }
   

   This is the other problem that my initial attempt to fix the original problem uncovered: the exception subscriber for the serialization module is currently A) not yet supporting HttpException(500, …) instances, B) not yet supporting fatal error exceptions.

   (And I see that the logic can be simplified slightly, doing that in my next reroll.)

5. +++ b/core/modules/serialization/src/EventSubscriber/DefaultExceptionSubscriber.php
   --- a/core/modules/serialization/tests/src/Unit/EventSubscriber/DefaultExceptionSubscriberTest.php
   +++ b/core/modules/serialization/tests/src/Unit/EventSubscriber/DefaultExceptionSubscriberTest.php
   

   This is the expanded test coverage for the fixes in the serialization module's exception subscriber.

Addressing my own feedback from #27.

I am not convinced that catching 500 is a good idea.
When you have a 500 you want to simply stop and not care about it.

If you want that, then we should also modify:

• \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber::onException(), it currently does:
  

  $method = 'on' . $format;
      if (!method_exists($this, $method)) {
        if ($exception instanceof HttpExceptionInterface) {
          $this->onFormatUnknown($event);
          $response = $event->getResponse();
          $response->headers->set('Content-Type', 'text/plain');
        }
        else {
          $this->onHtml($event);
        }
      }
      else {
        $this->$method($event);
      }
    }
  

  Note how it's only checking if ($exception instanceof HttpExceptionInterface) { after it decided to already handle it!

• The above is also why we have this in \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber::onJson():
  

      $exception = $event->getException();
      $error = Error::decodeException($exception);
  
      // Display the message if the current error reporting level allows this type
      // of message to be displayed,
      $data = NULL;
      if (error_displayable($error) && $message = $exception->getMessage()) {
        $data = ['message' => sprintf('A fatal error occurred: %s', $message)];
      }
  

  All of that is only relevant for fatal exceptions!

I had started writing a comparison of the pros and cons of each. But you're right. There's no comparison: I like your proposal (and hence point 1), because I agree there is far less that can break. If fatal errors occur within the serializer, then the patch in #28 is going to cause a lot of pain. #22 would then be much better.

Reuploading #22, unchanged.

Self-review:

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
   @@ -280,8 +280,8 @@ public function testPostDxWithoutCriticalBaseFields() {
   -    $this->assertSame(['application/json'], $response->getHeader('Content-Type'));
   -    $this->assertSame('{"message":"A fatal error occurred: Internal Server Error"}', (string) $response->getBody());
   +    $this->assertSame(['text/plain; charset=UTF-8'], $response->getHeader('Content-Type'));
   +    $this->assertSame('Internal Server Error', (string) $response->getBody());
   
   +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -501,17 +501,17 @@ public function testGet() {
   -    $this->assertNotSame([static::$mimeType], $response->getHeader('Content-Type'));
   +    $this->assertSame(['text/plain; charset=UTF-8'], $response->getHeader('Content-Type'));
   ...
   -    $this->assertSame(['application/json'], $response->getHeader('Content-Type'));
   +    $this->assertSame(['text/plain; charset=UTF-8'], $response->getHeader('Content-Type'));
   

   All changing to text/plain, great!

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
   @@ -303,10 +303,8 @@ public function testPostDxWithoutCriticalBaseFields() {
   -    // @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());
   +    // @todo Uncomment, remove next line in https://www.drupal.org/node/2820364.
   +    $this->assertResourceErrorResponse(500, FALSE, $response);
   

   But then why does this one continue to use the MIME type of the format? This is a 500 response, it should also use text/plain!

   Investigating…

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -501,17 +501,17 @@ public function testGet() {
   +    // should still result in a response with the same Content-Type.
   

   Nit:

   // should result in a text/plain response.
   

The code I cited in #30 is to blame:

$method = 'on' . $format;
    if (!method_exists($this, $method)) {
      if ($exception instanceof HttpExceptionInterface) {
        $this->onFormatUnknown($event);
        $response = $event->getResponse();
        $response->headers->set('Content-Type', 'text/plain');
      }
      else {
        $this->onHtml($event);
      }
    }
    else {
      $this->$method($event);
    }
  }

In case of a fatal error for a non-HTML format, that… calls onHtml() anyway (intentionally so: to have that super reliable output in case of fatal errors, that don't rely on any other subsystems). That method then eventually sets a response like this:

    $content = $this->t('The website encountered an unexpected error. Please try again later.');
    $content .= $message ? '</br></br>' . $message : '';
    $response = new Response($content, 500);

    if ($exception instanceof HttpExceptionInterface) {
      $response->setStatusCode($exception->getStatusCode());
      $response->headers->add($exception->getHeaders());
    }
    else {
      $response->setStatusCode(Response::HTTP_INTERNAL_SERVER_ERROR, '500 Service unavailable (with message)');
    }

    $event->setResponse($response);

As you can see, that doesn't set a Content-Type header.

So, when Response::prepare() is called to prepare it for sending, it does this:

            // Content-type based on the Request
            if (!$headers->has('Content-Type')) {
                $format = $request->getRequestFormat();
                if (null !== $format && $mimeType = $request->getMimeType($format)) {
                    $headers->set('Content-Type', $mimeType);
                }
            }

Tada! We're sending something that's somewhere in between plain text and HTML as if it actually contained content of the requested format.

The solution is simple: just always set the 'Content-Type' => 'text/plain' header in onHtml().

I think #32 is ready.

Note that by now, \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber really always does the same thing: it always ends up calling onHtml() for fatal errors. In other words: it's finally living up to its description:

/**
 * Last-chance handler for exceptions.
 *
 * This handler will catch any exceptions not caught elsewhere and report
 * them as an error page.
 */

So I think we should rename the class (to FatalErrorExceptionSubscriber) and simplify the logic (get rid of the current onException() and make the current onHtml() the new onException(), then get rid of everything else).

But that… is for another issue.

If modules add additional formats

#32 caused a few more text/html responses to change to text/plain. In updating those assertions, I also noticed a few incorrect comments directly impacted by this, so fixed those too.

#37 :)

+++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionSubscriber.php
@@ -119,7 +119,7 @@ protected function onHtml(GetResponseForExceptionEvent $event) {
-    $response = new Response($content, 500);
+    $response = new Response($content, 500, ['Content-Type' => 'text/plain']);

This caused two fails in ExceptionHandlingTest. Fixes are trivial. And prove that only fatal error responses are affected :)

Created follow-up per second half of #33: #2853300: Standardize fatal error/exception handling: backtrace for all formats, not just HTML.

Yay, this unblocked #2853300: Standardize fatal error/exception handling: backtrace for all formats, not just HTML!