API Strongly-type action method signatures

code/BatchActions/CMSBatchAction_Archive.php

@@ -4,6 +4,7 @@ namespace SilverStripe\CMS\BatchActions;
 
 use SilverStripe\ORM\SS_List;
 use SilverStripe\Admin\CMSBatchAction;
+use SilverStripe\Control\HTTPResponse;
 
 /**
  * Delete items batch action.
@@ -15,7 +16,7 @@ class CMSBatchAction_Archive extends CMSBatchAction
         return _t(__CLASS__ . '.TITLE', 'Unpublish and archive');
     }
 
-    public function run(SS_List $pages)
+    public function run(SS_List $pages): HTTPResponse
     {
         return $this->batchaction(
             $pages,

code/BatchActions/CMSBatchAction_Publish.php

@@ -3,6 +3,7 @@
 namespace SilverStripe\CMS\BatchActions;
 
 use SilverStripe\Admin\CMSBatchAction;
+use SilverStripe\Control\HTTPResponse;
 use SilverStripe\ORM\SS_List;
 
 /**
@@ -15,7 +16,7 @@ class CMSBatchAction_Publish extends CMSBatchAction
         return _t(__CLASS__ . '.PUBLISH_PAGES', 'Publish');
     }
 
-    public function run(SS_List $pages)
+    public function run(SS_List $pages): HTTPResponse
     {
         return $this->batchaction(
             $pages,

code/BatchActions/CMSBatchAction_Restore.php

@@ -4,6 +4,7 @@ namespace SilverStripe\CMS\BatchActions;
 
 use SilverStripe\Admin\CMSBatchAction;
 use SilverStripe\CMS\Model\SiteTree;
+use SilverStripe\Control\HTTPResponse;
 use SilverStripe\ORM\ArrayList;
 use SilverStripe\ORM\SS_List;
 use SilverStripe\Versioned\Versioned;
@@ -20,7 +21,7 @@ class CMSBatchAction_Restore extends CMSBatchAction
         return _t(__CLASS__ . '.RESTORE', 'Restore');
     }
 
-    public function run(SS_List $pages)
+    public function run(SS_List $pages): HTTPResponse
     {
         // Sort pages by depth
         $pageArray = $pages->toArray();

code/BatchActions/CMSBatchAction_Unpublish.php

@@ -3,6 +3,7 @@
 namespace SilverStripe\CMS\BatchActions;
 
 use SilverStripe\Admin\CMSBatchAction;
+use SilverStripe\Control\HTTPResponse;
 use SilverStripe\ORM\SS_List;
 
 /**
@@ -15,7 +16,7 @@ class CMSBatchAction_Unpublish extends CMSBatchAction
         return _t(__CLASS__ . '.UNPUBLISH_PAGES', 'Unpublish');
     }
 
-    public function run(SS_List $pages)
+    public function run(SS_List $pages): HTTPResponse
     {
         return $this->batchaction(
             $pages,

code/Controllers/CMSMain.php

@@ -23,6 +23,7 @@ use SilverStripe\Control\Director;
 use SilverStripe\Control\HTTPRequest;
 use SilverStripe\Control\HTTPResponse;
 use SilverStripe\Control\HTTPResponse_Exception;
+use SilverStripe\Control\PjaxResponseNegotiator;
 use SilverStripe\Core\Cache\MemberCacheFlusher;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Core\Convert;
@@ -204,7 +205,7 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
         CMSBatchActionHandler::register('publish', CMSBatchAction_Publish::class);
     }
 
-    public function index($request)
+    public function index(HTTPRequest $request): HTTPResponse
     {
         // In case we're not showing a specific record, explicitly remove any session state,
         // to avoid it being highlighted in the tree, and causing an edit form to show.
@@ -215,7 +216,7 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
         return parent::index($request);
     }
 
-    public function getResponseNegotiator()
+    public function getResponseNegotiator(): PjaxResponseNegotiator
     {
         $negotiator = parent::getResponseNegotiator();
 
@@ -653,11 +654,8 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
     /**
      * Get a subtree underneath the request param 'ID'.
      * If ID = 0, then get the whole tree.
-     *
-     * @param HTTPRequest $request
-     * @return string
      */
