Merge pull request #418 from clue-labs/browser-signature

Update `Browser` signature to take `$connector` as first argument
reactphp · Jul 28, 2021 · e1c5e4f · e1c5e4f
2 parents 77f56dc + 12a4946
commit e1c5e4f
Show file tree

Hide file tree

Showing 8 changed files with 172 additions and 31 deletions.
diff --git a/README.md b/README.md
@@ -259,7 +259,6 @@ like this:
 
 ```php
 $browser = new React\Http\Browser(
-    null,
     new React\Socket\Connector(
         null,
         array(
@@ -610,7 +609,7 @@ $connector = new React\Socket\Connector(null, array(
     'dns' => false
 ));
 
-$browser = new React\Http\Browser(null, $connector);
+$browser = new React\Http\Browser($connector);
 ```
 
 See also the [HTTP CONNECT proxy example](examples/11-client-http-connect-proxy.php).
@@ -637,7 +636,7 @@ $connector = new React\Socket\Connector(null, array(
     'dns' => false
 ));
 
-$browser = new React\Http\Browser(null, $connector);
+$browser = new React\Http\Browser($connector);
 ```
 
 See also the [SOCKS proxy example](examples/12-client-socks-proxy.php).
@@ -666,7 +665,7 @@ $connector = new React\Socket\Connector(null, array(
     'dns' => false
 ));
 
-$browser = new React\Http\Browser(null, $connector);
+$browser = new React\Http\Browser($connector);
 ```
 
 See also the [SSH proxy example](examples/13-client-ssh-proxy.php).
@@ -687,7 +686,7 @@ $connector = new React\Socket\FixedUriConnector(
     new React\Socket\UnixConnector()
 );
 
