From: ArthurHoaro Date: Fri, 21 Aug 2020 08:50:44 +0000 (+0200) Subject: Move error handling to dedicated controller instead of middleware X-Git-Tag: v0.12.0-beta~4^2 X-Git-Url: https://git.immae.eu/?p=github%2Fshaarli%2FShaarli.git;a=commitdiff_plain;h=0c6fdbe12bbbb336348666b14b82096f24d5858b Move error handling to dedicated controller instead of middleware --- diff --git a/application/container/ContainerBuilder.php b/application/container/ContainerBuilder.php index 2e8c1ee3..4a1a6ea7 100644 --- a/application/container/ContainerBuilder.php +++ b/application/container/ContainerBuilder.php @@ -9,6 +9,7 @@ use Shaarli\Bookmark\BookmarkServiceInterface; use Shaarli\Config\ConfigManager; use Shaarli\Feed\FeedBuilder; use Shaarli\Formatter\FormatterFactory; +use Shaarli\Front\Controller\Visitor\ErrorController; use Shaarli\History; use Shaarli\Http\HttpAccess; use Shaarli\Netscape\NetscapeBookmarkUtils; @@ -148,6 +149,10 @@ class ContainerBuilder ); }; + $container['errorHandler'] = function (ShaarliContainer $container): ErrorController { + return new ErrorController($container); + }; + return $container; } } diff --git a/application/front/ShaarliMiddleware.php b/application/front/ShaarliMiddleware.php index a2a3837b..c015c0c6 100644 --- a/application/front/ShaarliMiddleware.php +++ b/application/front/ShaarliMiddleware.php @@ -3,7 +3,6 @@ namespace Shaarli\Front; use Shaarli\Container\ShaarliContainer; -use Shaarli\Front\Exception\ShaarliFrontException; use Shaarli\Front\Exception\UnauthorizedException; use Slim\Http\Request; use Slim\Http\Response; @@ -53,35 +52,12 @@ class ShaarliMiddleware $this->checkOpenShaarli($request, $response, $next); return $next($request, $response); - } catch (ShaarliFrontException $e) { - // Possible functional error - $this->container->pageBuilder->reset(); - $this->container->pageBuilder->assign('message', nl2br($e->getMessage())); - - $response = $response->withStatus($e->getCode()); - - return $response->write($this->container->pageBuilder->render('error', $this->container->basePath)); } catch (UnauthorizedException $e) { $returnUrl = urlencode($this->container->environment['REQUEST_URI']); return $response->withRedirect($this->container->basePath . '/login?returnurl=' . $returnUrl); - } catch (\Throwable $e) { - // Unknown error encountered - $this->container->pageBuilder->reset(); - if ($this->container->conf->get('dev.debug', false)) { - $this->container->pageBuilder->assign('message', $e->getMessage()); - $this->container->pageBuilder->assign( - 'stacktrace', - nl2br(get_class($e) .': '. PHP_EOL . $e->getTraceAsString()) - ); - } else { - $this->container->pageBuilder->assign('message', t('An unexpected error occurred.')); - } - - $response = $response->withStatus(500); - - return $response->write($this->container->pageBuilder->render('error', $this->container->basePath)); } + // Other exceptions are handled by ErrorController } /** diff --git a/application/front/controller/visitor/ErrorController.php b/application/front/controller/visitor/ErrorController.php new file mode 100644 index 00000000..10aa84c8 --- /dev/null +++ b/application/front/controller/visitor/ErrorController.php @@ -0,0 +1,45 @@ +container->pageBuilder->reset(); + + if ($throwable instanceof ShaarliFrontException) { + // Functional error + $this->assignView('message', nl2br($throwable->getMessage())); + + $response = $response->withStatus($throwable->getCode()); + } else { + // Internal error (any other Throwable) + if ($this->container->conf->get('dev.debug', false)) { + $this->assignView('message', $throwable->getMessage()); + $this->assignView( + 'stacktrace', + nl2br(get_class($throwable) .': '. PHP_EOL . $throwable->getTraceAsString()) + ); + } else { + $this->assignView('message', t('An unexpected error occurred.')); + } + + $response = $response->withStatus(500); + } + + + return $response->write($this->render('error')); + } +} diff --git a/tests/front/ShaarliMiddlewareTest.php b/tests/front/ShaarliMiddlewareTest.php index d435f506..05aa34a9 100644 --- a/tests/front/ShaarliMiddlewareTest.php +++ b/tests/front/ShaarliMiddlewareTest.php @@ -74,7 +74,8 @@ class ShaarliMiddlewareTest extends TestCase } /** - * Test middleware execution with controller throwing a known front exception + * Test middleware execution with controller throwing a known front exception. + * The exception should be thrown to be later handled by the error handler. */ public function testMiddlewareExecutionWithFrontException(): void { @@ -99,16 +100,14 @@ class ShaarliMiddlewareTest extends TestCase }); $this->container->pageBuilder = $pageBuilder; - /** @var Response $result */ - $result = $this->middleware->__invoke($request, $response, $controller); + $this->expectException(LoginBannedException::class); - static::assertInstanceOf(Response::class, $result); - static::assertSame(401, $result->getStatusCode()); - static::assertContains('error', (string) $result->getBody()); + $this->middleware->__invoke($request, $response, $controller); } /** * Test middleware execution with controller throwing a not authorized exception + * The middle should send a redirection response to the login page. */ public function testMiddlewareExecutionWithUnauthorizedException(): void { @@ -136,9 +135,10 @@ class ShaarliMiddlewareTest extends TestCase } /** - * Test middleware execution with controller throwing a not authorized exception + * Test middleware execution with controller throwing a not authorized exception. + * The exception should be thrown to be later handled by the error handler. */ - public function testMiddlewareExecutionWithServerExceptionWith(): void + public function testMiddlewareExecutionWithServerException(): void { $request = $this->createMock(Request::class); $request->method('getUri')->willReturnCallback(function (): Uri { @@ -148,9 +148,11 @@ class ShaarliMiddlewareTest extends TestCase return $uri; }); + $dummyException = new class() extends \Exception {}; + $response = new Response(); - $controller = function (): void { - throw new \Exception(); + $controller = function () use ($dummyException): void { + throw $dummyException; }; $parameters = []; @@ -165,12 +167,9 @@ class ShaarliMiddlewareTest extends TestCase }) ; - /** @var Response $result */ - $result = $this->middleware->__invoke($request, $response, $controller); + $this->expectException(get_class($dummyException)); - static::assertSame(500, $result->getStatusCode()); - static::assertContains('error', (string) $result->getBody()); - static::assertSame('An unexpected error occurred.', $parameters['message']); + $this->middleware->__invoke($request, $response, $controller); } public function testMiddlewareExecutionWithUpdates(): void diff --git a/tests/front/controller/visitor/ErrorControllerTest.php b/tests/front/controller/visitor/ErrorControllerTest.php new file mode 100644 index 00000000..e497bfef --- /dev/null +++ b/tests/front/controller/visitor/ErrorControllerTest.php @@ -0,0 +1,70 @@ +createContainer(); + + $this->controller = new ErrorController($this->container); + } + + /** + * Test displaying error with a ShaarliFrontException: display exception message and use its code for HTTTP code + */ + public function testDisplayFrontExceptionError(): void + { + $request = $this->createMock(Request::class); + $response = new Response(); + + $message = 'error message'; + $errorCode = 418; + + // Save RainTPL assigned variables + $assignedVariables = []; + $this->assignTemplateVars($assignedVariables); + + $result = ($this->controller)( + $request, + $response, + new class($message, $errorCode) extends ShaarliFrontException {} + ); + + static::assertSame($errorCode, $result->getStatusCode()); + static::assertSame($message, $assignedVariables['message']); + static::assertArrayNotHasKey('stacktrace', $assignedVariables); + } + + /** + * Test displaying error with any exception (no debug): only display an error occurred with HTTP 500. + */ + public function testDisplayAnyExceptionErrorNoDebug(): void + { + $request = $this->createMock(Request::class); + $response = new Response(); + + // Save RainTPL assigned variables + $assignedVariables = []; + $this->assignTemplateVars($assignedVariables); + + $result = ($this->controller)($request, $response, new \Exception('abc')); + + static::assertSame(500, $result->getStatusCode()); + static::assertSame('An unexpected error occurred.', $assignedVariables['message']); + static::assertArrayNotHasKey('stacktrace', $assignedVariables); + } +}