Merge pull request #3926 from dhensby/pulls/handle-request-cleanup

Cleaning up Controller::handleRequest

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

docs/en/changelogs/4.0.0.md

@@ -0,0 +1,98 @@
+# 4.0.0 (unreleased)
+
+## Overview
+
+### Framework
+
+ * Deprecate `SQLQuery` in favour `SQLSelect`
+ * `DataList::filter` by null now internally generates "IS NULL" or "IS NOT NULL" conditions appropriately on queries
+ * `Controller::init` visibility changed to protected
+ * Initialising controllers now the responsibility of `Controller::doInit()`
+ * `handleRequest`
+
+## Upgrading
+
+### Update code that uses SQLQuery
+
+SQLQuery is still implemented, but now extends the new SQLSelect class and has some methods
+deprecated. Previously this class was used for both selecting and deleting, but these
+have been superceded by the specialised SQLSelect and SQLDelete classes.
+
+Take care for any code or functions which expect an object of type `SQLQuery`, as
+these references should be replaced with `SQLSelect`. Legacy code which generates
+`SQLQuery` can still communicate with new code that expects `SQLSelect` as it is a
+subclass of `SQLSelect`, but the inverse is not true.
+
+### Update implementations of augmentSQL
+
+Since this method now takes a `SQLSelect` as a first parameter, existing code referencing the deprecated `SQLQuery`
+type will raise a PHP error.
+
+E.g.
+
+Before:
+
+	:::php
+	function augmentSQL(SQLQuery &$query, DataQuery &$dataQuery = null) {
+		$locale = Translatable::get_current_locale();
+		if(!preg_match('/("|\'|`)Locale("|\'|`)/', implode(' ', $query->getWhere())))  {
+			$qry = sprintf('"Locale" = \'%s\'', Convert::raw2sql($locale));
+			$query->addWhere($qry); 
+		}
+	}
+
+After:
+
+	:::php
+	function augmentSQL(SQLSelect $query, DataQuery $dataQuery = null) {
+		$locale = Translatable::get_current_locale();
+		if(!preg_match('/("|\'|`)Locale("|\'|`)/', implode(' ', $query->getWhereParameterised($parameters))))  {
+			$query->addWhere(array(
+				'"Locale"' => $locale
+			));
+		}
+	}
+
+
+### Update to Controller initialisation
+
+`Controller::init` is now protected, so any overloaded instances of this method need to have their visibility changed to
+`protected`
+
+Before:
+
+	:::php
+	public function init() {
+		parent::init();
+		
+		if (!Member::currentMemberID()) {
+			Security::permissionFailure($this);
+		}
+	}
+
+After:
+
+	:::php
+	protected function init() {
+		parent::init();
+		
+		if (!Member::currentMemberID()) {
+			Security::permissionFailure($this);
+		}
+	}
+
+Initialising controllers now need to be done with the `doInit` method.
+
+Before:
+
+	:::php
+	
+	$controller = ModelAsController::controller_for(Page::get()->first());
+	$controller->init();
+
+After:
+
+	:::php
+	
+	$controller = ModelAsController::controller_for(Page::get()->first());
+	$controller->doInit();

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