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

Request protocol version #1698

Merged
merged 2 commits into from Dec 22, 2015

Conversation

Projects
None yet
3 participants
@abbert

abbert commented Dec 22, 2015

Property protocolVersion of Request defaults to 1.1. This change makes it change to the actual request protocol version. Is str_replace('HTTP/' sufficient?

Albert Bakker added some commits Dec 22, 2015

@AndrewCarterUK

This comment has been minimized.

Show comment
Hide comment
@AndrewCarterUK

AndrewCarterUK Dec 22, 2015

Contributor

👎 in current form

The 'host' header isn't supported by HTTP/1.0 so you need to make sure you strip it in that scenario.

Contributor

AndrewCarterUK commented Dec 22, 2015

👎 in current form

The 'host' header isn't supported by HTTP/1.0 so you need to make sure you strip it in that scenario.

@akrabat

This comment has been minimized.

Show comment
Hide comment
@akrabat

akrabat Dec 22, 2015

Member

I don't understand what problem you are having.

The protocol is set via the Environment which merges the information from $_SERVER, so if it's set in $_SERVER, then we pick it up.

Member

akrabat commented Dec 22, 2015

I don't understand what problem you are having.

The protocol is set via the Environment which merges the information from $_SERVER, so if it's set in $_SERVER, then we pick it up.

@AndrewCarterUK

This comment has been minimized.

Show comment
Hide comment
@AndrewCarterUK

AndrewCarterUK Dec 22, 2015

Contributor

@akrabat The protocol version is hard-coded to 1.1 in the message class (I think).

I don't actually think this is the wrong approach. The PSR-7 specification is only for the representation of HTTP/1.1 messages, there are features of it that are incompatible with HTTP/1.0 - so I think it's dangerous pretending to support it.

Contributor

AndrewCarterUK commented Dec 22, 2015

@akrabat The protocol version is hard-coded to 1.1 in the message class (I think).

I don't actually think this is the wrong approach. The PSR-7 specification is only for the representation of HTTP/1.1 messages, there are features of it that are incompatible with HTTP/1.0 - so I think it's dangerous pretending to support it.

@AndrewCarterUK

This comment has been minimized.

Show comment
Hide comment
@AndrewCarterUK

AndrewCarterUK Dec 22, 2015

Contributor

Although that said, you can still do withProtocolVersion() :s

Contributor

AndrewCarterUK commented Dec 22, 2015

Although that said, you can still do withProtocolVersion() :s

@akrabat

This comment has been minimized.

Show comment
Hide comment
Member

akrabat commented Dec 22, 2015

@abbert

This comment has been minimized.

Show comment
Hide comment
@abbert

abbert Dec 22, 2015

@AndrewCarterUK Yes indeed. That's why I thought it needed to be implemented. Also, this comment is from the MessageInterface: The version string MUST contain only the HTTP version number (e.g., "1.1", "1.0").

@akrabat when I delete the lines of code I added in the constructor of Request the test will fail..That means the protocolVersion is never overwritten with the version in the actual request

abbert commented Dec 22, 2015

@AndrewCarterUK Yes indeed. That's why I thought it needed to be implemented. Also, this comment is from the MessageInterface: The version string MUST contain only the HTTP version number (e.g., "1.1", "1.0").

@akrabat when I delete the lines of code I added in the constructor of Request the test will fail..That means the protocolVersion is never overwritten with the version in the actual request

@AndrewCarterUK

This comment has been minimized.

Show comment
Hide comment
@AndrewCarterUK

AndrewCarterUK Dec 22, 2015

Contributor

Reading this I think @abbert is right - I can't see where the protocol version is extracted from the environment?

Contributor

AndrewCarterUK commented Dec 22, 2015

Reading this I think @abbert is right - I can't see where the protocol version is extracted from the environment?

@akrabat

This comment has been minimized.

Show comment
Hide comment
@akrabat

akrabat Dec 22, 2015

Member

Yeah - I understand the issue now.

Member

akrabat commented Dec 22, 2015

Yeah - I understand the issue now.

akrabat added a commit to akrabat/Slim that referenced this pull request Dec 22, 2015

@akrabat akrabat merged commit daabd4d into slimphp:3.x Dec 22, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 91.151%
Details
@akrabat

This comment has been minimized.

Show comment
Hide comment
@akrabat

akrabat Dec 22, 2015

Member

Thanks very much for this @abbert!

Member

akrabat commented Dec 22, 2015

Thanks very much for this @abbert!

@abbert

This comment has been minimized.

Show comment
Hide comment
@abbert

abbert Dec 22, 2015

No problem :)

abbert commented Dec 22, 2015

No problem :)

@akrabat akrabat added this to the 3.1.0 milestone Jan 1, 2016

@akrabat akrabat added the improvement label Jan 1, 2016

rivetchip pushed a commit to rivetchip/Slim that referenced this pull request Jan 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment