DISCUSSION: Replace `ActionResponse` with `ResponseInterface`?

[mhsdesign]

while working on https://github.com/neos/flow-development-collection/pull/3232 and https://github.com/neos/flow-development-collection/issues/2492

I stumbled over the fact that the whole ActionResponse abstraction is "broken".

In https://github.com/neos/flow-development-collection/pull/3294 i experimented with returning a psr response directly from a controller:

public function processRequest(ActionRequest $request): ResponseInterface;

[mhsdesign]

christian wrote

I am personally not totally opposed to this, BUT it was always meant to be a level of separation just like the ActionRequest is. Granted I see way less reason for the response to be that, especially the way it exists here, but I am sure this will be in question.

[mhsdesign]

Yes i know. I also thought a bit about this before i wanted to experiment with this.

The way i see it is that the current ActionResponse is since flow 7 broken, as it allows to "replace" the http reponse. This behaviour is in many cases currently undefined and behaves unexpectedly: https://github.com/neos/flow-development-collection/issues/2492

But even before Flow 7 and prs 7, the ActionResponse was at least partially inconsistent and the abstraction broken. The response had these properties:

• content StreamInterface
• redirectUri UriInterface
• statusCode int
• contentType string
• cookies Cookie[]
• headers array<string, array>

the first 5 are a well fit abstraction, but the headers are an escape hatch to the lower level with possibly undeclared behaviour:

What if i actually set cookies AND a Set-Cookie header? Will getCookie understand this? What if i set a custom Location header AND redirect uri, what will win and what will getHeader return? And most importantly contentType vs Content-Type.

After reading the code, i can say that the "headers" functionality is just an array carried around with no validation or transformation. So that will cause conflicts one would ran into with duplicated headers set on the final result.

Additionally the mutability of the ActionResponse is completely unpredictable to work with. We might offer this as a legacy behaviour in the action controller, but we should prevent leaking this mutable action response anywhere far outside.

The ViewInterface must support the usecase to render a psr message (see fusion view) and it doesnt really make sense to transform a lower level abstraction to a higher one as this might not always even be possible and features might not be supported.

in our case transforming the ResponseInterface to an actual ActionResponse is actually possible (without using the httpRequest hack) as the action response allows the lower level headers feature already so that could be a way https://github.com/neos/flow-development-collection/pull/3287

but im clearly in favour of working with the pretty immutable ResponseInterface directly.

For the ActionRequest it seems to be a different case, as we need the additional controller information and subrequest details.

[mhsdesign]

christian wrote

For the ActionRequest it seems to be a different case, as we need the additional controller information and subrequest details.

I did actually think about replacing it with \Psr\Http\Message\ServerRequestInterface::withAttribute at some point, there is still some stuff to be considered, especially nesting behavior for subrequests (that I think we need for some functionality) but in general we could store our special stuff in custom attributes.

[mhsdesign]

@bwaidelich mentioned once:

I remember that this kind of logic holes were the reason why I was in favor of a response abstraction that is not so closely related to the HTTP response.

but im not sure that we can ever found a well fit response abstraction and then we will always still have the problem that a view might return a psr response which is too low level for us to handle, if were not that low level ourselves.

---

It seems this ActionResponse::setHttpHeader is not that ooold. It was introduce with Flow 7, and previously one would use a SetHeader component: https://github.com/neos/flow-development-collection/pull/2219

So it seems the abstraction of the response was intact before psr 7.

Now we just need to find out what to do :D

Either we remove httpResponse and headers from the ActionResponse, which will raise the question how to set any of the two, or we remove the whole ActionResponse (at least from the dispatcher, it might be still used internally)

[kitsunet]

Given that we haven't had that many complains about it in the last years I feel "broken" over-dramatizes this 😆 it certainly has many problems but broken it is not, as it demonstratively works in many common usecases.

[kitsunet]

Yeah the setHeader was introduced on pressure of "ease of use" from people. I liked the SetHeader thing. it was clearly marked as escape hatch

[mhsdesign]

