Silverstripe-framework: Use PSR-7 for HTTPRequest and HTTPResponse

[chillu]

We should use http://www.php-fig.org/psr/psr-7/, since it makes it easier to apply generic middleware to SilverStripe projects (not necessarily aware of the SilverStripe framework).

One example of this would be an OAuth2 server like http://http://oauth2.thephpleague.com/, which we could tie into SilverStripe through the existing RequestFilter API.

@robbieaverill has made a start on this with https://github.com/robbieaverill/psr7-adapters

It'll also improve our isolation by handling $_SERVER through HTTPRequest. At the moment, features like BasicAuth in core can't be decoupled from those globals since there's no object encapsulating this state.

@sminnee @tractorcow Do you think this is small enough to make it into 4.0? Haven't read through the PSR-7 spec yet, thought I'd just start the discussion. Or would it make more sense to leave this until a larger scale refactoring of the controller API (e.g. Reduce coupling to Controller::curr())

Related

• RFC: App object

[robbieaverill]

My two cents from writing the adapters:

• Most of the required data for PSR-7's [Server]RequestInterface is already available in HTTPRequest. There's just a couple of parts that probably wouldn't be used very often that aren't exposed already, e.g. cookies and server variables.
• The ResponseInterface is very similar already to HTTPResponse
• The primary difference from the SilverStripe interfaces is the immutability, and getting used to using the public API like withXyz(... instead of add...
• Consideration for whether to bundle guzzlehttp/psr7 into the framework as a dependency (like my adapter classes are doing for the sake of simplicity), or whether to change the HTTP classes to be PSR-7 compliant instead (I ended up doing most of this before I realised that Guzzle had a package - then face palmed hard - there's not _too much_ work in it)

My opinion: It would be useful to have some form of PSR-7 compliance in the framework before it's stable - not sure which form it would take though.

[sminnee]

@chillu It would be good to sketch out what would be required to allow the integration of that OAuth server? I think that approaching "adopt PSR-7" in a vacuum might be counterproductive.

In particular, it looks like the OAuth module expects not only PSR-7 but a middleware system? If this card was "allow use of PSR-7 middlewares with Director" it might push us in the right direction?

With that focus in place, we could determine whether a wrapper-object or new methods on our existing objects to implement the requisite interfaces was better.

[sminnee]

In particular, the middleware interface is this (technically it's a callable not an interface, but you get the idea:

public function __invoke(Psr\Http\Message\ServerRequestInterface $request, Psr\Http\Message\ResponseInterface $response, callable $next)

which returns Psr\Http\Message\ResponseInterface

[sminnee]

PSR-15, still in draft, describes a middleware approach. However, it's different (and more complex) than what The League have done.
https://github.com/php-fig/fig-standards/blob/master/proposed/http-middleware/middleware.md

[sminnee]

It looks like The League have provided middleware that works with Slim. It seems that Silex does things differently. So working with "generic middlewares" isn't really practical at this stage.

Thankfully we can still be more interoperable without this. The core of one of the OAuth middleware is:

$response = $this->server->respondToAccessTokenRequest($request, $response);

So it expects to receive a response object detailing the request produced so far, and return a mutated response object. If we allowed some kind of interaction of this pattern, then it would be a few lines of code to build middlewares out of core PSR-7-aware handlers.

In order to integrate this with SilverStripe we'd need for SilverStripe code to work directly with PSR-7 response objects, otherwise we'd be converting the response to/from PSR-7 objects everytime time we step in and out of PSR-7-aware code.

The biggest hurdle to use modifying our existing request/response objects is that the PSR excepts immutable object with the exception of getBody(), which returns a mutable stream interface.

[micmania1]

The StreamInterface would solve the problem with streaming large private assets through PHP.

[chillu]

It looks like even Symfony isn't too enthusiastic about switching their HttpFoundation request/response classes to PSR-7, calling it a "change to the whole architecture" 1. They've got a PSR-7 adapter (docs) just like Robbie's. I can't see any active discussions or work on the next major Symfony release (4.0) supporting PSR-7 directly either. Laravel just uses Symfony's HttpFoundation so they're in the same boat.

More discussion:
https://github.com/symfony/symfony/issues/15414
https://github.com/symfony/symfony/issues/17392
https://github.com/symfony/symfony/issues/14723

I tend to agree. We can proxy through diverging APIs like getVars() vs. getQueryParams(), and postVars() vs. getParsedBody(). But: Most SilverStripe projects will at some point call setBody(), setStatusCode() or addHeader() - and all of these now need to reassign the response object due to immutability (related criticism). Which gets even more awkward because Controller has a $response property which needs to be reassigned on mutation.

My feeling is that it's too disruptive for SS4. The main benefit of PSR-7 to us as a framework is using generic middleware, and we can still achieve that through the existing PSR-7 bridge.

[sminnee]

Agree, too disruptive for SS4 but I'd like to see it in SS5 if possible. I think we're less fearful of "changes to the whole architecture" than most. ;-)

[tractorcow]

Shifting it to affects/v5

[robbieaverill]

I'm keen to see this make SS5 aswell.

PSR-15 is now accepted as well.

Assuming we're going to be adjusting our HTTPRequest and HTTPResponses to match the interfaces, I guess the question about how we approach this is whether we do it directly in the HTTP classes or use a bridge module like Symfony have done (as @chillu pointed out).

My gut feeling is that we should jump into it. We can deprecate methods like ->setBody() at 5.0 rather than removing them in 5.0 to minimise dev disruption maybe.

[robbieaverill]

@chillu are you happy if I pick this up in my own time?