Normalise the signature of controller actions in core code

[maxime-rainville]

The standard signature for our controller action should be something like this:

public function action(HTTPRequest $request): HTTPResponse;

or

public function action(HTTPRequest $request): array|string|HTTPResponse;

Currently you can choose not to expect the first parameter which has caused us occasional problem.

We should normalise all our actions to expect to official signature.

Maybe we should throw exception/warning if your controller action don't match the expected signature.

Acceptance criteria

• All internal Silverstripe action use the official signature
• Custom controller action for individual project do not break if their signature doesn't match the official expected signature.
• If your controller overrides a pre-existing internal Silverstripe CMS actions, then it has to match our official signature.

Notes

• This card excludes type checking controller action.

Related

• Redirector Controller index() silverstripe-cms#2635

New issues created

• SPIKE Investigate extending SiteTree/ContentController instead of Page/PageController #10537
• Strongly type BuildTask::run() method signatures #10535
• Audit allowed_actions #10540
• Remove legacy tasks silverstripe-cms#2789

PRs

• API Strongly-type action method signatures silverstripe-admin#1381
• API Strongly-type action method signatures silverstripe-asset-admin#1300
• API Strongly-type action method signatures silverstripe-campaign-admin#244
• API Strongly-type action method signatures silverstripe-cms#2788
• API Strongly-type action method signatures silverstripe-errorpage#75
• API Strongly-type action method signatures #10536
• API Strongly-type action method signatures silverstripe-versioned-admin#257
• DOC Document controller method signatures developer-docs#102
• API Strongly-type action method signatures silverstripe-frameworktest#127

Shared run of silverstripe/installer containing PRs above (excluding frameworktest)

• https://github.com/creative-commoners/silverstripe-installer/actions/runs/3270799417

[maxime-rainville]

Side note. The return type doesn't have to be HTTPResponse. It can be:

• array
• HTML string
• an HTTResponse

[maxime-rainville]

• index should always require an HTTPRequest no mater what.
• leave the return type blank

Not sure what to do with the strict return type and the warning. Might need more feedback from @silverstripe/core-team.

[emteknetnz]

Enforcing the HTTPRequest paramater seems good. If any projects break on upgrade, it's an easy fix, just update the method signature (or call, I'm not 100% sure which it is, either way it's easy)

I'd be inclined to leave the return type blank, or change it to mixed (functionally identical), or a union type (probably best)

Otherwise we're taking away functionality. For instance, one of the current use cases is:
(return) an array. In this case the values in the array are available in the templates and the controller completes as usual by returning a HTTPResponse with the body set to the current template.

If we restricted this to HTTPResponse, then we've taken away the "values in the array are available in the templates" functionality, which some current projects may rely on, so we're adding upgrade pain for, I'm really not sure what value?

[emteknetnz]

I'm going to propose splitting this into 4 + 1 issues:

1. Update core classes to extend ContentController instead of PageController

• RedirectorPageController
• ErrorPageController

2. Update non-core classes to extend ContentController instead of PageController

• (blocked on creating non-core CMS 5 branches)
• BlogController
• BlogPostController
• IFramePageController
• RegistryPageController
• TaxonomyDirectoryController
• UserDefinedFormController
• CKANRegistryPageController (unless module is dropped)

3. Update methods signature that implement $allowed_actions to (HTTPRequest $request): HTTPResponse

• Core modules only
• Some implementations currently return string, change this to HTTPResponse::create()->setBody($string);

4. As above, though for non-core modules

• (blocked on creating non-core CMS 5 branches)

5. Update BuildTask:run() to use (HTTPRequest $request): void signature

• Have already created new issue for this
• Strongly type BuildTask::run() method signatures #10535

[GuySartorelli]

IMO PageController should be the base controller for all pages the same way Page is the base for the actual page classes. It makes it way easier to modify behaviour of pages for your project. If we are going to move away from that we should move away from it deliberately and not in small parts here and there.

