Proposal for a HTTP Client interface #24

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet

From the 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.

Motivation:

Doctrine has about 4 projects that need an HTTP client. Currently every projects implements them itself, which is annoying. We could abstract this into our "Common" library, however that would mean we would start being a "http client" vendor. Now personally, i dont care about http clients and would rather let others do this, however i also don't want to face vendor lock-in by deciding for any of the many http clients.

proposed/http-client.md
+ /**
+ * Return all headers of this response.
+ *
+ * @return array
@kriswallsmith

kriswallsmith Mar 24, 2012

What sort of array?

array('Content-Type: text/html');
array('Content-Type' => 'text/html');
array('Content-Type' => array('text/html'));
@beberlei

beberlei Mar 24, 2012

Good question, I am open for suggestions ;-)

I would say header names as keys for sure. The third one is the most correct one i guess.

@kriswallsmith

kriswallsmith Mar 24, 2012

Case-insensitivity of keys is also an important consideration if we use header names as keys. Set-Cookie: asdf and set-cookie: asdfasdf should go under the same key.

@mtdowling

mtdowling Mar 24, 2012

Contributor

Always returning an array for every header seems clunky to me. Why not just return an array or string depending on if multiple headers of the same name were encountered?

<?php
var_export($response->getHeaders());
array(
  'Content-Type' => 'text/html',
  'Set-Cookie' => array('foo', 'bar')
);

The same could apply for retrieving specific headers:
$response->getHeader('Content-Type'); // "text/html"
$response->getHeader('Set-Cookie'); // array('foo', 'bar')

@mattfarina

mattfarina Jul 14, 2012

While I like the idea of getting a single header retrieved as a string, shouldn't the response from this call be consistent?

Contributor

AmyStephen commented Mar 24, 2012

Any reason not to use https://github.com/symfony/symfony/tree/master/src/Symfony/Component/HttpFoundation as a standard? Symfony is already an 'http client' vendor and it's available now as a separate component. Drupal recently adopted it. I'm using it within Molajo. Excellent code, IMO.

HttpFoundation is designed for processing requests, not sending requests.

Contributor

lsmith77 commented Mar 24, 2012

@AmyStephen HttpFoundation could be a basis to fill in the proposed parameter for Request and proposed returned Response, but it does not include an interface for an HTTP client

Contributor

AmyStephen commented Mar 24, 2012

Missed that, sorry. Thanks guys.

+ * @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 Mar 24, 2012

Contributor

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 May 7, 2012

Contributor

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

@rmccue

rmccue 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 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 Jul 16, 2012

Contributor

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 Jan 7, 2014

Contributor

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

@philsturgeon

philsturgeon Jan 7, 2014

Contributor

Fan of that (number 2)

+ *
+ * @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 Mar 24, 2012

Contributor

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

@samdark

samdark Jan 7, 2014

Contributor

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

Contributor

mtdowling commented Mar 25, 2012

Please note that there is a discussion going on at https://groups.google.com/forum/?fromgroups#!topic/php-standards/kFEhviETdGM

igorw commented Apr 30, 2012

This might not be useful to so many people, but since it's really easy to add, having an async interface would be nice too.

I'm thinking:

interface ResponsePromise
{
    public function onResponse(callable $callback);
    public function onError(callable $callback);
}

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

Usage:

$client = new AsyncHttpClient();
$request = $client->request('GET', 'http://example.com/');
$request->onResponse(function (Response $response) {
    var_dump($response->getContent());
});
$request->onError(function (HttpException $e) {
    var_dump($e->getMessage());
});

Thoughts?

that wont work, what if the process is already finsihed when you assign the
callbacks?

On Mon, Apr 30, 2012 at 9:57 AM, Igor Wiedler <
reply@reply.github.com

wrote:

This might not be useful to so many people, but since it's really easy to
add, having an async interface would be nice too.

I'm thinking:

interface ResponsePromise
{
     public function onResponse(callable $callback);
     public function onError(callable $callback);
}

interface AsyncHttpClient
{
     public function request($method, $url, $content = null, array

$headers = array(), array $options = array());
}

Usage:

$client = new AsyncHttpClient();
$request = $client->request('GET', 'http://example.com/');
$request->onResponse(function (Response $response) {
     var_dump($response->getContent());
});
$request->onError(function (HttpException $e) {
     var_dump($e->getMessage());
});

Thoughts?


Reply to this email directly or view it on GitHub:
#24 (comment)

igorw commented Apr 30, 2012

I suppose forcing the client to buffer the request is suboptimal. A method on the promise to explicitly start the request would be an option, however I don't know if there are any conventions for this.

+ *
+ * @return string
+ */
+ public function getContentType();
@philsturgeon

philsturgeon May 7, 2012

Contributor

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 Jul 12, 2012

Contributor

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 Jan 7, 2014

Contributor

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

@philsturgeon

philsturgeon Jan 7, 2014

Contributor

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

@samdark

samdark Jan 7, 2014

Contributor

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 Jan 7, 2014

Contributor

