Fix the request target in PSR7 Request #30
Conversation
mtibben
changed the title
|
I've added a fix for the issue. The failing tests here are also failing on master. |
|
I opened #31 to fix the test suite. |
|
@mtibben You can now rebase to validate that tests pass now. |
|
I'm traveling over the next week without computer, feel free to rebase :)
…On Fri, 5 May 2017, 5:47 AM Fabien Potencier ***@***.***> wrote:
@mtibben <https://github.com/mtibben> You can now rebase to validate that
tests pass now.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#30 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA72E05_N9MVyCcrKWadhAkFLfH-9_3Kks5r2itFgaJpZM4NPS_g>
.
|
|
Rebased, and tests are passing @fabpot |
| @@ -55,7 +55,7 @@ public function createRequest(Request $symfonyRequest) | |||
| $request = new ServerRequest( | |||
| $server, | |||
| DiactorosRequestFactory::normalizeFiles($this->getFiles($symfonyRequest->files->all())), | |||
| $symfonyRequest->getUri(), | |||
| $symfonyRequest->getSchemeAndHttpHost().$symfonyRequest->getRequestUri(), | |||
There was a problem hiding this comment.
Is this change really required? Shouldn't the result be the same?
There was a problem hiding this comment.
getUri creates a URL in a very indirect way. This will likely be a problem due to the way Symfony requests normalise different parts of the URL. In comparison, getRequestUri is much more direct, you can see in prepareRequestUri how the URL comes directly from the superglobals or headers.
As we're dealing with a conversion between request types, we need to use the cleanest data we can when constructing the PSR7 request. Otherwise we can get a URL different to the original
| @@ -65,6 +65,7 @@ public function createRequest(Request $symfonyRequest) | |||
| ->withCookieParams($symfonyRequest->cookies->all()) | |||
| ->withQueryParams($symfonyRequest->query->all()) | |||
| ->withParsedBody($symfonyRequest->request->all()) | |||
| ->withRequestTarget($symfonyRequest->getRequestUri()) | |||
There was a problem hiding this comment.
Why is this necessary when we already pass the full URI to the constructor?
There was a problem hiding this comment.
Note that a HTTP request begins with a request target (e.g GET /request-target?query=string) not a URL.
In the same way, the PSR-7 Request interface does not mention URLs or constructors. It only knows about request targets. Therefore, I think we should be explicit about setting the request target correctly using withRequestTarget rather than relying on a particular implementation's constructor
There was a problem hiding this comment.
But we are dealing with a concrete implementation here. And if its constructor is not working as expected (which we should check) the implementation should be fixed instead.
There was a problem hiding this comment.
Yep I understand what you're saying, I was thinking the same myself. Yet when I looked at the implementation I thought it best to be explicit here. This implementation tokenizes the URL, and pieces together a request target if it is not set explicitly. By providing the request target string, you're giving the implementation the correct data - unfortunately the constructor is not giving you that opportunity. https://github.com/zendframework/zend-diactoros/blob/master/src/RequestTrait.php#L124-L129
There was a problem hiding this comment.
In fact tests fail without this line, exactly for this reason
Testing Symfony PSR-7 HTTP message Bridge Test Suite
F............ 13 / 13 (100%)
Time: 89 ms, Memory: 4.00MB
There was 1 failure:
1) Symfony\Bridge\PsrHttpMessage\Tests\Factory\DiactorosFactoryTest::testCreateRequest
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'/testCreateRequest?foo=1&bar[baz]=42'
+'/testCreateRequest?foo=1&bar%5Bbaz%5D=42'
/Users/michael/src/github.com/mtibben/psr-http-message-bridge/Tests/Factory/DiactorosFactoryTest.php:87
/Users/michael/src/github.com/mtibben/psr-http-message-bridge/vendor/symfony/phpunit-bridge/bin/.phpunit/phpunit-5.7/phpunit:5
FAILURES!
Tests: 13, Assertions: 63, Failures: 1.
|
I did not have the time to dig deeper, but I would prefer to first check why the |
|
Any further feedback on this @xabbuh? I don't believe |
|
I tried to debug this and IIRC the factory behaves differently depending on how you pass the request URI (through the constructor or with |
|
Thank you @mtibben. |
The request target is not being preserved correctly when using this library. This is because the query string is normalized, and we're not populating the PSR-7 request with the actual request target.
This is important for things like http signatures which uses the request target as an input to sign a request.