-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add main Connector facade #93
Conversation
src/Connector.php
Outdated
} | ||
|
||
if (!isset($this->connectors[$scheme])) { | ||
return Promise\reject(new RuntimeException('No connector registered for unknown URI scheme')); |
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'd suggest the following exception message:
return Promise\reject(new RuntimeException(
sprintf(
'No connector registered for URI scheme %s',
$scheme
)
));
$scheme = 'tcp'; | ||
if (strpos($uri, '://') !== false) { | ||
$scheme = (string)substr($uri, 0, strpos($uri, '://')); | ||
} |
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.
Suggestion: if no scheme is explicitely given and port is 443, use tls 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.
More magic:
google.com
with'tcp' => false
-> Use tlsgoogle.com:443
with'tls' => false
-> Use tcp
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.
Both suggestions make sense in higher-level protocols (e.g. HTTP/HTTPS), but I'm hesitant to add them in the low-level network protocols (TCP/IP etc.) here.
I believe this is best left up to consumers of this package (such as react/http-client), but I guess it makes sense to add some kind of support for these kind of things in future versions 👍
Thanks for the suggestion, updated and squashed to use improved error message |
This PR adds a new
Connector
class that acts as a facade for all underlying connectors, which are now marked as "advanced usage", but continue to work unchanged.The
Connector
class now makes it trivially easy to create plaintext TCP/IP, secure TLS and Unix domain socket connection streams like this:Resolves / closes #38