[HttpKernel] Allow to inject PSR-7 ServerRequest in controllers and to return a PSR-7 response. #14751
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| continue; | ||
| } | ||
|
|
||
| if ($param->getClass()) { |
There was a problem hiding this comment.
if ($class = $param->getClass()) {}2f73f0f to
cb6430e
Compare
|
@wouterj weren't you working on the argument resolver for the controller resolver? |
|
Yes: #11457 But it can't be done in a BC way... |
|
Ah this one is for 2.8. Will have to wait then :( |
| "symfony/yaml": "~2.0,>=2.0.5|~3.0.0" | ||
| "symfony/yaml": "~2.0,>=2.0.5|~3.0.0", | ||
| "symfony/psr-http-message-bridge": "dev-wip", | ||
| "zendframework/zend-diactoros": "~1.0" |
There was a problem hiding this comment.
Those 2 deps are actually soft ones, not hard ones.
There was a problem hiding this comment.
It's in the require-dev section (just for tests).
|
I don't really like how this gets mixed inside the ControllerResolver and the HttpKernel. IMO, for Symfony 2.x, this should be handled with a SensioFrameworkExtraBundle ParamConverter (for the Request injection) and a |
|
I'm challenging this idea of handling Psr7 req/resp directly in a Sf application at this state of the psr art. |
|
Well, for the controller, it is possible, given that the controller generally does not mutate the Request, and it is responsible for returning a response anyway. Allowing to use PSR-7 here would allow to reuse controllers written against PSR-7, which improves interoperability for people writing controllers. But I still don't like how the kernel gets more complex because of this PSR-7 support while it could be done outside of the kernel with the existing extension points. |
|
@stof It can be moved in SensioExtraFramework easily but I've some concern about that (ordered by priority):
The PSR-7 support in this patch is really some line of codes. The patch has grown a little because I refactored |
|
@dunglas The kernel.view event itself is not the reason for the performance impact (dispatching an event with a few listeners is not that costly). the issue with Regarding having it in core, it could be the case once we provide a clean extension point in core for resolving arguments (replacing what ParamConverters are doing without the need to alter the Request attributes with the converted params). And btw, a similar way could be done without depending on SensioFrameworkExtraBundle by using a |
|
To summarize, we have two alternatives: Proposal 1 Let the patch as is and refactor it when new extensions points are ready. Pro:
Cons:
Proposal 2 Move it to SensioExtraFramework and refactor it to move it back in core when extensions points are ready. Pros:
Cons:
I'm for the first choice :) |
|
If 1. is doable without changing any interface then fine, but if not, we should think twice before changing one and knowingly planning for deprecating it a few months later. I don't think we need to hurry up that much. Let's talk about it for 2.8 |
|
PR with ParamConveter + view listener version in SensioExtraFramework: sensiolabs/SensioFrameworkExtraBundle#360 |
|
Closing in favor of the PR on SensioFrameworkExtraBundle. |
This PR was squashed before being merged into the 3.0.x-dev branch (closes #360). Discussion ---------- PSR-7 ParamConverter and view listener. Replace symfony/symfony#14751 Commits ------- 705d448 PSR-7 ParamConverter and view listener.
Use the PSR-7 Bridge to inject an instance of
Psr\Http\Message\ServerRequestInterfacein controllers. Handle PSR-7Psr\Http\Message\ResponseInterfacereturned by controllers.