fix(handler_sharing): only share the curl handler instead of the whol…

…e handler stack (#80)
BedrockStreaming · Sep 7, 2022 · a766713 · a766713
1 parent 66785cf
commit a766713
Show file tree

Hide file tree

Showing 7 changed files with 54 additions and 42 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -9,7 +9,7 @@ jobs:
       - uses: actions/checkout@master
       - uses: shivammathur/setup-php@v2
         with:
-          php-version: 7.4
+          php-version: 8.1
       - run: composer install --prefer-dist --no-interaction
       - run: bin/php-cs-fixer fix --ansi --dry-run --using-cache=no --verbose
 

diff --git a/README.md b/README.md
@@ -2,19 +2,19 @@
 
 [![Build Status](https://github.com/BedrockStreaming/GuzzleHttpBundle/actions/workflows/ci.yml/badge.svg)](https://github.com/BedrockStreaming/GuzzleHttpBundle/actions/workflows/ci.yml) [![Latest Stable Version](http://poser.pugx.org/m6web/guzzle-http-bundle/v)](https://packagist.org/packages/m6web/guzzle-http-bundle) [![Total Downloads](http://poser.pugx.org/m6web/guzzle-http-bundle/downloads)](https://packagist.org/packages/m6web/guzzle-http-bundle) [![License](http://poser.pugx.org/m6web/guzzle-http-bundle/license)](https://packagist.org/packages/m6web/guzzle-http-bundle) [![PHP Version Require](http://poser.pugx.org/m6web/guzzle-http-bundle/require/php)](https://packagist.org/packages/m6web/guzzle-http-bundle)
 
-The GuzzleHttpBundle provide a guzzle client as symfony service.
+The GuzzleHttpBundle provide Guzzle clients as Symfony services.
 
 ## Installation
 
-Require the bundle with Composer :
+Require the bundle with Composer:
 
 ```bash
 $ composer require m6web/guzzle-http-bundle 
 ```
 
 > For older Symfony versions, you can try to install an older version of this bundle.
 
-If you don't use Symfony Flex, register the bundle in your kernel :
+If you don't use Symfony Flex, register the bundle in your kernel:
 
 ```php
 
@@ -40,7 +40,7 @@ m6web_guzzlehttp:
 All subkey under clients defines an instance of guzzle http client. These services are named `m6web_guzzlehttp_`+subkey expect for the
  `default` subkey that define the main service `m6web_guzzlehttp`.
 
-Then you can ask container for your client :
+Then you can ask container for your client:
 
 ```php
 // in a controller
@@ -84,7 +84,9 @@ When a cache system is available, you can use `cache_force` and `cache_ttl` in a
 
 ## DataCollector
 
-Datacollector is available when the symfony profiler is enabled. The collector allows you to see the following data :
+A data collector is available when the Symfony profiler is enabled. 
+
+It allows you to inspect the following data:
 
  - Method
  - Url
@@ -93,9 +95,9 @@ Datacollector is available when the symfony profiler is enabled. The collector a
  - Redirect count
  - Redirect time
  - Cache hit
- - Cache ttl
+ - Cache TTL
 
-**NOTE :** If you choose guzzle for `redirect_handler`, The redirect count and redirect time will always set to zero.
+**NOTE:** If you choose Guzzle for `redirect_handler`, The redirect count and redirect time will always be zero.
 Cache information are available when a cache system is set.
 
 ## Cache system
@@ -113,7 +115,7 @@ m6web_guzzlehttp:
                 service: my_cache_service
 ```
 
-We provide an "In memory" cache class that you can use by defining a new cache service and use it in Guzzle configuration :
+We provide an "In memory" cache class that you can use by defining a new cache service and use it in Guzzle configuration:
 
 ```yaml
 # app/config/config.yml
@@ -154,17 +156,17 @@ m6_redis:
 
 ```
 
-For more information on how to setup the RedisBundle, refer to the README in the project.
+For more information on how to set up the RedisBundle, refer to the README in the project.
 
 We provide also the same cache system for APCU through the [ApcuBundle](https://github.com/M6Web/ApcuBundle).
 
 ## Configuration reference
 
-As some configuration options accept multiples data types, all services references must sart with a `@` character.
+As some configuration options accept multiples data types, all services references must start with a `@` character.
 
 ```yaml
 m6web_guzzlehttp:
-    clients_share_the_same_handler: false          # Use "true" if you want all your clients to share the same Guzzle handler. It means that both your asynchronous requests context AND your middlewares will be shared between clients.
+    clients_share_the_same_handler: false          # Use "true" if you want all your clients to share the same Guzzle handler. It means that your different clients will be able to send asynchronous requests altogether.
     clients:
         default:
             base_uri: ""                           # Base uri to prepend on request uri
@@ -175,7 +177,7 @@ m6web_guzzlehttp:
                 cache_server_errors: true          # at false, no server errors will be cached
                 cache_client_errors: true          # at false, no client errors will be cached
                 default_ttl: 3600                  # default ttl for cache entry in seconds
-                ignore_cache_errors: false         # at true, no Exception would be throw when cache is unavailable
+                ignore_cache_errors: false         # if true, no exception would be thrown when cache is unavailable
                 use_header_ttl: false              # use the cache-control header to set the ttl
                 service: '@my_cache_service'       # reference to service who implements the cache interface
             headers:                               # optional. Default request headers
@@ -255,14 +257,13 @@ Acme\Infra\GraphQL\Client\MyMiddleware:
 
 ## Contributing
 
-First of all, thank you for contributing !
+First, thank you for contributing!
 
-Here are few rules to follow for a easier code review before the maintainers accept and merge your request.
+Here are few rules to follow for an easier code review before the maintainers accept and merge your pull request:
 
-- you MUST follow the Symfony2 coding standard : you can use `make cs-fix` to fix the files
-- you MUST run the test
 - you MUST write or update tests
 - you MUST write or update documentation
+- the CI must pass on your pull request
 
 ## Running the test
 

diff --git a/composer.json b/composer.json
@@ -8,7 +8,7 @@
     {
       "name": "M6Web",
       "email" : "[email protected]",
-      "homepage": "http://tech.m6web.fr/"
+      "homepage": "https://tech.bedrockstreaming.com/"
     }
   ],
   "require": {

diff --git a/src/DependencyInjection/M6WebGuzzleHttpExtension.php b/src/DependencyInjection/M6WebGuzzleHttpExtension.php
@@ -30,7 +30,8 @@ public function load(array $configs, ContainerBuilder $container)
         $isDebugEnabled = $container->getParameter('kernel.debug');
 
         if ($useSameHandler = $config['clients_share_the_same_handler']) {
-            $this->addGuzzleProxyHandlerAndHandlerStack($container, null, $config, $isDebugEnabled);
+            // this Handler will be used by all clients
+            $this->setGuzzleProxyHandler($container, null, $config, $isDebugEnabled);
         }
 
         foreach ($config['clients'] as $clientId => $clientConfig) {
@@ -60,13 +61,23 @@ protected function loadClient(ContainerBuilder $container, string $clientId, arr
         if ($config['allow_redirects']['max'] == 0) {
             $config['allow_redirects'] = false;
         }
+
         if ($useSameHandler) {
-            $container->setAlias($this->buildGuzzleProxyHandlerServiceName($clientId), $this->buildGuzzleProxyHandlerServiceName(null))->setPublic(true);
-            $container->setAlias($this->buildGuzzleHandlerStackServiceName($clientId), $this->buildGuzzleHandlerStackServiceName(null))->setPublic(true);
+            $container
+                ->setAlias($this->buildGuzzleProxyHandlerServiceName($clientId), $this->buildGuzzleProxyHandlerServiceName(null))
+                ->setPublic(true);
         } else {
-            $this->addGuzzleProxyHandlerAndHandlerStack($container, $clientId, $config, $isDebugEnabled);
+            $this->setGuzzleProxyHandler($container, $clientId, $config, $isDebugEnabled);
         }
 
+        // a different HandlerStack is built for each client because each client has its own middlewares,
+        // but all HandlerStacks may use the same underlying Handler depending on the value of $useSameHandler
+        $handlerStackDefinition = new Definition('%m6web_guzzlehttp.guzzle.handlerstack.class%');
+        $handlerStackDefinition->setPublic(true);
+        $handlerStackDefinition->setFactory(['%m6web_guzzlehttp.guzzle.handlerstack.class%', 'create']);
+        $handlerStackDefinition->setArguments([new Reference($this->buildGuzzleProxyHandlerServiceName($clientId))]);
+        $container->setDefinition($this->buildGuzzleHandlerStackServiceName($clientId), $handlerStackDefinition);
+
         $handlerStackReference = new Reference($this->buildGuzzleHandlerStackServiceName($clientId));
 
         $middlewareEventDispatcherDefinition = new Definition('%m6web_guzzlehttp.middleware.eventdispatcher.class%');
@@ -129,18 +140,6 @@ protected function loadClient(ContainerBuilder $container, string $clientId, arr
         $container->setDefinition($containerKey, $guzzleClientDefinition);
     }
 
-    private function addGuzzleProxyHandlerAndHandlerStack(ContainerBuilder $container, ?string $clientId, array $config, bool $isDebugEnabled): void
-    {
-        $this->setGuzzleProxyHandler($container, $clientId, $config, $isDebugEnabled);
-        $handlerStackDefinition = new Definition('%m6web_guzzlehttp.guzzle.handlerstack.class%');
-
-        $handlerStackDefinition->setPublic(true);
-        $handlerStackDefinition->setFactory(['%m6web_guzzlehttp.guzzle.handlerstack.class%', 'create']);
-        $handlerStackDefinition->setArguments([new Reference($this->buildGuzzleProxyHandlerServiceName($clientId))]);
-
-        $container->setDefinition($this->buildGuzzleHandlerStackServiceName($clientId), $handlerStackDefinition);
-    }
-
     /**
      * Set proxy handler definition for the client
      */
@@ -282,15 +281,15 @@ private function buildGuzzleProxyHandlerServiceName(?string $clientId): string
     {
         return sprintf(
             'm6web_guzzlehttp.guzzle.proxyhandler%s',
-            !empty($clientId) ? '_'.$clientId : ''
+            null !== $clientId ? '_'.$clientId : ''
         );
     }
 
     private function buildGuzzleHandlerStackServiceName(?string $clientId): string
     {
         return sprintf(
             'm6web_guzzlehttp.guzzle.handlerstack%s',
-            !empty($clientId) ? '.'.$clientId : ''
+            null !== $clientId ? '.'.$clientId : ''
         );
     }
 }
diff --git a/tests/Units/DependencyInjection/M6WebGuzzleHttpExtension.php b/tests/Units/DependencyInjection/M6WebGuzzleHttpExtension.php
@@ -54,7 +54,7 @@ public function testDefaultConfig()
             ->integer($curlOpt[CURLOPT_MAXREDIRS])
                 ->isEqualTo(5)
             ->integer($curlOpt[CURLOPT_REDIR_PROTOCOLS])
-                ->isEqualTo((CURLPROTO_HTTP | CURLPROTO_HTTPS))
+                ->isEqualTo(CURLPROTO_HTTP | CURLPROTO_HTTPS)
             ->boolean($curlOpt[CURLOPT_AUTOREFERER])
                 ->isTrue()
         ;
@@ -165,6 +165,8 @@ public function testMultiClientSameHandlerConfig()
                 ->isFalse()
             ->boolean($container->has('m6web_guzzlehttp.guzzle.handlerstack.default'))
                 ->isTrue()
+            ->boolean($container->has('m6web_guzzlehttp.guzzle.proxyhandler_default'))
+                ->isTrue()
             ->boolean($container->has('m6web_guzzlehttp_myclient'))
                 ->isTrue()
             ->array($arguments = $container->getDefinition('m6web_guzzlehttp_myclient')->getArgument(0))
@@ -194,8 +196,12 @@ public function testMultiClientSameHandlerConfig()
                 ->hasKey('User-Agent')
             ->boolean($container->has('m6web_guzzlehttp.guzzle.handlerstack.myclient'))
                 ->isTrue()
+            ->boolean($container->has('m6web_guzzlehttp.guzzle.proxyhandler_myclient'))
+                ->isTrue()
             ->object($container->get('m6web_guzzlehttp.guzzle.handlerstack.myclient'))
-                ->isIdenticalTo($container->get('m6web_guzzlehttp.guzzle.handlerstack.default'))
+                ->isNotIdenticalTo($container->get('m6web_guzzlehttp.guzzle.handlerstack.default'))
+            ->object($container->get('m6web_guzzlehttp.guzzle.proxyhandler_myclient'))
+                ->isIdenticalTo($container->get('m6web_guzzlehttp.guzzle.proxyhandler_default'))
         ;
     }
 
@@ -223,6 +229,8 @@ public function testMultiClientConfig()
                 ->isFalse()
             ->boolean($container->has('m6web_guzzlehttp.guzzle.handlerstack.default'))
                 ->isTrue()
+            ->boolean($container->has('m6web_guzzlehttp.guzzle.proxyhandler_default'))
+                ->isTrue()
             ->boolean($container->has('m6web_guzzlehttp_myclient'))
                 ->isTrue()
             ->array($arguments = $container->getDefinition('m6web_guzzlehttp_myclient')->getArgument(0))
@@ -252,8 +260,12 @@ public function testMultiClientConfig()
                 ->hasKey('User-Agent')
             ->boolean($container->has('m6web_guzzlehttp.guzzle.handlerstack.myclient'))
                 ->isTrue()
+            ->boolean($container->has('m6web_guzzlehttp.guzzle.proxyhandler_myclient'))
+                ->isTrue()
             ->object($container->get('m6web_guzzlehttp.guzzle.handlerstack.myclient'))
                 ->isNotIdenticalTo($container->get('m6web_guzzlehttp.guzzle.handlerstack.default'))
+            ->object($container->get('m6web_guzzlehttp.guzzle.proxyhandler_myclient'))
+                ->isNotIdenticalTo($container->get('m6web_guzzlehttp.guzzle.proxyhandler_default'))
         ;
     }
 

diff --git a/tests/Units/Handler/CurlMultiHandler.php b/tests/Units/Handler/CurlMultiHandler.php
@@ -108,7 +108,7 @@ public function testCacheSetWithHeader()
                     ->twice()
                         ->withAtLeastArguments(['1' => $this->getSerializedResponse($response1), '2' => 500])
                         ->withAtLeastArguments(['1' => $this->getSerializedResponse($response2), '2' => 500])
-            ;
+        ;
 
         // A header in the Vary should be in the cache even if it's an X-
         $cacheMock->getMockController()->resetCalls();
@@ -127,7 +127,7 @@ public function testCacheSetWithHeader()
                     ->twice()
                 ->call('set')
                     ->twice()
-            ;
+        ;
     }
 
     public function testCacheGet()
@@ -384,7 +384,7 @@ public function testGetKey()
             ->then
                 ->string($testedClass->getPublicKey($request))
                 ->isEqualTo('GET-https://httpbin.org/get-012c059df30be6f6c77e1b8447d7a15c')
-            ;
+        ;
 
         $this->if(
             $request = new \mock\GuzzleHttp\Psr7\Request(

diff --git a/tests/Units/Middleware/EventDispatcherMiddleware.php b/tests/Units/Middleware/EventDispatcherMiddleware.php
@@ -103,6 +103,6 @@ function () use ($errorCallable) {
                     ->call('dispatch')
                         ->withArguments($eventSend, GuzzleHttpErrorEvent::EVENT_ERROR_NAME)
                             ->once()
-            ;
+        ;
     }
 }
-Original file line number
+Diff line change
@@ Expand Up / @@ -103,6 +103,6 @@ function () use ($errorCallable) { @@
                         ->call('dispatch')
                             ->withArguments($eventSend, GuzzleHttpErrorEvent::EVENT_ERROR_NAME)
                                 ->once()
-                ;
+            ;
         }
     }