Merge pull request #10536 from creative-commoners/pulls/5/action-signature

API Strongly-type action method signatures

src/Control/Controller.php

@@ -73,10 +73,8 @@ class Controller extends RequestHandler implements TemplateGlobalProvider
      * The response object that the controller returns.
      *
      * Set in {@link handleRequest()}.
-     *
-     * @var HTTPResponse
      */
-    protected $response;
+    protected HTTPResponse $response;
 
     /**
      * Default URL handlers.
@@ -95,6 +93,12 @@ class Controller extends RequestHandler implements TemplateGlobalProvider
         'handleIndex',
     ];
 
+    public function __construct()
+    {
+        parent::__construct();
+        $this->setResponse(HTTPResponse::create());
+    }
+
     /**
      * Initialisation function that is run before any action on the controller is called.
      *
@@ -142,12 +146,11 @@ class Controller extends RequestHandler implements TemplateGlobalProvider
      *
      * Also set the URLParams
      */
-    public function setRequest($request)
+    public function setRequest(HTTPRequest $request): static
     {
-        $return = parent::setRequest($request);
+        parent::setRequest($request);
         $this->setURLParams($this->getRequest()->allParams());
-
-        return $return;
+        return $this;
     }
 
     /**
@@ -192,11 +195,8 @@ class Controller extends RequestHandler implements TemplateGlobalProvider
      * 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 HTTPRequest $request
-     * @return HTTPResponse
      */