-$browser = new Browser(null, $connector);
+$browser = new React\Http\Browser($connector);
 
 $client->get('http://localhost/info')->then(function (Psr\Http\Message\ResponseInterface $response) {
     var_dump($response->getHeaders(), (string)$response->getBody());
@@ -1875,11 +1874,15 @@ and keeps track of pending incoming HTTP responses.
 $browser = new React\Http\Browser();
 ```
 
-This class takes an optional `LoopInterface|null $loop` parameter that can be used to
-pass the event loop instance to use for this object. You can use a `null` value
-here in order to use the [default loop](https://github.com/reactphp/event-loop#loop).
-This value SHOULD NOT be given unless you're sure you want to explicitly use a
-given event loop instance.
+This class takes two optional arguments for more advanced usage:
+
+```php
+// constructor signature as of v1.5.0
+$browser = new React\Http\Browser(?ConnectorInterface $connector = null, ?LoopInterface $loop = null);
+
+// legacy constructor signature before v1.5.0
+$browser = new React\Http\Browser(?LoopInterface $loop = null, ?ConnectorInterface $connector = null);
+```
 
 If you need custom connector settings (DNS resolution, TLS parameters, timeouts,
 proxy servers etc.), you can explicitly pass a custom instance of the
@@ -1897,9 +1900,15 @@ $connector = new React\Socket\Connector(null, array(
     )
 ));
 
-$browser = new React\Http\Browser(null, $connector);
+$browser = new React\Http\Browser($connector);
 ```
 
+This class takes an optional `LoopInterface|null $loop` parameter that can be used to
+pass the event loop instance to use for this object. You can use a `null` value
+here in order to use the [default loop](https://github.com/reactphp/event-loop#loop).
+This value SHOULD NOT be given unless you're sure you want to explicitly use a
+given event loop instance.
+
 > Note that the browser class is final and shouldn't be extended, it is likely to be marked final in a future release.
 
 #### get()

diff --git a/examples/11-client-http-connect-proxy.php b/examples/11-client-http-connect-proxy.php
@@ -21,7 +21,8 @@
     'tcp' => $proxy,
     'dns' => false
 ));
-$browser = new Browser(null, $connector);
+
+$browser = new Browser($connector);
 
 // demo fetching HTTP headers (or bail out otherwise)
 $browser->get('https://www.google.com/')->then(function (ResponseInterface $response) {

diff --git a/examples/12-client-socks-proxy.php b/examples/12-client-socks-proxy.php
@@ -18,7 +18,8 @@
     'tcp' => $proxy,
     'dns' => false
 ));
-$browser = new Browser(null, $connector);
+
+$browser = new Browser($connector);
 
 // demo fetching HTTP headers (or bail out otherwise)
 $browser->get('https://www.google.com/')->then(function (ResponseInterface $response) {

diff --git a/examples/13-client-ssh-proxy.php b/examples/13-client-ssh-proxy.php
@@ -17,7 +17,8 @@
     'tcp' => $proxy,
     'dns' => false
 ));
-$browser = new Browser(null, $connector);
+
+$browser = new Browser($connector);
 
 // demo fetching HTTP headers (or bail out otherwise)
 $browser->get('https://www.google.com/')->then(function (ResponseInterface $response) {

diff --git a/examples/14-client-unix-domain-sockets.php b/examples/14-client-unix-domain-sockets.php
@@ -14,7 +14,7 @@
     new UnixConnector()
 );
 
-$browser = new Browser(null, $connector);
+$browser = new Browser($connector);
 
 // demo fetching HTTP headers (or bail out otherwise)
 $browser->get('http://localhost/info')->then(function (ResponseInterface $response) {

diff --git a/src/Browser.php b/src/Browser.php
@@ -32,11 +32,15 @@ class Browser
      * $browser = new React\Http\Browser();
      * ```
      *
-     * This class takes an optional `LoopInterface|null $loop` parameter that can be used to
-     * pass the event loop instance to use for this object. You can use a `null` value
-     * here in order to use the [default loop](https://github.com/reactphp/event-loop#loop).
-     * This value SHOULD NOT be given unless you're sure you want to explicitly use a
-     * given event loop instance.
+     * This class takes two optional arguments for more advanced usage:
+     *
+     * ```php
+     * // constructor signature as of v1.5.0
+     * $browser = new React\Http\Browser(?ConnectorInterface $connector = null, ?LoopInterface $loop = null);
+     *
+     * // legacy constructor signature before v1.5.0
+     * $browser = new React\Http\Browser(?LoopInterface $loop = null, ?ConnectorInterface $connector = null);
+     * ```
      *
      * If you need custom connector settings (DNS resolution, TLS parameters, timeouts,
      * proxy servers etc.), you can explicitly pass a custom instance of the
@@ -54,15 +58,32 @@ class Browser
      *     )
      * ));
      *
-     * $browser = new React\Http\Browser(null, $connector);
+     * $browser = new React\Http\Browser($connector);
      * ```
      *
-     * @param ?LoopInterface $loop
-     * @param ?ConnectorInterface $connector [optional] Connector to use.
-     *     Should be `null` in order to use default Connector.
+     * This class takes an optional `LoopInterface|null $loop` parameter that can be used to
+     * pass the event loop instance to use for this object. You can use a `null` value
+     * here in order to use the [default loop](https://github.com/reactphp/event-loop#loop).
+     * This value SHOULD NOT be given unless you're sure you want to explicitly use a
+     * given event loop instance.
+     *
+     * @param null|ConnectorInterface|LoopInterface $connector
+     * @param null|LoopInterface|ConnectorInterface $loop
+     * @throws \InvalidArgumentException for invalid arguments
      */
-    public function __construct(LoopInterface $loop = null, ConnectorInterface $connector = null)
+    public function __construct($connector = null, $loop = null)
     {
+        // swap arguments for legacy constructor signature
+        if (($connector instanceof LoopInterface || $connector === null) && ($loop instanceof ConnectorInterface || $loop === null)) {
+            $swap = $loop;
+            $loop = $connector;
+            $connector = $swap;
+        }
+
+        if (($connector !== null && !$connector instanceof ConnectorInterface) || ($loop !== null && !$loop instanceof LoopInterface)) {
+            throw new \InvalidArgumentException('Expected "?ConnectorInterface $connector" and "?LoopInterface $loop" arguments');
+        }
+
         $loop = $loop ?: Loop::get();
         $this->transaction = new Transaction(
             Sender::createFromLoop($loop, $connector),

diff --git a/tests/BrowserTest.php b/tests/BrowserTest.php
@@ -3,8 +3,8 @@
 namespace React\Tests\Http;
 
 use Clue\React\Block;
-use React\Http\Browser;
 use Psr\Http\Message\RequestInterface;
+use React\Http\Browser;
 use React\Promise\Promise;
 use RingCentral\Psr7\Uri;
 
@@ -21,7 +21,7 @@ public function setUpBrowser()
     {
         $this->loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
         $this->sender = $this->getMockBuilder('React\Http\Io\Transaction')->disableOriginalConstructor()->getMock();
-        $this->browser = new Browser($this->loop);
+        $this->browser = new Browser(null, $this->loop);
 
         $ref = new \ReflectionProperty($this->browser, 'transaction');
         $ref->setAccessible(true);
@@ -43,6 +43,114 @@ public function testConstructWithoutLoopAssignsLoopAutomatically()
         $this->assertInstanceOf('React\EventLoop\LoopInterface', $loop);
     }
 
+    public function testConstructWithConnectorAssignsGivenConnector()
+    {
+        $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
+
+        $browser = new Browser($connector);
+
+        $ref = new \ReflectionProperty($browser, 'transaction');
+        $ref->setAccessible(true);
+        $transaction = $ref->getValue($browser);
+
+        $ref = new \ReflectionProperty($transaction, 'sender');
+        $ref->setAccessible(true);
+        $sender = $ref->getValue($transaction);
+
+        $ref = new \ReflectionProperty($sender, 'http');
+        $ref->setAccessible(true);
+        $client = $ref->getValue($sender);
+
+        $ref = new \ReflectionProperty($client, 'connector');
+        $ref->setAccessible(true);
+        $ret = $ref->getValue($client);
+
+        $this->assertSame($connector, $ret);
+    }
+
+    public function testConstructWithConnectorWithLegacySignatureAssignsGivenConnector()
+    {
+        $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
+
+        $browser = new Browser(null, $connector);
+
+        $ref = new \ReflectionProperty($browser, 'transaction');
+        $ref->setAccessible(true);
+        $transaction = $ref->getValue($browser);
+
+        $ref = new \ReflectionProperty($transaction, 'sender');
+        $ref->setAccessible(true);
+        $sender = $ref->getValue($transaction);
+
+        $ref = new \ReflectionProperty($sender, 'http');
+        $ref->setAccessible(true);
+        $client = $ref->getValue($sender);
+
+        $ref = new \ReflectionProperty($client, 'connector');
+        $ref->setAccessible(true);
+        $ret = $ref->getValue($client);
+
+        $this->assertSame($connector, $ret);
+    }
+
+    public function testConstructWithLoopAssignsGivenLoop()
+    {
+        $browser = new Browser(null, $this->loop);
+
+        $ref = new \ReflectionProperty($browser, 'transaction');
+        $ref->setAccessible(true);
+        $transaction = $ref->getValue($browser);
+
+        $ref = new \ReflectionProperty($transaction, 'loop');
+        $ref->setAccessible(true);
+        $loop = $ref->getValue($transaction);
+
+        $this->assertSame($this->loop, $loop);
+    }
+
+    public function testConstructWithLoopWithLegacySignatureAssignsGivenLoop()
+    {
+        $browser = new Browser($this->loop);
+
+        $ref = new \ReflectionProperty($browser, 'transaction');
+        $ref->setAccessible(true);
+        $transaction = $ref->getValue($browser);
+
+        $ref = new \ReflectionProperty($transaction, 'loop');
+        $ref->setAccessible(true);
+        $loop = $ref->getValue($transaction);
+
+        $this->assertSame($this->loop, $loop);
+    }
+
+    public function testConstructWithInvalidConnectorThrows()
+    {
+        $this->setExpectedException('InvalidArgumentException');
+        new Browser('foo');
+    }
+
+    public function testConstructWithInvalidLoopThrows()
+    {
+        $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
+
+        $this->setExpectedException('InvalidArgumentException');
+        new Browser($connector, 'foo');
+    }
+
+    public function testConstructWithConnectorTwiceThrows()
+    {
+        $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
+
+        $this->setExpectedException('InvalidArgumentException');
+        new Browser($connector, $connector);
+    }
+
+    public function testConstructWithLoopTwiceThrows()
+    {
+        $this->setExpectedException('InvalidArgumentException');
+        new Browser($this->loop, $this->loop);
+    }
+
     public function testGetSendsGetRequest()
     {
         $that = $this;
@@ -390,7 +498,7 @@ public function testCancelGetRequestShouldCancelUnderlyingSocketConnection()
         $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
         $connector->expects($this->once())->method('connect')->with('example.com:80')->willReturn($pending);
 
-        $this->browser = new Browser($this->loop, $connector);
+        $this->browser = new Browser($connector, $this->loop);
 
         $promise = $this->browser->get('http://example.com/');
         $promise->cancel();

diff --git a/tests/FunctionalBrowserTest.php b/tests/FunctionalBrowserTest.php
@@ -30,7 +30,7 @@ class FunctionalBrowserTest extends TestCase
     public function setUpBrowserAndServer()
     {
         $this->loop = $loop = Factory::create();
-        $this->browser = new Browser($this->loop);
+        $this->browser = new Browser(null, $this->loop);
 
         $http = new HttpServer($this->loop, new StreamingRequestMiddleware(), function (ServerRequestInterface $request) use ($loop) {
             $path = $request->getUri()->getPath();
@@ -398,7 +398,7 @@ public function testVerifyPeerEnabledForBadSslRejects()
             )
         ));
 
-        $browser = new Browser($this->loop, $connector);
+        $browser = new Browser($connector, $this->loop);
 
         $this->setExpectedException('RuntimeException');
         Block\await($browser->get('https://self-signed.badssl.com/'), $this->loop);
@@ -420,7 +420,7 @@ public function testVerifyPeerDisabledForBadSslResolves()
             )
         ));
 
-        $browser = new Browser($this->loop, $connector);
+        $browser = new Browser($connector, $this->loop);
 
         Block\await($browser->get('https://self-signed.badssl.com/'), $this->loop);
     }