[Core: Module] Handle 500 errors properly

[johnsaigle]

Brief summary of changes

Previously LORIS would crash and show a blank page on 500 errors OR mistakenly show a 404 page.

This was because the try-catch in Module only had a catch case for NotFound exceptions. Thus all exceptions were treated as 404 as a result even though it was wrong of reasons.

On top of this, the actual exception message would be swallowed and would not appear in the error logs, making things very difficult to debug.

This PR fixes this issues and serves well-decorated 500 pages containing a message to the user. A special message is returned for ConfigurationExceptions as they reflect errors on the user's part rather than on LORIS's part. The exception information is also logged to assist with debugging.

This resolves issue...

Should address numerous issues that have arisen in PRs #3921 #3881 #4162, maybe more.

To test this change...

• Throw an exception in the code anywhere. Instead of crashing you will either get a well-decorated generic 500 page or a page telling you that you have invalid Config settings.

[driusan]

That explanation doesn't make sense.. if it doesn't have catches for other exception types they keep propagating, they don't get handled by the wrong handler.

try {
    throw new Exception("Test");
} catch (BadFunctionCallException $e) { /* A random more specific exception type from SPL */
    print "I caught something";
}

print "I am here";

Results in:

$ php test.php 
PHP Fatal error:  Uncaught Exception: Test in /Users/driusan/Code/Loris/test.php:4
Stack trace:
#0 {main}
  thrown in /Users/driusan/Code/Loris/test.php on line 4

Fatal error: Uncaught Exception: Test in /Users/driusan/Code/Loris/test.php:4
Stack trace:
#0 {main}
  thrown in /Users/driusan/Code/Loris/test.php on line 4

(But it's still an improvement to handle more types, so there's no reason to block it just because the explanation doesn't make sense..)

[driusan]

debug statement

[driusan]

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

[driusan]

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

[johnsaigle]

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

Loris/src/Http/Error.php

Line 50 in 15c0a27

$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?

[driusan]

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?

[johnsaigle]

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

[johnsaigle]

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.

[driusan]

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

[johnsaigle]

@ridz1208 This is the PR we were talking about

[ridz1208]

@driusan please re-review