[emteknetnz]

PageController would remain the base controller for all project pages, just not the ones listed above

IMO the existing way of have a project PageController be the base page for core classes it's a very bad piece of architecture that should be rectified in this major release

[GuySartorelli]

PageController would remain the base controller for all project pages, just not the ones listed above

Right. What I'm saying is that that's a departure from what is expected and from the way Page inheritance works - you'd end up with pages that are descendents of Page but their controllers aren't descendents of PageController. Which is weird.

I'm not against removing that paradigm but that should be a very conscious and intentional decision, probably including the removal of the un-namespaced Pahe and PageController altogether if that's what we're moving towards.
And before we even consider going down that road we should consider the impact that will have on developers and the way they customise behaviour. Having to throw an extension on SiteTree and ContentController to affect vendor pages is a bit of a departure to the way developers thing about customising vendor pages atm.

[emteknetnz]

OK I've split this off as a spike #10537

[michalkleiner]

With the mandatory request, what will be the recommended approach to using controller actions from CLI tasks? Create an empty request and pass that through?

[madmatt]

With the mandatory request, what will be the recommended approach to using controller actions from CLI tasks? Create an empty request and pass that through?

In my humble opinion, that feels like a bit of a code smell - wouldn't that method be better placed on either a Page object (because it's an action you take on a model), or on some kind of service object? I can't think of anything off the top of my head, but maybe I'm not thinking laterally enough :-)

(Either way, yeah I suppose the workaround is to pass through a blank object via HTTPRequest::create())

[GuySartorelli]

Imo controllers are exactly the right place for most cli actions - in fact if Silverstripe was more strictly MVC there would be very little logic on models at all.

But I also agree that passing an empty request to a controller is a bit of a smell. A CLIRequest could make sense except that it would have to be a subclass of HttpRequest under the current proposal which just isn't correct.

Maybe having a mandatory request object passed in isn't the best way forward. Or perhaps we need some higher level request type that HttpRequest and CliRequest could both subclass.

[michalkleiner]

With the mandatory request, what will be the recommended approach to using controller actions from CLI tasks? Create an empty request and pass that through?

wouldn't that method be better placed on either a Page object (because it's an action you take on a model), or on some kind of service object? I can't think of anything off the top of my head, but maybe I'm not thinking laterally enough :-)

The actual functionality/code can be a part of a service, a controller, or a static method on a model, but it still needs to be invoked somehow — through a controller, possibly using a route. There's a workaround of doing it as a dev task or a build task, but some actions definitely can be invoked both via browser, e.g. a health check ping from a monitoring service such as Pingdom, or through CLI via cronjob. sake might be partially working around this (not sure if that's accurate, haven't looked at the sake code for a while) but I think we should keep CLI support in mind.

[emteknetnz]

With the mandatory request, what will be the recommended approach to using controller actions from CLI tasks? Create an empty request and pass that through?

As noted on the issue I split off the strongly type BuildTask::run(), which is the default method on BuildTask, it already has a dynamically typed $request parameter which is an HTTPRequest object. While it's obviously not a real HTTPRequest, it does convert any command line arguments to the equivalent of passing in a query string to HTTPRequest

I've written some simple code to hopefully clarify this:

PageController.php

<?php

class PageController extends SilverStripe\CMS\Controllers\ContentController
{
    private static $allowed_actions = [ 'myaction' ];
    public function myaction($request)
    {
        return array_merge(['xyz' => 123], ['abc' => $request->getVar('abc')]);
    }
}

MyTask.php

<?php

class MyTask extends SilverStripe\Dev\BuildTask
{
    public function run($request)
    {
        echo get_class($request) . "\n";
        echo $request->getVar('abc') . "\n";
        echo PageController::create()->myaction($request)['xyz'] . "\n";
        echo PageController::create()->myaction($request)['abc'] . "\n";
    }
}

