Skip to content
This repository has been archived by the owner on Mar 5, 2020. It is now read-only.

Exception message not transferred to response #24

Open
cxj opened this issue Jun 8, 2017 · 9 comments
Open

Exception message not transferred to response #24

cxj opened this issue Jun 8, 2017 · 9 comments

Comments

@cxj
Copy link
Contributor

cxj commented Jun 8, 2017

In \Relay\Middleware\ExceptionHandler\__invoke(), it attempts to place the exception message in the Response, but yet by the time Radar gets the Response back to call the Responder, the message has been overwritten. This results in a frustrating to debug HTTP 500 error with no further information.

Here's the stack at the try { in the above method:

ExceptionHandler.php:68, Relay\Middleware\ExceptionHandler->__invoke()
Runner.php:73, Relay\Runner->__invoke()
ResponseSender.php:40, Relay\Middleware\ResponseSender->__invoke()
Runner.php:73, Relay\Runner->__invoke()
Relay.php:60, Relay\Relay->__invoke()
Adr.php:138, Radar\Adr\Adr->run()
index.php:270, {main}()

Here's the relevant data at that same point:

$e = {ReflectionException} [8]
 message = "Class Clx\Domain\App\GetNpiData does not exist"
 *Exception*string = ""
 code = -1
 file = "/Users/chris/web2/vendor/aura/di/src/Resolver/Reflector.php"
 line = 77
 *Exception*trace = {array} [21]
 *Exception*previous = null
$next = {Relay\Runner} [2]
 queue = {array} [0]
 resolver = {Radar\Adr\Resolver} [1]
$request = {Zend\Diactoros\ServerRequest} [13]
 attributes = {array} [0]
 cookieParams = {array} [2]
 parsedBody = {array} [0]
 queryParams = {array} [0]
 serverParams = {array} [25]
 uploadedFiles = {array} [0]
 method = "GET"
 requestTarget = null
 uri = {Zend\Diactoros\Uri} [9]
 headers = {array} [10]
 headerNames = {array} [10]
 protocol = "1.1"
 stream = {Zend\Diactoros\PhpInputStream} [4]
$response = {Zend\Diactoros\Response} [7]
 phrases = {array} [65]
 reasonPhrase = ""
 statusCode = 200
 headers = {array} [0]
 headerNames = {array} [0]
 protocol = "1.1"
 stream = {Zend\Diactoros\Stream} [2]

Note that the correct exception message regarding the missing class is present at this point inside the exception object.

But then this line in the catch is called:

            $response = $this->exceptionResponse->withStatus(500);

Here's that method from \Zend\Diactoros\Response:

    public function withStatus($code, $reasonPhrase = '')
    {
        $new = clone $this;
        $new->setStatusCode($code);
        $new->reasonPhrase = $reasonPhrase;
        return $new;
    }

Note that it overwrites the reasonPhrase with an empty string if no second argument is passed to the method call.

I think that Relay\Middleware\ExceptionHandler\__invoke() should probably be passing $e->message like this to withStatus(500, $e->getMessage()).

@cxj
Copy link
Contributor Author

cxj commented Jun 8, 2017

Ah heck. It's even more complicated than this. The message is being placed in the body, but I was not seeing the body at all, because it's an AJAX call. So what I really need is some way to bubble the error up to my code (out of Relay) where I can log it, or do something otherwise intelligent with it.

@pmjones
Copy link
Contributor

pmjones commented Jun 9, 2017

The message is being placed in the body, but I was not seeing the body at all, because it's an AJAX call.

Ah so! Is it that HTTP 500 response is not supposed to supply a content-body ?

@cxj
Copy link
Contributor Author

cxj commented Jun 9, 2017

I'm trying to decipher that right now, from this W3C document.

10.5 Server Error 5xx
Response status codes beginning with the digit “5” indicate cases in which the server is aware that it has erred or is incapable of performing the request. Except when responding to a HEAD request, the server SHOULD include an entity containing an explanation of the error situation, and whether it is a temporary or permanent condition. User agents SHOULD display any included entity to the user. These response codes are applicable to any request method.
10.5.1 500 Internal Server Error
The server encountered an unexpected condition which prevented it from fulfilling the request.

The question becomes, what is an "entity" that should be included? Is that a header or body?

@cxj
Copy link
Contributor Author

cxj commented Jun 9, 2017

Section 1.3 Terminology:

entity
The information transferred as the payload of a request or response. An entity consists of metainformation in the form of entity-header fields and content in the form of an entity-body, as described in section 7.

The answer remains vague. :-p

@cxj
Copy link
Contributor Author

cxj commented Jun 9, 2017

4.3 Message Body
The message-body (if any) of an HTTP message is used to carry the entity-body associated with the request or response. The message-body differs from the entity-body only when a transfer-coding has been applied, as indicated by the Transfer-Encoding header field (section 14.41).
message-body = entity-body | entity-body encoded as per Transfer-Encoding

My brain is starting to hurt.

@cxj
Copy link
Contributor Author

cxj commented Jun 9, 2017

7 Entity
Request and Response messages MAY transfer an entity if not otherwise restricted by the request method or response status code. An entity consists of entity-header fields and an entity-body, although some responses will only include the entity-headers.

@cxj
Copy link
Contributor Author

cxj commented Jun 9, 2017

My conclusion at the moment is that returning an error message (description) in either a header or body meets the SHOULD of the HTTP spec.

My general inclination for most errors would be to set error status and message in a Payload, and let the application's specific Responder deal with appropriately notifying the user. But this is in the middleware. It's harder to think through an appropriate general solution, since we might be in the middle of a chain of several middlewares. Hmm.

@pmjones
Copy link
Contributor

pmjones commented Jun 10, 2017

My general inclination for most errors would be to set error status and message in a Payload, and let the application's specific Responder deal with appropriately notifying the user.

That's my inclination as well.

@cxj
Copy link
Contributor Author

cxj commented Jun 12, 2017

I think in my problem situation, the Domain class which was supposed to create the Payload is the missing class, so it's fallen back on the infrastructure (Relay, Radar) to deal with the problem. There's not really the usual Payload with its convenient attributes, I don't think. But somehow I need to inform the Responder so that it can do something more intelligent, even though that rather feels like the wrong place for the logic. Hmm.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants