Updated the client to use buzz Browser instead of FileGetContents

[nixilla]

The adapter should use Browser object instead of FileGetContents. Otherwise the user will not be able to use Curl client and pre and post listeners.

Also setMaxRedirects(0) is only required for tests, so it should not be in the constructor.

I also tried to add CurlHttpAdapterTest but your tests are sending payload with GET request, and Buzz (used with Curl) choose not to send any payload with the GET/HEAD method. So I would suggest to change your tests.

[sagikazarmark]

I think this was added in order to move the redirection stuff to plugins.

[sagikazarmark]

Although we should possibly only add it, when the underlying client is not passed:

https://github.com/php-http/guzzle6-adapter/blob/master/src/Client.php#L31

[nixilla]

Well, by default it is 5, therefore it follows redirects. I didn't have to change this value ever. Setting it to 0 is used by your test in order to verify that you get 302. I'd say do not change it, and let the user do it.

[Nyholm]

We want to make sure all adapters behaves inte the same way. This means if adapter A returns a 302 response adapter B has to do that as well. No adapter should ever follow redirects. If your application needs to follow redirects you should use the RedirectPlugin as Mark suggested.

I know if feels like you are "walking across the river to get some water" (not sure that expression works in English) but this makes sense for the implementation. In fancy words, this is the Liskov substitution principle.

[sagikazarmark]

If you take a look at what I wrote: we only modify it if there is no custom client (meaning we create one). If you inject a custom client, you have full control over it.

[nixilla]

I've amended the code. Is this what you're after?

[dbu]

you only set max redirects if there was no client passed. i guess its ok that way. there is still risk that people pass a default Browser and then are surprised when switching to another adapter that redirects are no longer followed. but if they pass a custom buzz client, switching will be tricky anyways.

[Nyholm]

I think I had a reson why I did not use the Brower. Maybe I thought it was unneeded overhead. Maybe I was wrong. I did not consider your use case where you have plenty of listeners. Or maybe I thought that your listeners should be rewritten as Httplug plugins...

Right now I think I your are correct. We should use the browser.
Also, when we test, we should test will all the Buzz clients.

[dbu]

this change is a BC break, if i assume correctly that Browser is more restrictive than FileGetContents. does buzz have 2 alternative clients? should we maybe create a second adapter for the browser based client so the user can choose between the FileGetContents based one and the Browser one?

or can we simply allow both classes in the constructor and verify that its either of those, to avoid the BC break?

[Nyholm]

@dbu To answer your questions about Browser or FileGetContents:

FileGetContents, Curl and MultiCurl are classes implementing a ClientInterface. The Browser class takes a ClientInterface in order to actually send requests. The Browser is 90% syntactic sugar. The other 10% is support for listeners (same as Httplug plugins). Here is a list of listeners.

[dbu]

thanks @Nyholm. then i think the right course of action is to allow any ClientInterface in the constructor, and keep creating the FileGetContents if none is passed, as it seems we don't need the syntactic sugar.

[Nyholm]

thanks @Nyholm. then i think the right course of action is to allow any ClientInterface in the constructor, and keep creating the FileGetContents if none is passed, as it seems we don't need the syntactic sugar.

That does not help @nixilla, he want use the filters in the Browser class. Also, ClientInterface does only contain a send function. There is no way of setMaxRedirects. But that might be okey. We only need setMaxRedirects if we are creating the client.

An idea:
Let our class take a ClientInterface in the constructor and if someone (like @nixilla) does not want to use Httplug plugins and just stick with the Buzz listeners for a while, we should have a setBrowser function.
What do you think about that?

[dbu]

i think that would help nixilla. if we allow ClientInterface, he can pass the browser, already set up with the plugins and whatever, as its also a ClientInterface. and indeed, we should only setMaxRedirects when we instantiate the GetFileContents client ourselves. if we get passed a client, we should not touch it. afaik thats the same with the guzzle adapter.

[Nyholm]

i think that would help nixilla. if we allow ClientInterface, he can pass the browser, already set up with the plugins and whatever, as its also a ClientInterface.

