Skip to content
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

Fix parsing CONNECT request without Host header #201

Merged
merged 1 commit into from
Jun 16, 2017

Conversation

clue
Copy link
Member

@clue clue commented Jun 15, 2017

This means that all of these examples now correctly return the same URI and also the same Host header value example.com without the default port in this case:

GET / HTTP/1.1\r\nHost: example.com\r\n\r\n
GET http://example.com/ HTTP/1.1\r\n\r\n
CONNECT example.com:80 HTTP/1.1\r\n\r\n

Builds on top of #158 and #173

@clue clue added this to the v0.7.1 milestone Jun 15, 2017
@WyriHaximus WyriHaximus requested review from jsor and WyriHaximus June 15, 2017 20:02
@WyriHaximus
Copy link
Member

Since we already have code in master for v0.8.0 shouldn't this PR target the 0.7 branch?

$data = "CONNECT example.com:443 HTTP/1.1\r\n\r\n";
$this->connection->emit('data', array($data));

$this->assertInstanceOf('RingCentral\Psr7\Request', $requestAssertion);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this test against Psr\Http\Message\ServerRequestInterface instead of the implementation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes and no :-)

This test is in line with all other tests, but I agree that we should also change all other tests as well. I'll keep this as-is for now and will look into filing a follow-up clean-up PR? 👍

@clue
Copy link
Member Author

clue commented Jun 16, 2017

Since we already have code in master for v0.8.0 shouldn't this PR target the 0.7 branch?

Yes and no :-)

Given the current state of the repo, both are actually valid options and I'd vote to just tag a v0.7.1 on the master branch soon-ish 👍

Only a single other PR has been merged to the master branch and it only affects internal functionality without exposing anything to the public API. This means that it's safe to change its milestone and simply release this as part of the next minor release.

We could maintain separate release branches, but this would actually require additional effort right now and would provide very little value otherwise. We would have to file this PR against the master branch to target the future v0.8.0 release and then file another PR to backport this to the 0.7 release branch. This would thus involve twice the work and could potentially be confusing and/or additional work in case we find that either of this introduces any regressions etc.

My vote would be to get this fix out rather sooner than later, but I'm open to either option 👍

@WyriHaximus
Copy link
Member

Using master sounds good to me then 👍

Sign up for free to 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