Merge pull request #8466 from nextcloud/strict_middlewaredipatcher

Make the middlewareDispatcher strict

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(

lib/private/AppFramework/Middleware/MiddlewareDispatcher.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *
@@ -30,7 +31,7 @@ namespace OC\AppFramework\Middleware;
 
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http\Response;
-use OCP\AppFramework\MiddleWare;
+use OCP\AppFramework\Middleware;
 
 /**
  * This class is used to store and run all the middleware in correct order
@@ -53,7 +54,7 @@ class MiddlewareDispatcher {
 	 * Constructor
 	 */
 	public function __construct(){
-		$this->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;
 	}
 
@@ -84,10 +85,10 @@ 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);
+		$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);
 		}

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 '';
 	}
 
 

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

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