RFC: PSR-7 continuation

[christianm (Christian Müller)]

This is about continuing to move towards fully embracing the PSR-7 HTTP standard. This has relatively far fetching consequences and is rather low level so before I go on with the change I started and invest a lot of time, I wanted to make sure we are on the same page beforehand.

We implement PSR-7 interfaces in our HTTP classes but generally do not use any of that, but obviously only if we were to use it fully we can expect some benefit.
So we are at a crossroads with this and I am willing to pick this up again.

Effectively we only have two ways to go forward:

a) The easy way out, we abandon PSR-7 and remove it again
b) We go all the way, which will mean some breaking changes for the next major, a lot of refactoring and changing of mindset and code.

To do b) I roughly collected the steps in this overview: State of PSR in Flow

But to give this a bit more detail:

We obviously need to deprecate all methods in our HTTP classes that are not covered by the interfaces. These methods must be replaced by helpers or boilerplate code throughout our codebase then.

Then we need to change our code to deal with replacing of request and response objects instead of manipulating the same instance again and again. That might introduce some changes in places where we rely on this behaviour, eg. we now always need a return value while previously just having the request object was enough.

Next up we would adjust all type hints to hint only to the PSR interfaces and make sure at this point that nothing is used that isn’t covered by them. This shouldn’t break existing code but suddenly existing code would probably use methods that “officially” no longer exist.

At this point we might as well think about a way to retrieve the current component context to make it easier to get to a request/response. But that’s optional.

Finally we would remove all deprecated methods in the next major.

Now while there are two options to go with, this is more of a way to get potential discussions about this done and your backing before investing a lot of time. Option a) isn’t really an option for me in this project.

• a) We don’t do PSR-7
• b) Let’s get PSR-7 done, done

[jonnitto (Jon Uhlmann)]

As I am not a PHP developer, I’ll go with the majority. But thank you, Christian, to bring this up!

[christianm (Christian Müller)]

This would be part one:

TASK: Deprecating non PSR-7 methods

Still need to polish but could be merge ready soon-ish

[gerhard_boden]

Thanks for taking the burden to fully implement this important PSR standard!

[markusguenther (Markus Günther)]

Puh thats a lot of work but I guess it is worth it. A framework that follows the standards will be easier to use in combination of other systems.

And PSR-7 is one of the most important standards for interoperability…

[bwaidelich (Bastian Waidelich)]

The voting from August was unambiguous, so let’s get a grip on this one

I talked to @christianm in length about this and every time I forget half of what we discussed before.
So let me try to summarize what we came up with so far¹:

TL;DR:

This post wasn’t meant to get that long, but there is no shorter way. So if it’s too long: Just don’t read it

The current architecture

http://yuml.me/edit/6fe5eb0e

Response

http://yuml.me/edit/710be027

The new architecture

Request

http://yuml.me/edit/e1e96034

Response

http://yuml.me/edit/18d4cfb1

Consequences

Http\Request

Will be removed without replacement. Instead we should code against Psr\Http\Message\ServerRequestInterface everywhere we talk about the incoming HTTP request and against Psr\Http\Message\RequestInterface for outgoing requests.
For the implementation we could just use one of the existing packages, for example guzzlehttp/psr7.

ActionRequest

…is no longer related to the HTTP Request directly as that led to lots of confusion and inter-dependencies like the above.
Instead it should get everything it needs (controllerName, controllerActionName, arguments , format…) at construction time.

To be discussed: Can we get rid of nested ActionRequests and instead add something like ActionRequest::withArgumentNamespace(string $namespace): ActionRequest?

For the usual action handling it should not be required to access the Main (or HTTP) request. If that’s needed, it should be done via the RequestHandler (see below)

The ActionRequest can probably be immutable in the future

CommandRequest

(formerly Cli\Request)

Same as ActionRequest, but they definitely don’t need a common interface and command requests definitely don’t need to be nested

ActionResponse

(formerly Mvc\Response)

Currently the Mvc\Response is not used at all(!). Instead actions are dispatched with the main Http\Response via the DispatchComponent (and by creating a new “sub-Http\Response” for sub requests like plugins, widgets, form, …)

The ActionResponse has to stay mutable to respect the whole dispatch architecture of our MVC framework (that changes the response until its dispatched flag is set).

