Skip to content

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Dec 21, 2017
Merged

Fix composer version conflict with react/http v0.8 #28

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1a91065
Fix composer version conflict with react/http v0.8
Dec 15, 2017
f305b04
Added possibility to build Http Client passing a Connector instead of…
Dec 19, 2017
4e2ca57
ReactFactory is now compatible with both react/http-client v0.4 and v0.5
Dec 19, 2017
07dfeda
Applied StyleCI suggested fixes
Dec 19, 2017
4389772
- E_USER_DEPRECATED now triggered when using deprecated behavior in R…
Dec 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we keep allowing 0.4.8 or are there BC breaks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"react/dns": "^0.4.1",
"react/stream": "^0.4.3",
"react/stream": "^0.4.3 || ^0.7",
"react/socket": "^0.8",
"php-http/discovery": "^1.0"
},
"require-dev": {
Expand Down
22 changes: 18 additions & 4 deletions src/ReactFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@
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\Socket\Connector;
use React\Socket\DnsConnector;

/**
* Factory wrapper for React instances.
Expand Down Expand Up @@ -54,13 +55,26 @@ public static function buildDnsResolver(
public static function buildHttpClient(
LoopInterface $loop,
DnsResolver $dns = null
) {
$dnsConnector = static::buildDnsConnector($loop, $dns);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@samuelnogueira samuelnogueira Dec 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return new HttpClient($loop, $dnsConnector);
}

/**
* @param LoopInterface $loop
* @param DnsResolver|null $dns
*
* @return DnsConnector
*/
public static function buildDnsConnector(
LoopInterface $loop,
DnsResolver $dns = null
) {
if (null === $dns) {
$dns = self::buildDnsResolver($loop);
}

$factory = new HttpClientFactory();

return $factory->create($loop, $dns);
return new DnsConnector(new Connector($loop), $dns);
}
}