Problem/Motivation

Exception handling of HttpExceptions does not pass on headers in the responses it creates. For example, you could throw a MethodNotAllowedHttpException which adds an Allow header, however this won't be passed on because we currently only do something like:

$response = new JsonResponse(['message' => $event->getException()->getMessage()], $exception->getStatusCode());

So 405 responses would be missing that header. Same goes for any HttpExceptionInterface exception which is adding headers. HttpException itself allows arbitrary headers to be set.

Proposed resolution

Pass on the headers from HttpExceptions to the response, like:

$response = new JsonResponse(['message' => $event->getException()->getMessage()], $exception->getStatusCode(), $execption->getHeaders());

Remaining tasks

This needs some test coverage. Speaking to WimLeers, EntityResourceTestBase is a good place to add some assertions for this as it already tests 405 responses, but only the body, not the headers. We could alternatively just test this in a generic way in DefaultExceptionSubscriberTest maybe

User interface changes

N/A

API changes

Correct headers will be added to some responses. More of an addition I guess :)

Data model changes

N/A

CommentFileSizeAuthor
#15 interdiff-2838949-15.txt906 bytesdamiankloip
#15 2838949-15.patch9.5 KBdamiankloip
#13 interdiff-2838949-13.txt2.06 KBdamiankloip
#13 2838949-13.patch9.54 KBdamiankloip
#10 interdiff-2838949-10-with-fixes.txt2.14 KBdamiankloip
#10 2838949-10-PASS.patch7.48 KBdamiankloip
#10 interdiff-2838949-10.txt5.82 KBdamiankloip
#10 2838949-10-tests-only-FAIL.patch5.34 KBdamiankloip
#6 2838949-6.patch1.43 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip created an issue. See original summary.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip at Acquia commented
Status: Active » Postponed

Currently blocking this on #2739617: Make it easier to write on4xx() exception subscribers

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Title: HttpException Handling Does Not Pass Headers to Responses » [PP-1] HttpException Handling Does Not Pass Headers to Responses
Issue tags: +API-First Initiative

Confirming!

I think we should test this both in EntityResourceTestBase (integration test) and in DefaultExceptionSubscriberTest (unit test).

damiankloip’s picture

damiankloip CreditAttribution: damiankloip at Acquia commented

Looks like we also will need to fix DefaultExceptionSubscriberTest here too, it currently makes little sense :) :

$e = new MethodNotAllowedHttpException(['json'], 'test message');

But the first parameter is a list of ALLOWED METHODS... Plus not sure HTTP method not allowed == content type not allowed. We should change this to NotAcceptableHttpException.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

Hah! :D

damiankloip’s picture

damiankloip CreditAttribution: damiankloip at Acquia commented
FileSize
2838949-6.patch1.43 KB

Here are some fixes for the DefaultExceptionSubscriberTest. This subscriber is not actually the problem. This correctly sets headers see onException and other methods. We will actually need a new unit test for the ExceptionJsonSubscriber class.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip at Acquia commented

Just for background, test was originally added in #2594777-12: Accessing a non existing format results in a 500

damiankloip’s picture

damiankloip CreditAttribution: damiankloip at Acquia commented
Status: Postponed » Needs review

Unblocked!

damiankloip’s picture

damiankloip CreditAttribution: damiankloip at Acquia commented
Title: [PP-1] HttpException Handling Does Not Pass Headers to Responses » HttpException Handling Does Not Pass Headers to Responses
damiankloip’s picture

damiankloip CreditAttribution: damiankloip at Acquia commented
FileSize
2838949-10-tests-only-FAIL.patch5.34 KB
interdiff-2838949-10.txt5.82 KB
2838949-10-PASS.patch7.48 KB
interdiff-2838949-10-with-fixes.txt2.14 KB

OK, here is a patch with tests and fixes. When looking at this, I also realised it is not just \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber that had this problem but also Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber.

I also found another bug with the latter from the added test coverage, the response content type is never set, if we know the format and can respond to it, we should set it.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community 
  1. +++ b/core/modules/serialization/src/EventSubscriber/DefaultExceptionSubscriber.php
@@ -64,12 +64,17 @@ protected static function getPriority() {
+    $headers = $exception->getHeaders();
...
+    $response = new Response($encoded_content, $exception->getStatusCode(), $headers);

    This part is clear, and is unquestionably a bugfix. 

  2. +++ b/core/modules/serialization/src/EventSubscriber/DefaultExceptionSubscriber.php
@@ -64,12 +64,17 @@ protected static function getPriority() {
+    $headers['Content-Type'] = $request->getMimeType($format);

    But is this part really necessary?

    \Symfony\Component\HttpFoundation\Response::prepare() sets it automatically:

                // 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);
                }
            }

    But I guess it's better to be specific. Because that means we can actually ensure (and test) the correct behavior ourselves, rather than relying on Symfony magic.

    Which means we're also protected against upstream changes.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Reviewed & tested by the community » Needs work

Oh wait!

This should still update EntityResourceTestBase! Every place where a 405 response is being asserted, we should add an assertion for the presence of the Allow header with the correct header value. Easy peasy :)

damiankloip’s picture

damiankloip CreditAttribution: damiankloip at Acquia commented
Status: Needs work » Needs review
FileSize
2838949-13.patch9.54 KB
interdiff-2838949-13.txt2.06 KB

That's a good point! It's easy enough for us to add some new assertions in there for that!

Status: Needs review » Needs work

The last submitted patch, 13: 2838949-13.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip at Acquia commented
Status: Needs work » Needs review
FileSize
2838949-15.patch9.5 KB
interdiff-2838949-15.txt906 bytes

Over zealous removal of _format from the Request path in the test.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community

Perfect! :)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: HttpException Handling Does Not Pass Headers to Responses » HttpException Handling Does Not Pass Headers to Responses — results e.g. in missing Allow header for 405 responses
Issue summary: View changes
Parent issue: » #2794263: REST: top priorities for Drupal 8.3.x

Clarifying title. Which then also explains why this should be a top priority.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Component: base system » request processing system
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 98e7ac3 and pushed to 8.3.x. Thanks!

  • alexpott committed 98e7ac3 on 8.3.x 
    Issue #2838949 by damiankloip: HttpException Handling Does Not Pass...

  • alexpott committed 98e7ac3 on 8.4.x 
    Issue #2838949 by damiankloip: HttpException Handling Does Not Pass...

  • alexpott committed 98e7ac3 on 8.4.x 
    Issue #2838949 by damiankloip: HttpException Handling Does Not Pass...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.