If we re-introduce a dedicated ActionResponse it should never mutate the HTTP Response itself. Instead it’s the dispatchers responsibility to apply changes to the HTTP response (see example below)

IMO the ActionResponse should be specific to MVC, but probably needs some setters for convenience. For example:

final class ActionResponse
{
   public function setContent(string $content): void

   public function setContentType(string $contentType): void

   public function setRedirectUri(UriInterface $uri, int $statusCode = 303): void

   public function setStatusCode(int $statusCode): void
}

To be discussed: Are there more properties of the ActionResponse that have to be mutable from the action, like setHeader($name, $value)?

HttpRequestHandlerInterface

Not directly related to PSR-7 but due to the consequences mentioned above, we would have to change the interface from

interface HttpRequestHandlerInterface extends RequestHandlerInterface
{
    public function getHttpRequest();

    public function getHttpResponse();
}

to something like:

interface HttpRequestHandlerInterface extends RequestHandlerInterface
{
    public function getComponentContext(): ComponentContext;
}

That would finally give us an API way to get hold of HTTP Request & Response and to “mutate” them if really needed:

class SomeController extends ActionController
{
    /**
     * @Flow\Inject
     * HttpRequestHandlerInterface
     */
    protected $requestHandler;

    public function someAction()
    {
        $componentChain = $this->requestHandler->getComponentContext();
        $httpResponse = $componentChain->getHttpResponse()->withHeader('X-Foo', 'Bar');
        $componentChain->replaceHttpResponse($httpResponse);
    }
}

Example Dispatch

This is how I could imagine the DispatchComponent to look after the rework:

// ...
$actionRequest = ActionRequest::fromArguments($arguments);
$actionResponse = new ActionResponse();
$this->dispatcher->dispatch($actionRequest, $actionResponse);
$httpResponse = $actionResponse->mergeToHttpResponse($componentContext->getHttpResponse());
$componentContext->replaceHttpResponse($httpResponse);

Sub Request

// ...
$pluginRequest = $actionRequest->withArgumentNamespace($pluginNamespace)->withArgument($pluginArguments);
$pluginResponse = new ActionResponse();
$this->dispatcher->dispatch($pluginRequest, $pluginResponse);
$actionResponse->mergeActionResponse($pluginResponse);

---

¹ it’s based on what I discussed with Christian, but I added some new suggestions that we didn’t talk about yet

[christianm (Christian Müller)]

Will answer longer later. So far all makes sense and seems in line with my thoughts.

Funnily the code in the dispatch component I have here pretty much looks like you sketched.

[aberl (Alexander Berl)]

Thanks for the write-up, that looks pretty good already. Here’s a little bit of input:

IMO the ActionResponse should be specific to MVC, but probably needs some setters for convenience.

Yes! The controller still needs to be able to access/set all the important Response parts, namely Body/Content, Status, Headers and Cookies. Especially the headers are a core feature used a lot, in order to set cache-control or CORS headers.
The Body/Content can be conveniently set by returning a string/resource from the action. A redirect could be handled by returning a RedirectResponse (instead of throwing/catching).

That would finally give us an API way to get hold of HTTP Request & Response and to “mutate” them if really needed:

class SomeController extends ActionController
{
    /**
     * @Flow\Inject
     * HttpRequestHandlerInterface
     */
    protected $requestHandler;

    public function someAction()
    {
        $componentChain = $this->requestHandler->getComponentContext();
        $httpResponse = $componentChain->getHttpResponse()->withHeader('X-Foo', 'Bar');
        $componentChain->replaceHttpResponse($httpResponse);
    }
}

Please, let’s not do this! If you want to alter the response, the API for that should be the MvcResponse. If you want to alter the Request, you should create an HttpComponent (or later http middleware after PSR-15). The component context should not be changed from inside the Controller at all, IMO not even be visible. What the controller might need though, is (read) access to the Http Request, in order to make some decisions. This is currently possible through the RequestHandler as is. But let’s keep the controller decoupled from the Dispatcher/ComponentChain stuff.

In terms of PSR-15 the Http Components are all a middleware and the DispatchComponent is the final/inner RequestHandler, that creates a Response and delegates filling it through the Mvc ActionResponse/Controller API to the application code.

