-
Notifications
You must be signed in to change notification settings - Fork 4
Add support for using Buzz\Browser instance (fixes #2) #4
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,10 +2,13 @@ | |
|
||
namespace Http\Adapter\Buzz; | ||
|
||
use Buzz\Browser; | ||
use Buzz\Client\ClientInterface; | ||
use Buzz\Client\Curl; | ||
use Buzz\Client\FileGetContents; | ||
use Buzz\Exception as BuzzException; | ||
use Buzz\Message\Request as BuzzRequest; | ||
use Buzz\Message\RequestInterface as BuzzRequestInterface; | ||
use Buzz\Message\Response as BuzzResponse; | ||
use Http\Client\HttpClient; | ||
use Http\Discovery\MessageFactoryDiscovery; | ||
|
@@ -30,13 +33,28 @@ class Client implements HttpClient | |
private $messageFactory; | ||
|
||
/** | ||
* @param FileGetContents|null $client | ||
* @param MessageFactory|null $messageFactory | ||
* @param ClientInterface|Browser|null $client | ||
* @param MessageFactory|null $messageFactory | ||
*/ | ||
public function __construct(FileGetContents $client = null, MessageFactory $messageFactory = null) | ||
public function __construct($client = null, MessageFactory $messageFactory = null) | ||
{ | ||
$this->client = $client ?: new FileGetContents(); | ||
$this->client->setMaxRedirects(0); | ||
$this->client = $client; | ||
|
||
if ($this->client === null) { | ||
$this->client = new FileGetContents(); | ||
$this->client->setMaxRedirects(0); | ||
} | ||
|
||
if ((!$this->client instanceof ClientInterface) && (!$this->client instanceof Browser)) { | ||
throw new \InvalidArgumentException( | ||
sprintf( | ||
'The client passed to the Buzz adapter must either implement %s or be an instance of %s. You passed %s.', | ||
ClientInterface::class, | ||
Browser::class, | ||
is_object($client) ? get_class($client) : gettype($client) | ||
) | ||
); | ||
} | ||
|
||
$this->messageFactory = $messageFactory ?: MessageFactoryDiscovery::find(); | ||
} | ||
|
@@ -46,6 +64,8 @@ public function __construct(FileGetContents $client = null, MessageFactory $mess | |
*/ | ||
public function sendRequest(RequestInterface $request) | ||
{ | ||
$this->assertRequestHasValidBody($request); | ||
|
||
$buzzRequest = $this->createRequest($request); | ||
|
||
try { | ||
|
@@ -133,4 +153,31 @@ private function getBuzzHeaders(BuzzResponse $response) | |
|
||
return $headers; | ||
} | ||
|
||
/** | ||
* Assert that the request has a valid body based on the request method. | ||
* | ||
* @param RequestInterface $request | ||
*/ | ||
private function assertRequestHasValidBody(RequestInterface $request) | ||
{ | ||
$validMethods = [ | ||
BuzzRequestInterface::METHOD_POST, | ||
BuzzRequestInterface::METHOD_PUT, | ||
BuzzRequestInterface::METHOD_DELETE, | ||
BuzzRequestInterface::METHOD_PATCH, | ||
BuzzRequestInterface::METHOD_OPTIONS, | ||
]; | ||
|
||
// 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()->getSize() && | ||
!in_array(strtoupper($request->getMethod()), $validMethods, true) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about custom methods, can they not have a body? that is, does buzz only keep the body if its one of those methods? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. indeed. okay, thanks. @sagikazarmark do you have anything to add? from my point of view this PR is good to squash and merge. |
||
) { | ||
throw new \InvalidArgumentException( | ||
sprintf('%s does not support %s requests with a body', Curl::class, $request->getMethod()) | ||
); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
<?php | ||
|
||
namespace Http\Adapter\Buzz\Tests; | ||
|
||
use Buzz\Browser; | ||
use Buzz\Client\FileGetContents; | ||
|
||
class BrowserHttpAdapterTest extends HttpAdapterTest | ||
{ | ||
/** | ||
* {@inheritdoc} | ||
*/ | ||
protected function createBuzzClient() | ||
{ | ||
$browser = new Browser(); | ||
$browser->getClient()->setMaxRedirects(0); | ||
|
||
return $browser; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
<?php | ||
|
||
namespace Http\Adapter\Buzz\Tests; | ||
|
||
use Buzz\Client\Curl; | ||
use Http\Adapter\Buzz\Client; | ||
use Buzz\Message\RequestInterface as BuzzRequestInterface; | ||
|
||
class CurlHttpAdapterTest extends HttpAdapterTest | ||
{ | ||
/** | ||
* {@inheritdoc} | ||
*/ | ||
protected function createBuzzClient() | ||
{ | ||
$curl = new Curl(); | ||
$curl->setMaxRedirects(0); | ||
|
||
return $curl; | ||
} | ||
|
||
/** | ||
* @dataProvider requestProvider | ||
* @group integration | ||
*/ | ||
public function testSendRequest($method, $uri, array $headers, $body) | ||
{ | ||
$validMethods = [ | ||
BuzzRequestInterface::METHOD_POST, | ||
BuzzRequestInterface::METHOD_PUT, | ||
BuzzRequestInterface::METHOD_DELETE, | ||
BuzzRequestInterface::METHOD_PATCH, | ||
BuzzRequestInterface::METHOD_OPTIONS, | ||
]; | ||
|
||
if (!in_array($method, $validMethods, true) && $body) { | ||
$this->setExpectedException( | ||
\InvalidArgumentException::class, | ||
sprintf('Buzz\Client\Curl does not support %s requests with a body', $method) | ||
); | ||
} | ||
|
||
parent::testSendRequest($method, $uri, $headers, $body); | ||
} | ||
|
||
/** | ||
* @dataProvider requestWithOutcomeProvider | ||
* @group integration | ||
*/ | ||
public function testSendRequestWithOutcome($uriAndOutcome, $protocolVersion, array $headers, $body) | ||
{ | ||
if ($body && $protocolVersion === '1.1') { | ||
$this->setExpectedException( | ||
\InvalidArgumentException::class, | ||
'Buzz\Client\Curl does not support GET requests with a body' | ||
); | ||
} | ||
|
||
parent::testSendRequestWithOutcome($uriAndOutcome, $protocolVersion, $headers, $body); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
<?php | ||
|
||
namespace Http\Adapter\Buzz\Tests; | ||
|
||
class DefaultHttpAdapterTest extends HttpAdapterTest | ||
{ | ||
/** | ||
* {@inheritdoc} | ||
*/ | ||
protected function createBuzzClient() | ||
{ | ||
// returning null here should cause the adapter to create a default client for us | ||
return null; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
<?php | ||
|
||
namespace Http\Adapter\Buzz\Tests; | ||
|
||
use Http\Adapter\Buzz\Client; | ||
use Http\Message\MessageFactory\GuzzleMessageFactory; | ||
|
||
class InvalidHttpAdapterTest extends \PHPUnit_Framework_TestCase | ||
{ | ||
/** | ||
* @expectedException \InvalidArgumentException | ||
* @expectedExceptionMessage The client passed to the Buzz adapter must either implement Buzz\Client\ClientInterface | ||
* or be an instance of Buzz\Browser. You passed stdClass. | ||
*/ | ||
public function testConstructorThrowsExceptionWhenInvalidClientInstanceIsUsed() | ||
{ | ||
return new Client( | ||
new \stdClass(), | ||
new GuzzleMessageFactory() | ||
); | ||
} | ||
|
||
/** | ||
* @expectedException \InvalidArgumentException | ||
* @expectedExceptionMessage The client passed to the Buzz adapter must either implement Buzz\Client\ClientInterface | ||
* or be an instance of Buzz\Browser. You passed string. | ||
*/ | ||
public function testConstructorThrowsExceptionWhenNonObjectClientIsUsed() | ||
{ | ||
return new Client( | ||
'This should be an object!', | ||
new GuzzleMessageFactory() | ||
); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the default be FileGetContents or Browser?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that a Browser would be an unnecessary overhead here, if the user doesn't care about the underlying instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, will leave this as is then.