Add response status code for the Whoops error handler #476
Conversation
|
|
||
| $this->response->withStatus(StatusCode::STATUS_INTERNAL_SERVER_ERROR)->will([$this->response, 'reveal']); | ||
| $this->response->getBody()->will([$this->stream, 'reveal']); | ||
| $this->response->getStatusCode()->willReturn(StatusCode::STATUS_MOVED_PERMANENTLY); |
There was a problem hiding this comment.
wait... shouldn't this be StatusCode::STATUS_INTERNAL_SERVER_ERROR here?
There was a problem hiding this comment.
No, it's deliberately an error code below 400 to make sure Utils::getStatusCode does its job and returns the default 500 code. However I don't know if it should be tested here and just rely on Stratigility tests.
There was a problem hiding this comment.
Uhm, it doesn't make sense since the default error code is 0. So it's already converted and tested in the previous tests.
| $this->response->withStatus(StatusCode::STATUS_INTERNAL_SERVER_ERROR)->will([$this->response, 'reveal']); | ||
| $this->response->getStatusCode()->willReturn(StatusCode::STATUS_INTERNAL_SERVER_ERROR); | ||
| $this->response->withStatus(StatusCode::STATUS_IM_A_TEAPOT)->will([$this->response, 'reveal']); | ||
| $this->response->getStatusCode()->willReturn(StatusCode::STATUS_IM_A_TEAPOT); |
There was a problem hiding this comment.
Not sure I understand this change. $error here is a simple RuntimeException, so I'd expect an internal server error. Or are you redefining the $error somehow?
There was a problem hiding this comment.
Looks good, in general. I think we need an additional test for when $e has an exception code that falls elsewhere in the 400-599 range, to validate that the response status is set accordingly.
I might have misunderstood that... or you wanted to see changes to the default RunTimeException error code.
Add response status code for the Whoops error handler
fixes #475