Merge pull request #16 from clue-labs/legacy

Explicitly pass "legacy" protocol scheme to avoid probing protocol

README.md

@@ -50,6 +50,15 @@ $factory->createClient('localhost')->then(
 
 The `$uri` parameter must be a valid URI which must contain a host part and can optionally contain a port.
 
+This method defauls to probing the Quassel IRC core for the correct protocol to
+use (newer "datastream" protocol or original "legacy" protocol).
+If you have trouble with the newer "datastream" protocol, you can force using
+the old "legacy" protocol by prefixing the `legacy` scheme identifier like this:
+
+```php
+$factory->createClient('legacy://quassel.example.com:1234');
+```
+
 ### Client
 
 The `Client` is responsible for exchanging messages with your Quassel IRC core

src/Factory.php

@@ -10,6 +10,8 @@
 use React\Dns\Resolver\Factory as ResolverFactory;
 use React\SocketClient\Connector;
 use Clue\React\Quassel\Io\Protocol;
+use React\Promise;
+use InvalidArgumentException;
 
 class Factory
 {
@@ -31,23 +33,34 @@ public function __construct(LoopInterface $loop, ConnectorInterface $connector =
     public function createClient($address)
     {
         if (strpos($address, '://') === false) {
-            $address = 'dummy://' . $address;
+            $address = 'tcp://' . $address;
         }
         $parts = parse_url($address);
         if (!$parts || !isset($parts['host'])) {
-            return;
+            return Promise\reject(new InvalidArgumentException('Given argument "' . $address . '" is not a valid URI'));
         }
         if (!isset($parts['port'])) {
             $parts['port'] = 4242;
         }
 
-        $connector = $this->connector;
-        $prober = $this->prober;
+        // default to automatic probing protocol unless scheme is explicitly given
+        $probe = 0;
+        if (isset($parts['scheme'])) {
+            if ($parts['scheme'] === 'legacy') {
+                $probe = Protocol::TYPE_LEGACY;
+            } elseif ($parts['scheme'] !== 'tcp') {
+                return Promise\reject(new InvalidArgumentException('Given URI scheme "' . $parts['scheme'] . '" is invalid'));
+            }
+        }
+
+        $promise = $this->connector->create($parts['host'], $parts['port']);
 
-        return $connector->create($parts['host'], $parts['port'])->then(
-            function (Stream $stream) use ($prober, $connector, $parts) {
-                $probe = 0;
+        // protocol probe not already set
+        if ($probe === 0) {
+            $connector = $this->connector;
+            $prober = $this->prober;
 
+            $promise = $promise->then(function (Stream $stream) use ($prober, &$probe, $connector, $parts) {
                 return $prober->probe($stream)->then(
                     function ($ret) use (&$probe, $stream) {
                         // probe returned successfully, create new client for this stream
@@ -56,17 +69,21 @@ function ($ret) use (&$probe, $stream) {
                         return $stream;
                     },
                     function ($e) use ($connector, $parts) {
+                        // probing failed
                         if ($e->getCode() === Prober::ERROR_CLOSED) {
                             // legacy servers will terminate connection while probing
+                            // let's just open a new connection and assume default probe
                             return $connector->create($parts['host'], $parts['port']);
                         }
                         throw $e;
                     }
-                )->then(
-                    function (Stream $stream) use (&$probe) {
-                        return new Client($stream, Protocol::createFromProbe($probe));
-                    }
                 );
+            });
+        }
+
+        return $promise->then(
+            function (Stream $stream) use (&$probe) {
+                return new Client($stream, Protocol::createFromProbe($probe));
             }
         );
     }

tests/FactoryTest.php

@@ -2,6 +2,9 @@
 
 use Clue\React\Quassel\Factory;
 use React\Promise\Deferred;
+use Clue\React\Quassel\Io\Protocol;
+use React\Promise;
+
 class FactoryTest extends TestCase
 {
     public function setUp()
@@ -31,4 +34,38 @@ public function testPassHostnameAndPortToConnector()
         $this->connector->expects($this->once())->method('create')->with($this->equalTo('example.com', 1234))->will($this->returnValue($deferred->promise()));
         $this->factory->createClient('example.com:1234');
     }
+
+    public function testInvalidUriWillRejectWithoutConnecting()
+    {
+        $this->connector->expects($this->never())->method('create');
+
+        $this->expectPromiseReject($this->factory->createClient('///'));
+    }
+
+    public function testInvalidSchemeWillRejectWithoutConnecting()
+    {
+        $this->connector->expects($this->never())->method('create');
+
+        $this->expectPromiseReject($this->factory->createClient('https://example.com:1234/'));
+    }
+
+    public function testWillInvokeProberAfterConnecting()
+    {
+        $stream = $this->getMockBuilder('React\Stream\Stream')->disableOriginalConstructor()->getMock();
+
+        $this->connector->expects($this->once())->method('create')->will($this->returnValue(Promise\resolve($stream)));
+        $this->prober->expects($this->once())->method('probe')->with($this->equalTo($stream))->will($this->returnValue(Promise\resolve(Protocol::TYPE_DATASTREAM)));
+
+        $this->expectPromiseResolve($this->factory->createClient('localhost'));
+    }
+
+    public function testWillNotInvokeProberIfSchemeIsProtocol()
+    {
+        $stream = $this->getMockBuilder('React\Stream\Stream')->disableOriginalConstructor()->getMock();
+
+        $this->connector->expects($this->once())->method('create')->will($this->returnValue(Promise\resolve($stream)));
+        $this->prober->expects($this->never())->method('probe');
+
+        $this->expectPromiseResolve($this->factory->createClient('legacy://localhost'));
+    }
 }