API change request handling to be more orthogonal

RequestHandler#handleAction now exists. It takes the request, and
the action to call on itself. All calls from handleRequest to call an action
will go through this method

Controller#handleAction has had it's signature changed to
match new RequestHandler#handleAction

RequestHandler#findAction has been added, which extracts the
"match URL to rules to find action" portion of RequestHandler#handleRequest
into a separate, overrideable function

GridField#handleAction has beeen renamed to handleAlterAction and
CMSBatchActionHandler#handleAction has been renamed to handleBatchAction to
avoid name clash with new RequestHandler#handleAction

Reason for change: The exact behaviour of request handling depended heavily
on whether you inherited from RequestHandler or Controller, and whether the
rule extracted it's action directly (like "foo/$ID" => 'foo') or dynamically
(like "$Action/$ID" => "handleAction"). This cleans up behaviour so
all calls follow the same path through handleRequest and handleAction, and
the additional behaviour that Controller adds is clear.

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

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) {
+	public function handleAction($request, $action) {
-		// urlParams, requestParams, and action are set for backward compatability 
 		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->hasMethod($action)) {
-		if(!$this->hasAction($this->action)) {
+			$result = parent::handleAction($request, $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 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);
 		}
 	}
 

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

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
+				if($request->match($rule, true)) {
-					//Director::setUrlParams($request->latestParams());
-				
 					if(isset($_REQUEST['debug_request'])) {
-						Debug::message("Rule '$rule' matched to action '$action' on $this->class."
+						Debug::message(
-							. " Latest request params: " . var_export($request->latestParams(), true));
+							"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);
+					return array('rule' => $rule, 'action' => $action);
-
-						// 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;
 				}
 			}
-			
+
 			$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;
 	}
 	
 	/**

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)) {

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
+		/* We no longer support using hacky attempting to handle URL parsing with broken rules */
-		 * 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. */
 		$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());
 	}

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