Skip to content

Conversation

@clue
Copy link
Member

@clueclue commented Apr 18, 2017

The absolute-formrequest-target was already supported in earlier version, but was neither tested nor documented. This PR makes sure that getUri() behaves consistent across different forms of request-target and adds matching documentation and an example for a plain HTTP proxy that relies on requests in absolute-form.

Example request as sent to a plain HTTP proxy:

GET http://www.google.com/ HTTP/1.1 Host: www.google.com 

Builds on top of #167, #158 and #157.

@clueclue added this to the v0.7.0 milestone Apr 18, 2017
@clueclueforce-pushed the uris branch 2 times, most recently from 4808542 to 708b143CompareApril 19, 2017 20:30
@clueclue changed the title Make getUri() behave consistent across different forms of request-targetValidate proxy requests in absolute-formApr 19, 2017
@clue
Copy link
MemberAuthor

clue commented Apr 19, 2017

Updated PR to remove unrelated changes so this now has a clear focus on requests in absolute-form:shipit:

Copy link
Member

@WyriHaximusWyriHaximus left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor nitpick

$this->assertSame('http://example.com/test', $requestAssertion->getRequestTarget());
$this->assertEquals('http://example.com/test', $requestAssertion->getUri());
$this->assertSame('/test', $requestAssertion->getUri()->getPath());
//$this->assertSame(array(), $requestAssertion->getQueryParams());
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this is commented out? To me commented out code shouldn't be committed tbh

$this->assertSame('http://example.com:8080/test', $requestAssertion->getRequestTarget());
$this->assertEquals('http://example.com:8080/test', $requestAssertion->getUri());
$this->assertSame('/test', $requestAssertion->getUri()->getPath());
//$this->assertSame(array(), $requestAssertion->getQueryParams());
Copy link
Member

Choose a reason for hiding this comment

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

Same as line 289

@clue
Copy link
MemberAuthor

clue commented Apr 20, 2017

Thanks for spotting! Updated and also rebased now that #170 is in, which did not preserve original request-target :shipit:

@WyriHaximusWyriHaximus merged commit 194658a into reactphp:masterApr 21, 2017
@WyriHaximus
Copy link
Member

👍

@clueclue deleted the uris branch April 21, 2017 07:06
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@clue@WyriHaximus@jsor