search

silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
721 stars 822 forks source link

Normalise the signature of controller actions in core code #10466

Closed maxime-rainville closed 2 years ago

maxime-rainville commented 2 years ago

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

Notes

Related

New issues created

PRs

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

maxime-rainville commented 2 years ago

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

maxime-rainville commented 2 years ago

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

emteknetnz commented 2 years ago

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 commented 2 years ago

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

1. Update core classes to extend ContentController instead of PageController

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

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

4. As above, though for non-core modules

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

GuySartorelli commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

OK I've split this off as a spike https://github.com/silverstripe/silverstripe-framework/issues/10537

michalkleiner commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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
emteknetnz commented 2 years ago

There are a few "official signatures" in the PRs above:

There's the regular controller endpoint action: myendpointaction(HTTPRequest $request): HTTPResponse

There's the Form action (as outlined in docs) myformaction(array $data, Form $form): HTTPResponse

I also standardised redirect() redirect(string $url, int $code = 302): HTTPResponse

I've also updated quite a few return types for various internal methods relating to implementation of action methods e.g. run(SS_List $objs): HTTPResponse;

Some currently have a mixed return types such as HTTPResponse|ViewableData_Customised. When I encountered this sort of thing I usually just left things as they were unless it was very easy to change to HTTPRequest, e.g. return HTTPRequest::create()->setBody($string);

I've done a special CI run of silverstripe/installer which is green containing the PRs in the description (excluding frameworktest) using vendor-code-patcher

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

GuySartorelli commented 2 years ago

You made a strong argument in https://github.com/silverstripe/silverstripe-framework/issues/10466#issuecomment-1226789946 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 commented 2 years ago

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

In regards to the other Acceptance criteria

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 commented 2 years ago

"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 commented 2 years ago

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: https://github.com/silverstripe/silverstripe-framework/pull/10536#discussion_r995142123 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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

PRS merged - please update the draft PR now

GuySartorelli commented 2 years ago

All PRs merged