From 6e19ae737feb0e820d60e1628aabc0cefda6f609 Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Tue, 18 Oct 2022 18:21:09 +1300 Subject: [PATCH] API Strongly-type action method signatures --- code/BatchActions/CMSBatchAction_Archive.php | 3 +- code/BatchActions/CMSBatchAction_Publish.php | 3 +- code/BatchActions/CMSBatchAction_Restore.php | 3 +- .../BatchActions/CMSBatchAction_Unpublish.php | 3 +- code/Controllers/CMSMain.php | 84 +++++++------------ code/Controllers/CMSPageAddController.php | 9 +- code/Controllers/CMSPageEditController.php | 17 ++-- code/Controllers/ContentController.php | 4 +- code/Controllers/ModelAsController.php | 21 ++--- code/Controllers/OldPageRedirector.php | 3 +- code/Controllers/RootURLController.php | 6 +- tests/php/Controllers/CMSBatchActionsTest.php | 4 +- 12 files changed, 55 insertions(+), 105 deletions(-) diff --git a/code/BatchActions/CMSBatchAction_Archive.php b/code/BatchActions/CMSBatchAction_Archive.php index 0f185743..7520ffac 100644 --- a/code/BatchActions/CMSBatchAction_Archive.php +++ b/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, diff --git a/code/BatchActions/CMSBatchAction_Publish.php b/code/BatchActions/CMSBatchAction_Publish.php index 2ed3fd15..ffbfda98 100644 --- a/code/BatchActions/CMSBatchAction_Publish.php +++ b/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, diff --git a/code/BatchActions/CMSBatchAction_Restore.php b/code/BatchActions/CMSBatchAction_Restore.php index 340d9db0..f8e701d8 100644 --- a/code/BatchActions/CMSBatchAction_Restore.php +++ b/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(); diff --git a/code/BatchActions/CMSBatchAction_Unpublish.php b/code/BatchActions/CMSBatchAction_Unpublish.php index ad095092..6ae1fcda 100644 --- a/code/BatchActions/CMSBatchAction_Unpublish.php +++ b/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, diff --git a/code/Controllers/CMSMain.php b/code/Controllers/CMSMain.php index ab0a9703..b0a96b4b 100644 --- a/code/Controllers/CMSMain.php +++ b/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]*]*>/', '', $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 ''; } - 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)) { diff --git a/code/Controllers/CMSPageAddController.php b/code/Controllers/CMSPageAddController.php index d198d41f..c058946a 100644 --- a/code/Controllers/CMSPageAddController.php +++ b/code/Controllers/CMSPageAddController.php @@ -192,12 +192,7 @@ class CMSPageAddController extends CMSPageEditController return $form; } - /** - * @param array $data - * @param Form $form - * @return HTTPResponse - */ - public function doAdd($data, $form) + public function doAdd(array $data, Form $form): HTTPResponse { $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()); } diff --git a/code/Controllers/CMSPageEditController.php b/code/Controllers/CMSPageEditController.php index 9e8c4776..b15ff275 100644 --- a/code/Controllers/CMSPageEditController.php +++ b/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); - if (is_null($results)) { - return null; + $response = $handler->addToCampaign($record, $data); + $message = $response->getBody(); + 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; } /** diff --git a/code/Controllers/ContentController.php b/code/Controllers/ContentController.php index 204fd2e3..2e75ebe7 100644 --- a/code/Controllers/ContentController.php +++ b/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; diff --git a/code/Controllers/ModelAsController.php b/code/Controllers/ModelAsController.php index 8f9233ca..1fffb669 100644 --- a/code/Controllers/ModelAsController.php +++ b/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(); - - 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); - } + $result = $this->getNestedController()->handleRequest($this->getRequest()); + $result = $result; } 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(); diff --git a/code/Controllers/OldPageRedirector.php b/code/Controllers/OldPageRedirector.php index 5e526e11..5bd39da9 100644 --- a/code/Controllers/OldPageRedirector.php +++ b/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() ?? ''); diff --git a/code/Controllers/RootURLController.php b/code/Controllers/RootURLController.php index e8522989..48ab297a 100644 --- a/code/Controllers/RootURLController.php +++ b/code/Controllers/RootURLController.php @@ -85,11 +85,7 @@ class RootURLController extends Controller implements Resettable } } - /** - * @param HTTPRequest $request - * @return HTTPResponse - */ - public function handleRequest(HTTPRequest $request) + public function handleRequest(HTTPRequest $request): HTTPResponse { self::$is_at_root = true; $this->beforeHandleRequest($request); diff --git a/tests/php/Controllers/CMSBatchActionsTest.php b/tests/php/Controllers/CMSBatchActionsTest.php index dae4836d..62fc9fdd 100644 --- a/tests/php/Controllers/CMSBatchActionsTest.php +++ b/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