Move action's return value check to Controller::invokeAction().

This allows adding return typehint to invokeAction() and make ActionDispatcher::_invoke() cleaner.

phpstan.neon

@@ -36,6 +36,7 @@ parameters:
         - '#Parameter \#1 \$table of method Cake\\ORM\\Query::addDefaultTypes\(\) expects Cake\\ORM\\Table, .+RepositoryInterface given.#'
         - '#Parameter \#1 \$table of method Cake\\ORM\\Query::_addAssociationsToTypeMap\(\) expects Cake\\ORM\\Table, .+RepositoryInterface given.#'
         - '#Parameter \#1 \$connection of method Cake\\Database\\Schema\\SqlGeneratorInterface::(create|drop|truncate)Sql\(\) expects Cake\\Database\\Connection, Cake\\Datasource\\ConnectionInterface given#'
+        - '#Property Cake\\Controller\\Controller::\$response \(Cake\\Http\\Response\) does not accept Psr\\Http\\Message\\ResponseInterface#'
         -
             message: '#Right side of && is always false#'
             path: 'src/Cache/Engine/MemcachedEngine.php'

src/Controller/Controller.php

@@ -29,10 +29,12 @@ use Cake\Log\LogTrait;
 use Cake\ORM\Locator\LocatorAwareTrait;
 use Cake\Routing\Router;
 use Cake\View\ViewVarsTrait;
+use Psr\Http\Message\ResponseInterface;
 use ReflectionClass;
 use ReflectionException;
 use ReflectionMethod;
 use RuntimeException;
+use UnexpectedValueException;
 
 /**
  * Application controller class for organization of business logic.
@@ -489,10 +491,11 @@ class Controller implements EventListenerInterface, EventDispatcherInterface
      * Dispatches the controller action. Checks that the action
      * exists and isn't private.
      *
-     * @return mixed The resulting response.
-     * @throws \ReflectionException
+     * @return \Psr\Http\Message\ResponseInterface The resulting response.
+     * @throws \Cake\Controller\Exception\MissingActionException If controller action is not found.
+     * @throws \UnexpectedValueException If return value of action method is not null or ResponseInterface instance.
      */
-    public function invokeAction()
+    public function invokeAction(): ?ResponseInterface
     {
         $request = $this->request;
 
@@ -504,15 +507,22 @@ class Controller implements EventListenerInterface, EventDispatcherInterface
                 'plugin' => $request->getParam('plugin'),
             ]);
         }
+
         /** @var callable $callable */
         $callable = [$this, $request->getParam('action')];
 
         $result = $callable(...array_values($request->getParam('pass')));
-        if ($result instanceof Response) {
-            $this->response = $result;
+        if ($result === null) {
+            return $result;
+        }
+
+        if (!$result instanceof ResponseInterface) {
+            throw new UnexpectedValueException(
+                'Controller actions can only return ResponseInterface instance or null.'
+            );
         }
 
-        return $result;
+        return $this->response = $result;
     }
 
     /**

src/Http/ActionDispatcher.php

@@ -18,7 +18,6 @@ namespace Cake\Http;
 
 use Cake\Controller\Controller;
 use Cake\Routing\Router;
-use LogicException;
 use Psr\Http\Message\ResponseInterface;
 
 /**
@@ -69,7 +68,6 @@ class ActionDispatcher
      *
      * @param \Cake\Controller\Controller $controller The controller to invoke.
      * @return \Psr\Http\Message\ResponseInterface The response
-     * @throws \LogicException If the controller action returns a non-response value.
      */
     protected function _invoke(Controller $controller): ResponseInterface
     {
@@ -79,11 +77,7 @@ class ActionDispatcher
         }
 
         $response = $controller->invokeAction();
-        if ($response !== null && !($response instanceof ResponseInterface)) {
-            throw new LogicException('Controller actions can only return Cake\Http\Response or null.');
-        }
-
-        if (!$response && $controller->isAutoRenderEnabled()) {
+        if ($response === null && $controller->isAutoRenderEnabled()) {
             $controller->render();
         }
 
@@ -91,10 +85,7 @@ class ActionDispatcher
         if ($result instanceof ResponseInterface) {
             return $result;
         }
-        if (!$response) {
-            $response = $controller->getResponse();
-        }
 
-        return $response;
+        return $controller->getResponse();
     }
 }

tests/TestCase/Controller/ControllerTest.php

@@ -757,6 +757,30 @@ class ControllerTest extends TestCase
     }
 
     /**
+     * test invalid return value from action method.
+     *
+     * @return void
+     */
+    public function testInvokeActionException()
+    {
+        $this->expectException(\UnexpectedValueException::class);
+        $this->expectExceptionMessage('Controller actions can only return ResponseInterface instance or null');
+
+        $url = new ServerRequest([
+            'url' => 'test/willCauseException',
+            'params' => [
+                'controller' => 'Test',
+                'action' => 'willCauseException',
+                'pass' => [],
+            ],
+        ]);
+        $response = $this->getMockBuilder(Response::class)->getMock();
+
+        $Controller = new TestController($url, $response);
+        $result = $Controller->invokeAction();
+    }
+
+    /**
      * test that a classes namespace is used in the viewPath.
      *
      * @return void

tests/TestCase/Http/ActionDispatcherTest.php

@@ -77,28 +77,6 @@ class ActionDispatcherTest extends TestCase
     }
 
     /**
-     * test invalid response from dispatch process.
-     *
-     * @return void
-     */
-    public function testDispatchInvalidResponse()
-    {
-        $this->expectException(\LogicException::class);
-        $this->expectExceptionMessage('Controller actions can only return Cake\Http\Response or null');
-        $req = new ServerRequest([
-            'url' => '/cakes',
-            'params' => [
-                'plugin' => null,
-                'controller' => 'Cakes',
-                'action' => 'invalid',
-                'pass' => [],
-            ],
-        ]);
-        $res = new Response();
-        $result = $this->dispatcher->dispatch($req, $res);
-    }
-
-    /**
      * Test dispatch with autorender
      *
      * @return void

tests/test_app/TestApp/Controller/TestController.php

@@ -69,6 +69,11 @@ class TestController extends ControllerTestAppController
         return $this->response->withStringBody('I am from the controller.');
     }
 
+    public function willCauseException()
+    {
+        return '';
+    }
+
     // phpcs:disable
     protected function protected_m()
     {