mark classes as final and methods and properties as private

We add interfaces for final classes so that they can be mocked in testing. We chose to use the Interface suffix here to avoid name clashes, rather than renaming the existing PHP classes. The interfaces are only relevant for unit testing, we do not expect people to provide their own implementations. HttpClientPoolItem is purely internal and we therefore don't want an interface for it. It is marked with @Final and explained to not be meant to extend.
php-http · Dec 20, 2018 · fadb51a · fadb51a
1 parent 5541efd
commit fadb51a
Show file tree

Hide file tree

Showing 23 changed files with 326 additions and 306 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,10 @@
 - RetryPlugin will no longer retry requests when the response failed with a HTTP code < 500.
 - Abstract method `HttpClientPool::chooseHttpClient()` has now an explicit return type (`Http\Client\Common\HttpClientPoolItem`)
 - Interface method `Plugin::handleRequest(...)` has now an explicit return type (`Http\Promise\Promise`)
+- Made  classes final that are not intended to be extended.
+  Added interfaces for BatchClient, HttpClientRouter and HttpMethodsClient.
+  (These interfaces use the `Interface` suffix to avoid name collisions.)
+- Added an interface for HttpClientPool and moved the abstract class to the HttpClientPool sub namespace.
 
 ### Removed
 - Deprecated option `debug_plugins` has been removed from `PluginClient`

diff --git a/spec/HttpClientPoolItemSpec.php → ...HttpClientPool/HttpClientPoolItemSpec.php b/spec/HttpClientPoolItemSpec.php → ...HttpClientPool/HttpClientPoolItemSpec.php
@@ -1,6 +1,6 @@
 <?php
 
-namespace spec\Http\Client\Common;
+namespace spec\Http\Client\Common\HttpClientPool;
 
 use Http\Client\Exception;
 use Http\Client\Exception\TransferException;

diff --git a/spec/HttpClientPool/LeastUsedClientPoolSpec.php b/spec/HttpClientPool/LeastUsedClientPoolSpec.php
@@ -2,7 +2,7 @@
 
 namespace spec\Http\Client\Common\HttpClientPool;
 
-use Http\Client\Common\HttpClientPoolItem;
+use Http\Client\Common\HttpClientPool\HttpClientPoolItem;
 use Http\Client\HttpAsyncClient;
 use Http\Client\HttpClient;
 use Http\Promise\Promise;

diff --git a/spec/HttpClientPool/RandomClientPoolSpec.php b/spec/HttpClientPool/RandomClientPoolSpec.php
@@ -2,7 +2,7 @@
 
 namespace spec\Http\Client\Common\HttpClientPool;
 
-use Http\Client\Common\HttpClientPoolItem;
+use Http\Client\Common\HttpClientPool\HttpClientPoolItem;
 use Http\Client\HttpAsyncClient;
 use Http\Client\HttpClient;
 use Http\Promise\Promise;

diff --git a/spec/HttpClientPool/RoundRobinClientPoolSpec.php b/spec/HttpClientPool/RoundRobinClientPoolSpec.php
@@ -2,7 +2,7 @@
 
 namespace spec\Http\Client\Common\HttpClientPool;
 
-use Http\Client\Common\HttpClientPoolItem;
+use Http\Client\Common\HttpClientPool\HttpClientPoolItem;
 use Http\Client\HttpAsyncClient;
 use Http\Client\HttpClient;
 use Http\Promise\Promise;

diff --git a/spec/HttpClientRouterSpec.php b/spec/HttpClientRouterSpec.php
@@ -2,14 +2,15 @@
 
 namespace spec\Http\Client\Common;
 
+use Http\Client\Common\HttpClientRouter;
 use Http\Message\RequestMatcher;
 use Http\Client\HttpAsyncClient;
 use Http\Client\HttpClient;
 use Http\Promise\Promise;
 use Psr\Http\Message\RequestInterface;
 use Psr\Http\Message\ResponseInterface;
 use PhpSpec\ObjectBehavior;
-use Http\Client\Common\HttpClientRouter;
+use Http\Client\Common\HttpClientRouterInterface;
 use Http\Client\Exception\RequestException;
 
 class HttpClientRouterSpec extends ObjectBehavior
