From a773b055fc8ee3ae41a50e74542efa8b331b0602 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Wed, 21 Feb 2018 08:54:39 +0100 Subject: [PATCH 1/3] Make the middlewareDispatcher strict Signed-off-by: Roeland Jago Douma --- .../Middleware/MiddlewareDispatcher.php | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php b/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php index 60d2ae8b5c9..30efe815888 100644 --- a/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php +++ b/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php @@ -1,4 +1,5 @@ middlewares = array(); + $this->middlewares = []; $this->middlewareCounter = 0; } @@ -71,7 +72,7 @@ class MiddlewareDispatcher { * returns an array with all middleware elements * @return array the middlewares */ - public function getMiddlewares(){ + public function getMiddlewares(): array { return $this->middlewares; } @@ -87,7 +88,7 @@ class MiddlewareDispatcher { public function beforeController(Controller $controller, $methodName){ // we need to count so that we know which middlewares we have to ask in // case there is an exception - $middlewareCount = count($this->middlewares); + $middlewareCount = \count($this->middlewares); for($i = 0; $i < $middlewareCount; $i++){ $this->middlewareCounter++; $middleware = $this->middlewares[$i]; @@ -111,7 +112,7 @@ class MiddlewareDispatcher { * exception * @throws \Exception the passed in exception if it can't handle it */ - public function afterException(Controller $controller, $methodName, \Exception $exception){ + public function afterException(Controller $controller, string $methodName, \Exception $exception): Response { for($i=$this->middlewareCounter-1; $i>=0; $i--){ $middleware = $this->middlewares[$i]; try { @@ -134,8 +135,8 @@ class MiddlewareDispatcher { * @param Response $response the generated response from the controller * @return Response a Response object */ - public function afterController(Controller $controller, $methodName, Response $response){ - for($i=count($this->middlewares)-1; $i>=0; $i--){ + public function afterController(Controller $controller, string $methodName, Response $response): Response { + for($i= \count($this->middlewares)-1; $i>=0; $i--){ $middleware = $this->middlewares[$i]; $response = $middleware->afterController($controller, $methodName, $response); } @@ -153,8 +154,8 @@ class MiddlewareDispatcher { * @param string $output the generated output from a response * @return string the output that should be printed */ - public function beforeOutput(Controller $controller, $methodName, $output){ - for($i=count($this->middlewares)-1; $i>=0; $i--){ + public function beforeOutput(Controller $controller, string $methodName, string $output): string { + for($i= \count($this->middlewares)-1; $i>=0; $i--){ $middleware = $this->middlewares[$i]; $output = $middleware->beforeOutput($controller, $methodName, $output); } From ca9f364fd4746093a38cdcaf2456f5f86aebb89f Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Wed, 21 Feb 2018 10:54:17 +0100 Subject: [PATCH 2/3] Fix tests Signed-off-by: Roeland Jago Douma --- lib/private/AppFramework/Http/Dispatcher.php | 3 -- .../Middleware/MiddlewareDispatcher.php | 2 +- .../lib/AppFramework/Http/DispatcherTest.php | 28 +++++++++++-------- .../Middleware/MiddlewareDispatcherTest.php | 2 ++ 4 files changed, 19 insertions(+), 16 deletions(-) diff --git a/lib/private/AppFramework/Http/Dispatcher.php b/lib/private/AppFramework/Http/Dispatcher.php index 6219ba47a41..7b9ad015de6 100644 --- a/lib/private/AppFramework/Http/Dispatcher.php +++ b/lib/private/AppFramework/Http/Dispatcher.php @@ -105,9 +105,6 @@ class Dispatcher { } catch(\Exception $exception){ $response = $this->middlewareDispatcher->afterException( $controller, $methodName, $exception); - if ($response === null) { - throw $exception; - } } $response = $this->middlewareDispatcher->afterController( diff --git a/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php b/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php index 30efe815888..e1262b6c712 100644 --- a/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php +++ b/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php @@ -85,7 +85,7 @@ class MiddlewareDispatcher { * @param string $methodName the name of the method that will be called on * the controller */ - public function beforeController(Controller $controller, $methodName){ + public function beforeController(Controller $controller, string $methodName){ // we need to count so that we know which middlewares we have to ask in // case there is an exception $middlewareCount = \count($this->middlewares); diff --git a/tests/lib/AppFramework/Http/DispatcherTest.php b/tests/lib/AppFramework/Http/DispatcherTest.php index 95fa3c2a047..221e27e3b68 100644 --- a/tests/lib/AppFramework/Http/DispatcherTest.php +++ b/tests/lib/AppFramework/Http/DispatcherTest.php @@ -26,10 +26,12 @@ namespace Test\AppFramework\Http; use OC\AppFramework\Http\Dispatcher; use OC\AppFramework\Http\Request; +use OC\AppFramework\Middleware\MiddlewareDispatcher; use OC\AppFramework\Utility\ControllerMethodReflector; use OCP\AppFramework\Http; use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Http\DataResponse; +use OCP\AppFramework\Http\Response; use OCP\AppFramework\Controller; use OCP\IConfig; @@ -75,7 +77,9 @@ class TestController extends Controller { class DispatcherTest extends \Test\TestCase { + /** @var MiddlewareDispatcher */ private $middlewareDispatcher; + /** @var Dispatcher */ private $dispatcher; private $controllerMethod; private $response; @@ -120,14 +124,13 @@ class DispatcherTest extends \Test\TestCase { $this->reflector = new ControllerMethodReflector(); $this->dispatcher = new Dispatcher( - $this->http, $this->middlewareDispatcher, $this->reflector, + $this->http, + $this->middlewareDispatcher, + $this->reflector, $this->request ); - $this->response = $this->getMockBuilder( - '\OCP\AppFramework\Http\Response') - ->disableOriginalConstructor() - ->getMock(); + $this->response = $this->createMock(Response::class); $this->lastModified = new \DateTime(null, new \DateTimeZone('GMT')); $this->etag = 'hi'; @@ -162,7 +165,7 @@ class DispatcherTest extends \Test\TestCase { ->with($this->equalTo($this->controller), $this->equalTo($this->controllerMethod), $this->equalTo($exception)) - ->will($this->returnValue(null)); + ->willThrowException($exception); return; } } else { @@ -221,10 +224,9 @@ class DispatcherTest extends \Test\TestCase { public function testDispatcherReturnsArrayWith2Entries() { - $this->setMiddlewareExpectations(); + $this->setMiddlewareExpectations(''); - $response = $this->dispatcher->dispatch($this->controller, - $this->controllerMethod); + $response = $this->dispatcher->dispatch($this->controller, $this->controllerMethod); $this->assertNull($response[0]); $this->assertEquals(array(), $response[1]); $this->assertNull($response[2]); @@ -264,12 +266,14 @@ class DispatcherTest extends \Test\TestCase { public function testExceptionThrowsIfCanNotBeHandledByAfterException() { $out = 'yo'; $httpHeaders = 'Http'; - $responseHeaders = array('hell' => 'yeah'); + $responseHeaders = ['hell' => 'yeah']; $this->setMiddlewareExpectations($out, $httpHeaders, $responseHeaders, true, false); $this->expectException(\Exception::class); - $response = $this->dispatcher->dispatch($this->controller, - $this->controllerMethod); + $this->dispatcher->dispatch( + $this->controller, + $this->controllerMethod + ); } diff --git a/tests/lib/AppFramework/Middleware/MiddlewareDispatcherTest.php b/tests/lib/AppFramework/Middleware/MiddlewareDispatcherTest.php index b2f78c17fe6..e6c2c1b4f53 100644 --- a/tests/lib/AppFramework/Middleware/MiddlewareDispatcherTest.php +++ b/tests/lib/AppFramework/Middleware/MiddlewareDispatcherTest.php @@ -283,6 +283,8 @@ class MiddlewareDispatcherTest extends \Test\TestCase { ->method('beforeController'); $m3->expects($this->never()) ->method('afterController'); + $m3->method('beforeOutput') + ->will($this->returnArgument(2)); $this->dispatcher->registerMiddleware($m3); From 5825c27a120dbcf9c1fae80393b410c66696f9ed Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Wed, 21 Feb 2018 13:28:40 +0100 Subject: [PATCH 3/3] Make sure that render always returns a string Signed-off-by: Roeland Jago Douma --- lib/public/AppFramework/Http/Response.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/public/AppFramework/Http/Response.php b/lib/public/AppFramework/Http/Response.php index e47e01349e7..512b312dae1 100644 --- a/lib/public/AppFramework/Http/Response.php +++ b/lib/public/AppFramework/Http/Response.php @@ -232,11 +232,11 @@ class Response { /** * By default renders no output - * @return string|null + * @return string * @since 6.0.0 */ public function render() { - return null; + return ''; }