From e5f1ca3bbe83673e2c4f6cc0310f5c1404933533 Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Mon, 23 Feb 2015 13:46:00 +0000 Subject: [PATCH] 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 | 2 +- admin/code/ModelAdmin.php | 2 +- admin/code/SecurityAdmin.php | 2 +- cli/CliController.php | 4 +- control/Controller.php | 184 +++++++++++++++++++++---------- control/RequestHandler.php | 6 +- dev/DevelopmentAdmin.php | 2 +- dev/SapphireInfo.php | 2 +- dev/TaskRunner.php | 2 +- model/DatabaseAdmin.php | 2 +- security/CMSSecurity.php | 2 +- security/Security.php | 4 +- tests/FakeController.php | 2 +- tests/security/BasicAuthTest.php | 4 +- 14 files changed, 145 insertions(+), 75 deletions(-) diff --git a/admin/code/LeftAndMain.php b/admin/code/LeftAndMain.php index 43f5db89e..6c5e34888 100644 --- a/admin/code/LeftAndMain.php +++ b/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); diff --git a/admin/code/ModelAdmin.php b/admin/code/ModelAdmin.php index 158f67b7d..b3263be60 100644 --- a/admin/code/ModelAdmin.php +++ b/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(); diff --git a/admin/code/SecurityAdmin.php b/admin/code/SecurityAdmin.php index fdd7d0cca..562fdf407 100755 --- a/admin/code/SecurityAdmin.php +++ b/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'); } diff --git a/cli/CliController.php b/cli/CliController.php index 671f82e39..790823b99 100644 --- a/cli/CliController.php +++ b/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(); } } diff --git a/control/Controller.php b/control/Controller.php index d198bc30c..54a3e20d1 100644 --- a/control/Controller.php +++ b/control/Controller.php @@ -1,4 +1,5 @@ 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; } /** diff --git a/control/RequestHandler.php b/control/RequestHandler.php index 6f66a0f0b..019bfac2f 100644 --- a/control/RequestHandler.php +++ b/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; } } diff --git a/dev/DevelopmentAdmin.php b/dev/DevelopmentAdmin.php index ff60a3993..7d7a52fe4 100644 --- a/dev/DevelopmentAdmin.php +++ b/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) diff --git a/dev/SapphireInfo.php b/dev/SapphireInfo.php index 73b28a6b3..50e197fa4 100644 --- a/dev/SapphireInfo.php +++ b/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(); } diff --git a/dev/TaskRunner.php b/dev/TaskRunner.php index 0c7bba1dd..c596ee6f8 100644 --- a/dev/TaskRunner.php +++ b/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()); diff --git a/model/DatabaseAdmin.php b/model/DatabaseAdmin.php index 5befb3f6f..31457bfb9 100644 --- a/model/DatabaseAdmin.php +++ b/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 diff --git a/security/CMSSecurity.php b/security/CMSSecurity.php index 577cfd259..ae80a11bc 100644 --- a/security/CMSSecurity.php +++ b/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 diff --git a/security/Security.php b/security/Security.php index 31822dfd2..b82486540 100644 --- a/security/Security.php +++ b/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; } diff --git a/tests/FakeController.php b/tests/FakeController.php index b54af6a6e..abdc8bf02 100644 --- a/tests/FakeController.php +++ b/tests/FakeController.php @@ -20,6 +20,6 @@ class FakeController extends Controller { $this->setResponse(new SS_HTTPResponse()); - $this->init(); + $this->doInit(); } } diff --git a/tests/security/BasicAuthTest.php b/tests/security/BasicAuthTest.php index 517e447ee..cfc69a1b6 100644 --- a/tests/security/BasicAuthTest.php +++ b/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(); }