Proposal for a HTTP Client interface #24

Closed
wants to merge 4 commits into
from
View
114 proposed/http-client.md
@@ -0,0 +1,114 @@
+## Introduction
+
+Many libraries and applications require an HTTP client to talk to other servers. In general all these libraries either ship their own HTTP client library, or use low-level PHP functionality such as `file_get_contents`, ext/curl or the ext/socket. Depending on the use-cases there are very tricky implementation details to be handled such as proxies, SSL, authentication protocols and many more. Not every small library can afford to implement all the different details. However there are only a few http client libraries that are widely used between different projects, because of NIH or fears of vendor lock-in.
+
+## Goal
+
+This proposal aims at providing a very simple HTTP client interface to allow interopability between HTTP clients of different libraries and allow PHP libraries to ship HTTP client functionality without the necessity to implement a client.
+
+## Interfaces
+
+A single interface is proposed
+
+ <?php
+ namespace PSR\Http\Client;
+
+ /**
+ * A HTTP Client
+ */
+ interface HttpClient
+ {
+ /**
+ * Send a http-request and return a http-response.
+ *
+ * @param string $method HTTP method, uppercase
+ * @param string $url Url to send HTTP request to
+ * @param string $content Content of the request, can be empty.
@mtdowling
mtdowling added a line comment Mar 24, 2012

Should $content accept a string OR array for application/x-www-form-urlencoded POST requests?

@samdark
samdark added a line comment Jan 7, 2014

There's no easy way to send files as multipart if $content is just string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ * @param array $headers Array of Headers, header Name is the key.
@samdark
samdark added a line comment Jan 7, 2014

RFC2616:

Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma. The order in which header fields with the same field-name are received is therefore significant to the interpretation of the combined field value, and thus a proxy MUST NOT change the order of these field values when a message is forwarded.

Cache-Control: no-cache
Cache-Control: no-store

Is valid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ * @param array $options Vendor specific options to activate specific features.
+ * @throws HttpException If no response can be created an exception should be thrown.
+ * @return Response
+ */
+ public function request($method, $url, $content = null, array $headers = array(), array $options = array());
@mtdowling
mtdowling added a line comment Mar 24, 2012

I'd like to see the method signature become:

public function request($method, $url, array $headers = array(), $content = null, array $options = array());

I find that I almost always have to set headers (e.g. Accept, Content-Type) when sending requests to REST APIs. I think that moving $headers before $content would help to make it easier to send GET/HEAD/DELETE requests. Doing it this way, Guzzle could just add a new request method to its client and implement the HttpClient interface.

@philsturgeon
philsturgeon added a line comment May 7, 2012

Agreed, I set headers on a request more often than setting content.

@rmccue
rmccue added a line comment Jul 12, 2012

+1 on headers earlier. Requests uses $url, $headers, $data, $type, $options, which is based on usage statistics. (It also means you can just do Requests::request('http://example.com/') and forget the rest, with GET implied.)

@mattfarina
mattfarina added a line comment Jul 14, 2012

Two thoughts,

  1. Drupal (before it was arrayified) had $headers before content (I use headers more than content) and $method (aka type) later as well with GET being assumed as a default. I think getting a sane order is important to ease the pain of a lot of use. drupal_http_request is the Drupal version.
  2. An idea would be to make this chainable method calls. Instead of numerous arguments on one method having one method per argument. Spitballing here:
$foo = new Client();
$foo->method('GET')->url('http://foo.com')->execute();
@Crell
Crell added a line comment Jul 16, 2012

I have to agree with Matt's note 2 here. Sending an HTTP request is a Command. It should be a Command object, not a method. Vis, you create a Request/Client/Whatever object, configure it with whatever methods in whatever order you want, then execute()/send()/whatever(). And get back a Response. That gives maximum flexibility to users, as well as allows for specific implementers to add additional methods if needs be without breaking the interface.

@samdark
samdark added a line comment Jan 7, 2014

+1 for @mattfarina idea number 2 and @Crell corrections about request object.

@philsturgeon
philsturgeon added a line comment Jan 7, 2014

Fan of that (number 2)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ }
+
+A value objects come with this interface, the Response which is returned from the client. Their definition is as follows:
+
+ <?php
+ namespace PSR\Http\Client;
+
+ /**
+ * Http Response returned from {@see HttpClient::request}.
+ */
+ class Response
+ {
+ /**
+ * Status code for the response
+ *
+ * @return int
+ */
+ public function getStatusCode();
@samdark
samdark added a line comment Jan 7, 2014

Status message is useful for exceptions so probably worth adding as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ /**
+ * Get content type for the response
+ *
+ * @return string
+ */
+ public function getContentType();
@philsturgeon
philsturgeon added a line comment May 7, 2012

If we start adding helper functions for headers like this, would we not need to start adding a whole lot more? Expiry dates, cache settings, language, etc?

@Marlinc
Marlinc added a line comment Jul 12, 2012

Why not create a Headers object? So you could get/set headers in it and pass that to the request
And so you could use that to get the header info instead of all those methods in the response class

@samdark
samdark added a line comment Jan 7, 2014

@Marlinc isn't it too many objects for a well-defined (by RFC) topic?

@philsturgeon
philsturgeon added a line comment Jan 7, 2014

I figure getHeader('Content-Type') would make the most sense. No point adding methods for the sake of it.

@samdark
samdark added a line comment Jan 7, 2014

Why not? It's one of the most used ones. We're getting less typo prone way of doing it + IDE code autocompletion.

@philsturgeon
philsturgeon added a line comment Jan 7, 2014
@samdark
samdark added a line comment Jan 7, 2014

OK. Makes sense from this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ /**
+ * Get the content of this response
+ *
+ * @return string
+ */
+ public function getContent();
+
+ /**
+ * Return all headers of this response.
+ *
+ * Header names are returned as lower-case keys. Their values are returned
+ * in an array themselves to support multiple occurances of headers.
+ *
+ * @example array(array("content-type" => array("text/html"))
+ *
+ * @return array
+ */
+ public function getHeaders();
@samdark
samdark added a line comment Jan 7, 2014

Same as above. Multiple same-named headers are possible.

@nvartolomei
nvartolomei added a line comment Jan 7, 2014

What's wrong with this? Key is a string and value is an array, so we can have multiple values for one key like

$headers = [
    'Cache-Control' => [
        'no-cache',
        'no-store'
    ]
];
@samdark
samdark added a line comment Jan 7, 2014

Ah, then it's totally fine. One question is should it be always an array or sometimes it will be a scalar?

@samdark
samdark added a line comment Jan 7, 2014

Also it cannot be forced programmaticaly via interface if made this way so there's a verbal agreement (not that bad but still).

@philsturgeon
philsturgeon added a line comment Jan 7, 2014

We don't need to programmatically enforce header names, as they can be anything.

You can have multiple headers of the same name and they override each other, that should be your responsibility to sort them out as a developer though, right?

@samdark
samdark added a line comment Jan 7, 2014

They should not override each other as stated in RFC2616.

@philsturgeon
philsturgeon added a line comment Jan 7, 2014

It depends where you are talking about the override happening. If you are creating a $headers array to pass to setHeaders() then you as a developer are responsible for working that out properly, right?

If its setHeader('Cache-Control') then that is a completely different story. But RFC2616 has no relevance to the creation of a PHP array in the developers own domain.

@samdark
samdark added a line comment Jan 7, 2014

Yes. getHeaders should return something like:

[
  header name -> header value,
  header name -> [header value1, header value2],
]

It's not only Cache-Control that can have multiple values. For example:

WWW-Authenticate: Negotiate 
WWW-Authenticate: Basic realm="EXAMPLE.COM" 

So setting headers should support arrays as well.

@philsturgeon
philsturgeon added a line comment Jan 7, 2014

Well it's not at all possible for PHP to return that structure.

[
  header name => header value,
  header name => [header value1, header value2],
]

Keys with matching names aren't possible.

It could maybe return:

[
  header name => [
    header value,
    [header value1, header value2],
  ]
]

That structure could maybe go into setHeaders, but it's getting a bit disgusting.

@samdark
samdark added a line comment Jan 7, 2014

I meant

[
  header name1 -> header value1,
  header name2 -> [header value2, header value3],
]

sorry for confusion.

@philsturgeon
philsturgeon added a line comment Jan 7, 2014
@mtdowling
mtdowling added a line comment Jan 7, 2014

I actually think header values should be managed in an object. Here's what I'm thinking (it's an almost rewritten fork of this PR): https://github.com/mtdowling/fig-standards/blob/modified-http/proposed/http.md#32-psrhttpheadervaluesinterface

@samdark
samdark added a line comment Jan 7, 2014

Umm, it's just extends \Countable, \Traversable, \ArrayAccess. Isn't it just the same as array (except consuming more memory and being an object)? What are pros?

@philsturgeon
philsturgeon added a line comment Jan 7, 2014

That looks boss. You should factor this feedback in (above):

An idea would be to make this chainable method calls. Instead of numerous arguments on one method having one method per argument. Spitballing here:

$foo = new Client();
$foo->method('GET')->url('http://foo.com')->execute();
@philsturgeon
philsturgeon added a line comment Jan 7, 2014
@samdark
samdark added a line comment Jan 7, 2014
  1. It doesn't have anything to do with chaining. Chaining is just request methods returning the instance of request that collects data including headers. I see no pro for these headers to be objects if these objects do not define any structure.
  2. Type hinting doesn't make sense for an object that accepts any key-value pairs. It is a wrapper for the purpose of having a wrapper.
@mtdowling
mtdowling added a line comment Jan 7, 2014

Being able to treat the values of a header as both an array and a string are the best of both worlds:

echo $response->getHeader('Cache-Control');
//> no-cache, no-store

echo $response->getHeader('Cache-Control')[0];
//> no-cache

echo count($response->getHeader('Cache-Control'));
//> 2

foreach ($response->getHeader('Cache-Control') as $index => $value) {
    echo $value . ', ';
}
//> no-cache, no-store
@samdark
samdark added a line comment Jan 7, 2014

@mtdowling looks a bit too magical to me.

@philsturgeon
philsturgeon added a line comment Jan 7, 2014

@samdark I'm sorry to have caused confusion.

It doesn't have anything to do with chaining. Chaining is just request methods returning the instance of request that collects data including headers.

Right, this specific conversation has nothing to do with chaining. I was suggesting that should go into @mtdowling's PR, which is a fork of this.

Type hinting doesn't make sense for an object that accepts any key-value pairs. It is a wrapper for the purpose of having a wrapper.

Instead of a method expecting array it can type hint against HeaderValuesInterface, which is marginally more useful in some scenarios. I wasn't trying to suggest that we need type hinting on the key/value pair strings or anything weird like that.

@mtdowling thats some great stuff right there.

@rmccue
rmccue added a line comment Jan 8, 2014

IMO, easier to move the logic up the stack into the header dictionary. Requests uses a case-insensitive dictionary which implements ArrayAccess, giving:

// X-Multiple: first-value
// x-MULTIPLE: second-value
$value = $headers['x-multiple'];
// 'first-value,second-value'
$values = $headers->getValues('x-multiple');
// array( 'first-value', 'second-value' )

Originally, it didn't have a distinction here and always used the first form, however thanks to the wonderful world of HTTP specifications, that doesn't always work. Notably, cookie headers will cause problems if you always concatenate with a comma, as commas are used in the old specification in the Expires part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ /**
+ * Return a specified header of this response or null if not found,
+ *
+ * Returns an array no matter if single or multiple occurances of a header exist.
+ *
+ * @return array|null
+ */
+ public function getHeader($name);
+ }
+
+Also an exception is shipped:
+
+ <?php
+ namespace PSR\Http\Client;
+
+ class HttpException extends \RuntimeException
+ {
+ }
+
+## Sample code
+
+Here is an example usage:
+
+ <?php
+ $client = create_http_client(); // implementation specific
+ $response = $client->request('GET', 'http://www.php.net');
+
+ if ($response->getStatusCode() == 200) {
+ $content = $response->getContent();
+ }
+
+ $response = $client->request('GET', 'http://api/returning.json');
+
+ if ($response->getContentType() == 'application/json') {
+ $json = json_decode($response->getContent());
+ }
+