Add support for using Buzz\Browser instance (fixes #2) #4

lboynton · 2016-07-06T09:30:15Z

This change allows instances of Buzz\Browser to be used in the adapater, which is what people generally use with Buzz. Also allow any instance implementing Buzz's ClientInterface to be used, such as the curl implementation.

Note that MultiCurl does not work at this time as it needs to be flushed to send requests.

lboynton · 2016-07-06T09:32:16Z

src/Client.php

+        $this->client = $client;
+
+        if ($this->client === null) {
+            $this->client = new FileGetContents();


Should the default be FileGetContents or Browser?

It seems to me that a Browser would be an unnecessary overhead here, if the user doesn't care about the underlying instance.

OK, will leave this as is then.

lboynton · 2016-07-06T10:19:22Z

As mentioned in #2, the tests fail when using Buzz's Curl instance because no request body is sent for GET, HEAD and TRACE requests. The tests explicitly send a request body rather use query string parameters in these cases, which isn't what you would do in practice.

sagikazarmark · 2016-07-06T10:46:50Z

src/Client.php

+        }
+
+        if ((!$this->client instanceof ClientInterface) && (!$this->client instanceof Browser)) {
+            throw new InvalidArgumentException(


We prefer using \InvalidArgumentException. (Generally root namespace classes should not be imported.)

sagikazarmark · 2016-07-06T10:48:47Z

Hm, since I haven't written the adapter tests I can't say for sure why we send a body for GET requests. @egeloen any ideas? Is this something that we should fix in the test suite?

lboynton · 2016-07-06T10:51:41Z

This is the relevant section in the test suite btw: https://github.com/php-http/adapter-integration-tests/blob/master/src/HttpBaseTest.php#L188

dbu · 2016-07-07T06:34:02Z

src/Client.php

+        if ((!$this->client instanceof ClientInterface) && (!$this->client instanceof Browser)) {
+            throw new \InvalidArgumentException(
+                sprintf(
+                    'Buzz adapter client of instance %s must implement %s or be an instance of %s',


Can we say 'The client passed to the Buzz adapter must either implement %s or be an instance of %s. You passed %s'.

dbu · 2016-07-07T06:40:18Z

thanks @lboynton , looks good!

GET requests are allowed to have a body. see for example elasticsearch...

so i think if the tests want to explicitly set a body, its fair to test that the body exists. is this what we talk about, or do the tests expect an empty body to be automatically present even when not doing anything? the later would sound like an implementation detail that should not be required by the tests.

lboynton · 2016-07-07T07:24:57Z

Yes GET requests can have a body, but the Curl class in Buzz does not send a body for GET requests. So the current tests fail with the Curl class, but in practice the Curl class will probably work fine with the adapter as people don't generally use GET bodies. Perhaps this should be documented in the adapter.

dbu · 2016-07-07T08:00:15Z

okay, so that is essentially a bug of the buzz curl class. imo what we should do in the adapter is throw an exception when a body is set on a GET request to tell the user this is not supported. if somebody wants to use elasticsearch for example, they will be glad to have an exception rather than nothing working as expected.

the tests are currently green, not sure why. but i think in that case it would be ok to make the acceptance test skip the test about GET body, or have an alternative test that ensures we get the exception when setting a body.

the test suite should also do GET requests without body, if it not already does.

lboynton · 2016-07-07T14:22:59Z

OK I've added an exception which is thrown when trying to send a request body for GET, HEAD or TRACE requests when using the Curl client.

I've overridden the test methods to assert this behaviour. I'm not sure if this is the best approach or if I should modify the acceptance tests directly to be more flexible.

Nyholm · 2016-07-07T18:19:43Z

src/Client.php

+        // The Buzz Curl client does not send request bodies for request methods such as GET, HEAD and TRACE. Instead of
+        // silently ignoring the request body in these cases, throw an exception to make users aware.
+        if ($this->client instanceof Curl &&
+            $request->getBody()->getContents() &&


Use getSize() instead of getContents(), I believe that would be faster.

Nyholm · 2016-07-07T18:23:17Z

Awesome work. I think this looks really good! Thank you.

dbu · 2016-07-07T20:11:26Z

src/Client.php

+        // silently ignoring the request body in these cases, throw an exception to make users aware.
+        if ($this->client instanceof Curl &&
+            $request->getBody()->getSize() &&
+            !in_array(strtoupper($request->getMethod()), $validMethods, true)


what about custom methods, can they not have a body? that is, does buzz only keep the body if its one of those methods?

The request methods which have request bodies are defined with CURLOPT_POSTFIELDS here: https://github.com/kriswallsmith/Buzz/blob/master/lib/Buzz/Client/AbstractCurl.php#L96

case RequestInterface::METHOD_POST: case RequestInterface::METHOD_PUT: case RequestInterface::METHOD_DELETE: case RequestInterface::METHOD_PATCH: case RequestInterface::METHOD_OPTIONS: $options[CURLOPT_POSTFIELDS] = $fields = static::getPostFields($request);

You can see it's just POST, PUT, DELETE, PATCH and OPTIONS.

indeed. okay, thanks. @sagikazarmark do you have anything to add? from my point of view this PR is good to squash and merge.

dbu · 2016-07-07T20:15:54Z

great work, i think this is exactly right. also the tests seem to me what they should be. i consider it a bug or at least unexpected restriction from buzz to not support bodies in GET requests, so we should not make the test suite encourage such behaviour ;-)
your test alterations are exactly at the right place.

apart from that question about custom HTTP methods with body, this looks good to merge.

This change allows instances of Buzz\Browser to be used in the adapater, which is what people generally use with Buzz. Also allow any instance implementing Buzz's ClientInterface to be used, such as the curl implementation. Note that: - MultiCurl does not work at this time as it needs to be flushed to send requests. - The Curl client does not apply request bodies to certain request methods such as GET, HEAD and TRACE. In order to inform users about this, we throw an exception if the Curl client is being used, and we're trying to send a request body with one of these methods.

lboynton · 2016-07-12T11:32:53Z

I've squashed my commits, is there anything else that needs doing on this?

dbu · 2016-07-12T13:42:24Z

thanks a lot!

lboynton · 2016-07-12T13:50:38Z

Thanks all! 🎉

sagikazarmark · 2016-07-12T13:52:52Z

Sorry that I kind of missed this PR. Quickly reviewed it and it looks good to me. Thank you @lboynton for your work.

lboynton reviewed Jul 6, 2016
View reviewed changes

sagikazarmark reviewed Jul 6, 2016
View reviewed changes

lboynton force-pushed the support-browser-instance branch from 609b6d3 to 27c6c20 Compare July 6, 2016 10:50

dbu reviewed Jul 7, 2016
View reviewed changes

lboynton force-pushed the support-browser-instance branch 4 times, most recently from f3f3e8c to 97c6b78 Compare July 7, 2016 14:19

lboynton force-pushed the support-browser-instance branch 2 times, most recently from 79e5ef8 to dc8c4a2 Compare July 7, 2016 16:57

Nyholm reviewed Jul 7, 2016
View reviewed changes

lboynton force-pushed the support-browser-instance branch 2 times, most recently from b8f6ea3 to 2b5275c Compare July 7, 2016 19:53

dbu reviewed Jul 7, 2016
View reviewed changes

lboynton force-pushed the support-browser-instance branch from 2b5275c to 89ff8c2 Compare July 7, 2016 20:34

dbu merged commit 4b1310a into php-http:master Jul 12, 2016

lboynton deleted the support-browser-instance branch July 12, 2016 13:53

Add support for using Buzz\Browser instance (fixes #2) #4

Add support for using Buzz\Browser instance (fixes #2) #4

Uh oh!

Conversation

lboynton commented Jul 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lboynton commented Jul 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Jul 6, 2016

Uh oh!

lboynton commented Jul 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Jul 7, 2016

Uh oh!

lboynton commented Jul 7, 2016

Uh oh!

dbu commented Jul 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lboynton commented Jul 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Jul 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lboynton Jul 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Jul 7, 2016

Uh oh!

lboynton commented Jul 12, 2016

Uh oh!

dbu commented Jul 12, 2016

Uh oh!

lboynton commented Jul 12, 2016

Uh oh!

sagikazarmark commented Jul 12, 2016

Uh oh!

Uh oh!

lboynton commented Jul 6, 2016 •

edited

Loading

dbu commented Jul 7, 2016 •

edited

Loading

lboynton Jul 7, 2016 •

edited

Loading