My point is adding a method for an arbitrary HTTP Header means we either need to define headers for all sorts of HTTP headers, or have a strangely inconsistent API.

status code, headers, body. That is what we need, and that is what the methods provide so far. Adding a method just for content type is bizarre. 

@samdark

samdark Jan 7, 2014

Contributor

OK. Makes sense from this point.

Are there any news about this, working on a lib, i could need this?

njh commented Apr 25, 2013

Is this ever likely to be approved?

@njh njh referenced this pull request in njh/easyrdf Apr 25, 2013

Open

HTTP client change #133

Member

dragoonis commented Apr 25, 2013

@njh it's still in progress

makasim commented Apr 26, 2013

@beberlei what should be done to move it forward? Is it really possible to have this PR be approved? Could I help somehow?

https://github.com/payment/httpclient
i needed one for by saferpay library

Contributor

philsturgeon commented Apr 26, 2013

I think a problem here is that this is too alien to the existing HTTP clients to really be a useful standard.

http://guzzlephp.org/tour/http.html
https://github.com/kriswallsmith/Buzz
https://github.com/rmccue/Requests

All of these systems have different methods for get(), posts(), put(), delete() for example, because they all work differently.

Delete does not have a body, so no need for $content on that one. get() needs to turn $content into a query string, and the rest have to either detect an array and set a default mime type, or whatever.

This is only an interface of course, but every implementation will need to have these different methods to do much, so I feel like adding them to the interface would be much more useful. I want to know that there is a get() method, not just hope there is one in the specific implementation.

I could be wrong :)

Contributor

philsturgeon commented May 16, 2013

The whole point of the HTTP PSR is so you don't need bridge packages or adapter classes, so that's not a great idea.

@philsturgeon i don't think, then ever will be an http psr

Contributor

philsturgeon commented May 16, 2013

image

@philsturgeon but i think its much better writing bridges against an interface, than, have nothing (cause i don't want to force others using, buzz, or guzzle or whatever)

rmccue commented May 16, 2013

Personally, I'm not going to change Requests to a PSR interface (because a. backwards compatibility, and b. extra dependencies), but I'm definitely going to offer a bridge if/when the PSR is stable.

Contributor

philsturgeon commented May 16, 2013

Thats the point, you type hint against an interface (which Buzz, Guzzle and Requests will likely all support as they're in the FIG) and you can have one adapter instead of 3. Then anyone can build their own HTTP request library and you never need to write a bridge. Updates to these packages will never need updates to your code, because they will remain spec compliant.

Ponies and rainbows for everyone.

Phil Sturgeon

On Thursday, May 16, 2013 at 10:56 AM, Dominik Zogg wrote:

@philsturgeon (https://github.com/philsturgeon) but i think its much better writing bridges against an interface, than, have nothing (cause i don't want to force others using, buzz, or guzzle or whatever)


Reply to this email directly or view it on GitHub (#24 (comment)).

Contributor

philsturgeon commented May 16, 2013

Right, you can offer a separate class to expose a PSR\HTTP compatible API without recoding your whole package.

Phil Sturgeon

On Thursday, May 16, 2013 at 11:00 AM, Ryan McCue wrote:

Personally, I'm not going to change Requests to a PSR interface (because a. backwards compatibility, and b. extra dependencies), but I'm definitely going to offer a bridge if/when the PSR is stable.


Reply to this email directly or view it on GitHub (#24 (comment)).

rmccue commented May 16, 2013

Right, you can offer a separate class to expose a PSR\HTTP compatible API without recoding your whole package.

Exactly. I meant the previous more as a comment that instead of users writing bridges, the library authors can do so and users can rely on a common interface.

Contributor

philsturgeon commented May 16, 2013

Gotcha :)

+ * @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.
+ * @param array $headers Array of Headers, header Name is the key.
@samdark

samdark Jan 7, 2014

Contributor

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.

+ *
+ * @return int
+ */
+ public function getStatusCode();
@samdark

samdark Jan 7, 2014

Contributor

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

+ *
+ * @return array
+ */
+ public function getHeaders();
@samdark

samdark Jan 7, 2014

Contributor

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

@nvartolomei

nvartolomei 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 Jan 7, 2014

Contributor

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

@samdark

samdark Jan 7, 2014

Contributor

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 Jan 7, 2014

Contributor

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 Jan 7, 2014

Contributor
  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 Jan 7, 2014

Contributor

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 Jan 7, 2014

Contributor

@mtdowling looks a bit too magical to me.

@philsturgeon

philsturgeon Jan 7, 2014

Contributor

@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 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.

Should we also include some method to tell the class to set how many redirects to follow as well?

Contributor

philsturgeon commented Oct 14, 2014

I would suggest that now we have the HTTP Message client, this is going to need a serious do-over. Hope you lot are ok with me closing it. It's super old, and can always be used for an updated version.

@lanthaler lanthaler referenced this pull request in lanthaler/JsonLD Jun 21, 2015

Open

Add support for HTTP libraries (support PSR-7) #4

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