[bwaidelich (Bastian Waidelich)]

@aberl Thanks for your feedback, valuable as usual!

@aberl

The controller still needs to be able to access/set all the important Response parts, namely Body/Content, Status, Headers and Cookies

I don’t think so to be honest. Well yes and no… Let me elaborate:
IMO it’s the responsibility of the action dispatcher (1st level HTTP component, below: widget, form, plugin, …) to pass those details up the chain and to the HTTP response.
The common scenario is: The action sets content that is embedded in the parent response.

Additionally the action might want to specify its content type and/or request a redirect.

Setting cache control or CORS headers doesn’t really make sense directly from within the action because it could be rendered nested within a content element within a widget within a form finisher (OK, that’s probably exaggerated but you get the point).

Instead it might make sense to extend the ActionResponse by some cache lifetime, tags and hash properties that then could be calculated to proper cache headers (e.g. E-Tag, …).

@aberl

A redirect could be handled by returning a RedirectResponse

Yes… Using an exception to control the flow was probably not the best idea, but the current MVC architecture builds upon the fact of having a mutable Request and Response object that can’t be replaced.
Changing this might be a project worth thinking about, but I’m not sure if we should make the (breaking) change any bigger!?

@aberl

Please, let’s not do this! If you want to alter the response, the API for that should be the MvcResponse.

I agree, and I probably should have put it more clearly:
The API should be in the ActionResponse but it should be restrictive to allow for better composition and nesting (see above). Therefore probably people will reach the limits of the API (until it is extended to serve all common use cases) and will want to do nasty things directly from the action.

With the mutable HTTP request/response they could do (cover your eyes):

$bootstrap = Bootstrap::$staticObjectManager->get(Bootstrap::class);
/* @var Bootstrap $bootstrap */
$requestHandler = $bootstrap->getActiveRequestHandler();
if ($requestHandler instanceof HttpRequestHandlerInterface) {
    $requestHandler->getHttpResponse()->setStatus(123);
}

That’s no longer possible. So we thought it might be necessary to have a way to get hold of the HTTP Component chain somehow.
But you are right, probably we should not mess with that and keep things decoupled even if it means that people will have to rework their side-effecty-code (including ourselves)

@aberl

In terms of PSR-15 the Http Components are all a middleware and the DispatchComponent is the final/inner RequestHandler

It’s good that you mention PSR-15 again, we should not forget about it. But where do you read that the dispatch component should be the final handler?

[aberl (Alexander Berl)]

@bwaidelich

Setting cache control or CORS headers doesn’t really make sense directly from within the action because it could be rendered nested within a content element…

That’s a valid point, but there are also plain Flow applications that will build up the whole Response inside the single action. It would be bad to force them to shift such decisions to a http component, where they need to build one big huge switch statement to change headers based on the controller+action combination.

