diff --git a/admin/code/CMSBatchActionHandler.php b/admin/code/CMSBatchActionHandler.php index 7b1c4c001..706495041 100644 --- a/admin/code/CMSBatchActionHandler.php +++ b/admin/code/CMSBatchActionHandler.php @@ -13,7 +13,7 @@ class CMSBatchActionHandler extends RequestHandler { static $url_handlers = array( '$BatchAction/applicablepages' => 'handleApplicablePages', '$BatchAction/confirmation' => 'handleConfirmation', - '$BatchAction' => 'handleAction', + '$BatchAction' => 'handleBatchAction', ); protected $parentController; @@ -66,7 +66,7 @@ class CMSBatchActionHandler extends RequestHandler { return Controller::join_links($this->parentController->Link(), $this->urlSegment); } - public function handleAction($request) { + public function handleBatchAction($request) { // This method can't be called without ajax. if(!$request->isAjax()) { $this->parentController->redirectBack(); diff --git a/control/Controller.php b/control/Controller.php index 47066074f..e722090a8 100644 --- a/control/Controller.php +++ b/control/Controller.php @@ -182,36 +182,25 @@ class Controller extends RequestHandler implements TemplateGlobalProvider { * Controller's default action handler. It will call the method named in $Action, if that method exists. * If $Action isn't given, it will use "index" as a default. */ - public function handleAction($request) { - // urlParams, requestParams, and action are set for backward compatability + public function handleAction($request, $action) { foreach($request->latestParams() as $k => $v) { if($v || !isset($this->urlParams[$k])) $this->urlParams[$k] = $v; } - $this->action = str_replace("-","_",$request->param('Action')); + $this->action = $action; $this->requestParams = $request->requestVars(); - if(!$this->action) $this->action = 'index'; - - if(!$this->hasAction($this->action)) { - $this->httpError(404, "The action '$this->action' does not exist in class $this->class"); - } - - // run & init are manually disabled, because they create infinite loops and other dodgy situations - if(!$this->checkAccessAction($this->action) || in_array(strtolower($this->action), array('run', 'init'))) { - return $this->httpError(403, "Action '$this->action' isn't allowed on class $this->class"); - } - - if($this->hasMethod($this->action)) { - $result = $this->{$this->action}($request); - + + if($this->hasMethod($action)) { + $result = parent::handleAction($request, $action); + // If the action returns an array, customise with it before rendering the template. if(is_array($result)) { - return $this->getViewer($this->action)->process($this->customise($result)); + return $this->getViewer($action)->process($this->customise($result)); } else { return $result; } } else { - return $this->getViewer($this->action)->process($this); + return $this->getViewer($action)->process($this); } } diff --git a/control/HTTPRequest.php b/control/HTTPRequest.php index 89a883058..129905083 100644 --- a/control/HTTPRequest.php +++ b/control/HTTPRequest.php @@ -610,7 +610,7 @@ class SS_HTTPRequest implements ArrayAccess { for($i=0;$i<$count;$i++) { $value = array_shift($this->dirParts); - if(!$value) break; + if($value === null) break; $return[] = $value; } diff --git a/control/RequestHandler.php b/control/RequestHandler.php index f84f49298..12e50be04 100644 --- a/control/RequestHandler.php +++ b/control/RequestHandler.php @@ -145,89 +145,141 @@ class RequestHandler extends ViewableData { $this->request = $request; $this->setDataModel($model); + + $match = $this->findAction($request); + + // If nothing matches, return this object + if (!$match) return $this; + + // Start to find what action to call. Start by using what findAction returned + $action = $match['action']; + + // We used to put "handleAction" as the action on controllers, but (a) this could only be called when + // you had $Action in your rule, and (b) RequestHandler didn't have one. $Action is better + if ($action == 'handleAction') { + Deprecation::notice('3.2.0', 'Calling handleAction directly is deprecated - use $Action instead'); + $action = '$Action'; + } + + // Actions can reference URL parameters, eg, '$Action/$ID/$OtherID' => '$Action', + if($action[0] == '$') { + $action = str_replace("-", "_", $request->latestParam(substr($action,1))); + } + + if(!$action) { + if(isset($_REQUEST['debug_request'])) { + Debug::message("Action not set; using default action method name 'index'"); + } + $action = "index"; + } else if(!is_string($action)) { + user_error("Non-string method name: " . var_export($action, true), E_USER_ERROR); + } + + $className = get_class($this); + + if(!$this->hasAction($action)) { + return new SS_HTTPResponse("Action '$action' isn't available on class $className.", 404); + } + + if(!$this->checkAccessAction($action) || in_array(strtolower($action), array('run', 'init'))) { + return new SS_HTTPResponse("Action '$action' isn't allowed on class $className.", 403); + } + + try { + $result = $this->handleAction($request, $action); + } + catch (SS_HTTPResponse_Exception $e) { + return $e->getResponse(); + } + catch(PermissionFailureException $e) { + $result = Security::permissionFailure(null, $e->getMessage()); + } + + if($result instanceof SS_HTTPResponse && $result->isError()) { + if(isset($_REQUEST['debug_request'])) Debug::message("Rule resulted in HTTP error; breaking"); + return $result; + } + + // If we return a RequestHandler, call handleRequest() on that, even if there is no more URL to + // parse. It might have its own handler. However, we only do this if we haven't just parsed an + // empty rule ourselves, to prevent infinite loops. Also prevent further handling of controller + // actions which return themselves to avoid infinite loops. + $matchedRuleWasEmpty = $request->isEmptyPattern($match['rule']); + $resultIsRequestHandler = is_object($result) && $result instanceof RequestHandler; + if($this !== $result && !$matchedRuleWasEmpty && $resultIsRequestHandler) { + $returnValue = $result->handleRequest($request, $model); + + // Array results can be used to handle + if(is_array($returnValue)) $returnValue = $this->customise($returnValue); + + return $returnValue; + + // If we return some other data, and all the URL is parsed, then return that + } else if($request->allParsed()) { + return $result; + + // But if we have more content on the URL and we don't know what to do with it, return an error. + } else { + return $this->httpError(404, "I can't handle sub-URLs of a $this->class object."); + } + + return $this; + } + + protected function findAction($request) { + $handlerClass = ($this->class) ? $this->class : get_class($this); + // We stop after RequestHandler; in other words, at ViewableData while($handlerClass && $handlerClass != 'ViewableData') { - $urlHandlers = Config::inst()->get($handlerClass, 'url_handlers', Config::FIRST_SET); + $urlHandlers = Config::inst()->get($handlerClass, 'url_handlers', Config::UNINHERITED); if($urlHandlers) foreach($urlHandlers as $rule => $action) { if(isset($_REQUEST['debug_request'])) { Debug::message("Testing '$rule' with '" . $request->remaining() . "' on $this->class"); } - if($params = $request->match($rule, true)) { - // Backwards compatible setting of url parameters, please use SS_HTTPRequest->latestParam() instead - //Director::setUrlParams($request->latestParams()); - + + if($request->match($rule, true)) { if(isset($_REQUEST['debug_request'])) { - Debug::message("Rule '$rule' matched to action '$action' on $this->class." - . " Latest request params: " . var_export($request->latestParams(), true)); + Debug::message( + "Rule '$rule' matched to action '$action' on $this->class. ". + "Latest request params: " . var_export($request->latestParams(), true) + ); } - - // Actions can reference URL parameters, eg, '$Action/$ID/$OtherID' => '$Action', - if($action[0] == '$') $action = $params[substr($action,1)]; - - if($this->checkAccessAction($action)) { - if(!$action) { - if(isset($_REQUEST['debug_request'])) { - Debug::message("Action not set; using default action method name 'index'"); - } - $action = "index"; - } else if(!is_string($action)) { - user_error("Non-string method name: " . var_export($action, true), E_USER_ERROR); - } - - try { - if(!$this->hasMethod($action)) { - return $this->httpError(404, "Action '$action' isn't available on class " - . get_class($this) . "."); - } - $result = $this->$action($request); - } catch(SS_HTTPResponse_Exception $responseException) { - $result = $responseException->getResponse(); - } catch(PermissionFailureException $e) { - $result = Security::permissionFailure(null, $e->getMessage()); - } - } else { - return $this->httpError(403, "Action '$action' isn't allowed on class " . get_class($this)); - } - - if($result instanceof SS_HTTPResponse && $result->isError()) { - if(isset($_REQUEST['debug_request'])) Debug::message("Rule resulted in HTTP error; breaking"); - return $result; - } - - // If we return a RequestHandler, call handleRequest() on that, even if there is no more URL to - // parse. It might have its own handler. However, we only do this if we haven't just parsed an - // empty rule ourselves, to prevent infinite loops. Also prevent further handling of controller - // actions which return themselves to avoid infinite loops. - if($this !== $result && !$request->isEmptyPattern($rule) && is_object($result) - && $result instanceof RequestHandler) { - $returnValue = $result->handleRequest($request, $model); - - // Array results can be used to handle - if(is_array($returnValue)) $returnValue = $this->customise($returnValue); - - return $returnValue; - - // If we return some other data, and all the URL is parsed, then return that - } else if($request->allParsed()) { - return $result; - - // But if we have more content on the URL and we don't know what to do with it, return an error. - } else { - return $this->httpError(404, "I can't handle sub-URLs of a $this->class object."); - } - - return $this; + return array('rule' => $rule, 'action' => $action); } } - + $handlerClass = get_parent_class($handlerClass); } - - // If nothing matches, return this object - return $this; + } + + /** + * Given a request, and an action name, call that action name on this RequestHandler + * + * Must not raise SS_HTTPResponse_Exceptions - instead it should return + * + * @param $request + * @param $action + * @return SS_HTTPResponse + */ + protected function handleAction($request, $action) { + $className = get_class($this); + + if(!$this->hasMethod($action)) { + return new SS_HTTPResponse("Action '$action' isn't available on class $className.", 404); + } + + $res = $this->extend('beforeCallActionHandler', $request, $action); + if ($res) return reset($res); + + $actionRes = $this->$action($request); + + $res = $this->extend('afterCallActionHandler', $request, $action); + if ($res) return reset($res); + + return $actionRes; } /** diff --git a/forms/gridfield/GridField.php b/forms/gridfield/GridField.php index 25fc90dae..996763ea4 100644 --- a/forms/gridfield/GridField.php +++ b/forms/gridfield/GridField.php @@ -612,7 +612,7 @@ class GridField extends FormField { $stateChange = Session::get($id); $actionName = $stateChange['actionName']; $args = isset($stateChange['args']) ? $stateChange['args'] : array(); - $html = $this->handleAction($actionName, $args, $data); + $html = $this->handleAlterAction($actionName, $args, $data); // A field can optionally return its own HTML if($html) return $html; } @@ -642,7 +642,7 @@ class GridField extends FormField { * @return type * @throws InvalidArgumentException */ - public function handleAction($actionName, $args, $data) { + public function handleAlterAction($actionName, $args, $data) { $actionName = strtolower($actionName); foreach($this->getComponents() as $component) { if(!($component instanceof GridField_ActionProvider)) { diff --git a/tests/control/RequestHandlingTest.php b/tests/control/RequestHandlingTest.php index a17b25655..ef011565d 100644 --- a/tests/control/RequestHandlingTest.php +++ b/tests/control/RequestHandlingTest.php @@ -65,17 +65,13 @@ class RequestHandlingTest extends FunctionalTest { } public function testBadBase() { - /* Without a double-slash indicator in the URL, the entire URL is popped off the stack. The controller's - * default action handlers have been designed for this to an extend: simple actions can still be called. - * This is the set-up of URL rules written before this new request handler. */ + /* We no longer support using hacky attempting to handle URL parsing with broken rules */ $response = Director::test("testBadBase/method/1/2"); - $this->assertEquals("This is a method on the controller: 1, 2", $response->getBody()); + $this->assertNotEquals("This is a method on the controller: 1, 2", $response->getBody()); $response = Director::test("testBadBase/TestForm", array("MyField" => 3), null, "POST"); - $this->assertEquals("Form posted", $response->getBody()); + $this->assertNotEquals("Form posted", $response->getBody()); - /* It won't, however, let you chain requests to access methods on forms, or form fields. In order to do that, - * you need to have a // marker in your URL parsing rule */ $response = Director::test("testBadBase/TestForm/fields/MyField"); $this->assertNotEquals("MyField requested", $response->getBody()); } diff --git a/tests/forms/GridFieldTest.php b/tests/forms/GridFieldTest.php index 8b8028133..8047a3491 100644 --- a/tests/forms/GridFieldTest.php +++ b/tests/forms/GridFieldTest.php @@ -239,7 +239,7 @@ class GridFieldTest extends SapphireTest { public function testHandleActionBadArgument() { $this->setExpectedException('InvalidArgumentException'); $obj = new GridField('testfield', 'testfield'); - $obj->handleAction('prft', array(), array()); + $obj->handleAlterAction('prft', array(), array()); } /** @@ -248,7 +248,7 @@ class GridFieldTest extends SapphireTest { public function testHandleAction() { $config = GridFieldConfig::create()->addComponent(new GridFieldTest_Component); $obj = new GridField('testfield', 'testfield', ArrayList::create(), $config); - $this->assertEquals('handledAction is executed', $obj->handleAction('jump', array(), array())); + $this->assertEquals('handledAction is executed', $obj->handleAlterAction('jump', array(), array())); } /**