-    public function getsubtree($request)
+    public function getsubtree(HTTPRequest $request): HTTPResponse
     {
         $html = $this->getSiteTreeFor(
             $this->config()->get('tree_class'),
@@ -672,7 +670,7 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
         $html = preg_replace('/^[\s\t\r\n]*<ul[^>]*>/', '', $html ?? '');
         $html = preg_replace('/<\/ul[^>]*>[\s\t\r\n]*$/', '', $html ?? '');
 
-        return $html;
+        return $this->getResponse()->setBody($html);
     }
 
     /**
@@ -680,11 +678,8 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
      * Similar to {@link getsubtree()}, but doesn't enforce loading
      * all children with the node. Useful to refresh views after
      * state modifications, e.g. saving a form.
-     *
-     * @param HTTPRequest $request
-     * @return HTTPResponse
      */
-    public function updatetreenodes($request)
+    public function updatetreenodes(HTTPRequest $request): HTTPResponse
     {
         $data = [];
         $ids = explode(',', $request->getVar('ids') ?? '');
@@ -752,17 +747,15 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
      * - 'SiblingIDs': Array of all sibling nodes to the moved node (incl. the node itself).
      *   In case of a 'ParentID' change, relates to the new siblings under the new parent.
      *
-     * @param HTTPRequest $request
-     * @return HTTPResponse JSON string with a
      * @throws HTTPResponse_Exception
      */
-    public function savetreenode($request)
+    public function savetreenode(HTTPRequest $request): HTTPResponse
     {
         if (!SecurityToken::inst()->checkRequest($request)) {
-            return $this->httpError(400);
+            $this->httpError(400);
         }
         if (!$this->CanOrganiseSitetree()) {
-            return $this->httpError(
+            $this->httpError(
                 403,
                 _t(
                     __CLASS__.'.CANT_REORGANISE',
@@ -775,14 +768,14 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
         $id = $request->requestVar('ID');
         $parentID = $request->requestVar('ParentID');
         if (!is_numeric($id) || !is_numeric($parentID)) {
-            return $this->httpError(400);
+            $this->httpError(400);
         }
 
         // Check record exists in the DB
         /** @var SiteTree $node */
         $node = DataObject::get_by_id($className, $id);
         if (!$node) {
-            return $this->httpError(
+            $this->httpError(
                 500,
                 _t(
                     __CLASS__.'.PLEASESAVE',
@@ -794,7 +787,7 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
         // Check top level permissions
         $root = $node->getParentType();
         if (($parentID == '0' || $root == 'root') && !SiteConfig::current_site_config()->canCreateTopLevel()) {
-            return $this->httpError(
+            $this->httpError(
                 403,
                 _t(
                     __CLASS__.'.CANT_REORGANISE',
@@ -1026,7 +1019,7 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
         return $pageTypes;
     }
 
-    public function doSearch($data, $form)
+    public function doSearch(array $data, Form $form): HTTPResponse
     {
         return $this->getsubtree($this->getRequest());
     }
@@ -1551,17 +1544,14 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
     /**
      * Callback to request the list of page types allowed under a given page instance.
      * Provides a slower but more precise response over SiteTreeHints
-     *
-     * @param HTTPRequest $request
-     * @return HTTPResponse
      */
-    public function childfilter($request)
+    public function childfilter(HTTPRequest $request): HTTPResponse
     {
         // Check valid parent specified
         $parentID = $request->requestVar('ParentID');
         $parent = SiteTree::get()->byID($parentID);
         if (!$parent || !$parent->exists()) {
-            return $this->httpError(404);
+            $this->httpError(404);
         }
 
         // Build hints specific to this class
@@ -1748,12 +1738,9 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
     /**
      * Save and Publish page handler
      *
-     * @param array $data
-     * @param Form $form
-     * @return HTTPResponse
      * @throws HTTPResponse_Exception
      */
-    public function save($data, $form)
+    public function save(array $data, Form $form): HTTPResponse
     {
         $className = $this->config()->get('tree_class');
 
@@ -1896,12 +1883,9 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
      *
      * @uses SiteTree->doRevertToLive()
      *
-     * @param array $data
-     * @param Form $form
-     * @return HTTPResponse
      * @throws HTTPResponse_Exception
      */
-    public function revert($data, $form)
+    public function revert(array $data, Form $form): HTTPResponse
     {
         if (!isset($data['ID'])) {
             throw new HTTPResponse_Exception("Please pass an ID in the form content", 400);
@@ -1949,12 +1933,9 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
      *
      * @see deletefromlive()
      *
-     * @param array $data
-     * @param Form $form
-     * @return HTTPResponse
      * @throws HTTPResponse_Exception
      */
-    public function delete($data, $form)
+    public function delete(array $data, Form $form): HTTPResponse
     {
         $id = $data['ID'];
         $record = SiteTree::get()->byID($id);
@@ -1984,12 +1965,9 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
     /**
      * Delete this page from both live and stage
      *
-     * @param array $data
-     * @param Form $form
-     * @return HTTPResponse
      * @throws HTTPResponse_Exception
      */
-    public function archive($data, $form)
+    public function archive(array $data, Form $form): HTTPResponse
     {
         $id = $data['ID'];
         /** @var SiteTree $record */
@@ -2017,14 +1995,14 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
         return $this->getResponseNegotiator()->respond($this->getRequest());
     }
 
-    public function publish($data, $form)
+    public function publish(array $data, Form $form): HTTPResponse
     {
         $data['publish'] = '1';
 
         return $this->save($data, $form);
     }
 
-    public function unpublish($data, $form)
+    public function unpublish(array $data, Form $form): HTTPResponse
     {
         $className = $this->config()->get('tree_class');
         /** @var SiteTree $record */
@@ -2161,10 +2139,8 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
 
     /**
      * @deprecated 5.0 Please use custom logic for this
-     * @param $request
-     * @return HTTPResponse|string|void
      */
-    public function publishall($request)
+    public function publishall(HTTPRequest $request): HTTPResponse
     {
         if (!Permission::check('ADMIN')) {
             return Security::permissionFailure($this);
@@ -2178,7 +2154,7 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
         if (isset($this->requestParams['confirm'])) {
             // Protect against CSRF on destructive action
             if (!SecurityToken::inst()->checkRequest($request)) {
-                return $this->httpError(400);
+                $this->httpError(400);
             }
 
             $start = 0;
@@ -2228,17 +2204,13 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
                 '</form>';
         }
 
-        return $response;
+        return HTTPResponse::create()->setBody($response);
     }
 
     /**
      * Restore a completely deleted page from the SiteTree_versions table.
-     *
-     * @param array $data
-     * @param Form $form
-     * @return HTTPResponse
      */
-    public function restore($data, $form)
+    public function restore(array $data, Form $form): HTTPResponse
     {
         if (!isset($data['ID']) || !is_numeric($data['ID'])) {
             return new HTTPResponse("Please pass an ID in the form content", 400);
@@ -2265,7 +2237,7 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
         return $this->getResponseNegotiator()->respond($this->getRequest());
     }
 
-    public function duplicate($request)
+    public function duplicate(HTTPRequest $request): HTTPResponse
     {
         // Protect against CSRF on destructive action
         if (!SecurityToken::inst()->checkRequest($request)) {
@@ -2309,11 +2281,11 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
         return new HTTPResponse("CMSMain::duplicate() Bad ID: '$id'", 400);
     }
 
-    public function duplicatewithchildren($request)
+    public function duplicatewithchildren(HTTPRequest $request): HTTPResponse
     {
         // Protect against CSRF on destructive action
         if (!SecurityToken::inst()->checkRequest($request)) {
-            return $this->httpError(400);
+            $this->httpError(400);
         }
         Environment::increaseTimeLimitTo();
         if (($id = $this->urlParams['ID']) && is_numeric($id)) {

code/Controllers/CMSPageAddController.php

@@ -192,12 +192,7 @@ class CMSPageAddController extends CMSPageEditController
         return $form;
     }
 
-    /**
+    public function doAdd(array $data, Form $form): HTTPResponse
-     * @param array $data
-     * @param Form $form
-     * @return HTTPResponse
-     */
-    public function doAdd($data, $form)
     {
         $className = isset($data['PageType']) ? $data['PageType'] : "Page";
         $parentID = isset($data['ParentID']) ? (int)$data['ParentID'] : 0;
@@ -241,7 +236,7 @@ class CMSPageAddController extends CMSPageEditController
         return $this->redirect(Controller::join_links($editController->Link('show'), $record->ID));
     }
 
-    public function doCancel($data, $form)
+    public function doCancel(array $data, Form $form): HTTPResponse
     {
         return $this->redirect(CMSMain::singleton()->Link());
     }

code/Controllers/CMSPageEditController.php

@@ -53,30 +53,27 @@ class CMSPageEditController extends CMSMain
 
     /**
      * Action handler for adding pages to a campaign
-     *
-     * @param array $data
-     * @param Form $form
-     * @return DBHTMLText|HTTPResponse
      */
-    public function addtocampaign($data, $form)
+    public function addtocampaign(array $data, Form $form): HTTPResponse
     {
         $id = $data['ID'];
         $record = \Page::get()->byID($id);
 
         $handler = AddToCampaignHandler::create($this, $record);
-        $results = $handler->addToCampaign($record, $data);
+        $response = $handler->addToCampaign($record, $data);
-        if (is_null($results)) {
+        $message = $response->getBody();
-            return null;
+        if (empty($message)) {
+            return $response;
         }
 
         if ($this->getSchemaRequested()) {
             // Send extra "message" data with schema response
-            $extraData = ['message' => $results];
+            $extraData = ['message' => $message];
             $schemaId = Controller::join_links($this->Link('schema/AddToCampaignForm'), $id);
             return $this->getSchemaResponse($schemaId, $form, null, $extraData);
         }
 
-        return $results;
+        return $response;
     }
 
     /**

code/Controllers/ContentController.php

@@ -187,11 +187,9 @@ class ContentController extends Controller
      * This acts the same as {@link Controller::handleRequest()}, but if an action cannot be found this will attempt to
      * fall over to a child controller in order to provide functionality for nested URLs.
      *
-     * @param HTTPRequest $request
-     * @return HTTPResponse
      * @throws HTTPResponse_Exception
      */
-    public function handleRequest(HTTPRequest $request)
+    public function handleRequest(HTTPRequest $request): HTTPResponse
     {
         /** @var SiteTree $child */
         $child = null;

code/Controllers/ModelAsController.php

@@ -33,11 +33,9 @@ class ModelAsController extends Controller implements NestedController
      * Get the appropriate {@link ContentController} for handling a {@link SiteTree} object, link it to the object and
      * return it.
      *
-     * @param SiteTree $sitetree
      * @param string $action
-     * @return ContentController
      */
-    public static function controller_for(SiteTree $sitetree, $action = null)
+    public static function controller_for(SiteTree $sitetree, $action = null): ContentController
     {
         $controller = $sitetree->getControllerName();
 
@@ -72,10 +70,8 @@ class ModelAsController extends Controller implements NestedController
 
     /**
      * @uses ModelAsController::getNestedController()
-     * @param HTTPRequest $request
-     * @return HTTPResponse
      */
-    public function handleRequest(HTTPRequest $request)
+    public function handleRequest(HTTPRequest $request): HTTPResponse
     {
         $this->beforeHandleRequest($request);
 
@@ -95,14 +91,8 @@ class ModelAsController extends Controller implements NestedController
         }
 
         try {
-            $result = $this->getNestedController();
+            $result = $this->getNestedController()->handleRequest($this->getRequest());
-
+            $result = $result;
-            if ($result instanceof RequestHandler) {
-                $result = $result->handleRequest($this->getRequest());
-            } elseif (!($result instanceof HTTPResponse)) {
-                user_error("ModelAsController::getNestedController() returned bad object type '" .
-                    get_class($result)."'", E_USER_WARNING);
-            }
         } catch (HTTPResponse_Exception $responseException) {
             $result = $responseException->getResponse();
         }
@@ -112,10 +102,9 @@ class ModelAsController extends Controller implements NestedController
     }
 
     /**
-     * @return ContentController
      * @throws Exception If URLSegment not passed in as a request parameter.
      */
-    public function getNestedController()
+    public function getNestedController(): ContentController
     {
         $request = $this->getRequest();
 

code/Controllers/OldPageRedirector.php

@@ -17,10 +17,9 @@ class OldPageRedirector extends Extension
      * On every URL that generates a 404, we'll capture it here and see if we can
      * find an old URL that it should be redirecting to.
      *
-     * @param HTTPRequest $request The request object
      * @throws HTTPResponse_Exception
      */
-    public function onBeforeHTTPError404($request)
+    public function onBeforeHTTPError404(HTTPRequest $request)
     {
         // We need to get the URL ourselves because $request->allParams() only has a max of 4 params
         $params = preg_split('|/+|', $request->getURL() ?? '');

code/Controllers/RootURLController.php

@@ -85,11 +85,7 @@ class RootURLController extends Controller implements Resettable
         }
     }
 
-    /**
+    public function handleRequest(HTTPRequest $request): HTTPResponse
-     * @param HTTPRequest $request
-     * @return HTTPResponse
-     */
-    public function handleRequest(HTTPRequest $request)
     {
         self::$is_at_root = true;
         $this->beforeHandleRequest($request);

tests/php/Controllers/CMSBatchActionsTest.php

@@ -141,7 +141,7 @@ class CMSBatchActionsTest extends SapphireTest
         $this->assertEquals($archivedID, $list->first()->ParentID);
 
         // Run restore
-        $result = json_decode($action->run($list) ?? '', true);
+        $result = json_decode($action->run($list)->getBody(), true);
         $this->assertEquals(
             [
                 $archivedxID => $archivedxID,
@@ -161,7 +161,7 @@ class CMSBatchActionsTest extends SapphireTest
         $this->assertEquals(0, $list->last()->ParentID); // archived (parent)
 
         // Run restore
-        $result = json_decode($action->run($list) ?? '', true);
+        $result = json_decode($action->run($list)->getBody(), true);
         $this->assertEquals(
             [
                 // Order of archived is opposite to order items are passed in, as