-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
4.x Error Middleware #2398
4.x Error Middleware #2398
Conversation
…ling middleware.
…wed` exceptions while preparing route.
…hod to support http named exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some thoughts.
Slim/App.php
Outdated
@@ -555,51 +434,29 @@ public function group($pattern, $callable) | |||
public function run() | |||
{ | |||
// create request | |||
$request = Request::createFromGlobals($_SERVER); | |||
if (is_null($this->request)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null === $this->request
is better here no overhead of function call.
Slim/App.php
Outdated
@@ -682,22 +541,14 @@ public function __invoke(ServerRequestInterface $request, ResponseInterface $res | |||
$router = $this->getRouter(); | |||
|
|||
// If routing hasn't been done, then do it now so we can dispatch | |||
if (null === $routeInfo) { | |||
if (is_null($routeInfo)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should stay ===
$html = '<p>The application could not run because of the following error:</p>'; | ||
$html .= '<h2>Details</h2>'; | ||
$html .= $this->renderExceptionFragment($e); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can just return before the else and have this html set as a second return lowering cyclomatic complexity by 1.
{ | ||
$html = sprintf('<div><strong>Type:</strong> %s</div>', get_class($exception)); | ||
|
||
if (($code = $exception->getCode())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if someone passes an exception code of 0 this will never execute.
$html .= sprintf('<div><strong>Code:</strong> %s</div>', $code); | ||
} | ||
|
||
if (($message = $exception->getMessage())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if someone sets a message of '0' it also will fail to run.
*/ | ||
protected function logError($error) | ||
{ | ||
error_log($error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this error log should also be able to take in an outside error logging, as in be able to take in a logger middleware of some kind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I agree. A separate PR after this could allow for handling a PSR3 logger.
Slim/Handlers/ErrorHandler.php
Outdated
/** | ||
* Default Slim application error handler | ||
* | ||
* It outputs the error message and diagnostic information in either |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either-> one of the following
Slim/Middleware/ErrorMiddleware.php
Outdated
*/ | ||
if (method_exists($exception, 'getRequest')) { | ||
$request = $exception->getRequest(); | ||
if (!is_null($request)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=== null
* exact state of the request at the time of the exception can be | ||
* accessed by the end user if necessary | ||
*/ | ||
if (!is_null($exception)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!=== null
Slim/Error/AbstractErrorRenderer.php
Outdated
/** | ||
* Default Slim application error renderer | ||
* | ||
* It outputs the error message and diagnostic information in either JSON, XML, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either should be "one of the following"
Thank you @asheliahut for the code review. I'm aware this is a large PR and I really appreciate you going through everything. I have made the suggested changes besides the suggestion you made for the |
{ | ||
$e = $this->exception; | ||
|
||
$text = 'Slim Application Error:' . PHP_EOL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use \n
instead of PHP_EOL
to make integration tests compatible with windows.
* | ||
* @return ErrorRendererInterface | ||
* | ||
* @throws \RuntimeException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add use RuntimeException
on top to remove the backslash.
{ | ||
$renderer = $this->renderer; | ||
|
||
if (( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There'd be plenty of room to put this statement in one line.
if (($renderer !== null && !class_exists($renderer)) {
*/ | ||
protected function formatResponse() | ||
{ | ||
$e = $this->exception; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This extra variable ($e) is not really necessary.
Slim/Middleware/ErrorMiddleware.php
Outdated
* Get callable to handle scenarios where an error | ||
* occurs when processing the current request. | ||
* | ||
* @param string $type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What for values can be passed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand your question. Can you clarify?
Hi! I have some questions / ideas about this PR and the error handling in general.
|
The name of the middleware The question is how to handle PHP errors (extended from Throwable)? Are you planning a new separate Middleware (e.g. ErrorMiddleware) or a generic "ThrowableMiddleware" for Exceptions and Errors together? |
* @param \Exception|\Throwable $exception | ||
* @param bool $displayErrorDetails | ||
*/ | ||
public function __construct($exception, $displayErrorDetails); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interfaces should not have a constructor as you are limiting the ability to define implementation detail vs defining expected behaviour.
Instead, your methods render
and renderWithBody
should accept the exception instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pointing that out, I will refactor the interface accordingly!
Thank you for reviewing this PR! |
Hi! As I started to point out at #2393, I believe that there should be a separate
If you agree with these concerns, you can refer to #2393 for some remarks I’ve already made and I’ll be happy to help with the implementation. (I already have a draft but it is written for Slim 3.) |
protected function writeToErrorLog() | ||
{ | ||
$renderer = new PlainTextErrorRenderer(); | ||
$error = $renderer->render($this->exception, $this->displayErrorDetails); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the details should always be displayed in the error log? To me it doesn’t make sense to log a generic "Application error" without details.
(Maybe we are starting to cross boundaries and violating SRP as @odan pointed out?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do have a point. I can modify this so it’s always displaying the error details when writing to log if everyone is in favor of this option!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a production server I have to log all errors (into a file), but for security reasons I am not allowed to show the users any error details. Therefore, it is still necessary to be able to configure both options (displayErrorDetails and logging) independently of each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For more advanced requirements, I expect that the user should be able to easily replace Slim's error handling with their own.
I don't want this component to turn into a massive thing as we're Slim! However, we should be reasonably flexible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So how about I decouple $displayErrorDetails
from the logging and add another parameter $logErrorDetails
which can be enabled or disabled. So for instance you may not want to display the error details to the end user but you may want to have them displayed in your log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@l0gicgate Sounds good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? I want the error handling middleware to be the outermost middleware as there will be other middleware before routing that I want the error handler to pick up. |
@akrabat what I meant by that is |
Slim/App.php
Outdated
@@ -106,6 +87,7 @@ public function __construct(array $settings = [], ContainerInterface $container | |||
* Get container | |||
* | |||
* @return ContainerInterface|null | |||
* @codeCoverageIgnore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we ignoring code coverage for methods in App?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed and added tests for those methods.
Slim/App.php
Outdated
*/ | ||
protected $notFoundHandler; | ||
protected $request; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not want $request or $response as properties on App. They were intentionally removed from Slim 3's DIC because they are immutable and people use them as if they aren't.
This also means that I don't want set/getRequest()
and set/getResponse()
.
Also, run()
and process()
are separate for a reason: it allows someone to inject their own request and response when running Silm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed and modified run() signature to take in an optional $request parameter for test purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A number of bits and bobs in here to look at @l0gicgate.
If you'd like me to raise a PR against this one addressing them, let me know.
Slim/Middleware/ErrorMiddleware.php
Outdated
{ | ||
/** | ||
* @var bool | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a gap between each property. It just looks nicer. This applies to all files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Slim/Middleware/ErrorMiddleware.php
Outdated
public function __invoke(ServerRequestInterface $request, ResponseInterface $response, callable $next) | ||
{ | ||
$this->request = $request; | ||
$this->response = $response; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not store response. Each renderer should return a response that they have created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -60,6 +65,21 @@ public function performRouting(ServerRequestInterface $request) | |||
|
|||
// add route to the request's attributes | |||
$request = $request->withAttribute('route', $route); | |||
} elseif ($routeInfo[0] === Dispatcher::NOT_FOUND) { | |||
$exception = new HttpNotFoundException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call $exception->setRequest($request);
and then throw
directly here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
$exception = new HttpNotFoundException(); | ||
} elseif ($routeInfo[0] === Dispatcher::METHOD_NOT_ALLOWED) { | ||
$exception = new HttpNotAllowedException(); | ||
$exception->setAllowedMethods($routeInfo[1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call $exception->setRequest($request);
and then throw
directly here.
I know it's a duplication from above, but will be clearer to read and remove the test for not null below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Slim/Middleware/ErrorMiddleware.php
Outdated
/** | ||
* Retrieve request object from exception and replace current request object if not null | ||
*/ | ||
if (method_exists($exception, 'getRequest')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test for an instance of HttpException
. That way we can be sure that the Request object in getRequest() is a PSR7 Request object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
<?php | ||
namespace Slim\Exception; | ||
|
||
class HttpNotAllowedException extends HttpException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to HttpMethodNotAllowedException
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
$response = $this->response; | ||
$body = $this->renderer->renderWithBody($this->exception, $this->displayErrorDetails); | ||
|
||
if ($this->exception instanceof HttpNotAllowedException) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the exception should return a response, this can go into the exception itself.
Slim/Handlers/ErrorHandler.php
Outdated
* It outputs the error message and diagnostic information in one of the following formats: | ||
* JSON, XML, Plain Text or HTML based on the Accept header. | ||
*/ | ||
class ErrorHandler extends AbstractErrorHandler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would rather have an interface and rename AbstractErrorHandler
to ErrorHandler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
*/ | ||
public function __invoke( | ||
ServerRequestInterface $request, | ||
ResponseInterface $response, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Response not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, what if during the middleware progression a user did attach headers to the response, example if a CORS middleware appended headers, wouldn't we want to have the response object that reflects the correct state at the time the time the exception was thrown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does it come from given that the error handler is called from the catch() of the Middleware and that we won't have $response passed into the middleware in PSR-15?
* @package Slim | ||
* @since 4.0.0 | ||
*/ | ||
interface ErrorRendererInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we just need render
which returns a response.
Fix copyright year
Preface
The original PR #2222 was closed and some of the concepts and code will be implemented in this PR.
Description
This PR eliminates all native error handlers and replaces it with
ErrorMiddleware
which handles all errors and provides multiple different renderers (HTML, XML, JSON, Plain Text) to enhance error display.Implementation
Adding custom handlers for different named exceptions
Error Logging
If you would like to pipe in custom error logging to the default
ErrorHandler
that ships with Slim you can simply extend it and stub thelogError()
method.Error Handling/Rendering
The rendering is finally decoupled from the handling. Everything still works the way it previously did. It will still detect the content-type and render things appropriately with the help of
ErrorRenderers
. The coreErrorHandler
extends theAbstractErrorHandler
class which has been completely refactored. By default it will call the appropriateErrorRenderer
for the supported content types. Someone can now provide their custom error renderer by extending theAbstractErrorHandler
class and overloading the protectedrenderer
variable from the parent.HTTP Exceptions
I just think it makes sense to throw HTTP based exceptions within the application. These exceptions works with nicely with the proposed native renderers. They can each have a
description
andtitle
attribute as well to provide a bit more insight when the native HTML renderer is invoked.The base class
HttpSpecializedException
extendsException
and comes with the following sub classes :HttpBadRequestException
HttpForbiddenException
HttpInternalServerErrorException
HttpNotAllowedException
HttpNotFoundException
HttpNotImplementedException
HttpUnauthorizedException
The developer can extend the
HttpSpecializedException
class if they need any other response codes that we decide not to provide with the base repository. Example if you wanted a 504 gateway timeout exception that behaves like the native ones you would do the following:I
Status
Work in progress