Integrate the new ActionDispatcher into Dispatcher

Reduce code duplication by integrating the new dispatcher with the old
one. This removes some protected methods, but that is within our
backwards compability guidelines.

I've also added a new Dispatcher.invokeController method to allow
IntegrationTests and other use cases to more easily access the
controller being used in a request. This will be useful long term when
we mainline the PSR7 dispatching.

src/Http/ActionDispatcher.php

@@ -78,14 +78,24 @@ class ActionDispatcher
      */
     public function dispatch(Request $request, Response $response)
     {
-        Router::pushRequest($request);
+        if (Router::getRequest(true) !== $request) {
+            Router::pushRequest($request);
+        }
         $beforeEvent = $this->dispatchEvent('Dispatcher.beforeDispatch', compact('request', 'response'));
 
         $request = $beforeEvent->data['request'];
         if ($beforeEvent->result instanceof Response) {
             return $beforeEvent->result;
         }
-        $controller = $this->factory->create($request, $response);
+
+        // Use the controller built by an beforeDispatch
+        // event handler if there is one.
+        if (isset($beforeEvent->data['controller'])) {
+            $controller = $beforeEvent->data['controller'];
+        } else {
+            $controller = $this->factory->create($request, $response);
+        }
+
         $response = $this->_invoke($controller);
         if (isset($request->params['return'])) {
             return $response;
@@ -104,6 +114,8 @@ class ActionDispatcher
      */
     protected function _invoke(Controller $controller)
     {
+        $this->dispatchEvent('Dispatcher.invokeController', ['controller' => $controller]);
+
         $result = $controller->startupProcess();
         if ($result instanceof Response) {
             return $result;

src/Routing/Dispatcher.php

@@ -17,6 +17,7 @@ namespace Cake\Routing;
 use Cake\Controller\Controller;
 use Cake\Event\EventDispatcherTrait;
 use Cake\Event\EventListenerInterface;
+use Cake\Http\ActionDispatcher;
 use Cake\Network\Request;
 use Cake\Network\Response;
 use LogicException;
@@ -58,69 +59,15 @@ class Dispatcher
      */
     public function dispatch(Request $request, Response $response)
     {
-        $beforeEvent = $this->dispatchEvent('Dispatcher.beforeDispatch', compact('request', 'response'));
-
-        $request = $beforeEvent->data['request'];
-        if ($beforeEvent->result instanceof Response) {
-            if (isset($request->params['return'])) {
-                return $beforeEvent->result->body();
-            }
-            $beforeEvent->result->send();
-            return null;
-        }
-
-        if (!isset($beforeEvent->data['controller'])) {
-            throw new LogicException(
-                'The Dispatcher.beforeDispatch event did not create a controller. ' .
-                'Ensure you have added the ControllerFactoryFilter.'
-            );
+        $actionDispatcher = new ActionDispatcher(null, $this->eventManager());
+        foreach ($this->_filters as $filter) {
+            $actionDispatcher->addFilter($filter);
         }
-        $controller = $beforeEvent->data['controller'];
-
-        $response = $this->_invoke($controller);
+        $response = $actionDispatcher->dispatch($request, $response);
         if (isset($request->params['return'])) {
             return $response->body();
         }
-
-        $afterEvent = $this->dispatchEvent('Dispatcher.afterDispatch', compact('request', 'response'));
-        $afterEvent->data['response']->send();
-    }
-
-    /**
-     * Initializes the components and models a controller will be using.
-     * Triggers the controller action and invokes the rendering if Controller::$autoRender
-     * is true. If a response object is returned by controller action that is returned
-     * else controller's $response property is returned.
-     *
-     * @param \Cake\Controller\Controller $controller Controller to invoke
-     * @return \Cake\Network\Response The resulting response object
-     * @throws \LogicException If data returned by controller action is not an
-     *   instance of Response
-     */
-    protected function _invoke(Controller $controller)
-    {
-        $result = $controller->startupProcess();
-        if ($result instanceof Response) {
-            return $result;
-        }
-
-        $response = $controller->invokeAction();
-        if ($response !== null && !($response instanceof Response)) {
-            throw new LogicException('Controller action can only return an instance of Response');
-        }
-
-        if (!$response && $controller->autoRender) {
-            $response = $controller->render();
-        } elseif (!$response) {
-            $response = $controller->response;
-        }
-
-        $result = $controller->shutdownProcess();
-        if ($result instanceof Response) {
-            return $result;
-        }
-
-        return $response;
+        return $response->send();
     }
 
     /**
@@ -136,7 +83,6 @@ class Dispatcher
     public function addFilter(EventListenerInterface $filter)
     {
         $this->_filters[] = $filter;
-        $this->eventManager()->on($filter);
     }
 
     /**

src/Routing/Filter/RoutingFilter.php

@@ -48,7 +48,9 @@ class RoutingFilter extends DispatcherFilter
     public function beforeDispatch(Event $event)
     {
         $request = $event->data['request'];
-        Router::setRequestInfo($request);
+        if (Router::getRequest(true) !== $request) {
+            Router::setRequestInfo($request);
+        }
 
         try {
             if (empty($request->params['controller'])) {

src/TestSuite/IntegrationTestCase.php

@@ -125,7 +125,6 @@ abstract class IntegrationTestCase extends TestCase
 
     /**
      *
-     *
      * @var null|string
      */
     protected $_cookieEncriptionKey = null;
@@ -357,7 +356,7 @@ abstract class IntegrationTestCase extends TestCase
         $response = new Response();
         $dispatcher = DispatcherFactory::create();
         $dispatcher->eventManager()->on(
-            'Dispatcher.beforeDispatch',
+            'Dispatcher.invokeController',
             ['priority' => 999],
             [$this, 'controllerSpy']
         );
@@ -379,15 +378,13 @@ abstract class IntegrationTestCase extends TestCase
      * Adds additional event spies to the controller/view event manager.
      *
      * @param \Cake\Event\Event $event A dispatcher event.
+     * @param \Cake\Controller\Controller $controller Controller instance.
      * @return void
      */
-    public function controllerSpy($event)
+    public function controllerSpy($event, $controller)
     {
-        if (empty($event->data['controller'])) {
-            return;
-        }
-        $this->_controller = $event->data['controller'];
-        $events = $this->_controller->eventManager();
+        $this->_controller = $controller;
+        $events = $controller->eventManager();
         $events->on('View.beforeRender', function ($event, $viewFile) {
             if (!$this->_viewName) {
                 $this->_viewName = $viewFile;

tests/TestCase/Routing/DispatcherTest.php

@@ -24,177 +24,6 @@ use Cake\Routing\Filter\ControllerFactoryFilter;
 use Cake\TestSuite\TestCase;
 
 /**
- * A testing stub that doesn't send headers.
- */
-class DispatcherMockResponse extends Response
-{
-
-    protected function _sendHeader($name, $value = null)
-    {
-        return $name . ' ' . $value;
-    }
-}
-
-/**
- * TestDispatcher class
- */
-class TestDispatcher extends Dispatcher
-{
-
-    /**
-     * Controller instance, made publicly available for testing
-     *
-     * @var Controller
-     */
-    public $controller;
-
-    /**
-     * invoke method
-     *
-     * @param \Cake\Controller\Controller $controller
-     * @return \Cake\Network\Response $response
-     */
-    protected function _invoke(Controller $controller)
-    {
-        $this->controller = $controller;
-        return parent::_invoke($controller);
-    }
-}
-
-/**
- * MyPluginAppController class
- *
- */
-class MyPluginAppController extends Controller
-{
-}
-
-/**
- * MyPluginController class
- *
- */
-class MyPluginController extends MyPluginAppController
-{
-
-    /**
-     * name property
-     *
-     * @var string
-     */
-    public $name = 'MyPlugin';
-
-    /**
-     * index method
-     *
-     * @return void
-     */
-    public function index()
-    {
-        return true;
-    }
-
-    /**
-     * add method
-     *
-     * @return void
-     */
-    public function add()
-    {
-        return true;
-    }
-
-    /**
-     * admin_add method
-     *
-     * @param mixed $id
-     * @return void
-     */
-    public function admin_add($id = null)
-    {
-        return $id;
-    }
-}
-
-/**
- * OtherPagesController class
- *
- */
-class OtherPagesController extends MyPluginAppController
-{
-
-    /**
-     * name property
-     *
-     * @var string
-     */
-    public $name = 'OtherPages';
-
-    /**
-     * display method
-     *
-     * @param string $page
-     * @return void
-     */
-    public function display($page = null)
-    {
-        return $page;
-    }
-
-    /**
-     * index method
-     *
-     * @return void
-     */
-    public function index()
-    {
-        return true;
-    }
-}
-
-/**
- * ArticlesTestAppController class
- *
- */
-class ArticlesTestAppController extends Controller
-{
-}
-
-/**
- * ArticlesTestController class
- *
- */
-class ArticlesTestController extends ArticlesTestAppController
-{
-
-    /**
-     * name property
-     *
-     * @var string
-     */
-    public $name = 'ArticlesTest';
-
-    /**
-     * admin_index method
-     *
-     * @return void
-     */
-    public function admin_index()
-    {
-        return true;
-    }
-
-    /**
-     * fake index method.
-     *
-     * @return void
-     */
-    public function index()
-    {
-        return true;
-    }
-}
-
-/**
  * DispatcherTest class
  *
  */
@@ -217,7 +46,7 @@ class DispatcherTest extends TestCase
         Configure::write('App.webroot', 'webroot');
         Configure::write('App.namespace', 'TestApp');
 
-        $this->dispatcher = new TestDispatcher();
+        $this->dispatcher = new Dispatcher();
         $this->dispatcher->addFilter(new ControllerFactoryFilter());
     }
 
@@ -330,14 +159,14 @@ class DispatcherTest extends TestCase
                 'controller' => 'Pages',
                 'action' => 'display',
                 'pass' => ['extract'],
-                'return' => 1
             ]
         ]);
         $response = $this->getMock('Cake\Network\Response');
+        $response->expects($this->once())
+            ->method('send');
 
-        $this->dispatcher->dispatch($url, $response);
-        $expected = 'Pages';
-        $this->assertEquals($expected, $this->dispatcher->controller->name);
+        $result = $this->dispatcher->dispatch($url, $response);
+        $this->assertNull($result);
     }
 
     /**
@@ -365,65 +194,6 @@ class DispatcherTest extends TestCase
     }
 
     /**
-     * testPrefixDispatch method
-     *
-     * @return void
-     */
-    public function testPrefixDispatch()
-    {
-        $request = new Request([
-            'url' => 'admin/posts/index',
-            'params' => [
-                'prefix' => 'Admin',
-                'controller' => 'Posts',
-                'action' => 'index',
-                'pass' => [],
-                'return' => 1
-            ]
-        ]);
-        $response = $this->getMock('Cake\Network\Response');
-
-        $this->dispatcher->dispatch($request, $response);
-
-        $this->assertInstanceOf(
-            'TestApp\Controller\Admin\PostsController',
-            $this->dispatcher->controller
-        );
-        $expected = '/admin/posts/index';
-        $this->assertSame($expected, $request->here);
-    }
-
-    /**
-     * test prefix dispatching in a plugin.
-     *
-     * @return void
-     */
-    public function testPrefixDispatchPlugin()
-    {
-        Plugin::load('TestPlugin');
-
-        $request = new Request([
-            'url' => 'admin/test_plugin/comments/index',
-            'params' => [
-                'plugin' => 'TestPlugin',
-                'prefix' => 'Admin',
-                'controller' => 'Comments',
-                'action' => 'index',
-                'pass' => [],
-                'return' => 1
-            ]
-        ]);
-        $response = $this->getMock('Cake\Network\Response');
-
-        $this->dispatcher->dispatch($request, $response);
-
-        $this->assertInstanceOf(
-            'TestPlugin\Controller\Admin\CommentsController',
-            $this->dispatcher->controller
-        );
-    }
-
-    /**
      * test forbidden controller names.
      *
      * @expectedException \Cake\Routing\Exception\MissingControllerException

tests/TestCase/Routing/RequestActionTraitTest.php

@@ -73,29 +73,36 @@ class RequestActionTraitTest extends TestCase
 
         $result = $this->object->requestAction('');
         $this->assertFalse($result);
+        $this->assertNull(Router::getRequest(), 'requests were not popped off the stack, this will break url generation');
 
         $result = $this->object->requestAction('/request_action/test_request_action');
         $expected = 'This is a test';
         $this->assertEquals($expected, $result);
+        $this->assertNull(Router::getRequest(), 'requests were not popped off the stack, this will break url generation');
 
         $result = $this->object->requestAction(Configure::read('App.fullBaseUrl') . '/request_action/test_request_action');
         $expected = 'This is a test';
         $this->assertEquals($expected, $result);
+        $this->assertNull(Router::getRequest(), 'requests were not popped off the stack, this will break url generation');
 
         $result = $this->object->requestAction('/request_action/another_ra_test/2/5');
         $expected = 7;
         $this->assertEquals($expected, $result);
+        $this->assertNull(Router::getRequest(), 'requests were not popped off the stack, this will break url generation');
 
         $result = $this->object->requestAction('/tests_apps/index', ['return']);
         $expected = 'This is the TestsAppsController index view ';
         $this->assertEquals($expected, $result);
+        $this->assertNull(Router::getRequest(), 'requests were not popped off the stack, this will break url generation');
 
         $result = $this->object->requestAction('/tests_apps/some_method');
         $expected = 5;
         $this->assertEquals($expected, $result);
+        $this->assertNull(Router::getRequest(), 'requests were not popped off the stack, this will break url generation');
 
         $result = $this->object->requestAction('/request_action/paginate_request_action');
         $this->assertNull($result);
+        $this->assertNull(Router::getRequest(), 'requests were not popped off the stack, this will break url generation');
 
         $result = $this->object->requestAction('/request_action/normal_request_action');
         $expected = 'Hello World';

tests/TestCase/TestSuite/IntegrationTestCaseTest.php

@@ -181,7 +181,9 @@ class IntegrationTestCaseTest extends IntegrationTestCase
     {
         $this->post('/posts/index');
         $this->assertInstanceOf('Cake\Controller\Controller', $this->_controller);
+        $this->assertNotEmpty($this->_viewName, 'View name not set');
         $this->assertContains('Template' . DS . 'Posts' . DS . 'index.ctp', $this->_viewName);
+        $this->assertNotEmpty($this->_layoutName, 'Layout name not set');
         $this->assertContains('Template' . DS . 'Layout' . DS . 'default.ctp', $this->_layoutName);
 
         $this->assertTemplate('index');