!!! FEATURE: WIP Dispatcher and controller return ActionResponse (simpler controller pattern)

[kitsunet]

will not be merged directly into 9.0 but included in this mvc overhaul pr: #3311

This change needs an accompanying adjustment to Neos to adjust the
PluginImplementation as well as Modules.

The new SimpleActionController gives you a direct and simple way to
route an ActionRequest and return an ActionReponse with nothing in
between. Routing should work just like with other ActionControllers.

This is breaking if you implemented your own ControllerInterface
or overwrote or expect some of the api methods of the ActionController.
We now use a direct pattern of f(ActionRequest) => ActionResponse
in more places. Adjusting existing controllers should be easy though.
Additionally implementing your own dispatch loop (don't do this) will
need adjustments.

We discourage to manipulate $this->reponse in controllers,
although it should still work fine in actions for now, please consider
other options.

class Dispatcher
{
    public function dispatch(ActionRequest $request): ActionResponse;
}

[kitsunet]

Probably still has a few rough edges but I would like to discuss this before putting more time into it. I made it so that for probably the 90% userland case nothing should change, but open options, the few people that did overwrite/mess around with the interface and methods I changed shoudl have an easy time adapting. we could go way further with this paying for it with more breakiness.

[mhsdesign]

Hi :D Thanks for this wip approach to have something concrete to discuss!. This new controller interface and the SimpleActionController look both real good. But i see that we have to go through some lengths to achieve this response manipulating "feature".

Im not entirely sure if this is the best way to have a common interface for both legacy and new controllers. I think the code manipulation of the response is currently not perfectly readable but we can rely on it working and i think the readability decreases for our legacy layer when we use exception throwing with response as field for control flow (clever idea though ^^).

How about to keep our legacy controller code base as is and instead introduce a new interface?

interface LegacyActionControllerInterface
{
     public function processRequest(ActionRequest $request, ActionResponse $response): void;
} 

interface SimpleActionControllerInterface
{
     public function processRequest(ActionRequest $request): ActionResponse;
} 

I also have been thinking about an controller which would work entirely only with psr requests and responses so that our users can apply their knowledge from other php frameworks and dont have to panic: header(), echo "", die()
That can work if the dispatcher could figure out which method has to be called on the controller and the interface would just be a simple marker:

/** implement your actions by the pattern {$name}Action, process the psr requet and return a response */
interface PsrControllerInterface
{
   // @example
   // public function indexAction(ServerRequest $request): Response;
} 

[kitsunet]

But then you have a lot of weird checks and contortions in the lower layers? As in the dispatcher and any other code (plugins, widgets etc.) need to suddenly deal with two ways to call a controller. IDK I find the readability not too bad actually. And now the weirdness happens at least only inside the ActionController. And then we can step by step reduce the surface of this weirdness down to only the action and declare that the response is only globally available during the action method call. Could even be something we can do right now, not sure how much of our core contorllers do something beyond that.

Again PSR is a separate concern and not part of the dispatcher, that is a lower level we would have to build separate infrastructure for.

[mficzel]

I really like this approach. No property mapping and no mimetype negotiation to produce unexpected trouble \o/.

@mhsdesign i think that this makes more sense with FlowActionRequest / Response as otherwise it would be more like a middleware. The middleware approach will probably be the path anyway $someone chooses if one does not want to learn a new framework.

[kitsunet]

I think I do see a way in the future, we could move everything the ActionController does into the userland with services you can call. Like we deprecate regular actions at some point and say you just make it a SimpleController action and use the following services to get the same results (ala propertymapper -> validator -> view) and then the old code is just wrapped by a new simple controller action with a few lines of boilerplate to init the stuff.

Future backwards compat controller way suggestion:
This would come in a future PR then when we decide to deprecate ActionController at some point.

class MyOldStyleController extends ActionController {
    // your classic old action method that would need to be migrated
    public function oldStyleAction(MyObject $foo, string $bar) {
        // do stuff
        $this->view->....
        return ...;
    }
}

class MyOldStyleController extends SimpleActionController {
    // provides "legacy" functionality like mapAndValidate for arguments
    use ActionControllerCompatibility;

    // the new public action method that re-implements the magic and wraps the logic of the old one
    public function oldStyleAction(ActionRequest $request) {
         $preparedArguments = $this->mapAndValidate('oldStyleActionMethod', $request); // provided by ActionControllerCompatibility trait
         $preparedView = $this->prepareViewFor('oldStyleActionMethod', $request); // provided by ActionControllerCompatibility trait
         // we would provide request and view here as additional arguments as to avoid the object state.
         $result = $this->oldStyleActionMethod(...$preparedArguments, $request , $view);
         return $this->mapResultsAndViewToReponse($result, $view); // provided by ActionControllerCompatibility trait
    }

    // your classic old action method with extended signature to provide request and view, can basically do the same as before
    public function oldStyleActionMethod(MyObject $foo, string $bar, $request, $view) {
        // do stuff
        $view->....
        return ...;
    }
}

Total sketch code obviously but probably something we could rector...

[dlubitz]

I like that idea. I just would challange the name "SimpleActionController", but I have no better idea at the moment.

(Sorry for the typo change-requests at this point... I didn't expect that much and I was afraid it would be forgotten in the end)

[kitsunet]

Note that if you want to test this with Neos you need to apply the patch for it as well
neos/neos-development-collection#4738

[kitsunet]

Aaaand another unraveling, we do not really need Stop/ForwardActionException, we could just return.... For StopAction it's super easy, just return the (redirect/error/whatever) response. For ForwardAction something special is needed, it could stay an exception or we could attach the next request in some way to a response and just return that 🤔

[mhsdesign]

Ha why does it always take time to fall into place :D I like this line of thought and that would reduce the api surface of the controller again.

And the signature would change to

public function processRequest(ActionRequest $request): ActionResponse|ForwardToNextActionRequest;

right? Wasnt that what basti suggested in:

https://neos-project.slack.com/archives/C04Q0TC15/p1703858685808609?thread_ts=1703852389.168549&cid=C04Q0TC15

---

Additionally i did some small tweaking of things around:

kitsunet#4

commit for commit is quite simple but it summs up to something, if you dont like that to go all in here we probably could split this up, but then again it feels nice to have all breaking notes on one pr?

[mhsdesign]

Also i just stumbled upon the RequestInterface. It says "Contract for a dispatchable request." but due to the strict ActionRequest type in the dispatcher this is untrue now. Id say due to all the typings against ActionRequest the interface is completely useless and should be at least deprecated if not removed.

EDIT: Followup #3295

[kitsunet]

I have this whole removal of dispatched sitting around here and now wonder if I shoulnd't rather make this a separate PR, I think this PR will become a huge mess fast and no one wants to review huge messes 🙈

[mhsdesign]

Hmm i was actually thinking of branching 9.0 off to dispatcher-refactoring :D but that wouldnt be easier to review as well.

I think multiple prs is the way to go and we could possibly create depending prs where we agree to merge them not directly into this but wait first for this one to be merged.

For that workflow it would be better if this pr was made from a neos branch and not on a fork ^^ so depending prs would reside here in neos...

EDIT:

As one cannot change the source of a pr, i created a temporary branch in neos temporary-mvc-refactoring-target-branch where all depending prs should be made. All followups and stuff. Once this pr is merged into 9.0, we can retarget all prs to 9.0 as well and delete the temporary branch.

[mhsdesign]

Fyi this is my super small review pr kitsunet#4 (one commit) that should ideally go directly into this pr as the part have already been touched.

Further adjustments to e.g. introduce trait instead of simple action controller: (comment #3232 (comment))
Were extracted in an own pull request:

#3297

I would like to propose to revert the introduction of the SimpleActionController so we can discuss in a new pr what to do without blocking this pr and without having anything undiscussed merged.

[mhsdesign]

Aaaand im also not so happy with some of the naming choices in SpecialResponsesSupport. And would like to discuss this in a separate pr as well 🤔 (sorry :D)

Anything that is serious public api should deserve its own discussion and not be swallowed in a big change like this ...
If you like i could take your to separate this out into a new pr? (Feel free to force push it onto this pr #3298)

I would really like to get the basis of this change in rather quickly, as i prepared already some followups ^^

[mhsdesign]

we only need to clone as far as i know, because that will unset the dispatch state? But that will be hopefully be fixed via #3301

[mhsdesign]

I moved the simple controller over here and the special response stuff there

After merging kitsunet#4 into here and deciding if #3294 should go in here or separate (ActionResponse vs ResponseInterface) i think this is mergeable. :D

[mhsdesign]

We discussed that we like the idea to return psr responses instead of an ActionResponse

This PR will be merged into feature/dispatcher-returns-psr-responses from which we will open a blank new PR against 9.0.

This will be done to also merge #3294 into that branch, and have one final place for last reviews.

This PR with 76 comments escalated a bit, but after we stripped out the controversial parts into own prs most of the discussion that took place here is not related anymore to the core changes right now.