-
-
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
Merged
Merged
4.x Error Middleware #2398
Changes from 13 commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
d1c1646
Remove existing error handling and related tests.
l0gicgate 174f442
Add named http exceptions
l0gicgate 680042a
Refactor `App::run()` and `App::__invoke()` to support new error hand…
l0gicgate 74b9c66
Refactor `RoutingMiddleware` to throw `HttpNotFound` and `HttpNotAllo…
l0gicgate d2940f6
Rename `RoutingTest` to `RoutingMiddlewareTest` and refactor test met…
l0gicgate 524fb10
Add error handlers and error renderers
l0gicgate d5e2383
Add error handlers and error renderers supporting unit tests
l0gicgate 921bd02
Add http named exception supporting unit tests
l0gicgate bf6b979
Add error middleware implementation and supporting unit tests
l0gicgate 9ffd32c
Fix build errors.
l0gicgate f7aac1f
Reintegrate runner tests in `AppTest`
l0gicgate e34b6a7
Extend supporting unit tests for `AbstractErrorHandler` and `ErrorMid…
l0gicgate baac352
Refactor small code blocks and language structure from peer review.
l0gicgate 595f981
Refactor ErrorRendererInterface and refactor supporting tests as per …
l0gicgate 9bef7fa
Refactor AbstractErrorHandler to decouple displayErrorDetails from lo…
l0gicgate 897958f
Refactor writeErrorToLog to decouple displayErrorDetails from error l…
l0gicgate 616ae72
Refactored run() signature and removed codeCoverageIgnore instances
l0gicgate d260c6b
Added App tests for removed @codeCoverageIgnore
l0gicgate 44be8d9
Fix tests in ErrorMiddlewareTest for refactored App::run() method
l0gicgate bdfa9cf
Fix requested changes in error renderers
l0gicgate c941d53
Fix requested changes for named http exceptions
l0gicgate 5ef90ec
Rename AbstractErrorHandler to ErrorHandler
l0gicgate aaf0c57
Fix requested changes in ErrorMiddleware and Routing Middleware
l0gicgate c70f71d
Fix error handling bug in ErrorHandler
l0gicgate 8cefb1c
Add test coverage for PlainTextErrorRenderer
l0gicgate File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,17 +8,9 @@ | |
*/ | ||
namespace Slim; | ||
|
||
use Exception; | ||
use FastRoute\Dispatcher; | ||
use Psr\Container\ContainerInterface; | ||
use Psr\Http\Message\ResponseInterface; | ||
use Psr\Http\Message\ServerRequestInterface; | ||
use Slim\Exception\MethodNotAllowedException; | ||
use Slim\Exception\NotFoundException; | ||
use Slim\Handlers\Error; | ||
use Slim\Handlers\NotAllowed; | ||
use Slim\Handlers\NotFound; | ||
use Slim\Handlers\PhpError; | ||
use Slim\Http\Headers; | ||
use Slim\Http\Request; | ||
use Slim\Http\Response; | ||
|
@@ -27,7 +19,6 @@ | |
use Slim\Interfaces\RouteInterface; | ||
use Slim\Interfaces\RouterInterface; | ||
use Slim\Middleware\RoutingMiddleware; | ||
use Throwable; | ||
|
||
/** | ||
* App | ||
|
@@ -58,32 +49,21 @@ class App | |
protected $router; | ||
|
||
/** | ||
* @var callable | ||
* @var ServerRequestInterface|null | ||
*/ | ||
protected $notFoundHandler; | ||
protected $request; | ||
|
||
/** | ||
* @var callable | ||
* @var ResponseInterface|null | ||
*/ | ||
protected $notAllowedHandler; | ||
|
||
/** | ||
* @var callable | ||
*/ | ||
protected $errorHandler; | ||
|
||
/** | ||
* @var callable | ||
*/ | ||
protected $phpErrorHandler; | ||
protected $response; | ||
|
||
/** | ||
* @var array | ||
*/ | ||
protected $settings = [ | ||
'httpVersion' => '1.1', | ||
'responseChunkSize' => 4096, | ||
'displayErrorDetails' => false, | ||
'routerCacheFile' => false, | ||
]; | ||
|
||
|
@@ -95,6 +75,7 @@ class App | |
* Create new application | ||
* | ||
* @param array $settings | ||
* @param ContainerInterface|null $container | ||
*/ | ||
public function __construct(array $settings = [], ContainerInterface $container = null) | ||
{ | ||
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Removed and added tests for those methods. |
||
*/ | ||
public function getContainer() | ||
{ | ||
|
@@ -204,6 +186,7 @@ public function addSetting($key, $value) | |
* Set callable resolver | ||
* | ||
* @param CallableResolverInterface $resolver | ||
* @codeCoverageIgnore | ||
*/ | ||
public function setCallableResolver(CallableResolverInterface $resolver) | ||
{ | ||
|
@@ -228,6 +211,7 @@ public function getCallableResolver() | |
* Set router | ||
* | ||
* @param RouterInterface $router | ||
* @codeCoverageIgnore | ||
*/ | ||
public function setRouter(RouterInterface $router) | ||
{ | ||
|
@@ -257,144 +241,39 @@ public function getRouter() | |
} | ||
|
||
/** | ||
* Set callable to handle scenarios where a suitable | ||
* route does not match the current request. | ||
* | ||
* This service MUST return a callable that accepts | ||
* two arguments: | ||
* | ||
* 1. Instance of \Psr\Http\Message\ServerRequestInterface | ||
* 2. Instance of \Psr\Http\Message\ResponseInterface | ||
* | ||
* The callable MUST return an instance of | ||
* \Psr\Http\Message\ResponseInterface. | ||
* | ||
* @param callable $handler | ||
*/ | ||
public function setNotFoundHandler(callable $handler) | ||
{ | ||
$this->notFoundHandler = $handler; | ||
} | ||
|
||
/** | ||
* Get callable to handle scenarios where a suitable | ||
* route does not match the current request. | ||
* | ||
* @return callable|NotFound | ||
*/ | ||
public function getNotFoundHandler() | ||
{ | ||
if (!$this->notFoundHandler) { | ||
$this->notFoundHandler = new NotFound(); | ||
} | ||
|
||
return $this->notFoundHandler; | ||
} | ||
|
||
/** | ||
* Set callable to handle scenarios where a suitable | ||
* route matches the request URI but not the request method. | ||
* | ||
* This service MUST return a callable that accepts | ||
* three arguments: | ||
* | ||
* 1. Instance of \Psr\Http\Message\ServerRequestInterface | ||
* 2. Instance of \Psr\Http\Message\ResponseInterface | ||
* 3. Array of allowed HTTP methods | ||
* | ||
* The callable MUST return an instance of | ||
* \Psr\Http\Message\ResponseInterface. | ||
* | ||
* @param callable $handler | ||
*/ | ||
public function setNotAllowedHandler(callable $handler) | ||
{ | ||
$this->notAllowedHandler = $handler; | ||
} | ||
|
||
/** | ||
* Get callable to handle scenarios where a suitable | ||
* route matches the request URI but not the request method. | ||
* | ||
* @return callable|NotAllowed | ||
*/ | ||
public function getNotAllowedHandler() | ||
{ | ||
if (!$this->notAllowedHandler) { | ||
$this->notAllowedHandler = new NotAllowed(); | ||
} | ||
|
||
return $this->notAllowedHandler; | ||
} | ||
|
||
/** | ||
* Set callable to handle scenarios where an error | ||
* occurs when processing the current request. | ||
* | ||
* This service MUST return a callable that accepts three arguments: | ||
* | ||
* 1. Instance of \Psr\Http\Message\ServerRequestInterface | ||
* 2. Instance of \Psr\Http\Message\ResponseInterface | ||
* 3. Instance of \Error | ||
* | ||
* The callable MUST return an instance of | ||
* \Psr\Http\Message\ResponseInterface. | ||
* | ||
* @param callable $handler | ||
* @param ServerRequestInterface $request | ||
* @codeCoverageIgnore | ||
*/ | ||
public function setErrorHandler(callable $handler) | ||
public function setRequest(ServerRequestInterface $request) | ||
{ | ||
$this->errorHandler = $handler; | ||
$this->request = $request; | ||
} | ||
|
||
/** | ||
* Get callable to handle scenarios where an error | ||
* occurs when processing the current request. | ||
* | ||
* @return callable|Error | ||
* @return ServerRequestInterface|null | ||
* @codeCoverageIgnore | ||
*/ | ||
public function getErrorHandler() | ||
public function getRequest() | ||
{ | ||
if (!$this->errorHandler) { | ||
$this->errorHandler = new Error($this->getSetting('displayErrorDetails')); | ||
} | ||
|
||
return $this->errorHandler; | ||
return $this->request; | ||
} | ||
|
||
/** | ||
* Set callable to handle scenarios where a PHP error | ||
* occurs when processing the current request. | ||
* | ||
* This service MUST return a callable that accepts three arguments: | ||
* | ||
* 1. Instance of \Psr\Http\Message\ServerRequestInterface | ||
* 2. Instance of \Psr\Http\Message\ResponseInterface | ||
* 3. Instance of \Error | ||
* | ||
* The callable MUST return an instance of | ||
* \Psr\Http\Message\ResponseInterface. | ||
* | ||
* @param callable $handler | ||
* @param ResponseInterface $response | ||
* @codeCoverageIgnore | ||
*/ | ||
public function setPhpErrorHandler(callable $handler) | ||
public function setResponse(ResponseInterface $response) | ||
{ | ||
$this->phpErrorHandler = $handler; | ||
$this->response = $response; | ||
} | ||
|
||
/** | ||
* Get callable to handle scenarios where a PHP error | ||
* occurs when processing the current request. | ||
* | ||
* @return callable|PhpError | ||
* @return ResponseInterface|null | ||
* @codeCoverageIgnore | ||
*/ | ||
public function getPhpErrorHandler() | ||
public function getResponse() | ||
{ | ||
if (!$this->phpErrorHandler) { | ||
$this->phpErrorHandler = new PhpError($this->getSetting('displayErrorDetails')); | ||
} | ||
|
||
return $this->phpErrorHandler; | ||
return $this->response; | ||
} | ||
|
||
/******************************************************************************** | ||
|
@@ -555,51 +434,29 @@ public function group($pattern, $callable) | |
public function run() | ||
{ | ||
// create request | ||
$request = Request::createFromGlobals($_SERVER); | ||
if ($this->request === null) { | ||
$this->request = Request::createFromGlobals($_SERVER); | ||
} | ||
|
||
// create response | ||
$headers = new Headers(['Content-Type' => 'text/html; charset=UTF-8']); | ||
$response = new Response(200, $headers); | ||
$response = $response->withProtocolVersion($this->getSetting('httpVersion')); | ||
|
||
$response = $this->process($request, $response); | ||
|
||
$this->respond($response); | ||
return $response; | ||
} | ||
|
||
/** | ||
* Process a request | ||
* | ||
* This method traverses the application middleware stack and then returns the | ||
* resultant Response object. | ||
* | ||
* @param ServerRequestInterface $request | ||
* @param ResponseInterface $response | ||
* @return ResponseInterface | ||
*/ | ||
public function process(ServerRequestInterface $request, ResponseInterface $response) | ||
{ | ||
$router = $this->getRouter(); | ||
|
||
// Traverse middleware stack | ||
try { | ||
$response = $this->callMiddlewareStack($request, $response); | ||
} catch (Exception $e) { | ||
$response = $this->handleException($e, $request, $response); | ||
} catch (Throwable $e) { | ||
$response = $this->handlePhpError($e, $request, $response); | ||
if ($this->response === null) { | ||
$headers = new Headers(['Content-Type' => 'text/html; charset=UTF-8']); | ||
$this->response = new Response(200, $headers); | ||
$this->response = $this->response->withProtocolVersion($this->getSetting('httpVersion')); | ||
} | ||
|
||
$response = $this->finalize($response); | ||
// call middleware stack | ||
$this->response = $this->callMiddlewareStack($this->request, $this->response); | ||
$this->response = $this->finalize($this->response); | ||
|
||
return $response; | ||
return $this->respond($this->response); | ||
} | ||
|
||
/** | ||
* Send the response the client | ||
* | ||
* @param ResponseInterface $response | ||
* @return ResponseInterface | ||
*/ | ||
public function respond(ResponseInterface $response) | ||
{ | ||
|
@@ -658,6 +515,8 @@ public function respond(ResponseInterface $response) | |
} | ||
} | ||
} | ||
|
||
return $response; | ||
} | ||
|
||
/** | ||
|
@@ -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 ($routeInfo === null) { | ||
$routingMiddleware = new RoutingMiddleware($router); | ||
$request = $routingMiddleware->performRouting($request); | ||
$routeInfo = $request->getAttribute('routeInfo'); | ||
} | ||
|
||
if ($routeInfo[0] === Dispatcher::FOUND) { | ||
$route = $router->lookupRoute($routeInfo[1]); | ||
return $route->run($request, $response); | ||
} elseif ($routeInfo[0] === Dispatcher::METHOD_NOT_ALLOWED) { | ||
$notAllowedHandler = $this->getNotAllowedHandler(); | ||
return $notAllowedHandler($request, $response, $routeInfo[1]); | ||
} | ||
|
||
$notFoundHandler = $this->getNotFoundHandler(); | ||
return $notFoundHandler($request, $response); | ||
$route = $router->lookupRoute($routeInfo[1]); | ||
return $route->run($request, $response); | ||
} | ||
|
||
/** | ||
|
@@ -734,44 +585,4 @@ protected function isEmptyResponse(ResponseInterface $response) | |
|
||
return in_array($response->getStatusCode(), [204, 205, 304]); | ||
} | ||
|
||
/** | ||
* Call relevant error handler | ||
* | ||
* @param Exception $e | ||
* @param ServerRequestInterface $request | ||
* @param ResponseInterface $response | ||
* | ||
* @return ResponseInterface | ||
*/ | ||
protected function handleException(Exception $e, ServerRequestInterface $request, ResponseInterface $response) | ||
{ | ||
if ($e instanceof MethodNotAllowedException) { | ||
$notAllowedHandler = $this->getNotAllowedHandler(); | ||
return $notAllowedHandler($e->getRequest(), $e->getResponse(), $e->getAllowedMethods()); | ||
} | ||
|
||
if ($e instanceof NotFoundException) { | ||
$notFoundHandler = $this->getNotFoundHandler(); | ||
return $notFoundHandler($e->getRequest(), $e->getResponse()); | ||
} | ||
|
||
// Other exception, use $request and $response params | ||
$errorHandler = $this->getErrorHandler(); | ||
return $errorHandler($request, $response, $e); | ||
} | ||
|
||
/** | ||
* Call PHP error handler | ||
* | ||
* @param Throwable $e | ||
* @param ServerRequestInterface $request | ||
* @param ResponseInterface $response | ||
* @return ResponseInterface | ||
*/ | ||
protected function handlePhpError(Throwable $e, ServerRequestInterface $request, ResponseInterface $response) | ||
{ | ||
$handler = $this->getPhpErrorHandler(); | ||
return $handler($request, $response, $e); | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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()
andset/getResponse()
.Also,
run()
andprocess()
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.