Problem/Motivation
If a REST plugin throws an HttpException, the request handler will catch it and output a response using the exception message and status code. The HttpException class does not however, check that the status code is valid. So when creating the response, if the code was set to 0 for example, Symfony promptly throws another exception, which overrides the expected output.
Actual output:
{"message":"A fatal error occurred: The HTTP status code \u00220\u0022 is not valid."}
Expected output:
{"error":"Exception message here."}
Proposed resolution
Let's make Drupal more robust. If the code is < 100 or >= 600, then set the HTTP status code to 500.
Additionally, it may be beneficial to add documentation reminding REST plugin authors to a) stick with HttpExceptions, not generic exceptions, and b) use valid status codes.
Remaining tasks
Needs a new patch, and review.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|
Comments
Comment #1
MattA CreditAttribution: MattA commentedComment #2
MattA CreditAttribution: MattA commentedComment #3
MattA CreditAttribution: MattA commentedCleaned up the issue summary.
Comment #4
MattA CreditAttribution: MattA commentedComment #5
Wim LeersI worked on reproducing this. So I made the following change:
This produces a HTTP 500 response. Good so far.
The response body is:
This is the problem the IS complains about. Well, the reason is that by default verbose error logging is turned off, to not leak sensitive information. You must enable verbose error logging. After doing so, the HTTP 500 response's body is now:
Learn how to enable verbose error logging by looking at
example.settings.local.php
and https://www.drupal.org/node/2259531.Comment #6
MattA CreditAttribution: MattA commentedUhhh... it has been quite some time since I filed this, but I don't think that is quite what was going on originally.
In your example, you throw an
\Exception
which is NOT caught by theRequestHandler
, and sent further up the stack for Drupal to process. If I remember correctly, this issue was caused by throwing aSymfony\Component\HttpKernel\Exception\HttpException
with an invalid status code, which IS caught by the RequestHandler, and then mangled when it tries to generate the response and fails.Since
HttpException
messages should be output to clients, an empty response and{"message":"A fatal error occurred: ____"}
are both invalid responses in this situation. This can be seen by clients when they then decode the response. Errors are expected to be in the 'error' key, which is not generated by the completely separate "fatal error" exception.Comment #7
MattA CreditAttribution: MattA commentedComment #9
dawehner@MattA
Are you sure that setting an invalid code actually results in a Http exception?
I tried out the following:
Which results in the following response (with verbose):
and without verbose:
Comment #10
MattA CreditAttribution: MattA commentedI just tried it again using this line to simulate an exception thrown by a plugin.
And got:
There is a return statement in the catch block above, so the line you added would never be reached in this scenario.
Comment #12
Wim Leers\Symfony\Component\HttpFoundation\Response::setStatusCode()
throws that exception.We don't babysit broken code.
a) we could document this better, BUT there are plenty of examples, and they'll quickly notice in their tests that non-HTTP exceptions are not handled, and cause fatal PHP errors
b) using valid status codes is so basic, and so easy to debug (the response you cite in #10 even says it explicitly!) that this is IMHO fine.
IOW: I don't think we need to add yet more magic here.
Comment #13
Wim LeersBetter title.
Comment #14
joseph.olstadPlease re-open this issue as per rfc7231 and symfony issue 36823
Drupal is crashing currently on return codes outside of 1xx/5xx
https://tools.ietf.org/html/rfc7231#section-6
we're needing to support this for development on the linkchecker module.
I've made a pull request upstream to resolve the crashing which was promptly rebuked.
https://github.com/guzzle/psr7/pull/420