Dont add path to an url we already added a path to. (php-http#141)

# Conflicts:
#	CHANGELOG.md
#	src/Plugin/AddPathPlugin.php

Author: Nyholm

spec/Plugin/AddPathPluginSpec.php

@@ -37,9 +37,9 @@ function it_adds_path(
         $host->getPath()->shouldBeCalled()->willReturn('/api');
 
         $request->getUri()->shouldBeCalled()->willReturn($uri);
-        $request->withUri($uri)->shouldBeCalled()->willReturn($request);
+        $request->withUri($uri)->shouldBeCalledTimes(1)->willReturn($request);
 
-        $uri->withPath('/api/users')->shouldBeCalled()->willReturn($uri);
+        $uri->withPath('/api/users')->shouldBeCalledTimes(1)->willReturn($uri);
         $uri->getPath()->shouldBeCalled()->willReturn('/users');
 
         $this->beConstructedWith($host);

src/Plugin/AddPathPlugin.php

@@ -18,16 +18,6 @@ final class AddPathPlugin implements Plugin
      */
     private $uri;
 
-    /**
-     * Stores identifiers of the already altered requests.
-     *
-     * @var array
-     */
-    private $alteredRequests = [];
-
-    /**
-     * @param UriInterface $uri
-     */
     public function __construct(UriInterface $uri)
     {
         if ('' === $uri->getPath()) {
@@ -42,17 +32,42 @@ public function __construct(UriInterface $uri)
     }
 
     /**
+     * Adds a prefix in the beginning of the URL's path.
+     *
+     * The prefix is not added if that prefix is already on the URL's path. This will fail on the edge
+     * case of the prefix being repeated, for example if `https://example.com/api/api/foo` is a valid
+     * URL on the server and the configured prefix is `/api`.
+     *
+     * We looked at other solutions, but they are all much more complicated, while still having edge
+     * cases:
+     * - Doing an spl_object_hash on `$first` will lead to collisions over time because over time the
+     *   hash can collide.
+     * - Have the PluginClient provide a magic header to identify the request chain and only apply
+     *   this plugin once.
+     *
+     * There are 2 reasons for the AddPathPlugin to be executed twice on the same request:
+     * - A plugin can restart the chain by calling `$first`, e.g. redirect
+     * - A plugin can call `$next` more than once, e.g. retry
+     *
+     * Depending on the scenario, the path should or should not be added. E.g. `$first` could
+     * be called after a redirect response from the server. The server likely already has the
+     * correct path.
+     *
+     * No solution fits all use cases. This implementation will work fine for the common use cases.
+     * If you have a specific situation where this is not the right thing, you can build a custom plugin
+     * that does exactly what you need.
+     *
      * {@inheritdoc}
      */
     public function handleRequest(RequestInterface $request, callable $next, callable $first)
     {
-        $identifier = spl_object_hash((object) $first);
+        $prepend = $this->uri->getPath();
+        $path = $request->getUri()->getPath();
 
-        if (!array_key_exists($identifier, $this->alteredRequests)) {
+        if (substr($path, 0, strlen($prepend)) !== $prepend) {
             $request = $request->withUri($request->getUri()
-                ->withPath($this->uri->getPath().$request->getUri()->getPath())
-            );
-            $this->alteredRequests[$identifier] = $identifier;
+                 ->withPath($prepend.$path)
+             );
         }
 
         return $next($request);

tests/Plugin/AddPathPluginTest.php

@@ -0,0 +1,85 @@
+<?php
+
+namespace tests\Http\Client\Common\Plugin;
+
+use Http\Client\Common\Plugin;
+use Http\Client\Common\Plugin\AddPathPlugin;
+use Http\Client\Common\PluginClient;
+use Http\Client\HttpClient;
+use Nyholm\Psr7\Request;
+use Nyholm\Psr7\Response;
+use Nyholm\Psr7\Uri;
+use PHPUnit\Framework\TestCase;
+use Psr\Http\Message\RequestInterface;
+
+class AddPathPluginTest extends TestCase
+{
+    /**
+     * @var Plugin
+     */
+    private $plugin;
+
+    /**
+     * @var callable empty
+     */
+    private $first;
+
+    protected function setUp()
+    {
+        $this->first = function () {};
+        $this->plugin = new AddPathPlugin(new Uri('/api'));
+    }
+
+    public function testRewriteSameUrl()
+    {
+        $verify = function (RequestInterface $request) {
+            $this->assertEquals('https://example.com/api/foo', $request->getUri()->__toString());
+        };
+
+        $request = new Request('GET', 'https://example.com/foo', ['Content-Type'=>'text/html']);
+        $this->plugin->handleRequest($request, $verify, $this->first);
+
+        // Make a second call with the same $request object
+        $this->plugin->handleRequest($request, $verify, $this->first);
+
+        // Make a new call with a new object but same URL
+        $request = new Request('GET', 'https://example.com/foo', ['Content-Type'=>'text/plain']);
+        $this->plugin->handleRequest($request, $verify, $this->first);
+    }
+
+    public function testRewriteCallingThePluginTwice()
+    {
+        $request = new Request('GET', 'https://example.com/foo');
+        $this->plugin->handleRequest($request, function (RequestInterface $request) {
+            $this->assertEquals('https://example.com/api/foo', $request->getUri()->__toString());
+
+            // Run the plugin again with the modified request
+            $this->plugin->handleRequest($request, function (RequestInterface $request) {
+                $this->assertEquals('https://example.com/api/foo', $request->getUri()->__toString());
+            }, $this->first);
+        }, $this->first);
+    }
+
+    public function testRewriteWithDifferentUrl()
+    {
+        $request = new Request('GET', 'https://example.com/foo');
+        $this->plugin->handleRequest($request, function (RequestInterface $request) {
+            $this->assertEquals('https://example.com/api/foo', $request->getUri()->__toString());
+        }, $this->first);
+
+        $request = new Request('GET', 'https://example.com/bar');
+        $this->plugin->handleRequest($request, function (RequestInterface $request) {
+            $this->assertEquals('https://example.com/api/bar', $request->getUri()->__toString());
+        }, $this->first);
+    }
+
+    public function testRewriteWhenPathIsIncluded()
+    {
+        $verify = function (RequestInterface $request) {
+            $this->assertEquals('https://example.com/api/foo', $request->getUri()->__toString());
+        };
+
+        $request = new Request('GET', 'https://example.com/api/foo');
+        $this->plugin->handleRequest($request, $verify, $this->first);
+    }
+}