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

Exception message is discarded when using httpError #30

Closed
elliot-sawyer opened this issue May 7, 2019 · 5 comments · Fixed by #32
Closed

Exception message is discarded when using httpError #30

elliot-sawyer opened this issue May 7, 2019 · 5 comments · Fixed by #32

Comments

@elliot-sawyer
Copy link
Contributor

If a developer calls return this->httpError(401, 'Some message for unauthorised users');, a HTTPResponse_Exception is thrown. If an ErrorPage is not loaded, the user will receive a barebones controller response with a 401 that says "Some message for unauthorised users", which is obtained from the Exception::getMessage() method.

This message seems to get discarded somewhere when an ErrorPage is generated. It is not available to a published ErrorPage, and thus cannot be displayed on a template if your theme implements one.

@sminnee
Copy link
Member

sminnee commented May 27, 2019

  • ErrorPageControllerExtension::onBeforeHTTPError() should accept the 3rd argument, errorMessage
  • ErrorPage::response_for() should take a 2nd argument, errorMessage
  • The error message should probably be added to the bottom of $errorPage->Content, maybe limited to dev environments

@elliot-sawyer
Copy link
Contributor Author

I think exposing the message through a getter method is a better approach. Sometimes there are multiple reasons why someone might get a particular HTTP code, and the message might be user friendly.

Could we add something like getErrorMessage() instead? You can still display the message at the end of your Content in a message :

$Content 
<% if $ErrorMessage %>
Error: $ErrorMessage
<% end_if %>

@sminnee
Copy link
Member

sminnee commented May 27, 2019

But then people won’t see it unless they specially design their template for it, which won’t happen most of the time.

@sminnee
Copy link
Member

sminnee commented May 27, 2019

I guess if we force it on dev and make the message available as a variable for other environments. Maybe ResponseErrorMessage?

@elliot-sawyer
Copy link
Contributor Author

How about both?

    /**
     * Get a {@link HTTPResponse} to response to a HTTP error code if an
     * {@link ErrorPage} for that code is present. First tries to serve it
     * through the standard SilverStripe request method. Falls back to a static
     * file generated when the user hit's save and publish in the CMS
     *
     * @param int $statusCode
     * @param string|null $errorMessage A developer message to put in the response on dev envs
     * @return HTTPResponse
     */
    public static function response_for($statusCode, $errorMessage = null)
    {
        $this->setResponseErrorMessage($errorMessage);

        // first attempt to dynamically generate the error page
        /** @var ErrorPage $errorPage */
        $errorPage = ErrorPage::get()
            ->filter(array(
                "ErrorCode" => $statusCode
            ))->first();
        if ($errorPage) {
            Requirements::clear();
            Requirements::clear_combined_files();
            if ($this->getResponseErrorMessage() && Director::isDev()) {
                $errorPage->Content .= "\n<p><b>Error detail: "
                    . Convert::raw2xml($this->getResponseErrorMessage()) ."</b></p>";
            }
            $request = new HTTPRequest('GET', '');
            $request->setSession(Controller::curr()->getRequest()->getSession());
            return ModelAsController::controller_for($errorPage)
                ->handleRequest($request);
        }
        // then fall back on a cached version
        $content = self::get_content_for_errorcode($statusCode);
        if ($content) {
            $response = new HTTPResponse();
            $response->setStatusCode($statusCode);
            $response->setBody($content);
            return $response;
        }
        return null;
    }

    public function setResponseErrorMessage($errorMessage)
    {
        $this->ResponseErrorMessage = $errorMessage;
        return $this;
    }

    public function getResponseErrorMessage()
    {
        return $this->ResponseErrorMessage;
    }

Can test out later this evening

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

Successfully merging a pull request may close this issue.

3 participants