@@ -19,6 +20,11 @@ public function it_is_initializable()
         $this->shouldHaveType(HttpClientRouter::class);
     }
 
+    public function it_is_an_http_client_router()
+    {
+        $this->shouldImplement(HttpClientRouterInterface::class);
+    }
+
     public function it_is_an_http_client()
     {
         $this->shouldImplement(HttpClient::class);

diff --git a/spec/HttpMethodsClientSpec.php b/spec/HttpMethodsClientSpec.php
@@ -2,135 +2,88 @@
 
 namespace spec\Http\Client\Common;
 
-use GuzzleHttp\Psr7\Response;
 use Http\Client\Common\HttpMethodsClient;
 use Http\Client\HttpClient;
-use Http\Message\MessageFactory;
+use Http\Message\RequestFactory;
 use PhpSpec\ObjectBehavior;
 use Psr\Http\Message\RequestInterface;
 use Psr\Http\Message\ResponseInterface;
 
 class HttpMethodsClientSpec extends ObjectBehavior
 {
-    public function let(HttpClient $client, MessageFactory $messageFactory)
+    private static $requestData = [
+        'uri' => '/uri',
+        'headers' => [
+            'Content-Type' => 'text/plain',
+        ],
+        'body' => 'body',
+    ];
+
+    public function let(HttpClient $client, RequestFactory $requestFactory)
     {
         $this->beAnInstanceOf(
-            HttpMethodsClientStub::class, [
+            HttpMethodsClient::class, [
                 $client,
-                $messageFactory,
+                $requestFactory,
             ]
         );
     }
 
-    public function it_sends_a_get_request()
+    public function it_sends_a_get_request(HttpClient $client, RequestFactory $requestFactory, RequestInterface $request, ResponseInterface $response)
     {
-        $data = HttpMethodsClientStub::$requestData;
-
-        $this->get($data['uri'], $data['headers'])->shouldReturnAnInstanceOf(ResponseInterface::class);
+        $this->assert($client, $requestFactory, $request, $response, 'get');
     }
 
-    public function it_sends_a_head_request()
+    public function it_sends_a_head_request(HttpClient $client, RequestFactory $requestFactory, RequestInterface $request, ResponseInterface $response)
     {
-        $data = HttpMethodsClientStub::$requestData;
-
-        $this->head($data['uri'], $data['headers'])->shouldReturnAnInstanceOf(ResponseInterface::class);
+        $this->assert($client, $requestFactory, $request, $response, 'head');
     }
 
-    public function it_sends_a_trace_request()
+    public function it_sends_a_trace_request(HttpClient $client, RequestFactory $requestFactory, RequestInterface $request, ResponseInterface $response)
     {
-        $data = HttpMethodsClientStub::$requestData;
-
-        $this->trace($data['uri'], $data['headers'])->shouldReturnAnInstanceOf(ResponseInterface::class);
+        $this->assert($client, $requestFactory, $request, $response, 'trace');
     }
 
-    public function it_sends_a_post_request()
+    public function it_sends_a_post_request(HttpClient $client, RequestFactory $requestFactory, RequestInterface $request, ResponseInterface $response)
     {
-        $data = HttpMethodsClientStub::$requestData;
-
-        $this->post($data['uri'], $data['headers'], $data['body'])->shouldReturnAnInstanceOf(ResponseInterface::class);
+        $this->assert($client, $requestFactory, $request, $response, 'post', self::$requestData['body']);
     }
 
-    public function it_sends_a_put_request()
+    public function it_sends_a_put_request(HttpClient $client, RequestFactory $requestFactory, RequestInterface $request, ResponseInterface $response)
     {
-        $data = HttpMethodsClientStub::$requestData;
-
-        $this->put($data['uri'], $data['headers'], $data['body'])->shouldReturnAnInstanceOf(ResponseInterface::class);
+        $this->assert($client, $requestFactory, $request, $response, 'put', self::$requestData['body']);
     }
 
-    public function it_sends_a_patch_request()
+    public function it_sends_a_patch_request(HttpClient $client, RequestFactory $requestFactory, RequestInterface $request, ResponseInterface $response)
     {
-        $data = HttpMethodsClientStub::$requestData;
-
-        $this->patch($data['uri'], $data['headers'], $data['body'])->shouldReturnAnInstanceOf(ResponseInterface::class);
+        $this->assert($client, $requestFactory, $request, $response, 'patch', self::$requestData['body']);
     }
 
-    public function it_sends_a_delete_request()
+    public function it_sends_a_delete_request(HttpClient $client, RequestFactory $requestFactory, RequestInterface $request, ResponseInterface $response)
     {
-        $data = HttpMethodsClientStub::$requestData;
-
-        $this->delete($data['uri'], $data['headers'], $data['body'])->shouldReturnAnInstanceOf(ResponseInterface::class);
+        $this->assert($client, $requestFactory, $request, $response, 'delete', self::$requestData['body']);
     }
 
-    public function it_sends_a_options_request()
+    public function it_sends_an_options_request(HttpClient $client, RequestFactory $requestFactory, RequestInterface $request, ResponseInterface $response)
     {
-        $data = HttpMethodsClientStub::$requestData;
-
-        $this->options($data['uri'], $data['headers'], $data['body'])->shouldReturnAnInstanceOf(ResponseInterface::class);
+        $this->assert($client, $requestFactory, $request, $response, 'options', self::$requestData['body']);
     }
 
-    public function it_sends_request_with_underlying_client(HttpClient $client, MessageFactory $messageFactory, RequestInterface $request, ResponseInterface $response)
+    /**
+     * Run the actual test.
+     *
+     * As there is no data provider in phpspec, we keep separate methods to get new mocks for each test.
+     */
+    private function assert(HttpClient $client, RequestFactory $requestFactory, RequestInterface $request, ResponseInterface $response, string $method, string $body = null)
     {
         $client->sendRequest($request)->shouldBeCalled()->willReturn($response);
+        $this->mockFactory($requestFactory, $request, strtoupper($method), $body);
 
-        $this->beConstructedWith($client, $messageFactory);
-        $this->sendRequest($request)->shouldReturn($response);
+        $this->$method(self::$requestData['uri'], self::$requestData['headers'], self::$requestData['body'])->shouldReturnAnInstanceOf(ResponseInterface::class);
     }
-}
-
-class HttpMethodsClientStub extends HttpMethodsClient
-{
-    public static $requestData = [
-        'uri' => '/uri',
-        'headers' => [
-            'Content-Type' => 'text/plain',
-        ],
-        'body' => 'body',
-    ];
 
-    /**
-     * {@inheritdoc}
-     */
-    public function send($method, $uri, array $headers = [], $body = null): ResponseInterface
+    private function mockFactory(RequestFactory $requestFactory, RequestInterface $request, string $method, string $body = null)
     {
-        if ($uri !== self::$requestData['uri']) {
-            throw new \InvalidArgumentException('Invalid URI: '.$uri);
-        }
-
-        if ($headers !== self::$requestData['headers']) {
-            throw new \InvalidArgumentException('Invalid headers: '.print_r($headers, true));
-        }
-
-        switch ($method) {
-            case 'GET':
-            case 'HEAD':
-            case 'TRACE':
-                if (null !== $body) {
-                    throw new \InvalidArgumentException('Non-empty body');
-                }
-
-                return new Response();
-            case 'POST':
-            case 'PUT':
-            case 'PATCH':
-            case 'DELETE':
-            case 'OPTIONS':
-                if ($body !== self::$requestData['body']) {
-                    throw new \InvalidArgumentException('Invalid body: '.print_r($body, true));
-                }
-
-                return new Response();
-            default:
-                throw new \InvalidArgumentException('Invalid method: '.$method);
-        }
+        $requestFactory->createRequest($method, self::$requestData['uri'], self::$requestData['headers'], $body)->willReturn($request);
     }
 }
diff --git a/spec/Plugin/AddPathPluginSpec.php b/spec/Plugin/AddPathPluginSpec.php
@@ -46,7 +46,7 @@ public function it_adds_path(
         $this->handleRequest($request, PluginStub::next(), function () {});
     }
 
-    function it_removes_ending_slashes(
+    public function it_removes_ending_slashes(
         RequestInterface $request,
         UriInterface $host,
         UriInterface $host2,

diff --git a/src/BatchClient.php b/src/BatchClient.php
@@ -8,14 +8,7 @@
 use Psr\Http\Message\RequestInterface;
 use Psr\Http\Message\ResponseInterface;
 
-/**
- * BatchClient allow to sends multiple request and retrieve a Batch Result.
- *
- * This implementation simply loops over the requests and uses sendRequest with each of them.
- *
- * @author Joel Wurtz <jwurtz@jolicode.com>
- */
-class BatchClient implements HttpClient
+final class BatchClient implements BatchClientInterface
 {
     /**
      * @var HttpClient
@@ -27,28 +20,11 @@ public function __construct(HttpClient $client)
         $this->client = $client;
     }
 
-    /**
-     * {@inheritdoc}
-     */
     public function sendRequest(RequestInterface $request): ResponseInterface
     {
         return $this->client->sendRequest($request);
     }
 
-    /**
-     * Send several requests.
-     *
-     * You may not assume that the requests are executed in a particular order. If the order matters
-     * for your application, use sendRequest sequentially.
-     *
-     * @param RequestInterface[] The requests to send
-     *
-     * @return BatchResult Containing one result per request
-     *
-     * @throws BatchException If one or more requests fails. The exception gives access to the
-     *                        BatchResult with a map of request to result for success, request to
-     *                        exception for failures
-     */
     public function sendRequests(array $requests): BatchResult
     {
         $batchResult = new BatchResult();

diff --git a/src/BatchClientInterface.php b/src/BatchClientInterface.php
@@ -0,0 +1,34 @@
+<?php
+
+namespace Http\Client\Common;
+
+use Http\Client\Exception;
+use Http\Client\HttpClient;
+use Http\Client\Common\Exception\BatchException;
+use Psr\Http\Message\RequestInterface;
+
+/**
+ * BatchClient allow to sends multiple request and retrieve a Batch Result.
+ *
+ * This implementation simply loops over the requests and uses sendRequest with each of them.
+ *
+ * @author Joel Wurtz <jwurtz@jolicode.com>
+ */
+interface BatchClientInterface extends HttpClient
+{
+    /**
+     * Send several requests.
+     *
+     * You may not assume that the requests are executed in a particular order. If the order matters
+     * for your application, use sendRequest sequentially.
+     *
+     * @param RequestInterface[] The requests to send
+     *
+     * @return BatchResult Containing one result per request
+     *
+     * @throws BatchException If one or more requests fails. The exception gives access to the
+     *                        BatchResult with a map of request to result for success, request to
+     *                        exception for failures
+     */
+    public function sendRequests(array $requests): BatchResult;
+}
diff --git a/src/Deferred.php b/src/Deferred.php
@@ -9,7 +9,7 @@
 /**
  * A deferred allow to return a promise which has not been resolved yet.
  */
-class Deferred implements Promise
+final class Deferred implements Promise
 {
     private $value;
 

diff --git a/src/EmulatedHttpAsyncClient.php b/src/EmulatedHttpAsyncClient.php
@@ -6,13 +6,11 @@
 use Http\Client\HttpClient;
 
 /**
- * Emulates an async HTTP client.
- *
- * This should be replaced by an anonymous class in PHP 7.
+ * Emulates an async HTTP client with the help of a synchronous client.
  *
  * @author Márk Sági-Kazár <mark.sagikazar@gmail.com>
  */
-class EmulatedHttpAsyncClient implements HttpClient, HttpAsyncClient
+final class EmulatedHttpAsyncClient implements HttpClient, HttpAsyncClient
 {
     use HttpAsyncClientEmulator;
     use HttpClientDecorator;

diff --git a/src/EmulatedHttpClient.php b/src/EmulatedHttpClient.php
@@ -6,13 +6,11 @@
 use Http\Client\HttpClient;
 
 /**
- * Emulates an HTTP client.
- *
- * This should be replaced by an anonymous class in PHP 7.
+ * Emulates a synchronous HTTP client with the help of an asynchronous client.
  *
  * @author Márk Sági-Kazár <mark.sagikazar@gmail.com>
  */
-class EmulatedHttpClient implements HttpClient, HttpAsyncClient
+final class EmulatedHttpClient implements HttpClient, HttpAsyncClient
 {
     use HttpAsyncClientDecorator;
     use HttpClientEmulator;