Added more clear specification. #944

Nyholm · 2017-10-22T02:33:35Z

Im trying to address @joelwurtz comment and part of php-http#34

joelwurtz · 2017-10-22T08:39:20Z

proposed/http-client/http-client.md

-response. When there is an error during sending the request or an error with network an
-exception should be thrown. 
+response. The HTTP client MAY modify the response from the server before returning 
+the PSR-7 response. 


Something like this (minus typo ^^) would be better IMO:

"The HTTP client MAY modify the request, given by the user and sent to the server or the response from the server before returning. In this case the request and the response must be consistent between the body and headers. I.e. a server may return a gzip encoded body and the client may know how to decode this, when it decodes the body the client MUST also remove the header that specify this encoding."

if this is what we mean by this, then yes lets give this example to illustrate what we actually mean.

dbu

thank you!

dbu · 2017-10-22T16:33:44Z

proposed/http-client/http-client.md

-response. When there is an error during sending the request or an error with network an
-exception should be thrown. 
+response. The HTTP client MAY modify the response from the server before returning 
+the PSR-7 response. 


if this is what we mean by this, then yes lets give this example to illustrate what we actually mean.

dbu · 2017-10-22T16:38:12Z

proposed/http-client/http-client.md

+the server or the response from the server before returning. In this case the 
+request and the response must be consistent between the body and headers. I.e. a 
+server may return a gzip encoded body and the client may know how to decode this, 
+when it decodes the body the client MUST also remove the header that specify this 


i think this line mixes singular and plural. either "remove the headers that specify" or "remove the header that specifies". i think the second makes more sense, the encoding example would have just one header.

dbu

thank you!

sagikazarmark · 2017-10-22T23:32:45Z

proposed/http-client/http-client.md

@@ -12,8 +12,12 @@ interpreted as described in [RFC 2119](http://tools.ietf.org/html/rfc2119).
 ### Client

 An HTTP client has the responsibility to send a PSR-7 request and return a PSR-7
-response. The HTTP client MAY modify the response from the server before returning 
-the PSR-7 response. 
+response. The HTTP client MAY modify the request, given by the user and sent to 


"Under the hood the HTTP client MAY modify the request/response received from the user/server."

How does that sound?

This part tries to make it clear that under the contract surface anything could happen to request/response objects and there is no assurance that they will remain the same objects on the instance level, right?

sagikazarmark · 2017-10-22T23:33:04Z

proposed/http-client/http-client.md

-the PSR-7 response. 
+response. The HTTP client MAY modify the request, given by the user and sent to 
+the server or the response from the server before returning. In this case the 
+request and the response must be consistent between the body and headers. I.e. a 


I.e. -> "For example," ?

sagikazarmark · 2017-10-22T23:33:30Z

proposed/http-client/http-client.md

+the server or the response from the server before returning. In this case the 
+request and the response must be consistent between the body and headers. I.e. a 
+server may return a gzip encoded body and the client may know how to decode this, 
+when it decodes the body the client MUST also remove the header that specify this 


specify the encoding?

sagikazarmark

Sorry, chose the wrong option, see my outdated comments above.

michaelcullum · 2017-10-23T01:34:44Z

proposed/http-client/http-client.md

-`Psr\Http\Client\Exception\RequestException`.
+
+When there is an error with the network or the remote server cannot be reached, the
+HTTP client SHOULD thrown a `Psr\Http\Client\Exception\NetworkException`. If the 


thrown -> throw

Generally as a rule, SHOULD is recommended against as it cannot be relied upon by a client anymore. Is there rationale as to why this isn't a MUST?

in some circumstances it might be difficult to know what kind of error happened. there will probably always be some fuzzyness around the exception type. however, we are specific as to which things cause a network exception, so i think MUST should be fine here.

can you fix the thrown throw please?

ok if we say MUST here?

michaelcullum · 2017-10-23T01:36:44Z

proposed/http-client/http-client.md

-exception should be thrown. 
+response. The HTTP client MAY modify the request, given by the user and sent to 
+the server or the response from the server before returning. In this case the 
+request and the response must be consistent between the body and headers. I.e. a 


I.e. doesn't make much sense in this context. Perhaps use For example instead.

michaelcullum · 2017-10-23T01:38:05Z

proposed/http-client/http-client.md

+the server or the response from the server before returning. In this case the 
+request and the response must be consistent between the body and headers. I.e. a 
+server may return a gzip encoded body and the client may know how to decode this, 
+when it decodes the body the client MUST also remove the header that specifies this 


using a MUST inside an example doesn't make much sense. Suggestion:
and the client may know how to decode this, however the client should then also mutate the headers that specify the encoding

hm, the MUST is important here. should we move that out of the example to say that if the client modifies the request or response, it MUST also adjust the headers accordingly? (i'd assume that if it gzips a request, it must also adjust the content-length header, for example)

joelwurtz · 2017-11-15T15:10:19Z

proposed/http-client/http-client.md

 server may return a gzip encoded body and the client may know how to decode this, 
-when it decodes the body the client MUST also remove the header that specifies this 
+when it decodes the body the client MUST also remove the header that specifies the 
 encoding.


and update the content length header. (cf @dbu comment)

dbu

if we add the section about when to throw exceptions, should we also say here to not throw exceptions for any http status?

dbu · 2017-11-17T13:05:24Z

proposed/http-client/http-client.md

-`Psr\Http\Client\Exception\RequestException`.
+
+When there is an error with the network or the remote server cannot be reached, the
+HTTP client MUST thrown a `Psr\Http\Client\Exception\NetworkException`. If the 


s/thrown/throw

dbu · 2017-11-17T13:07:55Z

proposed/http-client/http-client.md

+When there is an error with the network or the remote server cannot be reached, the
+HTTP client MUST thrown a `Psr\Http\Client\Exception\NetworkException`. If the 
+request is invalid and cannot be sent the HTTP client MUST throw a 
+`Psr\Http\Client\Exception\RequestException`. Smaller issues like wrong HTTP version


i would swap the 2 points, so they are more in order of when they happen (invalid request is detected first, network problem only later)

dbu · 2017-11-17T13:10:45Z

proposed/http-client/http-client.md

-It SHOULD implement exceptions for `Psr\Http\Client\Exception\NetworkException` and
-`Psr\Http\Client\Exception\RequestException`.
+
+When there is an error with the network or the remote server cannot be reached, the


what about an invalid response, should we mention that here? like not proper http or such.

Nyholm · 2017-11-17T13:41:27Z

Thank you @dbu

dbu

thanks for the updates. now i only have nitpick comments left ;-)

dbu · 2017-11-17T13:43:13Z

proposed/http-client/http-client.md

-It SHOULD implement exceptions for `Psr\Http\Client\Exception\NetworkException` and
-`Psr\Http\Client\Exception\RequestException`.
+
+When the client get a request that is invalid and cannot be sent, the HTTP client 


When the HTTP client is passed a request that ... sent , the client

dbu · 2017-11-17T13:45:06Z

proposed/http-client/http-client.md

+Smaller issues like wrong HTTP version is not blocking the HTTP client to send the
+request and MUST not cause any exception. 
+
+No exception should be thrown if the remote server answers with an response that can


If the remote server answers with a response that can be parsed into a PSR-7 response, the client MUST NOT throw an exception. For example, response status codes in the 400 and 500 range MUST NOT cause an exception.

Nyholm · 2017-11-17T13:50:50Z

Thank you!

dbu

thank you. i am happy now.

@michaelcullum @sagikazarmark what do you think of this now? (as your status on this is "request changes")

Nyholm · 2017-11-20T08:16:42Z

I've addressed their issues. Im merging this to move forward. Feel free to open issues or PRs to make additional changes.

Added more clear specification.

1dc9392

Nyholm added the PSR-18: HTTP Client label Oct 22, 2017

joelwurtz reviewed Oct 22, 2017

View reviewed changes

dbu reviewed Oct 22, 2017

View reviewed changes

Updated according to feedback

0e8970d

dbu reviewed Oct 22, 2017

View reviewed changes

typo

a5b7315

dbu approved these changes Oct 22, 2017

View reviewed changes

sagikazarmark reviewed Oct 22, 2017

View reviewed changes

sagikazarmark requested changes Oct 22, 2017

View reviewed changes

michaelcullum requested changes Oct 23, 2017

View reviewed changes

Minor changes

a6bb541

joelwurtz reviewed Nov 15, 2017

View reviewed changes

Nyholm added 2 commits November 17, 2017 09:16

Added comment about content length header

cb8f158

Use MUST instead of SHOULD

dd829e4

joelwurtz approved these changes Nov 17, 2017

View reviewed changes

dbu reviewed Nov 17, 2017

View reviewed changes

Exception fixes

5aed379

dbu reviewed Nov 17, 2017

View reviewed changes

More updates

60d4ed8

dbu approved these changes Nov 17, 2017

View reviewed changes

Nyholm merged commit 17b6378 into php-fig:master Nov 20, 2017

Nyholm deleted the specification branch November 20, 2017 08:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added more clear specification. #944

Added more clear specification. #944

Nyholm commented Oct 22, 2017

joelwurtz Oct 22, 2017

dbu Oct 22, 2017

Nyholm Oct 22, 2017

dbu left a comment

dbu Oct 22, 2017

dbu Oct 22, 2017

Nyholm Oct 22, 2017

dbu left a comment

sagikazarmark Oct 22, 2017

sagikazarmark Oct 22, 2017

sagikazarmark Oct 22, 2017

sagikazarmark left a comment

michaelcullum Oct 23, 2017

dbu Oct 23, 2017

dbu Nov 17, 2017

michaelcullum Oct 23, 2017

michaelcullum Oct 23, 2017

dbu Oct 23, 2017

michaelcullum Oct 23, 2017

joelwurtz Nov 15, 2017 •

edited

Nyholm Nov 17, 2017

dbu left a comment

dbu Nov 17, 2017

dbu Nov 17, 2017

dbu Nov 17, 2017

Nyholm commented Nov 17, 2017

dbu left a comment

dbu Nov 17, 2017

dbu Nov 17, 2017

Nyholm commented Nov 17, 2017

dbu left a comment

Nyholm commented Nov 20, 2017

Added more clear specification. #944

Added more clear specification. #944

Conversation

Nyholm commented Oct 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagikazarmark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelwurtz Nov 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Nov 17, 2017

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Nov 17, 2017

dbu left a comment

Choose a reason for hiding this comment

Nyholm commented Nov 20, 2017

joelwurtz Nov 15, 2017 •

edited