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 all 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
70 changes: 69 additions & 1 deletion php/libraries/Module.class.inc
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* This file contains a class which encapsulates the concept of a "module"
* in LORIS.
*
* PHP Version 5
* PHP Version 7
*
* @category Main
* @package Loris
Expand All @@ -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 @@ -277,8 +283,70 @@ class Module extends \LORIS\Router\PrefixRouter implements RequestHandlerInterfa
)
)
);
/* 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);
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>