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.

Sign up for GitHub

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

Jump to bottom

4.x Error Middleware #2398

Merged
merged 25 commits into from
Mar 12, 2018
Merged

4.x Error Middleware #2398

Show file tree
Hide file tree
Changes from 12 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 Feb 14, 2018
174f442
Add named http exceptions
l0gicgate Feb 15, 2018
680042a
Refactor `App::run()` and `App::__invoke()` to support new error hand…
l0gicgate Feb 15, 2018
74b9c66
Refactor `RoutingMiddleware` to throw `HttpNotFound` and `HttpNotAllo…
l0gicgate Feb 15, 2018
d2940f6
Rename `RoutingTest` to `RoutingMiddlewareTest` and refactor test met…
l0gicgate Feb 15, 2018
524fb10
Add error handlers and error renderers
l0gicgate Feb 15, 2018
d5e2383
Add error handlers and error renderers supporting unit tests
l0gicgate Feb 15, 2018
921bd02
Add http named exception supporting unit tests
l0gicgate Feb 15, 2018
bf6b979
Add error middleware implementation and supporting unit tests
l0gicgate Feb 15, 2018
9ffd32c
Fix build errors.
l0gicgate Feb 15, 2018
f7aac1f
Reintegrate runner tests in `AppTest`
l0gicgate Feb 15, 2018
e34b6a7
Extend supporting unit tests for `AbstractErrorHandler` and `ErrorMid…
l0gicgate Feb 15, 2018
baac352
Refactor small code blocks and language structure from peer review.
l0gicgate Feb 15, 2018
595f981
Refactor ErrorRendererInterface and refactor supporting tests as per …
l0gicgate Feb 17, 2018
9bef7fa
Refactor AbstractErrorHandler to decouple displayErrorDetails from lo…
l0gicgate Feb 17, 2018
897958f
Refactor writeErrorToLog to decouple displayErrorDetails from error l…
l0gicgate Feb 19, 2018
616ae72
Refactored run() signature and removed codeCoverageIgnore instances
l0gicgate Feb 26, 2018
d260c6b
Added App tests for removed @codeCoverageIgnore
l0gicgate Feb 26, 2018
44be8d9
Fix tests in ErrorMiddlewareTest for refactored App::run() method
l0gicgate Feb 26, 2018
bdfa9cf
Fix requested changes in error renderers
l0gicgate Feb 28, 2018
c941d53
Fix requested changes for named http exceptions
l0gicgate Feb 28, 2018
5ef90ec
Rename AbstractErrorHandler to ErrorHandler
l0gicgate Feb 28, 2018
aaf0c57
Fix requested changes in ErrorMiddleware and Routing Middleware
l0gicgate Feb 28, 2018
c70f71d
Fix error handling bug in ErrorHandler
l0gicgate Feb 28, 2018
8cefb1c
Add test coverage for PlainTextErrorRenderer
l0gicgate Feb 28, 2018
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
271 changes: 41 additions & 230 deletions Slim/App.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -27,7 +19,6 @@
use Slim\Interfaces\RouteInterface;
use Slim\Interfaces\RouterInterface;
use Slim\Middleware\RoutingMiddleware;
use Throwable;

/**
* App
Expand Down Expand Up @@ -58,32 +49,21 @@ class App
protected $router;

/**
* @var callable
* @var ServerRequestInterface|null
*/
protected $notFoundHandler;
protected $request;
Copy link
Member

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.

Copy link
Member Author

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.


/**
* @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,
];

Expand All @@ -95,6 +75,7 @@ class App
* Create new application
*
* @param array $settings
* @param ContainerInterface|null $container
*/
public function __construct(array $settings = [], ContainerInterface $container = null)
{
Expand All @@ -106,6 +87,7 @@ public function __construct(array $settings = [], ContainerInterface $container
* Get container
*
* @return ContainerInterface|null
* @codeCoverageIgnore
Copy link
Member

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?

Copy link
Member Author

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.

*/
public function getContainer()
{
Expand Down Expand Up @@ -204,6 +186,7 @@ public function addSetting($key, $value)
* Set callable resolver
*
* @param CallableResolverInterface $resolver
* @codeCoverageIgnore
*/
public function setCallableResolver(CallableResolverInterface $resolver)
{
Expand All @@ -228,6 +211,7 @@ public function getCallableResolver()
* Set router
*
* @param RouterInterface $router
* @codeCoverageIgnore
*/
public function setRouter(RouterInterface $router)
{
Expand Down Expand Up @@ -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;
}

/********************************************************************************
Expand Down Expand Up @@ -555,51 +434,29 @@ public function group($pattern, $callable)
public function run()
{
// create request
$request = Request::createFromGlobals($_SERVER);
if (is_null($this->request)) {
Copy link

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.

$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 (is_null($this->response)) {
$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)
{
Expand Down Expand Up @@ -658,6 +515,8 @@ public function respond(ResponseInterface $response)
}
}
}

return $response;
}

/**
Expand All @@ -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)) {
Copy link

Choose a reason for hiding this comment

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

should stay ===

$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);
}

/**
Expand Down Expand Up @@ -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);
}
}
Loading