ActionResponse::replaceHttpResponse behaves unexpectedly

[albe]

Related to #2478

The replaceHttpResponse() should make sure that the ContentType (and any other headers really) that is potentially set inside a HttpResponse which is returned from a view should be applied. But apparently it isn't - sooo... what is happening then? Only thing I could imagine is this:

• the ActionResponse ($this->response) already has a ContentType set (e.g. from content negotiation)
• after replacing the HttpResponse, the ActionResponse gets merged into the HttpResponse (inside the dispatch middleware - see https://github.com/neos/flow-development-collection/blob/7.0/Neos.Flow/Classes/Mvc/DispatchMiddleware.php#L46 resp. https://github.com/neos/flow-development-collection/blob/7.0/Neos.Flow/Classes/Mvc/ActionResponse.php#L314)
• at that place, the ContentType from the view returned HttpResponse is overwritten again

This hot-fix pr https://github.com/neos/flow-development-collection/pull/2478 fixed the outlined single symptom, but there is a bigger general logic issue below that probably needs to be fixed - i.e. a replaceHttpResponse() not preventing the ActionResponse from overwriting properties in that HttpResponse again. Maybe that is even intended for some properties of the ActionResponse? Dunno. It basically boils down to what is expectation for the response of an action like this:

public function weirdResponseAction()
{
    $this->response->setContentType('application/xml');
    $this->response->setRedirectUri(new Uri('somewhereelse'), 304);
    $this->response->setContent('Hello World!');
    $this->response->setCookie(new Cookie(...));
    $this->response->setHttpHeader('X-My-Header', 'My-Header-Value');
    $this->response->replaceHttpResponse(new Response(200, ['Content-Type' => 'application/json'])); // which is equivalent to letting the view render a PSR Response
}

• should the response status be 200 or 304?
• should the response header Location be set to indicate a redirect? (should probably be in sync with the above)
• should the response content be "Hello World!" or empty?
• should the response content type be "application/json" or "application/xml"?
• should the response header X-My-Header be set?
• should the response contain a Set-Cookie header?
• should any of the above change if the replaceHttpResponse() call happened earlier?

So is replaceHttpResponse() a full/partial/conditional escape hatch from the ActionResponse abstraction?

[albe]

/cc @bwaidelich @kitsunet

[bwaidelich]

@albe Sorry for the lack of feedback – I have tried to understand the issue multiple times but it confuses me. 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 let's try:

should the response status be 200 or 304?

304 for the ActionResponse, 200 for the final HTTP response (since that was replaced)

should the response header Location be set to indicate a redirect?

not in the ActionResponse (since it wasn't set there explicitly) but in the final HTTP response, obviously

should the response content be "Hello World!" or empty?

"Hello World!" (and empty in the replaced HTTP reponse, probably)

should the response content type be "application/json" or "application/xml"?

json in the ActionResponse, xml in the final HTTP response

well, and so on..

In short I'd say replaceHttpResponse() should do just that, replace the response

[albe]

should the response status be 200 or 304?

304 for the ActionResponse, 200 for the final HTTP response (since that was replaced)

should the response header Location be set to indicate a redirect?

not in the ActionResponse (since it wasn't set there explicitly) but in the final HTTP response, obviously

So a 200 with a Location: somewhere Header? That sounds weird.

json in the ActionResponse, xml in the final HTTP response

Did you mix the two up? Because in my example it was $this->response->setContentType('application/xml').

In short I'd say replaceHttpResponse() should do just that, replace the response

I would say so too, though it somehow feels weird then to completely ignore all $this->response->setX() calls that might have happened in the action. Would it also be the same if replaceHttpResponse() were called before all the response->setX() calls? Probably yes.

Maybe we need to revisit the reason we have the replaceHttpResponse() method in the ActionResponse at all, and I think, IIRC, it was for Neos to keep allowing it to render a HttpResponse. So maybe we just need to find another way for the view/action to render a PSR Response and remove this method from the ActionResponse to make it less ambiguous? i.e. if you want to build a HttpResponse in your action/view, you need to return that and all $this->response->... calls are (hopefully) more obviously noops.

[bwaidelich]

So a 200 with a Location: somewhere Header? That sounds weird.

No, I'd expect replaceHttpResponse() to replace all previously set headers, too.

Did you mix the two up?

yes, sorry.

Maybe we need to revisit the reason we have the replaceHttpResponse() method in the ActionResponse at all, and I think, IIRC, it was for Neos to keep allowing it to render a HttpResponse

Yes, I think so too

[mhsdesign]

Hi :D I just stumbled over the replaceHttpResponse method and the merging in buildHttpResponse and the code already looks like trouble due to the possible ambiguity.

I completely agree this replaceHttpResponse completely breaks the abstraction of the ActionResponse and undermines it ^^. So either we say that the ActionResponse is rather a mutable ResponseBuilder which always stores the state in a current ResponseInterface and finally returns it, or the logic to replace the ResponseInterface must be handled differently.

These changes were introduced with: https://github.com/neos/flow-development-collection/pull/2207

it was for Neos to keep allowing it to render a HttpResponse

Yes the Neos Fusion view is one of the only views to return a psr response due to the Neos.Fusion:Http.Message: https://github.com/neos/neos-development-collection/blob/b7cde7481ba42d3f7a76949c84d7dfc8b7fdd27c/Neos.Neos/Classes/View/FusionView.php#L64

And it seems the old Flow 6.3 ReplaceHttpResponseComponent and ComponentContext::replaceHttpResponse worked exactly like bastian outlined:

https://github.com/neos/flow-development-collection/blob/701072a0c240a32442d5b80ad51464ce9571d57a/Neos.Flow/Classes/Mvc/Controller/ActionController.php#L823-L826

So that would be a reason to restore this predictable behaviour again.

I would propose to create an action response from a httpResponse. As this is currently not possible as the object reference is passed and expected to be mutated rather than a new response object to be returned, we could reset the action response first and then copy all the details over.

Edit: i would vouch for removing the ActionResponse: https://github.com/neos/flow-development-collection/issues/3289