Maybe we have an architectural issue here, that we handle both cases exactly the same? What if we had a generic ActionResponse that just renders content and a lower level MvcResponse that enriches the generic one with header and cookies? That would probably make the API more complicated and lead to weird if ($this->response instanceof MvcResponse) { checks inside actions.

Using an exception to control the flow was probably not the best idea, but the current MVC architecture builds upon the fact of having a mutable Request and Response object that can’t be replaced

Sorry, I think I don’t follow What does the mutablity of Request/Response have to do with throwing exceptions to redirect/forward?

Changing this might be a project worth thinking about, but I’m not sure if we should make the (breaking) change any bigger!?

Yes. We could go both ways to keep things b/c for now and see how it works out first. An action could then either throw an exception as before or return an object to get the same result of redirect/forward.

Therefore probably people will reach the limits of the API (until it is extended to serve all common use cases) and will want to do nasty things directly from the action.

I’m unsure there are so many nasty things you can do with the response alone. Status, body, headers (and cookies and/or cache-control as a special/convenience setter for a specific header).

Maybe to make that clear: I consider the API of the Mvc stack to be Controller::action(ActionRequest): ActionResponse - of course not literally, but conceptually. So theoretically, the ActionRequest could be immutable and the ActionResponse be a very shallow mutable API to alter the actual HttpResponse outside the Mvc dispatch. It could even contain a couple of the API methods we deprecated in HttpResponse, like appendContent TASK: Deprecating non PSR-7 methods by kitsunet · Pull Request #1366 · neos/flow-development-collection · GitHub

With the mutable HTTP request/response they could do (cover your eyes): [ugly code] That’s no longer possible.

And that is a good thing If you want to interact with the http response directly and alter it, create a http component/middleware.

But where do you read that the dispatch component should be the final handler?

Following PSR-15 Meta Document - PHP-FIG
The innermost middleware is the one that either creates the PSR-7 response, or delegates to last resort RequestHandler that can create a fallback response (i.e. NotFoundResponse or a RedirectResponse to a default page)

[bwaidelich (Bastian Waidelich)]

I spoke to @christianm again and we talked about a backwards compatible(!) solution for the next LTS that already gives people the chance to adjust their code to (or at least get acquainted with) the “new way of doing things” with Neos 5.0:

• We don’t add HttpRequestHandlerInterface::getComponentContext() just yet¹, but instead only deprecate HttpRequestHandlerInterface:: getHttpRequest() and HttpRequestHandlerInterface:: getHttpResponse() with an explanation to work around this dependency (see below)
• We add another method setComponentParameter() to the ActionResponse that allows for “unplanned extensibility”.

The resulting signature of the ActionResponse would then be sth like:

final class ActionResponse
{
   public function setContent(string $content): void

   public function setContentType(string $contentType): void

   public function setRedirectUri(UriInterface $uri, int $statusCode = 303): void

   public function setStatusCode(int $statusCode): void

   /** new **/
   public function setComponentParameter(string $componentClassName, string $parameterName, $value): void
}

With that in place, people could still mess with the HTTP request/response as much as they want, but via the ActionResponse:

$this->response->setComponentParameter(MyComponent::class, 'setMySpecialHeader', 'foo');

and write a corresponding HTTP Component:

final class MyComponent implements ComponentInterface
{

    public function handle(ComponentContext $componentContext)
    {
        $specialHeader = $componentContext->getParameter(static::class, 'setMySpecialHeader');
        if ($specialHeader !== null) {
            $modifiedResponse = $componentContext->getHttpResponse()->withAddedHeader('X-My-Header', $specialHeader);
            $componentContext->replaceHttpResponse($modifiedResponse);
        }
    }
}

And, in response to some of @aberl’s comments:

@aberl

there are also plain Flow applications that will build up the whole Response inside the single action. It would be bad to force them to shift such decisions to a http component, where they need to build one big huge switch statement to change headers based on the controller+action combination.

Yes, good point, but I still think that we should not encourage that kind of architecture: For middleware kind of cases MVC is not the matching pattern IMO, a custom HTTP component is much better suited.
Having said that, with the above it would be possible to replace the whole Response ofc:

$this->response->setComponentParameter(MyComponent::class, 'replaceHttpResponse', $myHttpResponse);

@aberl

What does the mutablity of Request/Response have to do with throwing exceptions to redirect/forward?

Sorry, I should have explained myself a bit clearer.
Controller::action(ActionRequest): ActionResponse
would make a lot of sense and in case of a redirect that could be a special RedirectResponse like you suggested.
But we currently have

Dispatcher::dispatch(RequestInterface $request, ResponseInterface $response): void

currently and solve redirects via an endless loop and exceptions.
I’m all for changing that at some point, but it would be yet another major breaking change.

---

¹ I don’t think that we need to re-introduce a way to get hold of the HTTP request/response or ComponentContext at all. With the extension mentioned above this should not be required. And (because it’s no interface) we could always extend the ActionResponse to provide more features out of the box (like “E-Tag”, “Content lifetime”, …)

[sorenmalling (Søren Malling)]

Follow up on this. I liked that we work towards PSR-15 middleware instead of the HTTP Component concept currently in Flow. Does this collide with some of these thougts?

[aberl (Alexander Berl)]

Some update: in the meantime, the PSR-7 change has been merged to Flow master, which will potentially end up as Flow 6.0. Now we need to iron out all the things we did not see and then also look how we can move towards PSR-15 middleware, which would be a big visible win.

Things to consider for psr-15:

• how to configure 3rd party middlewares into the kernel
• dynamic (runtime) and fixed (configuration/compiletime) middleware setup
• which middlewares to add by default
• b/c with HTTP components
• ?