But when seeing that all these steps taken to make the ActionResponse more and more coupled to the http response, that far than one could transform the one to the other, it seems only natural to take the next step and replace it completely with the psr ResponseInterface? As our abstraction is not necessary anymore as we want "ease of use", and that seems only fine?

[mhsdesign]

And it seems we need easy of use. I have seen now way to many times people doing this:

public function myAction...
    hearder(...
    echo ...
    die();

i hope that once we allow to directly return a psr response interface from the action (which doesnt work currently!) and once we use this ourselves instead of manipulating $this->response people will see and understand. Hopefully.

public function myAction...
    return new Response()

[bwaidelich]

What about supporting both, or even more return types?

[kitsunet]

We need an anti corruption layer though, an action might return more, but then what about the processRequest interface method which is the actual public border for controllers? that would already be somewhat nasty to be multi return, next step would be dispatcher and at that point I would really like to know what I get from my controller. After that we are in the middlewares which definitely want PSR.

[mhsdesign]

id say inside the "historic" action controller we can allow all kinds of things, like we already do from views string|ActionResponse|ResponseInterface|StreamInterface|\Stringable.

In an *Action of such an ActionController we can currently return a value, which will be set as content of our response and must be either a StreamInterface or anything else Utils::streamFor accepts: resource|string|int|float|bool|callable|\Iterator|\Stringable

Actually returning an ActionResponse or a ResponseInterface is both not supported, but i have no preservations in allowing both, or at least the ResponseInterface.

The only way to mutate the response currently is via $this->response, but id like to deprecate that in favour of returning.

---

The new SimpleActionController, might already just support the ResponseInterface internally and return it directly. (Especially since using ActionResponse->buildHttpResponse() is that easy if one gets ahold of an action response somehow).

---

For the public api of the controller \Neos\Flow\Mvc\Controller\ControllerInterface::processRequest And the dispatcher \Neos\Flow\Mvc\Dispatcher::dispatch i would very like to make them operate on the psr ResponseInterface directly with no conditional return type, so its obvious what to expect.

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

interface ControllerInterface
{
    public function processRequest(ActionRequest $request): ResponseInterface;
}

class MyController extends ActionController
{
    public function someAction()
    {
        return new \GuzzleHttp\Psr7\Response(body: 'hi');
    }

    public function someOtherAction()
    {
        // legacy but will still work
        $this->response->setCookie(new Cookie(...));
        return "my body";
    }
}

[mhsdesign]

Ha it seems flow went through much more than i could have imagined. The ActionResponse was actually a new abstraction introduced in 5.3 in preparation for psr7 https://github.com/neos/flow-development-collection/pull/1531 While looking into tooling for headers - see https://github.com/neos/flow-development-collection/issues/3291 - my thoughs were only confirmed. Im completely blown away how much work was put into psr7 AND psr15 refactorings. And flow actually looks in that parts quite shiny now.

Given that we haven't had that many complains about it in the last years I feel "broken" over-dramatizes this 😆 it certainly has many problems but broken it is not, as it demonstratively works in many common usecases.

So @kitsunet please forgive me from calling it broken. I was absolutely in the wrong of doing so ^^. Everything was super well discussed and happened exactly as it should :D Thank you all for the effort.

But back to why the ActionResponse knows its headers. I found an explanation when reading through @bwaidelich extensive (thank you) comment with the discussion how to move to psr15 https://github.com/neos/flow-development-collection/issues/2019#issuecomment-720463971. Its absolutely understandable that the behaviour of the ActionResponse was changed to its current state. The initial SetHeaderComponent and ReplaceHttpResponseComponent can just not be implemented differently in psr15 world:

[...] the fact that ActionResponse::setComponentParameter() won't work with the middleware approach since it is only possible to pass information (other than the HTTP response) down into the middleware chain, but the Dispatch MW has to be the inner most.

So its only natural that the response needs to now its headers.

[mhsdesign]

Funnily the idea to use Psr response is not that new ^^

In Flow 5.3 the ActionResponse actually implemented for backwards compatibility the psr ResponseInterface.

https://github.com/neos/flow-development-collection/blob/cc6640197d93caebd06b35ff851d1433cff0159b/Neos.Flow/Classes/Mvc/DispatchComponent.php#L82-L84