Merge pull request #40 from clue-labs/uris

Consistently require URL when creating client

Author: clue

README.md

@@ -99,12 +99,12 @@ $factory = new Factory($loop, $connector);
 
 #### createClient()
 
-The `createClient(string $amiUrl): PromiseInterface<Client>` method can be used to create a new [`Client`](#client).
-It helps with establishing a plain TCP/IP or secure SSL/TLS connection to the AMI
-and issuing an initial `login` action.
+The `createClient(string $url): PromiseInterface<Client>` method can be used to create a new [`Client`](#client).
+It helps with establishing a plain TCP/IP or secure TLS connection to the AMI
+and optionally issuing an initial `login` action.
 
 ```php
-$factory->createClient($amiUrl)->then(
+$factory->createClient($url)->then(
     function (Client $client) {
         // client connected (and authenticated)
     },
@@ -114,16 +114,18 @@ $factory->createClient($amiUrl)->then(
 );
 ```
 
-The `$amiUrl` contains the host and optional port to connect to:
+The method returns a [Promise](https://github.com/reactphp/promise) that will
+resolve with the [`Client`](#client) instance on success or will reject with an
+`Exception` if the URL is invalid or the connection or authentication fails.
+
+The `$url` parameter contains the host and optional port (which defaults to
+`5038` for plain TCP/IP connections) to connect to:
 
 ```php
-$factory->createClient('127.0.0.1:5038');
+$factory->createClient('localhost:5038');
 ```
 
-> If the `$amiUrl` is `null` (or omitted) this method defaults to connecting
-  to your local host (`127.0.0.1:5038`).
-
-The above examples to not pass any authentication details, so you may have to
+The above example does not pass any authentication details, so you may have to
 call `ActionSender::login()` after connecting or use the recommended shortcut
 to pass a username and secret for your AMI login details like this:
 
@@ -132,10 +134,11 @@ $factory->createClient('user:secret@localhost');
 ```
 
 The `Factory` defaults to establishing a plaintext TCP connection.
-If you want to connect through a secure TLS proxy, you can use the `tls` scheme:
+If you want to create a secure TLS connection, you can use the `tls` scheme
+(which defaults to port `5039`):
 
 ```php
-$factory->createClient('tls://user:secret@localhost:12345');
+$factory->createClient('tls://user:secret@localhost:5039');
 ```
 
 ### Client

composer.json

@@ -14,7 +14,7 @@
         "php": ">=5.3",
         "evenement/evenement": "^3.0 || ^2.0 || ^1.0",
         "react/event-loop": "^1.0 || ^0.5 || ^0.4 || ^0.3",
-        "react/promise": "^2.0 || ^1.0",
+        "react/promise": "^2.0 || ^1.1",
         "react/socket": "^1.0 || ^0.8.2"
     },
     "require-dev": {

src/Factory.php

@@ -7,6 +7,7 @@
 use React\Socket\Connector;
 use React\Socket\ConnectorInterface;
 use InvalidArgumentException;
+use React\Promise;
 
 class Factory
 {
@@ -23,15 +24,19 @@ public function __construct(LoopInterface $loop, ConnectorInterface $connector =
         $this->connector = $connector;
     }
 
-    public function createClient($address = null)
+    public function createClient($url)
     {
-        $parts = $this->parseUrl($address);
+        $parts = parse_url((strpos($url, '://') === false ? 'tcp://' : '') . $url);
+        if (!$parts || !isset($parts['scheme'], $parts['host'])) {
+            return Promise\reject(new InvalidArgumentException('Given URL "' . $url . '" can not be parsed'));
+        }
 
-        if (isset($parts['scheme']) && $parts['scheme'] !== 'tcp') {
-            $parts['host'] = 'tls://' . $parts['host'];
+        // use default port 5039 for `tls://` or 5038 otherwise
+        if (!isset($parts['port'])) {
+            $parts['port'] = $parts['scheme'] === 'tls' ? 5039 : 5038;
         }
 
-        $promise = $this->connector->connect($parts['host'] . ':' . $parts['port'])->then(function (ConnectionInterface $stream) {
+        $promise = $this->connector->connect($parts['scheme'] . '://' . $parts['host'] . ':' . $parts['port'])->then(function (ConnectionInterface $stream) {
             return new Client($stream);
         });
 
@@ -53,25 +58,4 @@ function ($error) use ($client) {
 
         return $promise;
     }
-
-    private function parseUrl($target)
-    {
-        if ($target === null) {
-            $target = 'tcp://127.0.0.1';
-        }
-        if (strpos($target, '://') === false) {
-            $target = 'tcp://' . $target;
-        }
-
-        $parts = parse_url($target);
-        if ($parts === false || !isset($parts['host'])) {
-            throw new InvalidArgumentException('Given URL can not be parsed');
-        }
-
-        if (!isset($parts['port'])) {
-            $parts['port'] = '5038';
-        }
-
-        return $parts;
-    }
 }

tests/FactoryTest.php

@@ -2,6 +2,7 @@
 
 use Clue\React\Ami\Factory;
 use React\Promise\Promise;
+
 class FactoryTest extends TestCase
 {
     private $loop;
@@ -21,20 +22,20 @@ public function testDefaultCtor()
         $this->factory = new Factory($this->loop);
     }
 
-    public function testCreateClientUsesTcpConnectorWithDefaultLocation()
+    public function testCreateClientUsesDefaultPortForTcpConnection()
     {
         $promise = new Promise(function () { });
-        $this->tcp->expects($this->once())->method('connect')->with('127.0.0.1:5038')->willReturn($promise);
+        $this->tcp->expects($this->once())->method('connect')->with('tcp://localhost:5038')->willReturn($promise);
 
-        $this->factory->createClient();
+        $this->factory->createClient('localhost');
     }
 
-    public function testCreateClientUsesDefaultPortForTcpConnection()
+    public function testCreateClientUsesTlsPortForTlsConnection()
     {
         $promise = new Promise(function () { });
-        $this->tcp->expects($this->once())->method('connect')->with('localhost:5038')->willReturn($promise);
+        $this->tcp->expects($this->once())->method('connect')->with('tls://localhost:5039')->willReturn($promise);
 
-        $this->factory->createClient('localhost');
+        $this->factory->createClient('tls://localhost');
     }
 
     public function testCreateClientUsesTlsConnectorWithTlsLocation()
@@ -44,4 +45,11 @@ public function testCreateClientUsesTlsConnectorWithTlsLocation()
 
         $this->factory->createClient('tls://ami.local:1234');
     }
+
+    public function testCreateClientWithInvalidUrlWillRejectPromise()
+    {
+        $promise = $this->factory->createClient('///');
+
+        $promise->then(null, $this->expectCallableOnce());
+    }
 }