Skip to content

Fix the request target in PSR7 Request #30

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Fix the request target in PSR7 Request #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8aad1cd
Add test for request target
mtibben May 3, 2017
4bda7d5
Add the request target into the PSR7 request
mtibben May 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Factory/DiactorosFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change really required? Shouldn't the result be the same?

Copy link
Author

@mtibben mtibben May 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$symfonyRequest->getMethod(),
$body,
$headers
Expand All @@ -65,6 +65,7 @@ public function createRequest(Request $symfonyRequest)
->withCookieParams($symfonyRequest->cookies->all())
->withQueryParams($symfonyRequest->query->all())
->withParsedBody($symfonyRequest->request->all())
->withRequestTarget($symfonyRequest->getRequestUri())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary when we already pass the full URI to the constructor?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

;

foreach ($symfonyRequest->attributes->all() as $key => $value) {
Expand Down
5 changes: 5 additions & 0 deletions Tests/Factory/DiactorosFactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ public function testCreateRequest()
'REQUEST_METHOD' => 'POST',
'HTTP_HOST' => 'dunglas.fr',
'HTTP_X_SYMFONY' => '2.8',
'REQUEST_URI' => '/testCreateRequest?foo=1&bar[baz]=42',
'QUERY_STRING' => 'foo=1&bar[baz]=42',
),
'Content'
);
Expand All @@ -81,6 +83,9 @@ public function testCreateRequest()
$this->assertEquals('1', $queryParams['foo']);
$this->assertEquals('42', $queryParams['bar']['baz']);

$requestTarget = $psrRequest->getRequestTarget();
$this->assertEquals('/testCreateRequest?foo=1&bar[baz]=42', $requestTarget);

$parsedBody = $psrRequest->getParsedBody();
$this->assertEquals('Kévin Dunglas', $parsedBody['twitter']['@dunglas']);
$this->assertEquals('Les-Tilleuls.coop', $parsedBody['twitter']['@coopTilleuls']);
Expand Down