Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Application: fixed empty latest request in request to ErrorPresenter when createInitialRequest() failed. #163

Closed
wants to merge 1 commit into from

Conversation

@vlastavesely
Copy link
Contributor

vlastavesely commented Oct 14, 2016

When Application::createInitialRequest() ends with an exception (missing presenter), Application::processException() is called before Application::processRequest(). This is problem for processException() tries to read property Application::$requests that is, however, set later by processRequest(). So, even if application got request from router, ErrorPresenter gots NULL instead of that request.

For this one specific case it should be set manually.

@TomasVotruba

This comment has been minimized.

Copy link

TomasVotruba commented Oct 14, 2016

Could you please add test for that as well?

@vlastavesely vlastavesely force-pushed the vlastavesely:apprequest-fix branch from 0231515 to de1bc99 Oct 14, 2016
@vlastavesely

This comment has been minimized.

Copy link
Contributor Author

vlastavesely commented Oct 14, 2016

Test added.

@dg

This comment has been minimized.

Copy link
Member

dg commented Oct 17, 2016

This should be fixed in different way, because createInitialRequest should not sometimes change $requests as side effect.

@TomasVotruba

This comment has been minimized.

Copy link

TomasVotruba commented Oct 17, 2016

Any suggestions?

@vlastavesely

This comment has been minimized.

Copy link
Contributor Author

vlastavesely commented Oct 17, 2016

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.

    $request = $this->router->match($this->httpRequest);
    $this->processRequest($this->createInitialRequest($request));

    ...

    $this->processException($e, $request); // in Application::run(), if exception thrown

    ...

    $args = ['exception' => $e, 'request' => end($this->requests) ?: $request]; // in processException()

The second solution is simply to call Router::match() again if last response is NULL like that:

    $request = end($this->requests);
    if (!$request) {
        $request = $this->router->match($this->httpRequest);
    }
    $args = ['exception' => $e, 'request' => $request];

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.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Oct 18, 2016

I think that you should store the request object inside BadRequestException and processException should get the request object from BadRequestException if available.

@vlastavesely vlastavesely force-pushed the vlastavesely:apprequest-fix branch from de1bc99 to 16e149e Oct 19, 2016
@vlastavesely

This comment has been minimized.

Copy link
Contributor Author

vlastavesely commented Oct 19, 2016

Updated. Thanks for suggestion!

}

/**
* @return Request

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Oct 19, 2016

Contributor

@return Request|NULL

@@ -41,12 +41,23 @@ class BadRequestException extends \Exception
/** @var int */
protected $code = 404;

/** @var Request */

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Oct 19, 2016

Contributor

Request|NULL

}

/**

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Oct 19, 2016

Contributor

there should always be two empty lines between methods

@@ -172,7 +172,7 @@ public function processException($e)
$this->httpResponse->setCode($e instanceof BadRequestException ? ($e->getCode() ?: 404) : 500);
}

$args = ['exception' => $e, 'request' => end($this->requests) ?: NULL];
$args = ['exception' => $e, 'request' => end($this->requests) ?: $e->getRequest()];

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Oct 19, 2016

Contributor

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];

...

This comment has been minimized.

Copy link
@vlastavesely

vlastavesely Oct 19, 2016

Author Contributor

Hm, yes. If Router::match() fails, it can make problems. That check of type of $e is needed.

…when createInitialRequest() failed
@vlastavesely vlastavesely force-pushed the vlastavesely:apprequest-fix branch from 16e149e to d432d51 Oct 19, 2016
dg added a commit to dg/nette-application that referenced this pull request Oct 19, 2016
@dg

This comment has been minimized.

Copy link
Member

dg commented Oct 19, 2016

What about this? dg@022574b

@vlastavesely

This comment has been minimized.

Copy link
Contributor Author

vlastavesely commented Oct 19, 2016

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).

@dg

This comment has been minimized.

Copy link
Member

dg commented Oct 19, 2016

fixed dg@0ca9097

dg added a commit to dg/nette-application that referenced this pull request Oct 19, 2016
@vlastavesely

This comment has been minimized.

Copy link
Contributor Author

vlastavesely commented Oct 19, 2016

throw $isForward ? new BadRequestException($e->getMessage(), 0, $e) : $e;

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?

@dg

This comment has been minimized.

Copy link
Member

dg commented Oct 19, 2016

Yes, I noticed it. dg@9678905

@vlastavesely

This comment has been minimized.

Copy link
Contributor Author

vlastavesely commented Oct 19, 2016

OK, now it seems to be working in all cases.

@dg dg closed this in 7b07c4e Oct 19, 2016
@TomasVotruba

This comment has been minimized.

Copy link

TomasVotruba commented Oct 19, 2016

Great job, thank you guys

@vlastavesely vlastavesely deleted the vlastavesely:apprequest-fix branch May 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.