Cleaning up Controller::handleRequest

1. Separated responsibility of handleAction so that it no longer bootstraps the controller and cleans up after the request is handled.
2. NEW beforeHandleRequest to take responsibility of bootstrapping the controller
3. NEW afterHandleRequest to take responsibility of cleanup for the controller
4. NEW calling init on controllers deprecated in favour of callInit() which takes responsibility of enforcing that "base init" is called and the before and after hooks
5. NEW Added prepareResponse to Controller for dealing with responses from controllers
6. NEW setResponse added to controller for setting response objects on the controller

admin/code/LeftAndMain.php

@@ -355,7 +355,7 @@ class LeftAndMain extends Controller implements PermissionProvider {
 	 * @uses LeftAndMainExtension->accessedCMS()
 	 * @uses CMSMenu
 	 */
-	public function init() {
+	protected function init() {
 		parent::init();
 
 		Config::inst()->update('SSViewer', 'rewrite_hash_links', false);

admin/code/ModelAdmin.php

@@ -93,7 +93,7 @@ abstract class ModelAdmin extends LeftAndMain {
 	/**
 	 * Initialize the model admin interface. Sets up embedded jquery libraries and requisite plugins.
 	 */
-	public function init() {
+	protected function init() {
 		parent::init();
 
 		$models = $this->getManagedModels();

admin/code/SecurityAdmin.php

@@ -29,7 +29,7 @@ class SecurityAdmin extends LeftAndMain implements PermissionProvider {
 		'roles'
 	);
 
-	public function init() {
+	protected function init() {
 		parent::init();
 		Requirements::javascript(FRAMEWORK_ADMIN_DIR . '/client/dist/js/SecurityAdmin.js');
 	}

cli/CliController.php

@@ -15,7 +15,7 @@ abstract class CliController extends Controller {
 		'index'
 	);
 
-	public function init() {
+	protected function init() {
 		parent::init();
 		// Unless called from the command line, all CliControllers need ADMIN privileges
 		if(!Director::is_cli() && !Permission::check("ADMIN")) {
@@ -27,7 +27,7 @@ abstract class CliController extends Controller {
 		foreach(ClassInfo::subclassesFor($this->class) as $subclass) {
 			echo $subclass . "\n";
 			$task = new $subclass();
-			$task->init();
+			$task->doInit();
 			$task->process();
 		}
 	}

control/Controller.php

@@ -1,4 +1,5 @@
 <?php
+
 /**
  * Controllers are the cornerstone of all site functionality in SilverStripe. The {@link Director}
  * selects a controller to pass control to, and then calls {@link run()}. This method will execute
@@ -86,13 +87,38 @@ class Controller extends RequestHandler implements TemplateGlobalProvider {
 	 *
 	 * @uses BasicAuth::requireLogin()
 	 */
-	public function init() {
+	protected function init() {
 		if($this->basicAuthEnabled) BasicAuth::protect_site_if_necessary();
 
 		// This is used to test that subordinate controllers are actually calling parent::init() - a common bug
 		$this->baseInitCalled = true;
 	}
 
+	/**
+	 * A stand in function to protect the init function from failing to be called as well as providing before and
+	 * after hooks for the init function itself
+	 *
+	 * This should be called on all controllers before handling requests
+	 */
+	public function doInit() {
+		//extension hook
+		$this->extend('onBeforeInit');
+
+		// Safety call
+		$this->baseInitCalled = false;
+		$this->init();
+		if (!$this->baseInitCalled) {
+			user_error(
+				"init() method on class '$this->class' doesn't call Controller::init()."
+				. "Make sure that you have parent::init() included.",
+				E_USER_WARNING
+			);
+		}
+
+		$this->extend('onAfterInit');
+
+	}
+
 	/**
 	 * Returns a link to this controller. Overload with your own Link rules if they exist.
 	 *
@@ -102,28 +128,61 @@ class Controller extends RequestHandler implements TemplateGlobalProvider {
 		return get_class($this) .'/';
 	}
 
+	/**
+	 * {@inheritdoc}
+	 *
+	 * Also set the URLParams
+	 */
+	public function setRequest($request) {
+		$return = parent::setRequest($request);
+		$this->setURLParams($this->getRequest()->allParams());
+
+		return $return;
+	}
+
+	/**
+	 * A bootstrap for the handleRequest method
+	 *
+	 * @todo setDataModel and setRequest are redundantly called in parent::handleRequest() - sort this out
+	 *
+	 * @param SS_HTTPRequest $request
+	 * @param DataModel $model
+	 */
+	protected function beforeHandleRequest(SS_HTTPRequest $request, DataModel $model) {
+		//Push the current controller to protect against weird session issues
+		$this->pushCurrent();
+		//Set up the internal dependencies (request, response, datamodel)
+		$this->setRequest($request);
+		$this->setResponse(new SS_HTTPResponse());
+		$this->setDataModel($model);
+		//kick off the init functionality
+		$this->doInit();
+
+	}
+
+	/**
+	 * Cleanup for the handleRequest method
+	 */
+	protected function afterHandleRequest() {
+		//Pop the current controller from the stack
+		$this->popCurrent();
+	}
+
 	/**
 	 * Executes this controller, and return an {@link SS_HTTPResponse} object with the result.
 	 *
-	 * This method first does a few set-up activities:
-	 * - Push this controller ont to the controller stack - see {@link Controller::curr()} for
-	 *   information about this.
-	 * - Call {@link init()}
-	 * - Defer to {@link RequestHandler->handleRequest()} to determine which action should be executed.
+	 * This method defers to {@link RequestHandler->handleRequest()} to determine which action
+	 *    should be executed
 	 *
-	 * Note: $requestParams['executeForm'] support was removed, make the following change in your URLs:
-	 * "/?executeForm=FooBar" -> "/FooBar".
+	 * Note: You should rarely need to overload handleRequest() -
+	 * this kind of change is only really appropriate for things like nested
+	 * controllers - {@link ModelAsController} and {@link RootURLController}
+	 * are two examples here.  If you want to make more
+	 * orthodox functionality, it's better to overload {@link init()} or {@link index()}.
 	 *
-	 * Also make sure "FooBar" is in the $allowed_actions of your controller class.
-	 *
-	 * Note: You should rarely need to overload run() - this kind of change is only really appropriate
-	 * for things like nested controllers - {@link ModelAsController} and {@link RootURLController}
-	 * are two examples here. If you want to make more orthodox functionality, it's better to overload
-	 * {@link init()} or {@link index()}.
-	 *
-	 * Important: If you are going to overload handleRequest, make sure that you start the method with
-	 * $this->pushCurrent() and end the method with $this->popCurrent(). Failure to do this will create
-	 * weird session errors.
+	 * Important: If you are going to overload handleRequest,
+	 * make sure that you start the method with $this->beforeHandleRequest()
+	 * and end the method with $this->afterHandleRequest()
 	 *
 	 * @param SS_HTTPRequest $request
 	 * @param DataModel $model
@@ -131,60 +190,68 @@ class Controller extends RequestHandler implements TemplateGlobalProvider {
 	 * @return SS_HTTPResponse
 	 */
 	public function handleRequest(SS_HTTPRequest $request, DataModel $model) {
-		if(!$request) user_error("Controller::handleRequest() not passed a request!", E_USER_ERROR);
-
-		$this->pushCurrent();
-		$this->urlParams = $request->allParams();
-		$this->setRequest($request);
-		$this->getResponse();
-		$this->setDataModel($model);
-
-		$this->extend('onBeforeInit');
-
-		// Init
-		$this->baseInitCalled = false;
-		$this->init();
-		if(!$this->baseInitCalled) {
-			user_error("init() method on class '$this->class' doesn't call Controller::init()."
-				. "Make sure that you have parent::init() included.", E_USER_WARNING);
+		if (!$request) {
+			user_error("Controller::handleRequest() not passed a request!", E_USER_ERROR);
 		}
 
-		$this->extend('onAfterInit');
+		//set up the controller for the incoming request
+		$this->beforeHandleRequest($request, $model);
 
-		$response = $this->getResponse();
-		// If we had a redirection or something, halt processing.
-		if($response->isFinished()) {
-			$this->popCurrent();
-			return $response;
+		//if the before handler manipulated the response in a way that we shouldn't proceed, then skip our request
+		// handling
+		if (!$this->getResponse()->isFinished()) {
+
+			//retrieve the response for the request
+			$response = parent::handleRequest($request, $model);
+
+			//prepare the response (we can receive an assortment of response types (strings/objects/HTTPResponses)
+			$this->prepareResponse($response);
 		}
 
-		$body = parent::handleRequest($request, $model);
-		if($body instanceof SS_HTTPResponse) {
-			if(isset($_REQUEST['debug_request'])) {
-				Debug::message("Request handler returned SS_HTTPResponse object to $this->class controller;"
-					. "returning it without modification.");
+		//after request work
+		$this->afterHandleRequest();
+
+		//return the response
+		return $this->getResponse();
+	}
+
+	/**
+	 * Prepare the response (we can receive an assortment of response types (strings/objects/HTTPResponses) and
+	 * changes the controller response object appropriately
+	 *
+	 * @param $response
+	 */
+	protected function prepareResponse($response) {
+		if ($response instanceof SS_HTTPResponse) {
+			if (isset($_REQUEST['debug_request'])) {
+				Debug::message(
+					"Request handler returned SS_HTTPResponse object to $this->class controller;"
+					. "returning it without modification."
+				);
 			}
-			$response = $body;
 			$this->setResponse($response);
 
-		} else {
-			if($body instanceof Object && $body->hasMethod('getViewer')) {
-				if(isset($_REQUEST['debug_request'])) {
-					Debug::message("Request handler $body->class object to $this->class controller;"
-						. "rendering with template returned by $body->class::getViewer()");
+		}
+		else {
+			if ($response instanceof Object && $response->hasMethod('getViewer')) {
+				if (isset($_REQUEST['debug_request'])) {
+					Debug::message(
+						"Request handler $response->class object to $this->class controller;"
+						. "rendering with template returned by $response->class::getViewer()"
+					);
 				}
-				$body = $body->getViewer($this->getAction())->process($body);
+				$response = $response->getViewer($this->getAction())->process($response);
 			}
 
-			$response->setBody($body);
+			$this->getResponse()->setbody($response);
+
 		}
 
+		//deal with content if appropriate
+		ContentNegotiator::process($this->getResponse());
 
-		ContentNegotiator::process($response);
-		HTTP::add_cache_headers($response);
-
-		$this->popCurrent();
-		return $response;
+		//add cache headers
+		HTTP::add_cache_headers($this->getResponse());
 	}
 
 	/**
@@ -236,6 +303,7 @@ class Controller extends RequestHandler implements TemplateGlobalProvider {
 	 */
 	public function setURLParams($urlParams) {
 		$this->urlParams = $urlParams;
+		return $this;
 	}
 
 	/**

control/RequestHandler.php

@@ -194,7 +194,7 @@ class RequestHandler extends ViewableData {
 			if(!$this->hasAction($action)) {
 				return $this->httpError(404, "Action '$action' isn't available $classMessage.");
 			}
-			if(!$this->checkAccessAction($action) || in_array(strtolower($action), array('run', 'init'))) {
+			if(!$this->checkAccessAction($action) || in_array(strtolower($action), array('run', 'doInit'))) {
 				return $this->httpError(403, "Action '$action' isn't allowed $classMessage.");
 			}
 			$result = $this->handleAction($request, $action);
@@ -376,7 +376,7 @@ class RequestHandler extends ViewableData {
 			Config::UNINHERITED | Config::EXCLUDE_EXTRA_SOURCES
 		);
 		if(!is_array($actions) || !$actionsWithoutExtra) {
-			if($action != 'init' && $action != 'run' && method_exists($this, $action)) return true;
+			if($action != 'doInit' && $action != 'run' && method_exists($this, $action)) return true;
 		}
 
 		return false;
@@ -493,8 +493,10 @@ class RequestHandler extends ViewableData {
 	 * or {@link handleRequest()}, but in some based we want to set it manually.
 	 *
 	 * @param SS_HTTPRequest
+	 * @return $this
 	 */
 	public function setRequest($request) {
 		$this->request = $request;
+		return $this;
 	}
 }

dev/DevelopmentAdmin.php

@@ -29,7 +29,7 @@ class DevelopmentAdmin extends Controller {
 		'generatesecuretoken',
 	);
 
-	public function init() {
+	protected function init() {
 		parent::init();
 
 		// Special case for dev/build: Defer permission checks to DatabaseAdmin->init() (see #4957)

dev/SapphireInfo.php

@@ -11,7 +11,7 @@ class SapphireInfo extends Controller {
 		'environmenttype',
 	);
 
-	public function init() {
+	protected function init() {
 		parent::init();
 		if(!Director::is_cli() && !Permission::check('ADMIN')) return Security::permissionFailure();
 	}

dev/TaskRunner.php

@@ -15,7 +15,7 @@ class TaskRunner extends Controller {
 		'runTask',
 	);
 
-	public function init() {
+	protected function init() {
 		parent::init();
 
 		$isRunningTests = (class_exists('SapphireTest', false) && SapphireTest::is_running_test());

model/DatabaseAdmin.php

@@ -21,7 +21,7 @@ class DatabaseAdmin extends Controller {
 		'import'
 	);
 
-	public function init() {
+	protected function init() {
 		parent::init();
 
 		// We allow access to this controller regardless of live-status or ADMIN permission only

security/CMSSecurity.php

@@ -24,7 +24,7 @@ class CMSSecurity extends Security {
 	 */
 	private static $reauth_enabled = true;
 
-	public function init() {
+	protected function init() {
 		parent::init();
 
 		// Include CMS styles and js

security/Security.php

@@ -304,7 +304,7 @@ class Security extends Controller implements TemplateGlobalProvider {
 		);
 	}
 
-	public function init() {
+	protected function init() {
 		parent::init();
 
 		// Prevent clickjacking, see https://developer.mozilla.org/en-US/docs/HTTP/X-Frame-Options
@@ -450,7 +450,7 @@ class Security extends Controller implements TemplateGlobalProvider {
 
 		$controller = Page_Controller::create($tmpPage);
 		$controller->setDataModel($this->model);
-		$controller->init();
+		$controller->doInit();
 		return $controller;
 	}
 

tests/FakeController.php

@@ -20,6 +20,6 @@ class FakeController extends Controller {
 
 		$this->setResponse(new SS_HTTPResponse());
 
-		$this->init();
+		$this->doInit();
 	}
 }

tests/security/BasicAuthTest.php

@@ -133,7 +133,7 @@ class BasicAuthTest_ControllerSecuredWithPermission extends Controller implement
 
 	protected $template = 'BlankPage';
 
-	public function init() {
+	protected function init() {
 		self::$post_init_called = false;
 		self::$index_called = false;
 
@@ -155,7 +155,7 @@ class BasicAuthTest_ControllerSecuredWithoutPermission extends Controller implem
 
 	protected $template = 'BlankPage';
 
-	public function init() {
+	protected function init() {
 		BasicAuth::protect_entire_site(true, null);
 		parent::init();
 	}