Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Strongly-type action method signatures #10536

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 16 additions & 24 deletions src/Control/Controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
*
Expand Down Expand Up @@ -142,12 +146,11 @@ public function doInit()
*
* 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;
}

/**
Expand Down Expand Up @@ -192,11 +195,8 @@ protected function afterHandleRequest()
* 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!');
Expand Down Expand Up @@ -332,14 +332,9 @@ public function getURLParams()
/**
* 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;
}

Expand Down Expand Up @@ -628,17 +623,14 @@ public function popCurrent()

/**
* 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
GuySartorelli marked this conversation as resolved.
Show resolved Hide resolved
{
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);
Expand Down
8 changes: 1 addition & 7 deletions src/Control/HTTPResponse.php
Original file line number Diff line number Diff line change
Expand Up @@ -310,13 +310,7 @@ public function removeHeader($header)
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);
Expand Down
10 changes: 2 additions & 8 deletions src/Control/HTTPResponse_Exception.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
class HTTPResponse_Exception extends Exception
{

protected $response;
protected HTTPResponse $response;

/**
* @param HTTPResponse|string $body Either the plaintext content of the error
Expand Down Expand Up @@ -52,17 +52,11 @@ public function __construct($body = null, $statusCode = null, $statusDescription
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;
Expand Down
16 changes: 6 additions & 10 deletions src/Control/PjaxResponseNegotiator.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class PjaxResponseNegotiator
*/
protected $callbacks = [];

protected $response = null;
protected HTTPResponse $response;

/**
* Overridden fragments (if any). Otherwise uses fragments from the request.
Expand All @@ -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;
}
Expand All @@ -68,10 +65,9 @@ public function setResponse($response)
* @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);
Expand Down
17 changes: 4 additions & 13 deletions src/Control/RequestHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -543,11 +543,8 @@ public function getRequest()
/**
* 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;
Expand Down Expand Up @@ -581,12 +578,8 @@ public function Link($action = null)

/**
* 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();
Expand Down Expand Up @@ -655,10 +648,8 @@ public function getReferer()
* 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
Expand Down
4 changes: 3 additions & 1 deletion src/Dev/DevBuildController.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

use SilverStripe\Control\Controller;
use SilverStripe\Control\Director;
use SilverStripe\Control\HTTPRequest;
use SilverStripe\Control\HTTPResponse;
use SilverStripe\ORM\DatabaseAdmin;

class DevBuildController extends Controller
Expand All @@ -17,7 +19,7 @@ class DevBuildController extends Controller
'build'
];

public function build($request)
public function build(HTTPRequest $request): HTTPResponse
GuySartorelli marked this conversation as resolved.
Show resolved Hide resolved
{
if (Director::is_cli()) {
$da = DatabaseAdmin::create();
Expand Down
8 changes: 5 additions & 3 deletions src/Forms/FileField.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,22 @@
* <code>
* class ExampleFormController extends PageController {
*
* function Form() {
* public function Form(): Form
* {
* $fields = new FieldList(
* new TextField('MyName'),
* new FileField('MyFile')
* );
* $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
Expand Down
20 changes: 7 additions & 13 deletions src/Forms/FormRequestHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ public function Link($action = null)
* if the form is valid.
*
* @param HTTPRequest $request
* @return HTTPResponse
* @return mixed
* @throws HTTPResponse_Exception
*/
public function httpSubmission($request)
GuySartorelli marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -200,7 +200,7 @@ public function httpSubmission($request)
// 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))
);
Expand All @@ -209,7 +209,7 @@ public function httpSubmission($request)
$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)
);
Expand Down Expand Up @@ -261,7 +261,7 @@ public function httpSubmission($request)
);
}

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

/**
Expand Down Expand Up @@ -299,11 +299,8 @@ public function checkAccessAction($action)
* 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();
Expand All @@ -329,10 +326,8 @@ protected function getValidationErrorResponse(ValidationResult $result)

/**
* Redirect back to this form with an added #anchor link
*
* @return HTTPResponse
*/
public function redirectBackToForm()
public function redirectBackToForm(): HTTPResponse
{
$pageURL = $this->getReturnReferer();
if (!$pageURL) {
Expand Down Expand Up @@ -369,9 +364,8 @@ protected function addBackURLParam($link)
*
* @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');
Expand Down
7 changes: 1 addition & 6 deletions src/Security/MemberAuthenticator/LostPasswordHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -153,13 +153,8 @@ public function redirectToLostPassword()
* 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);
Expand Down
3 changes: 1 addition & 2 deletions src/Security/Security.php
Original file line number Diff line number Diff line change
Expand Up @@ -314,9 +314,8 @@ public function hasAuthenticator($authenticator)
*
* 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);

Expand Down
6 changes: 4 additions & 2 deletions tests/php/Control/Middleware/Control/TestController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading