php-http / httplug

HTTPlug, the HTTP client abstraction for PHP
http://httplug.io
MIT License
2.57k stars 39 forks source link

Adds configurable HTTP adapter interface #31

Closed sagikazarmark closed 9 years ago

sagikazarmark commented 9 years ago

This PR adds a configurable HTTP Adapter interface to the Client separated adapter contract.

Rationale

There are some options in the underlying HTTP clients which cannot be controlled with a single message (request). For example timeout can only be passed directly to the guzzle client object. The extra optional $options argument serves this purpose.

How does it work?

Since adapters are exchangeable, options should be abstracted away from underlying HTTP clients. The previously mentioned example (timeout) is a good start. Since the adapter tries to be a raw wrapper around clients providing a way to use PSR messages with them, options should be very limited. For example user agent, connection (previously available in adapters) should go away.

I can imagine that each adapter can provide a way to pass client specific options (for example under a guzzle key in the options array), but that should be avoided. However it does not break any interface to pass extra options.

Why not a Configurable message implementation/decorator)

Earlier the plan was to create Configurable message implementations so that options can directly be passed with the request. The reason for that was to be able to pass options per request even in multi request case (sendRequests). In most cases parallel request sending is emulated since only curl based clients can send parallel requests. However those clients does not allow passing options per requests, only one option array for all the requests. Emulated multi request sending should also adhere that behavior.

Some further pros of an options array:

sagikazarmark commented 9 years ago

/cc @egeloen @ddeboer

ddeboer commented 9 years ago

In general: :+1:. Two questions:

  1. Why not just merge this ConfigurableHttpAdapter interface into HttpAdapter? I know interface segregation is good, but that way you wouldn’t have to do discover whether the adapters supports options but just assume so (are there any adapters that don't?).
  2. Would it make sense to have a Configuration object that is passed instead of an array for $options? One advantage is that it could make explicit which options are permitted by having appropriate setters such as setTimeout() (but not setUserAgent()).
sagikazarmark commented 9 years ago
  1. The main point of HttpAdapter is to provide a layer between PSR and the underlying HTTP client. Configuration is just extra sauce IMO. Packages requiring the adapter shouldn't really pass options to it. It is only there to make it possible, but is not always a good idea IMHO.
  2. We had a Configuration object previously. The reason we dropped is that it is too restrictive and we have version it with the main adapter. This way it is decoupled.
ddeboer commented 9 years ago

Packages requiring the adapter shouldn't really pass options to it. It is only there to make it possible, but is not always a good idea IMHO.

True, but those packages can just ignore the $options argument, so we end up with the default empty array.

sagikazarmark commented 9 years ago

You are right. I guess it is overseparation of interfaces 😃

ddeboer commented 9 years ago

So what should we do? Add it to the HttpAdapter instead?

On Sun, May 31, 2015 at 3:39 PM, Márk Sági-Kazár notifications@github.com wrote:

You are right. I guess it is overseparation of interfaces 😃

Reply to this email directly or view it on GitHub: https://github.com/php-http/adapter/pull/31#issuecomment-107181938

sagikazarmark commented 9 years ago

I've already done it, just haven't pushed it yet. ;)

ddeboer commented 9 years ago

Cool!

On Sun, May 31, 2015 at 4:48 PM, Márk Sági-Kazár notifications@github.com wrote:

I've already done it, just haven't pushed it yet. ;)

Reply to this email directly or view it on GitHub: https://github.com/php-http/adapter/pull/31#issuecomment-107200385