Initial support

[dunglas]

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	symfony/symfony-docs#5331

• Composer dependencies
• Interfaces
• Stub
• HttpFoundationFactory implementation
• DiactorosFactory implementation
• StreamedResponse

[stof]

I'm not sure we need interfaces here (there is no reason to reimplement the conversion in a different way IMO). And I would rather name them converters than factories

[dunglas]

@stof I agree but it was to be consistent with the other side, and the other side needs an interface.

[stof]

does it actually need one though ? If we want to make it usable with other PSR-7 implementation, we could just accept a constructor callable to instantiate the Request/Response rather than forcing to reimplement the whole conversion (which is not only about the constructor)

[stof]

My idea was to have separate classes for the Request and the Response converters, because a typical use case will need to convert the Request one way and the Response the other way.

[dunglas]

The userland API I was thinking about was:

public function myAction(Request $mySymfonyRequest)
{
    $psr7Request = $this->get('http_message_factory')->createRequest($mySymfonyRequest);
    $psr7Response = $somePsr7Middleware->execute($psr7Request)

    return $this->get('http_foundation_factory')->createResponse($psr7Response);
}

What do you think about it?

[dunglas]

As you can see, the UploadedFile handling is tricky because PSR-7 doesn't give access to the original $_FILES['my_file']['tmp_name'] key.
My current implementation works but must be double-checked and enhanced concerning error handling, security and performances.

[stof]

@dunglas I don't think we will register them as services. Using them in a controller looks really weird. The use case is more for middleware compatibily IMO (a zend stragility middleware wrapping HttpKernel or an HttpKernel wrapping a zend diactoros server for instance).
And this is also why I don't think it make sense to have the Request and Response converters in the same class as you will not need both anyway

[stof]

I don't think this should be public

[dunglas]

Don't you think it can be useful in many cases to convert from Symfony to PSR UploadedFile and vice versa?

[stof]

Well, we could make it public later if we get requests about it. But for now, I would keep it private until we get more feedback (increasing the visibility of a method is easy. Decreasing it is forbidden outside major versions, and it is still a pain for major versions if we want to provide an easy upgrade)

[fabpot]

I was more thinking about having proxy classes. Having factories and services seems overkill here. The "converters" are going to be used at the edges anyway, not in your core code.

[stof]

@fabpot having proxy classes is possible for the HttpFoundation -> PSR-7 is possible (this is what https://github.com/Sam-Burns/psr7-symfony-httpfoundation started to do), but it is also a pain to ensure that we can never perform external mutation of the wrapped objects.
However, the opposite way is not possible (HttpFoundation does not define interfaces that we could reimplement on top of PSR-7).
And having only the 1-way adapter is useless as a typical usecase will need to convert the request one way and the response the other way.

[stof]

And this library only implements the easy parts for now (uploaded files and the body stream are not yet adapted)

[dunglas]

Agree with @stof, creating proxies will be a huge pain IMO.

The service was just an example. I'm thinking about a way to introduce progressively PSR-7 in the core of Symfony.

What do you think about the following:

Additionally to HttpFoundation\Request, allowing to inject Message\ServerRequestInterface in actions of controllers:

public function myAction(\Psr\Http\Message\ServerRequestInterface $psrRequest);

We will only need to adapt the following snippet https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php#L115 to convert the Symfony Request to the PSR one using the factory.

Similarly, it is possible to add methods returning instances of PSR ServerRequestInterface and ResponseInterface (using internally the factory) to Symfony\Component\HttpKernel\Event and Symfony\Component\HttpKernel\\GetResponseEvent.

It will improve drastically the interoperability of Symfony with other PSR-7 compliant libraries.

[jakzal]

@stof could you make a brain dump of what you had in mind to implement? :)

[stof]

@dunglas returning a PSR request from events would be a pain, because listeners are allowed to mutate the request, and we would then have to provide a way to set the updated PSR request in the event, which would then have to mutate the existing HttpFoundation Request. Too much trouble IMO.

[stof]

@jakzal My idea was 2 have 4 converter classes (Request/Response, both ways => 4 needed).
This is approximately what @dunglas started except he put the Request and Response conversion in the same class (which does not match the needs of actual use cases) and that I don't think interfaces are useful here (I don't see a reason to reimplement the whole conversion yourselves just to use a different PSR-7 implementation)

[dunglas]

@stof I've no strong opinion against splitting factories for request / response, lets do that if every one agree, but I cannot find the problem with the following:

$httpFoundationFactory = new \Symfony\Bridge\PsrHttpMessage\Factory\HttpFoundationFactory();
$psrFactory = new \Symfony\Bridge\PsrHttpMessage\Factory\DiactorosFactory();

$response = new \Zend\Diactoros\Response();

$done = function($request, $reponse) {
   $symfonyResponse = $httpFoundationFactory->createResponse($reponse);
  // Do the something the Symfony way
};

$server = new \Zend\Diactoros\Server(
    function ($psrFactory->createRequest($symfonyRequest), $response, $done) {
        $response->getBody()->write("Hello world!");    },
    $request,
    $response
);

[dunglas]

Added cookies support.

[dunglas]

All issues raised by @stof (thank you!) should be fixed now.

[stof]

why not reusing $symfonyRequest->getRequestUri() ?

[dunglas]

Because Symfony sometimes generates bad URIs. For instance with the request I build in the test, the generated URL is http://:/ (because some $_SERVER parameters are missing).

[weaverryan]

I like @stof's general idea - we should use as much information from Symfony's Request as possible, since it's already calculated there and I expect information to be the same. For example, what if the scheme is http in Symfony's Request, but due to some x-forwarded-proto differences (perhaps you don't have something a proxy trusted in Symfony's Request), Diactoros reads the untrusted x-forwarded-proto and treats it as https.

[dunglas]

Why not. So I'll fix the test with the correct $_SEVER keys but if we got some complaints about strange bugs here we should fix HttpFoundation directly.

[weaverryan]

This means that the streamed content will in fact be read into memory, correct?

[dunglas]

Because I use php://temp, after 2Mo the data will be stored in a temporary file on disk.

[dunglas]

• BinaryFileResponse support
• Use the URI of HttpFoundation

[fabpot]

Looks good to me.

@dunglas Anything else before merging and releasing 0.1?

[dunglas]

Looks good to me too for this one.

The last thing to do is deciding the strategy for cookie handling (copy/pasting BrowserKit, making BrowserKit a dependency or letting that as is but this can be done in a future version.

I've made some tests today with the standard edition of Symfony 2.6 + SensioFrameworkExtraBundle master and it works well.

[fabpot]

Thank you @dunglas.

[fabpot]

v 0.1 is released now: https://packagist.org/packages/symfony/psr-http-message-bridge

@dunglas Can you update the PR on the bundle?

[dunglas]

Done in fc8241c. Travis is running.

[weaverryan]

Wow, congrats! This certainly exceeded my expectations in speed. 🍻 for @dunglas

[malisetti]

Congratulations @dunglas