-    public function handleRequest(HTTPRequest $request)
+    public function handleRequest(HTTPRequest $request): HTTPResponse
     {
         if (!$request) {
             throw new \RuntimeException('Controller::handleRequest() not passed a request!');
@@ -332,14 +332,9 @@ class Controller extends RequestHandler implements TemplateGlobalProvider
     /**
      * Returns the HTTPResponse object that this controller is building up. Can be used to set the
      * status code and headers.
-     *
-     * @return HTTPResponse
      */
-    public function getResponse()
+    public function getResponse(): HTTPResponse
     {
-        if (!$this->response) {
-            $this->setResponse(new HTTPResponse());
-        }
         return $this->response;
     }
 
@@ -628,17 +623,14 @@ class Controller extends RequestHandler implements TemplateGlobalProvider
 
     /**
      * Redirect to the given URL.
-     *
-     * @param string $url
-     * @param int $code
-     * @return HTTPResponse
      */
-    public function redirect($url, $code = 302)
+    public function redirect(string $url, int $code = 302): HTTPResponse
     {
-        if ($this->getResponse()->getHeader('Location') && $this->getResponse()->getHeader('Location') != $url) {
+        $response = $this->getResponse();
+        if ($response->getHeader('Location') && $response->getHeader('Location') != $url) {
             user_error("Already directed to " . $this->getResponse()->getHeader('Location')
                 . "; now trying to direct to $url", E_USER_WARNING);
-            return null;
+            return $response;
         }
         $response = parent::redirect($url, $code);
         $this->setResponse($response);

src/Control/HTTPResponse.php

@@ -310,13 +310,7 @@ class HTTPResponse
         return $this;
     }
 
-    /**
-     * @param string $dest
-     * @param int $code
-     *
-     * @return $this
-     */
-    public function redirect($dest, $code = 302)
+    public function redirect(string $dest, int $code = 302): static
     {
         if (!in_array($code, self::$redirect_codes)) {
             trigger_error("Invalid HTTP redirect code {$code}", E_USER_WARNING);

src/Control/HTTPResponse_Exception.php

@@ -17,7 +17,7 @@ use Exception;
 class HTTPResponse_Exception extends Exception
 {
 
-    protected $response;
+    protected HTTPResponse $response;
 
     /**
      * @param HTTPResponse|string $body Either the plaintext content of the error
@@ -52,17 +52,11 @@ class HTTPResponse_Exception extends Exception
         parent::__construct((string) $this->getResponse()->getBody(), $this->getResponse()->getStatusCode());
     }
 
-    /**
-     * @return HTTPResponse
-     */
-    public function getResponse()
+    public function getResponse(): HTTPResponse
     {
         return $this->response;
     }
 
-    /**
-     * @param HTTPResponse $response
-     */
     public function setResponse(HTTPResponse $response)
     {
         $this->response = $response;

src/Control/PjaxResponseNegotiator.php

@@ -30,7 +30,7 @@ class PjaxResponseNegotiator
      */
     protected $callbacks = [];
 
-    protected $response = null;
+    protected HTTPResponse $response;
 
     /**
      * Overridden fragments (if any). Otherwise uses fragments from the request.
@@ -41,21 +41,18 @@ class PjaxResponseNegotiator
      * @param array $callbacks
      * @param HTTPResponse $response An existing response to reuse (optional)
      */
-    public function __construct($callbacks = [], $response = null)
+    public function __construct($callbacks = [], HTTPResponse $response = null)
     {
         $this->callbacks = $callbacks;
-        $this->response = $response;
+        $this->response = $response ?: HTTPResponse::create();
     }
 
-    public function getResponse()
+    public function getResponse(): HTTPResponse
     {
-        if (!$this->response) {
-            $this->response = new HTTPResponse();
-        }
         return $this->response;
     }
 
-    public function setResponse($response)
+    public function setResponse(HTTPResponse $response)
     {
         $this->response = $response;
     }
@@ -68,10 +65,9 @@ class PjaxResponseNegotiator
      * @param array $extraCallbacks List of anonymous functions or callables returning either a string
      * or HTTPResponse, keyed by their fragment identifier. The 'default' key can
      * be used as a fallback for non-ajax responses.
-     * @return HTTPResponse
      * @throws HTTPResponse_Exception
      */
-    public function respond(HTTPRequest $request, $extraCallbacks = [])
+    public function respond(HTTPRequest $request, $extraCallbacks = []): HTTPResponse
     {
         // Prepare the default options and combine with the others
         $callbacks = array_merge($this->callbacks, $extraCallbacks);

src/Control/RequestHandler.php

@@ -60,7 +60,7 @@ class RequestHandler extends ViewableData
      * @var HTTPRequest $request The request object that the controller was called with.
      * Set in {@link handleRequest()}. Useful to generate the {}
      */
-    protected $request = null;
+    protected HTTPRequest $request;
 
     /**
      * The DataModel for this request
@@ -543,11 +543,8 @@ class RequestHandler extends ViewableData
     /**
      * Typically the request is set through {@link handleAction()}
      * or {@link handleRequest()}, but in some based we want to set it manually.
-     *
-     * @param HTTPRequest $request
-     * @return $this
      */
-    public function setRequest($request)
+    public function setRequest(HTTPRequest $request): static
     {
         $this->request = $request;
         return $this;
@@ -581,12 +578,8 @@ class RequestHandler extends ViewableData
 
     /**
      * Redirect to the given URL.
-     *
-     * @param string $url
-     * @param int $code
-     * @return HTTPResponse
      */
-    public function redirect($url, $code = 302)
+    public function redirect(string $url, int $code = 302): HTTPResponse
     {
         $url = Director::absoluteURL($url);
         $response = new HTTPResponse();
@@ -655,10 +648,8 @@ class RequestHandler extends ViewableData
      * URL (see {@link Director::baseURL()}).
      *
      * @uses redirect()
-     *
-     * @return HTTPResponse
      */
-    public function redirectBack()
+    public function redirectBack(): HTTPResponse
     {
         // Prefer to redirect to ?BackURL, but fall back to Referer header
         // As a last resort redirect to base url

src/Dev/DevBuildController.php

@@ -4,6 +4,8 @@ namespace SilverStripe\Dev;
 
 use SilverStripe\Control\Controller;
 use SilverStripe\Control\Director;
+use SilverStripe\Control\HTTPRequest;
+use SilverStripe\Control\HTTPResponse;
 use SilverStripe\ORM\DatabaseAdmin;
 
 class DevBuildController extends Controller
@@ -17,7 +19,7 @@ class DevBuildController extends Controller
         'build'
     ];
 
-    public function build($request)
+    public function build(HTTPRequest $request): HTTPResponse
     {
         if (Director::is_cli()) {
             $da = DatabaseAdmin::create();

src/Forms/FileField.php

@@ -23,7 +23,8 @@ use SilverStripe\ORM\DataObjectInterface;
  * <code>
  * class ExampleFormController extends PageController {
  *
- *  function Form() {
+ *  public function Form(): Form
+ *  {
  *      $fields = new FieldList(
  *          new TextField('MyName'),
  *          new FileField('MyFile')
@@ -31,12 +32,13 @@ use SilverStripe\ORM\DataObjectInterface;
  *      $actions = new FieldList(
  *          new FormAction('doUpload', 'Upload file')
  *      );
- *    $validator = new RequiredFields(['MyName', 'MyFile']);
+ *      $validator = new RequiredFields(['MyName', 'MyFile']);
  *
  *      return new Form($this, 'Form', $fields, $actions, $validator);
  *  }
  *
- *  function doUpload($data, $form) {
+ *  public function doUpload((array $data, Form $form): HTTPResponse
+ *  {
  *      $file = $data['MyFile'];
  *      $content = file_get_contents($file['tmp_name']);
  *      // ... process content

src/Forms/FormRequestHandler.php

@@ -103,7 +103,7 @@ class FormRequestHandler extends RequestHandler
      * if the form is valid.
      *
      * @param HTTPRequest $request
-     * @return HTTPResponse
+     * @return mixed
      * @throws HTTPResponse_Exception
      */
     public function httpSubmission($request)
@@ -200,7 +200,7 @@ class FormRequestHandler extends RequestHandler
             // buttonClicked() validates that the action set above is valid
             && !$this->buttonClicked()
         ) {
-            return $this->httpError(
+            $this->httpError(
                 403,
                 sprintf('Action "%s" not allowed on controller (Class: %s)', $funcName, get_class($controller))
             );
@@ -209,7 +209,7 @@ class FormRequestHandler extends RequestHandler
             $this->hasMethod($funcName)
             && !$this->checkAccessAction($funcName)
         ) {
-            return $this->httpError(
+            $this->httpError(
                 403,
                 sprintf('Action "%s" not allowed on form request handler (Class: "%s")', $funcName, static::class)
             );
@@ -261,7 +261,7 @@ class FormRequestHandler extends RequestHandler
             );
         }
 
-        return $this->httpError(404, "Could not find a suitable form-action callback function");
+        $this->httpError(404, "Could not find a suitable form-action callback function");
     }
 
     /**
@@ -299,11 +299,8 @@ class FormRequestHandler extends RequestHandler
      * handles 'application/json' requests with a JSON object containing the error messages.
      * Behaviour can be influenced by setting {@link $redirectToFormOnValidationError},
      * and can be overruled by setting {@link $validationResponseCallback}.
-     *
-     * @param ValidationResult $result
-     * @return HTTPResponse
      */
-    protected function getValidationErrorResponse(ValidationResult $result)
+    protected function getValidationErrorResponse(ValidationResult $result): HTTPResponse
     {
         // Check for custom handling mechanism
         $callback = $this->form->getValidationResponseCallback();
@@ -329,10 +326,8 @@ class FormRequestHandler extends RequestHandler
 
     /**
      * Redirect back to this form with an added #anchor link
-     *
-     * @return HTTPResponse
      */
-    public function redirectBackToForm()
+    public function redirectBackToForm(): HTTPResponse
     {
         $pageURL = $this->getReturnReferer();
         if (!$pageURL) {
@@ -369,9 +364,8 @@ class FormRequestHandler extends RequestHandler
      *
      * @internal called from {@see Form::getValidationErrorResponse}
      * @param ValidationResult $result
-     * @return HTTPResponse
      */
-    protected function getAjaxErrorResponse(ValidationResult $result)
+    protected function getAjaxErrorResponse(ValidationResult $result): HTTPResponse
     {
         // Ajax form submissions accept json encoded errors by default
         $acceptType = $this->getRequest()->getHeader('Accept');

src/Security/MemberAuthenticator/LostPasswordHandler.php

@@ -153,13 +153,8 @@ class LostPasswordHandler extends RequestHandler
      * the logic, by returning FALSE. In this case, the user will be redirected back
      * to the form without further action. It is recommended to set a message
      * in the form detailing why the action was denied.
-     *
-     * @skipUpgrade
-     * @param array $data Submitted data
-     * @param LostPasswordForm $form
-     * @return HTTPResponse
      */
-    public function forgotPassword($data, $form)
+    public function forgotPassword(array $data, Form $form): HTTPResponse
     {
         // Run a first pass validation check on the data
         $dataValidation = $this->validateForgotPasswordData($data, $form);

src/Security/Security.php

@@ -314,9 +314,8 @@ class Security extends Controller implements TemplateGlobalProvider
      *
      * The alreadyLoggedIn value can contain a '%s' placeholder that will be replaced with a link
      * to log in.
-     * @return HTTPResponse
      */
-    public static function permissionFailure($controller = null, $messageSet = null)
+    public static function permissionFailure($controller = null, $messageSet = null): HTTPResponse
     {
         self::set_ignore_disallowed_actions(true);
 

tests/php/Control/Middleware/Control/TestController.php

@@ -3,12 +3,14 @@
 namespace SilverStripe\Control\Tests\Middleware\Control;
 
 use SilverStripe\Control\Controller;
+use SilverStripe\Control\HTTPRequest;
+use SilverStripe\Control\HTTPResponse;
 
 class TestController extends Controller
 {
-    public function index($request)
+    public function index(HTTPRequest $request): HTTPResponse
     {
-        return "Success";
+        return HTTPResponse::create()->setBody("Success");
     }
 
     public function Link($action = null)

tests/php/Control/RequestHandlingTest/TestFormHandler.php

@@ -3,6 +3,8 @@
 
 namespace SilverStripe\Control\Tests\RequestHandlingTest;
 
+use SilverStripe\Control\HTTPRequest;
+use SilverStripe\Control\HTTPResponse;
 use SilverStripe\Forms\FormRequestHandler;
 
 /**
@@ -29,13 +31,13 @@ class TestFormHandler extends FormRequestHandler
         return $this->form->Fields()->dataFieldByName($request->param('FieldName'));
     }
 
-    public function handleSubmission($request)
+    public function handleSubmission(HTTPRequest $request): HTTPResponse
     {
-        return "Form posted";
+        return HTTPResponse::create()->setBody("Form posted");
     }
 
-    public function handleGet($request)
+    public function handleGet(HTTPRequest $request): HTTPResponse
     {
-        return "Get request on form";
+        return HTTPResponse::create()->setBody("Get request on form");
     }
 }

tests/php/Forms/FormFactoryTest/ControllerExtension.php

@@ -3,9 +3,12 @@
 namespace SilverStripe\Forms\Tests\FormFactoryTest;
 
 use SilverStripe\Control\Controller;
+use SilverStripe\Control\HTTPRequest;
+use SilverStripe\Control\HTTPResponse;
 use SilverStripe\Core\Convert;
 use SilverStripe\Core\Extension;
 use SilverStripe\Forms\FieldList;
+use SilverStripe\Forms\Form;
 use SilverStripe\Forms\FormAction;
 use SilverStripe\Forms\LiteralField;
 use SilverStripe\ORM\DataObject;
@@ -75,13 +78,15 @@ class ControllerExtension extends Extension
         }
     }
 
-    public function publish($data, $form)
+    public function publish(array $data, Form $form): HTTPResponse
     {
         // noop
+        return HTTPResponse::create();
     }
 
-    public function preview()
+    public function preview(HTTPRequest $request): HTTPResponse
     {
         // noop
+        return HTTPResponse::create();
     }
 }

tests/php/Forms/FormTest/ControllerWithSecurityToken.php

@@ -3,6 +3,7 @@
 namespace SilverStripe\Forms\Tests\FormTest;
 
 use SilverStripe\Control\Controller;
+use SilverStripe\Control\HTTPResponse;
 use SilverStripe\Dev\TestOnly;
 use SilverStripe\Forms\EmailField;
 use SilverStripe\Forms\FieldList;
@@ -49,7 +50,7 @@ class ControllerWithSecurityToken extends Controller implements TestOnly
         return $form;
     }
 
-    public function doSubmit($data, $form, $request)
+    public function doSubmit(array $data, Form $form): HTTPResponse
     {
         $form->sessionMessage('Test save was successful', 'good');
         return $this->redirectBack();

tests/php/Forms/FormTest/ControllerWithSpecialSubmittedValueFields.php

@@ -82,13 +82,13 @@ class ControllerWithSpecialSubmittedValueFields extends Controller implements Te
         return $form;
     }
 
-    public function doSubmit($data, $form, $request)
+    public function doSubmit(array $data, Form $form): HTTPResponse
     {
         $form->sessionMessage('Test save was successful', 'good');
         return $this->redirectBack();
     }
 
-    public function doTriggerException($data, $form, $request)
+    public function doTriggerException(array $data, Form $form): HTTPResponse
     {
         $result = new ValidationResult();
         $result->addFieldError('Email', 'Error on Email field');

tests/php/Forms/FormTest/ControllerWithStrictPostCheck.php

@@ -3,6 +3,7 @@
 namespace SilverStripe\Forms\Tests\FormTest;
 
 use SilverStripe\Control\Controller;
+use SilverStripe\Control\HTTPResponse;
 use SilverStripe\Dev\TestOnly;
 use SilverStripe\Forms\EmailField;
 use SilverStripe\Forms\FieldList;
@@ -48,7 +49,7 @@ class ControllerWithStrictPostCheck extends Controller implements TestOnly
         return $form;
     }
 
-    public function doSubmit($data, $form, $request)
+    public function doSubmit(array $data, Form $form): HTTPResponse
     {
         $form->sessionMessage('Test save was successful', 'good');
         return $this->redirectBack();

tests/php/Forms/FormTest/TestController.php

@@ -3,6 +3,7 @@
 namespace SilverStripe\Forms\Tests\FormTest;
 
 use SilverStripe\Control\Controller;
+use SilverStripe\Control\HTTPResponse;
 use SilverStripe\Dev\TestOnly;
 use SilverStripe\Forms\CheckboxSetField;
 use SilverStripe\Forms\EmailField;
@@ -79,13 +80,13 @@ class TestController extends Controller implements TestOnly
         return $form;
     }
 
-    public function doSubmit($data, $form, $request)
+    public function doSubmit(array $data, Form $form): HTTPResponse
     {
         $form->sessionMessage('Test save was successful', 'good');
         return $this->redirectBack();
     }
 
-    public function doTriggerException($data, $form, $request)
+    public function doTriggerException(array $data, Form $form): HTTPResponse
     {
         $result = new ValidationResult();
         $result->addFieldError('Email', 'Error on Email field');
@@ -93,13 +94,13 @@ class TestController extends Controller implements TestOnly
         throw new ValidationException($result);
     }
 
-    public function doSubmitValidationExempt($data, $form, $request)
+    public function doSubmitValidationExempt(array $data, Form $form): HTTPResponse
     {
         $form->sessionMessage('Validation skipped', 'good');
         return $this->redirectBack();
     }
 
-    public function doSubmitActionExempt($data, $form, $request)
+    public function doSubmitActionExempt(array $data, Form $form): HTTPResponse
     {
         $form->sessionMessage('Validation bypassed!', 'good');
         return $this->redirectBack();

tests/php/Forms/GridField/GridField_URLHandlerTest/TestComponent.php

@@ -4,6 +4,7 @@ namespace SilverStripe\Forms\Tests\GridField\GridField_URLHandlerTest;
 
 use SilverStripe\Control\Controller;
 use SilverStripe\Control\HTTPRequest;
+use SilverStripe\Control\HTTPResponse;
 use SilverStripe\Control\RequestHandler;
 use SilverStripe\Forms\FieldList;
 use SilverStripe\Forms\Form;
@@ -83,9 +84,9 @@ class TestComponent extends RequestHandler implements GridField_URLHandler
         );
     }
 
-    public function doAction($data, $form)
+    public function doAction(array $data, Form $form): HTTPResponse
     {
-        return "Submitted " . $data['Test'] . " to component";
+        return HTTPResponse::create()->setBody("Submitted " . $data['Test'] . " to component");
     }
 
     public function testpage(GridField $gridField, HTTPRequest $request)

tests/php/Forms/GridField/GridField_URLHandlerTest/TestComponent_ItemRequest.php

@@ -2,6 +2,7 @@
 
 namespace SilverStripe\Forms\Tests\GridField\GridField_URLHandlerTest;
 
+use SilverStripe\Control\HTTPResponse;
 use SilverStripe\Control\RequestHandler;
 use SilverStripe\Forms\FieldList;
 use SilverStripe\Forms\Form;
@@ -53,9 +54,9 @@ class TestComponent_ItemRequest extends RequestHandler
         );
     }
 
-    public function doAction($data, $form)
+    public function doAction(array $data, Form $form): HTTPResponse
     {
-        return "Submitted " . $data['Test'] . " to item #" . $this->id;
+        return HTTPResponse::create()->setBody("Submitted " . $data['Test'] . " to item #" . $this->id);
     }
 
     public function testpage()

tests/php/Security/SecurityTest/NullController.php

@@ -3,6 +3,7 @@
 namespace SilverStripe\Security\Tests\SecurityTest;
 
 use SilverStripe\Control\Controller;
+use SilverStripe\Control\HTTPResponse;
 use SilverStripe\Dev\TestOnly;
 
 /**
@@ -11,9 +12,10 @@ use SilverStripe\Dev\TestOnly;
 class NullController extends Controller implements TestOnly
 {
 
-    public function redirect($url, $code = 302)
+    public function redirect(string $url, int $code = 302): HTTPResponse
     {
         // NOOP
+        return HTTPResponse::create();
     }
 
     public function Link($action = null)