vendor/bin/sake dev/tasks/MyTask abc=def

Running Task MyTask

SilverStripe\Control\HTTPRequest
def
123
def

[GuySartorelli]

You made a strong argument in #10466 (comment) that we should be looser with the return types than explicitly HTTPResponse - it looks based on the above comment that you've since changed your mind for that. I would argue that it's better to be looser to effectively say "these are the supported return types for actions, and if you override any of these actions you can use any of the supported return types"

[emteknetnz]

In the PRs above all I've effectively done is add strong typing to a bunch of internal methods. This meets the following two acceptance critiera

• "All internal Silverstripe action use the official signature"
• "If your controller overrides a pre-existing internal Silverstripe CMS actions, then it has to match our official signature."

In regards to the other Acceptance criteria

• "Custom controller action for individual project do not break if their signature doesn't match the official expected signature."

You can still do this in a project:

class PageController extends SilverStripe\CMS\Controllers\ContentController
{
    private static $allowed_actions = ['someaction'];
    public function someaction()
    {
        return 'this is my action';
    }

It worth nothing that everything will go via the following catch-all
Controller::handleRequest(HTTPRequest $request): HTTPResponse

Inside that catch-all there will be calls to things such
RequestHandler::handleAction($request, $action)

Which itself will call
$actionRes = $this->$action($request);
(PHP won't complain if the method being called doesn't have the $request param, such as in the index() example above)

Which is what will call the action method on project controllers. There is no type checking done here.

[GuySartorelli]

"All internal Silverstripe action use the official signature"
Are we saying the official signature is explicitly HTTPResponse and not the union array|string|HTTPResponse or some other union?

I think the latter is potentially better as it allows, as I mentioned, for developers to override core methods and return any of those types - but if you still think explicitly HTTPResponse is better for whatever reason then I'll review the PRs with HTTPResponse being returned as being "the official signature" in my head.

[GuySartorelli]

Also @michalkleiner do you still have concerns about the CLI or has Steve addressed that?
FWIW I did find a situation where normal controller actions are used in CLI: #10536 (comment)
With the proposed changes this will mean explicitly using HTTPRequest and HTTPReponse in CLI contexts (though they were already being used before - just not as explicitly required types).

[emteknetnz]

Are we saying the official signature is explicitly HTTPResponse and not the union array|string|HTTPResponse or some other union?

Yes

Note: it's not really that "official"

This PR is really just me refactoring to add strong typing to a bunch of things that are called internally. I ran into quite a few instances where methods wanted to return something that wasn't an HTTPResponse, so I left them as they were. That's fine, ultimately everything gets converted to HTTPResponse before going to the browser in Controller::handleRequest(HTTPRequest $request): HTTPResponse

People are still free to using whatever dynamic signature they want on their own controller actions in their project

[michalkleiner]

Also @michalkleiner do you still have concerns about the CLI or has Steve addressed that? FWIW I did find a situation where normal controller actions are used in CLI: #10536 (comment) With the proposed changes this will mean explicitly using HTTPRequest and HTTPReponse in CLI contexts (though they were already being used before - just not as explicitly required types).

All good with me, thanks for checking. We already used creating a new request so not a big change for us. I also realised that our other use case was running build tasks from queued jobs from the process method. And instantiating and running a build task from there requires creating a mock request for it, which won't change due to adding strict types here.

[GuySartorelli]

You'll also need a docs PR to point out these changes in the changelog.
Since it's mostly just reinforcing the types that were already being passed, it can probably be done with a simple paragraph, maybe linking to this issue for anyone that wants to know the exact details of what changed.

[emteknetnz]

Have made changes to PRs, added changelog, and updated and re-run the installer CI using vendor-patches https://github.com/creative-commoners/silverstripe-installer/actions/runs/3262595239

[GuySartorelli]

PRS merged - please update the draft PR now

[GuySartorelli]

All PRs merged