-
Notifications
You must be signed in to change notification settings - Fork 6
Fix composer version conflict with react/http v0.8 #28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
1a91065
f305b04
4e2ca57
07dfeda
4389772
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 |
---|---|---|
|
@@ -6,8 +6,10 @@ | |
use React\EventLoop\Factory as EventLoopFactory; | ||
use React\Dns\Resolver\Resolver as DnsResolver; | ||
use React\Dns\Resolver\Factory as DnsResolverFactory; | ||
use React\HttpClient\Factory as HttpClientFactory; | ||
use React\HttpClient\Client as HttpClient; | ||
use React\HttpClient\Factory as HttpClientFactory; | ||
use React\Socket\Connector; | ||
use React\Socket\ConnectorInterface; | ||
|
||
/** | ||
* Factory wrapper for React instances. | ||
|
@@ -43,24 +45,93 @@ public static function buildDnsResolver( | |
return $factory->createCached($dns, $loop); | ||
} | ||
|
||
/** | ||
* @param LoopInterface $loop | ||
* @param DnsResolver|null $dns | ||
* | ||
* @return ConnectorInterface | ||
*/ | ||
public static function buildConnector( | ||
LoopInterface $loop, | ||
DnsResolver $dns = null | ||
) { | ||
return null !== $dns | ||
? new Connector($loop, ['dns' => $dns]) | ||
: new Connector($loop); | ||
} | ||
|
||
/** | ||
* Build a React Http Client. | ||
* | ||
* @param LoopInterface $loop | ||
* @param DnsResolver $dns | ||
* @param LoopInterface $loop | ||
* @param ConnectorInterface|DnsResolver|null $connector Passing a DnsResolver instance is deprecated. Should pass a | ||
* ConnectorInterface instance. | ||
* | ||
* @return HttpClient | ||
*/ | ||
public static function buildHttpClient( | ||
LoopInterface $loop, | ||
DnsResolver $dns = null | ||
$connector = null | ||
) { | ||
if (class_exists(HttpClientFactory::class)) { | ||
// if HttpClientFactory class exists, use old behavior for backwards compatibility | ||
return static::buildHttpClient04($loop, $connector); | ||
} else { | ||
return static::buildHttpClient05($loop, $connector); | ||
} | ||
} | ||
|
||
/** | ||
* Builds a React Http client v0.4 style. | ||
* | ||
* @param LoopInterface $loop | ||
* @param DnsResolver|null $dns | ||
* | ||
* @return HttpClient | ||
*/ | ||
private static function buildHttpClient04( | ||
LoopInterface $loop, | ||
$dns = null | ||
) { | ||
// create dns resolver if one isn't provided | ||
if (null === $dns) { | ||
$dns = self::buildDnsResolver($loop); | ||
$dns = static::buildDnsResolver($loop); | ||
} | ||
|
||
// validate connector instance for proper error reporting | ||
if (!$dns instanceof DnsResolver) { | ||
throw new \InvalidArgumentException('$connector must be an instance of DnsResolver'); | ||
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. Here we should say |
||
} | ||
|
||
$factory = new HttpClientFactory(); | ||
|
||
return $factory->create($loop, $dns); | ||
} | ||
|
||
/** | ||
* Builds a React Http client v0.5 style. | ||
* | ||
* @param LoopInterface $loop | ||
* @param DnsResolver|ConnectorInterface|null $connector | ||
* | ||
* @return HttpClient | ||
*/ | ||
private static function buildHttpClient05( | ||
LoopInterface $loop, | ||
$connector = null | ||
) { | ||
// build a connector with given DnsResolver if provided (old deprecated behavior) | ||
if ($connector instanceof DnsResolver) { | ||
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. can you please add a deprecation warning inside the if?
|
||
$connector = static::buildConnector($loop, $connector); | ||
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. I think we should only build connector if it's a Resolver, when it's null we should let React do this job as it's an optional argument (less code to maintain as we don't have to follow the default behavior of react of creating a dns connector) |
||
} | ||
|
||
// validate connector instance for proper error reporting | ||
if (null !== $connector && !$connector instanceof ConnectorInterface) { | ||
throw new \InvalidArgumentException( | ||
'$connector must be an instance of DnsResolver or ConnectorInterface' | ||
); | ||
} | ||
|
||
return new HttpClient($loop, $connector); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
<?php | ||
|
||
namespace Http\Adapter\React\Tests; | ||
|
||
use Http\Adapter\React\ReactFactory; | ||
use PHPUnit\Framework\TestCase; | ||
use React\Dns\Resolver\Resolver; | ||
use React\EventLoop\LoopInterface; | ||
use React\HttpClient\Client; | ||
use React\HttpClient\Factory; | ||
use React\Socket\ConnectorInterface; | ||
|
||
/** | ||
* These tests don't really ensure the correct instances are baked into the returned http client, instead, they are | ||
* just testing the code against the expected use cases. | ||
* | ||
* @author Samuel Nogueira <[email protected]> | ||
*/ | ||
class ReactFactoryTest extends TestCase | ||
{ | ||
/** | ||
* @var \React\EventLoop\LoopInterface | ||
*/ | ||
private $loop; | ||
|
||
protected function setUp() | ||
{ | ||
$this->loop = $this->getMockBuilder(LoopInterface::class)->getMock(); | ||
} | ||
|
||
public function testBuildHttpClientWithConnector() | ||
{ | ||
if (class_exists(Factory::class)) { | ||
$this->markTestSkipped('This test only runs with react http client v0.5 and above'); | ||
} | ||
|
||
$connector = $this->getMockBuilder(ConnectorInterface::class)->getMock(); | ||
$client = ReactFactory::buildHttpClient($this->loop, $connector); | ||
$this->assertInstanceOf(Client::class, $client); | ||
} | ||
|
||
/** | ||
* @deprecated Building HTTP client passing a DnsResolver instance is deprecated. Should pass a ConnectorInterface | ||
* instance instead. | ||
*/ | ||
public function testBuildHttpClientWithDnsResolver() | ||
{ | ||
$connector = $this->getMockBuilder(Resolver::class)->disableOriginalConstructor()->getMock(); | ||
$client = ReactFactory::buildHttpClient($this->loop, $connector); | ||
$this->assertInstanceOf(Client::class, $client); | ||
} | ||
|
||
public function testBuildHttpClientWithoutConnector() | ||
{ | ||
$client = ReactFactory::buildHttpClient($this->loop); | ||
$this->assertInstanceOf(Client::class, $client); | ||
} | ||
|
||
/** | ||
* @expectedException \InvalidArgumentException | ||
*/ | ||
public function testBuildHttpClientWithInvalidConnectorThrowsException() | ||
{ | ||
$connector = $this->getMockBuilder(LoopInterface::class)->getMock(); | ||
ReactFactory::buildHttpClient($this->loop, $connector); | ||
} | ||
} |
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.
i think we should be more specific. say: "Only pass this argument if you need to customize DNS behaviour. With react 0.5, pass a connector, wth react 0.4 this must be a DnsResolver."