Skip to content

Conversation

@WyriHaximus
Copy link
Contributor

Currently the request is lacking the HTTP version from the PSR-7 request. This PR adds that to the request

@clue
Copy link
Owner

clue commented Sep 28, 2016

Thanks for your PR! 👍

Can you add a test to confirm this actually works?

The reason I'm asking is, this lib also works on PHP 5.3 which has to use http-client v0.3, which does not support this argument. I'm fine with this, but we should test and document this accordingly.

@WyriHaximus
Copy link
Contributor Author

Yes I just did, after looking at Sender again I realized that I completely missed that the constructor is public and thus this is testable. I'm aware of that 👍

@WyriHaximus
Copy link
Contributor Author

Took longer then expected but fixed the tests to work for both 0.3 and 0.4.

@clue
Copy link
Owner

clue commented Oct 6, 2016

Thanks for the update! 👍

Just to make this more obvious, can you add a functional test case (against httpbin), also to document the default behavior?

@WyriHaximus
Copy link
Contributor Author

Sure thing, I'll research and add that 👍

@WyriHaximus
Copy link
Contributor Author

There doesn't seem to be a way to get the HTTP version from HTTPBin, but since it's opensource and on github I'll give it a go adding that.

@clue
Copy link
Owner

clue commented Oct 7, 2016

There doesn't seem to be a way to get the HTTP version from HTTPBin, but since it's opensource and on github I'll give it a go adding that.

Duh, my bad! If you're going to add this to HTTPBin, this would be super-awesome! Otherwise, I'm fine with ditching my request and would accept just adding some documentation for this in the meantime 👍

@WyriHaximus
Copy link
Contributor Author

There doesn't seem to be a way to get the HTTP version from HTTPBin, but since it's opensource and on github I'll give it a go adding that.

Duh, my bad! If you're going to add this to HTTPBin, this would be super-awesome! Otherwise, I'm fine with ditching my request and would accept just adding some documentation for this in the meantime 👍

Lets see what the fine people at httpbin have to say about this first: postmanlabs/httpbin#305 😉

@WyriHaximus
Copy link
Contributor Author

Seems postmanlabs/httpbin#305 is taking longer then hope/expected. I'll add code documentation, do you want anything in the readme as well?

@clue
Copy link
Owner

clue commented Oct 19, 2016

Too bad, but I guess just documentation is just fine for now 👍 Just describe the default behavior and how (and when) this can be changed and then let's get this in :shipit:

@WyriHaximus
Copy link
Contributor Author

Will do, I already have the unit tests adapted but not committed yet. So once this PR is merged, I'll file a new one after with the updated tests.

@WyriHaximus
Copy link
Contributor Author

Added a note about it. Not sure if this is the right place, it was the best I could find though.

@clue
Copy link
Owner

clue commented Oct 21, 2016

Thanks, let's get this in! 👍

@clue clue merged commit e852252 into clue:master Oct 21, 2016
@WyriHaximus
Copy link
Contributor Author

🎉 !

@clue clue changed the title Add protocol version to request Obey explicitly set HTTP protocol version for outgoing requests Oct 21, 2016
@clue
Copy link
Owner

clue commented Oct 21, 2016

See also #59 which builds on top of this and reverts the default protocol version back to HTTP/1.0.

@WyriHaximus
Copy link
Contributor Author

Good catch, because this PR would technically introduce a BC

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.

2 participants