No, the Browser is not a ClientInterface. 😞 It complies with that interface but there is no implments ClientInterface.

and indeed, we should only setMaxRedirects when we instantiate the GetFileContents client ourselves. if we get passed a client, we should not touch it. afaik thats the same with the guzzle adapter.

You are correct. Agreed.

[dbu]

oh great. then lets not have a typehint but check with instanceof of either Browser or the interface and throw an exception. if both have the method we need we should accept both.

[Nyholm]

Awesome.

I will be happy to review that when @nixilla has made some changes.

[nixilla]

Dear All,

I refuse the change the code. The suggestions you've made will produce significant number of WTF per line of code. First of all, having an constructor which can accept any type of object is usually unexpected - and would cause a lot of WTFs.

The reason being - the adapter shouldn't know too much about internals of Buzz. The adapter shouldn't care whether user is using Curl or FileGetContents. The adapter should not care whether the user is using redirects or not. It's up to user how they configure it.

On the other hand, if user doesn't care about configuration, then default value should be reasonable. And this still means that adapter should not use internal to Buzz ClientInterface but the Browser itself. I believe the current version of code on this pull request is most optimal for both: current Buzz users, who already are familiar with the library, and you (The PHP-HTTP Group) guys who try to standardise the PHP HTTP world.

[dbu]

accepting more than one type is not elegant, but seems like the best option we have. if Browser does not implement ClientInterface but is the same in our context, its just a workaround for a not elegant buzz implementation detail. i don't see great risks for WTFs.

i agree that the adapter should not care which is passed to it - as long as its something it can use. which is what the instanceof can achieve, as there is no way to typehint alternative class/interface.

if nothing is passed, it seems to me there is no difference between FileGetContents or Browser. we intentionally want to keep the client at the bare minimum to ensure portability. if you need things like redirect, there is the php-http PluginClient with a redirect plugin. if we don't do this, your code only works when using buzz, but not with, say, a plain curl client implementation that has no redirect functionality out of the box. the point of the abstraction is portability between the different clients.

[nixilla]

@dbu

accepting more than one type is not elegant, but seems like the best option we have.

I don't understand what you are trying to achieve here. The best option is already on this pull request. I think you have on mind something I don't know/understand. Maybe you forget, that this is just an adapter for Buzz.

if we don't do this, your code only works when using buzz, but not with, say, a plain curl client implementation

Well, the project is called buzz-adapter, I would not expect it to work with anything else than buzz. Also adding code/solution that you may need in the future is generally bad idea.

[sagikazarmark]

@nixilla I think you are missing a point here regarding what adapters are. Although in your case you might want to use a concrete adapter, in some cases people don't want to know about the concrete implementation. For example an API client which uses an HTTP Client under the hood should not rely on an implementation. That's a configuration detail of your application. This is why it is important to make the adapter substitutable. Otherwise your code would behave differently with different client implementations.

The adapter should not care whether the user is using redirects or not. It's up to user how they configure it.

This is perfectly true, and you are allowed to do that. We only enforce our defaults when you don't pass a client to have the feature explained above.

The reason being - the adapter shouldn't know too much about internals of Buzz.

Based on what do you think that the HTTP Client classes are "internals"? Even the readme brings an example for them.

First of all, having an constructor which can accept any type of object

Not any type of object: Client and Browser. Since the Browser can act like a Client (@Nyholm do you know that listeners and any extra logic are actually executed when using the client method in the browser?) I don't think the fact that you get an exception instead of an error upon invalid type should make any difference.

[Nyholm]

do you know that listeners and any extra logic are actually executed when using the client method in the browser?

Yes, they are.

[lboynton]

I don't understand what you are trying to achieve here. The best option is already on this pull request. I think you have on mind something I don't know/understand. Maybe you forget, that this is just an adapter for Buzz.

Another reason to allow an instance of FileGetContents in the constructor is to maintain backwards compatibility, as mentioned earlier. Personally I don't see much of a problem accepting an instanceof ClientInterface or Browser in the constructor.