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

[Core: Module] Handle 500 errors properly #4163

Merged
merged 3 commits into from
Dec 10, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 74 additions & 0 deletions php/libraries/Module.class.inc
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ use \Psr\Http\Message\ResponseInterface;
*/
class Module extends \LORIS\Router\PrefixRouter implements RequestHandlerInterface
{
const GENERIC_500_ERROR = "We're sorry, LORIS has encountered an internal "
. "server error. Please contact your project administrator for more "
. "information.";
const CONFIGURATION_ERROR = "LORIS configuration settings are invalid or "
. "missing and as a result execution cannot proceed. Please contact "
. "your project administrator and/or verify your LORIS configuration.";
protected $name;
protected $dir;

Expand Down Expand Up @@ -265,6 +271,7 @@ class Module extends \LORIS\Router\PrefixRouter implements RequestHandlerInterfa
);
}
} catch (\NotFound $e) {
error_log("IN the NotFound exception");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug statement

return (new \LORIS\Middleware\PageDecorationMiddleware(
$request->getAttribute("user") ?? new \LORIS\AnonymousUser()
))->process(
Expand All @@ -277,8 +284,75 @@ class Module extends \LORIS\Router\PrefixRouter implements RequestHandlerInterfa
)
)
);
/* Catch more specific errors that may be thrown in the codebase.
* Without handling these exceptions explicitly, LORIS will display
* a 404 error and swallow the exception, hindering debugging efforts.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this first paragraph because that's not actually how PHP handles exceptions.

(The second paragraph is fine but there's no reason to have the word "NOTE" at the front.. it's just a comment.)

*
* NOTE The order of these catch statements matter and should go from
* most to least specific. Otherwise all Exceptions will be caught
* as their more generic parent class which reduces precision.
*/
} catch (\DatabaseException $e) {
error_log($e);
return $this->responseStatus500(
$request,
$user,
self::GENERIC_500_ERROR
);
} catch (\ConfigurationException $e) {
error_log($e);
error_log(self::CONFIGURATION_ERROR);
Copy link
Collaborator

@driusan driusan Nov 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's no reason to put the message for the users in the error log, you've already logged the exception.

return $this->responseStatus500(
$request,
$user,
self::CONFIGURATION_ERROR
);
} catch (\LorisException $e) {
error_log($e);
return $this->responseStatus500(
$request,
$user,
self::GENERIC_500_ERROR
);
} catch (\Exception $e) {
error_log($e);
return $this->responseStatus500(
$request,
$user,
self::GENERIC_500_ERROR
);
}

return $page->process($request, $page);
}

/**
* Return a properly-decorated response with status 500. Used to handle
* exceptions thrown within the codebase in a graceful way.
*
* @param ServerRequestInterface $request The request to process.
* @param \User $user The user to decorate the page with.
* @param string $message An error message explaining the
* 500 code to the user.
*
* @return ResponseInterface A properly decorated 500 response.
*/
function responseStatus500(
ServerRequestInterface $request,
\User $user,
string $message
): ResponseInterface {
return (new \LORIS\Middleware\PageDecorationMiddleware(
$user
))->process(
$request,
new \LORIS\Router\NoopResponder(
new \LORIS\Http\Error(
$request,
500,
$message
)
)
);
}
}
4 changes: 1 addition & 3 deletions smarty/templates/500.tpl
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
<div class="container">
{foreach from=$error_message item=message}
<h2>{$message}</h2>
{/foreach}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be changed from an array to a scalar, that will break any other places that use it.

Copy link
Contributor Author

@johnsaigle johnsaigle Nov 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue wasn't so much of array/scalar but that it's inconsistent with

$tpl_data = array(
(which uses message instead of error_message).

Do you prefer to change the Error class to use error_message or should the other calling code across LORIS be changed?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually know how many calling places there are, but wouldn't it be easier to change that to 'message' => [$message] instead of trying to hunt down the other ones?

Copy link
Contributor Author

@johnsaigle johnsaigle Nov 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you mean change the Error.php code, then I would need to change the 403 and 404 templates as well as their calling code instead; they use scalars instead of arrays in their templates (in contrast to 500).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a preference but they need to be standardized one way or another if they are going to use src/Http/Error.php.

<h3>{$message}</h3>
<div><a href="{$baseurl}">Go to main page</a></div>
</div>