Profiling might not work with CachePlugin

Collector/ProfilePlugin.php

@@ -65,27 +65,105 @@ public function handleRequest(RequestInterface $request, callable $next, callabl
 
         // wrap the next callback to profile the plugin request changes
         $wrappedNext = function (RequestInterface $request) use ($next, $profile) {
-            $profile->setRequest($this->formatter->formatRequest($request));
+            $this->onOutgoingRequest($request, $profile);
 
             return $next($request);
         };
 
         // wrap the first callback to profile the plugin request changes
         $wrappedFirst = function (RequestInterface $request) use ($first, $profile) {
-            $profile->setRequest($this->formatter->formatRequest($request));
+            $this->onOutgoingRequest($request, $profile);
 
             return $first($request);
         };
 
-        return $this->plugin->handleRequest($request, $wrappedNext, $wrappedFirst)->then(function (ResponseInterface $response) use ($profile) {
-            $profile->setResponse($this->formatter->formatResponse($response));
+        try {
+            $promise = $this->plugin->handleRequest($request, $wrappedNext, $wrappedFirst);
+        } catch (Exception $e) {
+            $this->onException($request, $profile, $e, $stack);
+
+            throw $e;
+        }
+
+        return $promise->then(function (ResponseInterface $response) use ($profile, $request, $stack) {
+            $this->onOutgoingResponse($response, $profile, $request, $stack);
 
             return $response;
-        }, function (Exception $exception) use ($profile) {
-            $profile->setFailed(true);
-            $profile->setResponse($this->formatter->formatException($exception));
+        }, function (Exception $exception) use ($profile, $request, $stack) {
+            $this->onException($request, $profile, $exception, $stack);
 
             throw $exception;
         });
     }
+
+    /**
+     * @param RequestInterface $request
+     * @param Profile          $profile
+     * @param Exception        $exception
+     * @param Stack            $stack
+     */
+    private function onException(
+        RequestInterface $request,
+        Profile $profile,
+        Exception $exception,
+        Stack $stack = null
+    ) {
+        $profile->setFailed(true);
+        $profile->setResponse($this->formatter->formatException($exception));
+        $this->collectRequestInformation($request, $stack);
+    }
+
+    /**
+     * @param RequestInterface $request
+     * @param Profile          $profile
+     */
+    private function onOutgoingRequest(RequestInterface $request, Profile $profile)
+    {
+        $profile->setRequest($this->formatter->formatRequest($request));
+    }
+
+    /**
+     * @param ResponseInterface $response
+     * @param Profile           $profile
+     * @param RequestInterface  $request
+     * @param Stack             $stack
+     */
+    private function onOutgoingResponse(ResponseInterface $response, Profile $profile, RequestInterface $request, Stack $stack = null)
+    {
+        $profile->setResponse($this->formatter->formatResponse($response));
+        $this->collectRequestInformation($request, $stack);
+    }
+
+    /**
+     * Collect request information when not already done by the HTTP client. This happens when using the CachePlugin
+     * and the cache is hit without re-validation.
+     *
+     * @param RequestInterface $request
+     * @param Stack|null       $stack
+     */
+    private function collectRequestInformation(RequestInterface $request, Stack $stack = null)
+    {
+        if (null === $stack) {
+            return;
+        }
+
+        if (empty($stack->getRequestTarget())) {
+            $stack->setRequestTarget($request->getRequestTarget());
+        }
+        if (empty($stack->getRequestMethod())) {
+            $stack->setRequestMethod($request->getMethod());
+        }
+        if (empty($stack->getRequestScheme())) {
+            $stack->setRequestScheme($request->getUri()->getScheme());
+        }
+        if (empty($stack->getRequestHost())) {
+            $stack->setRequestHost($request->getUri()->getHost());
+        }
+        if (empty($stack->getClientRequest())) {
+            $stack->setClientRequest($this->formatter->formatRequest($request));
+        }
+        if (empty($stack->getCurlCommand())) {
+            $stack->setCurlCommand($this->formatter->formatAsCurlCommand($request));
+        }
+    }
 }

Tests/Functional/ProfilingTest.php

@@ -0,0 +1,130 @@
+<?php
+
+namespace Http\HttplugBundle\Tests\Functional;
+
+use GuzzleHttp\Psr7\Request;
+use Http\Client\Common\Plugin;
+use Http\Client\Common\PluginClient;
+use Http\Discovery\StreamFactoryDiscovery;
+use Http\Discovery\UriFactoryDiscovery;
+use Http\HttplugBundle\Collector\Collector;
+use Http\HttplugBundle\Collector\Formatter;
+use Http\HttplugBundle\Collector\ProfileClient;
+use Http\HttplugBundle\Collector\ProfilePlugin;
+use Http\HttplugBundle\Collector\StackPlugin;
+use Http\Message\Formatter\CurlCommandFormatter;
+use Http\Message\Formatter\FullHttpMessageFormatter;
+use Http\Mock\Client;
+use Psr\Http\Message\RequestInterface;
+use Symfony\Component\Cache\Adapter\ArrayAdapter;
+use Symfony\Component\Stopwatch\Stopwatch;
+
+class ProfilingTest extends \PHPUnit_Framework_TestCase
+{
+    /**
+     * @var Collector
+     */
+    private $collector;
+
+    /**
+     * @var Formatter
+     */
+    private $formatter;
+
+    /**
+     * @var Stopwatch
+     */
+    private $stopwatch;
+
+    public function setUp()
+    {
+        $this->collector = new Collector([]);
+        $this->formatter = new Formatter(new FullHttpMessageFormatter(), new CurlCommandFormatter());
+        $this->stopwatch = new Stopwatch();
+    }
+
+    public function testProfilingWithCachePlugin()
+    {
+        $client = $this->createClient([
+            new Plugin\CachePlugin(new ArrayAdapter(), StreamFactoryDiscovery::find(), [
+                'respect_response_cache_directives' => [],
+                'default_ttl' => 86400,
+            ]),
+        ]);
+
+        $client->sendRequest(new Request('GET', 'https://example.com'));
+        $client->sendRequest(new Request('GET', 'https://example.com'));
+
+        $this->assertCount(2, $this->collector->getStacks());
+        $stack = $this->collector->getStacks()[1];
+        $this->assertEquals('GET', $stack->getRequestMethod());
+        $this->assertEquals('https', $stack->getRequestScheme());
+        $this->assertEquals('/', $stack->getRequestTarget());
+        $this->assertEquals('example.com', $stack->getRequestHost());
+    }
+
+    public function testProfilingWhenPluginThrowException()
+    {
+        $client = $this->createClient([
+            new ExceptionThrowerPlugin(),
+        ]);
+
+        $this->setExpectedException(\Exception::class);
+
+        try {
+            $client->sendRequest(new Request('GET', 'https://example.com'));
+        } finally {
+            $this->assertCount(1, $this->collector->getStacks());
+            $stack = $this->collector->getStacks()[0];
+            $this->assertEquals('GET', $stack->getRequestMethod());
+            $this->assertEquals('https', $stack->getRequestScheme());
+            $this->assertEquals('/', $stack->getRequestTarget());
+            $this->assertEquals('example.com', $stack->getRequestHost());
+        }
+    }
+
+    public function testProfiling()
+    {
+        $client = $this->createClient([
+            new Plugin\AddHostPlugin(UriFactoryDiscovery::find()->createUri('https://example.com')),
+            new Plugin\RedirectPlugin(),
+            new Plugin\RetryPlugin(),
+        ]);
+
+        $client->sendRequest(new Request('GET', '/'));
+
+        $this->assertCount(1, $this->collector->getStacks());
+        $stack = $this->collector->getStacks()[0];
+        $this->assertCount(3, $stack->getProfiles());
+        $this->assertEquals('GET', $stack->getRequestMethod());
+        $this->assertEquals('https', $stack->getRequestScheme());
+        $this->assertEquals('/', $stack->getRequestTarget());
+        $this->assertEquals('example.com', $stack->getRequestHost());
+    }
+
+    private function createClient(array $plugins, $clientName = 'Acme', array $clientOptions = [])
+    {
+        $plugins = array_map(function (Plugin $plugin) {
+            return new ProfilePlugin($plugin, $this->collector, $this->formatter, get_class($plugin));
+        }, $plugins);
+
+        array_unshift($plugins, new StackPlugin($this->collector, $this->formatter, $clientName));
+
+        $client = new Client();
+        $client = new ProfileClient($client, $this->collector, $this->formatter, $this->stopwatch);
+        $client = new PluginClient($client, $plugins, $clientOptions);
+
+        return $client;
+    }
+}
+
+class ExceptionThrowerPlugin implements Plugin
+{
+    /**
+     * {@inheritdoc}
+     */
+    public function handleRequest(RequestInterface $request, callable $next, callable $first)
+    {
+        throw new \Exception();
+    }
+}

Tests/Unit/Collector/ProfilePluginTest.php

@@ -11,6 +11,8 @@
 use Http\HttplugBundle\Collector\ProfilePlugin;
 use Http\HttplugBundle\Collector\Stack;
 use Http\Promise\FulfilledPromise;
+use Http\Promise\Promise;
+use Http\Promise\RejectedPromise;
 use Psr\Http\Message\RequestInterface;
 use Psr\Http\Message\ResponseInterface;
 
@@ -36,6 +38,11 @@ class ProfilePluginTest extends \PHPUnit_Framework_TestCase
      */
     private $response;
 
+    /**
+     * @var Promise
+     */
+    private $fulfilledPromise;
+
     /**
      * @var Stack
      */
@@ -46,6 +53,11 @@ class ProfilePluginTest extends \PHPUnit_Framework_TestCase
      */
     private $exception;
 
+    /**
+     * @var Promise
+     */
+    private $rejectedPromise;
+
     /**
      * @var Formatter
      */
@@ -62,8 +74,10 @@ public function setUp()
         $this->collector = $this->getMockBuilder(Collector::class)->disableOriginalConstructor()->getMock();
         $this->request = new Request('GET', '/');
         $this->response = new Response();
+        $this->fulfilledPromise = new FulfilledPromise($this->response);
         $this->currentStack = new Stack('default', 'FormattedRequest');
         $this->exception = new TransferException();
+        $this->rejectedPromise = new RejectedPromise($this->exception);
         $this->formatter = $this->getMockBuilder(Formatter::class)->disableOriginalConstructor()->getMock();
 
         $this->collector
@@ -74,9 +88,7 @@ public function setUp()
         $this->plugin
             ->method('handleRequest')
             ->willReturnCallback(function ($request, $next, $first) {
-                $next($request);
-
-                return new FulfilledPromise($this->response);
+                return $next($request);
             })
         ;
 
@@ -115,13 +127,15 @@ public function testCallDecoratedPlugin()
         ;
 
         $this->subject->handleRequest($this->request, function () {
+            return $this->fulfilledPromise;
         }, function () {
         });
     }
 
     public function testProfileIsInitialized()
     {
         $this->subject->handleRequest($this->request, function () {
+            return $this->fulfilledPromise;
         }, function () {
         });
 
@@ -133,6 +147,7 @@ public function testProfileIsInitialized()
     public function testCollectRequestInformations()
     {
         $this->subject->handleRequest($this->request, function () {
+            return $this->fulfilledPromise;
         }, function () {
         });
 
@@ -143,6 +158,7 @@ public function testCollectRequestInformations()
     public function testOnFulfilled()
     {
         $promise = $this->subject->handleRequest($this->request, function () {
+            return $this->fulfilledPromise;
         }, function () {
         });
 
@@ -156,7 +172,7 @@ public function testOnRejected()
         $this->setExpectedException(TransferException::class);
 
         $promise = $this->subject->handleRequest($this->request, function () {
-            throw new TransferException();
+            return $this->rejectedPromise;
         }, function () {
         });
 

composer.json

@@ -37,11 +37,13 @@
         "php-http/guzzle6-adapter": "^1.1.1",
         "php-http/react-adapter": "^0.2.1",
         "php-http/buzz-adapter": "^0.3",
+        "php-http/mock-client": "^1.0",
         "symfony/phpunit-bridge": "^3.2",
         "symfony/twig-bundle": "^2.8 || ^3.0",
         "symfony/twig-bridge": "^2.8 || ^3.0",
         "symfony/web-profiler-bundle": "^2.8 || ^3.0",
         "symfony/finder": "^2.7 || ^3.0",
+        "symfony/cache": "^3.1",
         "polishsymfonycommunity/symfony-mocker-container": "^1.0",
         "matthiasnoback/symfony-dependency-injection-test": "^1.0"
     },