Application: fixed empty latest request in request to ErrorPresenter when createInitialRequest() failed. #163
Conversation
|
Could you please add test for that as well? |
0231515 to
de1bc99
Compare
|
Test added. |
|
This should be fixed in different way, because createInitialRequest should not sometimes change $requests as side effect. |
|
Any suggestions? |
|
I'm thinking about that. I've found two another solutions but they are not as direct as the first one. One is to call Router::match() before Application::createInitialRequest(), save it into variable and pass it as second parameter into Application::processException() where it can be used if end($this->requests) is NULL. The second solution is simply to call Router::match() again if last response is NULL like that: In the second solution there is unnecessary calling Route::match() again if createInitialReqiest() fails (we already had apprequest from router so why call it again?), first solution is more similar to my original suggestion but not as direct as that one. |
|
I think that you should store the request object inside |
de1bc99 to
16e149e
Compare
|
Updated. Thanks for suggestion! |
src/Application/exceptions.php
Outdated
| } | ||
|
|
||
| /** | ||
| * @return Request |
src/Application/exceptions.php
Outdated
| /** @var int */ | ||
| protected $code = 404; | ||
|
|
||
| /** @var Request */ |
| $this->request = $request; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
there should always be two empty lines between methods
src/Application/Application.php
Outdated
| } | ||
|
|
||
| $args = ['exception' => $e, 'request' => end($this->requests) ?: NULL]; | ||
| $args = ['exception' => $e, 'request' => end($this->requests) ?: $e->getRequest()]; |
There was a problem hiding this comment.
This assumes that $this->requests can only be empty when BadRequestException was thrown which I'm not sure is something we can rely on.
What about rewriting the whole method like this?
if ($e instanceof BadRequestException) {
$httpCode = $e->getCode() ?: 404;
$lastRequest = $e->getRequest() ?: end($this->requests);
} else {
$httpCode = 500;
$lastRequest = end($this->requests);
if ($this->httpResponse instanceof Nette\Http\Response) {
$this->httpResponse->warnOnBuffer = FALSE;
}
}
$this->httpResponse->setCode($httpCode);
$params = ['exception' => $e, 'request' => $lastRequest];
...There was a problem hiding this comment.
Hm, yes. If Router::match() fails, it can make problems. That check of type of $e is needed.
…when createInitialRequest() failed
16e149e to
d432d51
Compare
|
What about this? dg@022574b |
|
It solves one problem but creates another. If no route matches, it ignores Application::$errorPresenter and shows Nette's default. Right now, I do not know where is problem, I'll check it later. With today's solution in my PR it works correctly (in all tested cases there is custom ErrorPresenter). |
|
fixed dg@0ca9097 |
|
This line makes problem. It throws InvalidPresenterException, so error code is 500. I think that for missing presenter would be better 404. What about throw always BadRequestException? |
|
Yes, I noticed it. dg@9678905 |
|
OK, now it seems to be working in all cases. |
|
Great job, thank you guys |
When
Application::createInitialRequest()ends with an exception (missing presenter),Application::processException()is called beforeApplication::processRequest(). This is problem forprocessException()tries to read propertyApplication::$requeststhat is, however, set later byprocessRequest(). So, even if application got request from router,ErrorPresentergotsNULLinstead of that request.For this one specific case it should be set manually.