Fix composer version conflict with react/http v0.8 #28
Conversation
- Upgraded dependency on react/http-client - Introduced dependency on react/socket so \Http\Adapter\React\buildDnsResolver and \Http\Adapter\React\buildHttpClient remain usable as before (no need for major version bump on php-http/react-adapter)
src/ReactFactory.php
Outdated
| @@ -54,13 +55,26 @@ public static function buildDnsResolver( | |||
| public static function buildHttpClient( | |||
| LoopInterface $loop, | |||
| DnsResolver $dns = null | |||
| ) { | |||
| $dnsConnector = static::buildDnsConnector($loop, $dns); | |||
There was a problem hiding this comment.
did the react client constructor signature change to expect the dns connector instead of the dns resolver? no auto-instantiation?
if so, we should probably follow the pattern of the DnsResolver and add another optional parameter for the connector. hm, but $dns is never needed when you pass the whole connector. maybe remove the typehint from $dns and do instanceof to see if its the resolver or the connector? and deprecate passing only the resolver?
There was a problem hiding this comment.
did the react client constructor signature change to expect the dns connector instead of the dns resolver? no auto-instantiation?
Yes it did. There is auto-instantiation though, the dns connector could be omitted, and we could simply deprecate the entire $dns param for the buildHttpClient method, however, wouldn't this be a major version bump? Also, the ability to control the dns connector/resolver instance would be lost (was it even needed in the first place?).
About your suggestion, doing the instanceof of the second buildHttpClient parameter to allow either the connector or the dns resolver to be passed looks like a good solution, to maintain retro-compatibility + IoC for the connector instance, I'll push a commit with that change.
There was a problem hiding this comment.
i would just add the flexibility but deprecate passing the resolver and recommend to pass the connector. i think the factory should keep allowing a custom dns resolution tool.
in a version 2 we can then add the typehint for the connector and thus drop dns resolver support in the http client factory method.
composer.json
Outdated
| @@ -13,9 +13,10 @@ | |||
| "require": { | |||
| "php": "^5.5 || ^7.0", | |||
| "php-http/httplug": "^1.0", | |||
| "react/http-client": "^0.4.8", | |||
| "react/http-client": "^0.5", | |||
There was a problem hiding this comment.
can we keep allowing 0.4.8 or are there BC breaks?
There was a problem hiding this comment.
Unfortunately there are: https://github.com/reactphp/http-client/releases/tag/v0.5.0
Furthermore, \React\HttpClient\Client's constructor signature has changed, so even calling it directly instead of using Factory won't work the same in v0.4 and v0.5.
There was a problem hiding this comment.
okay. i suggest we bump the minor version of this library then, and deprecate passing the dns resolver but still support it (see my comment above)
There was a problem hiding this comment.
I think we can be BC here and keep 0.4.8 by checking if this React\HttpClient\Factory class exists, then use old behavior in that case, otherwise use the new behavior
|
Since we are introducing a new public method |
… a DnsResolver - ReactFactory now builds a Connector instance instead of DnsConnector - Unit tests
src/ReactFactory.php
Outdated
| $dns = self::buildDnsResolver($loop); | ||
| // build a connector if none is given, or create one attaching given DnsResolver (old deprecated behavior) | ||
| if (null === $connector || $connector instanceof DnsResolver) { | ||
| $connector = static::buildConnector($loop, $connector); |
There was a problem hiding this comment.
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)
- Declared dependency on phpunit 4.8.36 (because FC layer) - Updated ReactFactory unit tests for compatibility with both phpunit 4 and 5
| $connector = null | ||
| ) { | ||
| // build a connector with given DnsResolver if provided (old deprecated behavior) | ||
| if ($connector instanceof DnsResolver) { |
There was a problem hiding this comment.
can you please add a deprecation warning inside the if?
@trigger_error(sprintf('Passing a %s to buildHttpClient is deprecated since version 2.1.0 and will be removed in 3.0. If you need no specific behaviour, omit the $dns argument, otherwise pass a %s', DnsResolver::class, ConnectorInterface::class), E_USER_DEPRECATED);
src/ReactFactory.php
Outdated
| * @param LoopInterface $loop | ||
| * @param DnsResolver $dns | ||
| * @param LoopInterface $loop | ||
| * @param ConnectorInterface|DnsResolver|null $connector Passing a DnsResolver instance is deprecated. Should pass a |
There was a problem hiding this comment.
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."
src/ReactFactory.php
Outdated
|
|
||
| // 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.
Here we should say For react 0.4, $dns must be an instance of DnsResolver
…eactFactory - Fixed scrutinizer reported issues
What's in this PR?
Upgraded dependency on react/http-client
Introduced dependency on react/socket so \Http\Adapter\React\buildDnsResolver and \Http\Adapter\React\buildHttpClient remain usable as before (no need for major version bump on php-http/react-adapter)