Skip to content
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

Allow Connector to have no Resolver #12

Closed
stivni opened this issue Aug 20, 2014 · 6 comments · Fixed by #46
Closed

Allow Connector to have no Resolver #12

stivni opened this issue Aug 20, 2014 · 6 comments · Fixed by #46

Comments

@stivni
Copy link

stivni commented Aug 20, 2014

...so I can connect to an ip address if I do something locally

It is already possible to call createSocketForAddress with the desired ip address, but it doesn't feel right that I need a dns resolver for that. One solution would be to have a resolving connector and non resolving connector. Or are there other suggestions?

I'm willing to try something in code if I know what the opinions are...

@cboden
Copy link
Contributor

cboden commented Aug 20, 2014

See #7. The API of the PR is good but see my comment about a Null pattern instead of an exception. Unfortunately the submitter hasn't commented or updated the request.

@stivni
Copy link
Author

stivni commented Aug 20, 2014

I get the Null pattern thing, but isn't it always the case, if your hostname is an ip address, that we don't need the call a dns server? This means that we could include this behaviour in the Resolver itself.

It still is the case the some people would not want a Resolver if they're absolutely sure they will be working with ip addresses, but for me that not really an issue for now...

@cboden
Copy link
Contributor

cboden commented Aug 20, 2014

Ah, I think I see now. This issue belongs in reactphp/dns in that case.

@clue
Copy link
Contributor

clue commented Feb 23, 2015

Allow Connector to have no Resolver

Big 👍 on the idea, makes perfect sense! Resolving this ticket is also a requisite for using this component as part of the DNS component (reactphp/dns#19).

One solution would be to have a resolving connector and non resolving connector. Or are there other suggestions?

Yes, this would be one possible solution. Would you care to file a PR to see how this works out? See also #7 and @cboden's suggestion for an alternative implementation.

[…] see my comment about a Null pattern instead of an exception.

Indeed, this has been raised several times already. I've filed a relevant feature request ticket in the DNS component to keep track of this: reactphp/dns#17

[…] if your hostname is an ip address, that we don't need the call a dns server? This means that we could include this behaviour in the Resolver itself.

Nice suggestion, makes perfect sense! I've filed a relevant feature request ticket in the DNS component to keep track of this: reactphp/dns#18

Thanks for discussing these concepts so far!

@clue
Copy link
Contributor

clue commented Jun 7, 2015

Allow Connector to have no Resolver

As an alternative to a NullResolver, we can also split our Connector into a TcpConnector($loop) and a ResolvingConnector($tcpConnector, $resolver). IMO this is a more SOLID design, however it's even more cumbersome to work with from an API consumer standpoint:

$loop = React\EventLoop\Factory::create();
$resolverFactory = new React\Dns\Resolver\Factory();
$resolver = $resolverFactory->create('8.8.8.8', $loop);
$tcp = new TcpConnector($loop);
$resolving = new ResolvingConnector($resolver, $tcp);
$secure = new SecureConnector($resolving, $loop);

$secure->create('www.google.com', 443);

Just putting this out here. I think it might still be worth implementing, should we consider providing a simpler facade API (see #38). The resulting API could eventually looks like this:

$loop = React\EventLoop\Factory::create();
$facade = …; // not decided yet, but should probably be a single call (see #38)

$facade->createConnection('tls://www.google.com:443');

@clue
Copy link
Contributor

clue commented Sep 7, 2015

See also #46 for a PR which implements the above suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants