Merge pull request #23 from PrestaShop/extract-client

Extracted the Client from SimpleCircuitBreaker
PrestaShop · Jan 10, 2019 · 921d252 · 921d252
2 parents ee5ebca + a0137dd
commit 921d252
Show file tree

Hide file tree

Showing 11 changed files with 249 additions and 40 deletions.
diff --git a/.github/main.workflow b/.github/main.workflow
@@ -0,0 +1,27 @@
+workflow "Code Quality" {
+  on = "push"
+  resolves = [
+    "PHPStan",
+    "PHP-CS-Fixer",
+    "Psalm"
+  ]
+}
+
+action "PHPStan" {
+  uses = "docker://oskarstark/phpstan-ga:with-extensions"
+  args = "analyse src tests --level max --configuration extension.neon"
+  secrets = ["GITHUB_TOKEN"]
+}
+
+action "PHP-CS-Fixer" {
+  uses = "docker://oskarstark/php-cs-fixer-ga"
+  secrets = ["GITHUB_TOKEN"]
+  args = "--config=.php_cs.dist --diff --dry-run"
+}
+
+action "Psalm" {
+  needs="PHPStan"
+  uses = "docker://mickaelandrieu/psalm-ga"
+  secrets = ["GITHUB_TOKEN"]
+  args = "--find-dead-code --diff --diff-methods"
+}
diff --git a/.travis.yml b/.travis.yml
@@ -9,8 +9,8 @@ notifications:
 
 php:
   - 7.2
-  - 5.6
   - 7.3
+  - 5.6
 
 matrix:
   fast_finish: true
@@ -29,13 +29,7 @@ script:
 
 jobs:
   include:
-    - stage: PHP CS Fixer
-      script:
-        - ./vendor/bin/php-cs-fixer fix --config=.php_cs.dist -v --dry-run --using-cache=no --path-mode=intersection `git diff --name-only --diff-filter=ACMRTUXB $TRAVIS_COMMIT_RANGE`
-    - stage: PHPStan Code quality
-      script:
-        - composer require --dev "phpstan/phpstan:^0.10" --ignore-platform-reqs && composer phpstan
-    - stage: PHPQA Full analysis
+    - stage: PHPQA Code Quality
       script:
         - docker run --rm -u $UID -v $PWD:/app eko3alpha/docker-phpqa --report --ignoredDirs vendor
 after_success:

diff --git a/README.md b/README.md
@@ -2,13 +2,11 @@
 
 [![codecov](https://codecov.io/gh/PrestaShop/circuit-breaker/branch/master/graph/badge.svg)](https://codecov.io/gh/PrestaShop/circuit-breaker) [![PHPStan](https://img.shields.io/badge/PHPStan-Level%207-brightgreen.svg?style=flat&logo=php)](https://shields.io/#/)
 
-> Experimental, don't use it yet!
-
 ## Main principles
 
 ![circuit breaker](https://user-images.githubusercontent.com/1247388/49721725-438bd700-fc63-11e8-8498-82ca681b15fb.png)
 
-This library is compatible with PHP 5.6+ and rely on Guzzle 6.
+This library is compatible with PHP 5.6+.
 
 ## Installation
 
@@ -27,6 +25,8 @@ the circuit breaker:
 * the **timeout**: define how much time we wait before consider the service unreachable;
 * the **threshold**: define how much time we wait before trying to access again the service;
 
+We also need to define which (HTTP) client will be used to call the service.
+
 The **fallback** callback will be used if the distant service is unreachable when the Circuit Breaker is Open (means "is used"). 
 
 > You'd better return the same type of response expected from your distant call.
@@ -40,6 +40,7 @@ $circuitBreaker = $circuitBreakerFactory->create(
         'closed' => [2, 0.1, 0],
         'open' => [0, 0, 10],
         'half_open' => [1, 0.2, 0],
+        'client' => ['proxy' => '192.168.16.1:10'],
     ]
 );
 
@@ -50,6 +51,9 @@ $fallbackResponse = function () {
 $circuitBreaker->call('https://api.domain.com', $fallbackResponse);
 ```
 
+> For the Guzzle implementation, the Client options are described
+> in the [HttpGuzzle documentation](http://docs.guzzlephp.org/en/stable/index.html).
+
 ## Tests
 
 ```
@@ -60,4 +64,20 @@ composer test
 
 ```
 composer cs-fix && composer phpstan
-```
+```
+
+We also use [PHPQA](https://github.com/EdgedesignCZ/phpqa#phpqa) to check the Code quality
+during the CI management of the contributions.
+
+If you want to use it (using Docker):
+
+```
+docker run --rm -u $UID -v $(pwd):/app eko3alpha/docker-phpqa --report --ignoredDirs vendor
+```
+
+If you want to use it (using Composer):
+
+```
+composer global require edgedesign/phpqa=v1.20.0 --update-no-dev
+phpqa --report --ignoreDirs vendor
+```
diff --git a/composer.json b/composer.json
@@ -19,8 +19,9 @@
     },
     "require-dev": {
         "friendsofphp/php-cs-fixer": "^2.12",
+        "phpunit/phpunit": "^5.7.0",
         "symfony/cache": "^3.4.0",
-        "phpunit/phpunit": "^5.7.0"
+        "vimeo/psalm": "^1.1"
     },
     "suggest": {
         "symfony/cache": "Allows use of Symfony Cache adapters to store transactions",
@@ -38,11 +39,13 @@
     },
     "scripts": {
         "phpstan": "@php ./vendor/bin/phpstan analyse src tests -l 7 -c extension.neon",
+        "psalm": "@php ./vendor/bin/psalm --find-dead-code --threads=8 --diff --diff-methods",
         "cs-fix": "@php ./vendor/bin/php-cs-fixer fix",
         "test": "@php ./vendor/bin/phpunit"
     },
     "scripts-descriptions": {
         "phpstan": "Execute PHPStan on PHP7.0+, you need to install it",
+        "psalm": "Execute Psalm on PHP7.0+, you need to install it",
         "cs-fix": "Check and fix coding styles using PHP CS Fixer",
         "test": "Launch PHPUnit test suite"
     },

diff --git a/psalm.xml.dist b/psalm.xml.dist
@@ -0,0 +1,55 @@
+<?xml version="1.0"?>
+<psalm
+    name="Psalm configuration for the PrestaShop Circuit Breaker"
+    totallyTyped="false"
+    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xmlns="https://getpsalm.org/schema/config"
+    xsi:schemaLocation="https://getpsalm.org/schema/config file:///home/dev/Projects/circuit-breaker/vendor/vimeo/psalm/config.xsd"
+>
+    <projectFiles>
+        <directory name="src" />
+        <ignoreFiles>
+            <directory name="vendor" />
+            <directory name="tests" />
+        </ignoreFiles>
+    </projectFiles>
+
+    <issueHandlers>
+        <LessSpecificReturnType errorLevel="info" />
+
+        <!-- level 3 issues - slightly lazy code writing, but provably low false-negatives -->
+
+        <DeprecatedMethod errorLevel="info" />
+        <DeprecatedProperty errorLevel="info" />
+        <DeprecatedClass errorLevel="info" />
+        <DeprecatedConstant errorLevel="info" />
+        <DeprecatedInterface errorLevel="info" />
+        <DeprecatedTrait errorLevel="info" />
+
+        <InternalMethod errorLevel="info" />
+        <InternalProperty errorLevel="info" />
+        <InternalClass errorLevel="info" />
+
+        <MissingClosureReturnType errorLevel="info" />
+        <MissingReturnType errorLevel="info" />
+        <MissingPropertyType errorLevel="info" />
+        <InvalidDocblock errorLevel="info" />
+        <MisplacedRequiredParam errorLevel="info" />
+
+        <PropertyNotSetInConstructor errorLevel="info" />
+        <MissingConstructor errorLevel="info" />
+        <MissingClosureParamType errorLevel="info" />
+        <MissingParamType errorLevel="info" />
+
+        <RedundantCondition errorLevel="info" />
+
+        <DocblockTypeContradiction errorLevel="info" />
+        <RedundantConditionGivenDocblockType errorLevel="info" />
+
+        <UnresolvableInclude errorLevel="info" />
+
+        <RawObjectIteration errorLevel="info" />
+
+        <InvalidStringClass errorLevel="info" />
+    </issueHandlers>
+</psalm>
diff --git a/src/Clients/GuzzleClient.php b/src/Clients/GuzzleClient.php
@@ -13,16 +13,26 @@
  */
 class GuzzleClient implements Client
 {
+    /**
+     * @var array the Client main options
+     */
+    private $mainOptions;
+
+    public function __construct(array $mainOptions = [])
+    {
+        $this->mainOptions = $mainOptions;
+    }
+
     /**
      * {@inheritdoc}
      */
     public function request($resource, array $options)
     {
         try {
-            $client = new OriginalGuzzleClient();
+            $client = new OriginalGuzzleClient($this->mainOptions);
             $method = isset($options['method']) ? $options['method'] : 'GET';
 
-            return $client->request($method, $resource, $options)->getBody();
+            return (string) $client->request($method, $resource, $options)->getBody();
         } catch (Exception $exception) {
             throw new UnavailableService($exception->getMessage());
         }

diff --git a/src/SimpleCircuitBreaker.php b/src/SimpleCircuitBreaker.php
@@ -3,8 +3,8 @@
 namespace PrestaShop\CircuitBreaker;
 
 use DateTime;
-use PrestaShop\CircuitBreaker\Clients\GuzzleClient;
 use PrestaShop\CircuitBreaker\Contracts\CircuitBreaker;
+use PrestaShop\CircuitBreaker\Contracts\Client;
 use PrestaShop\CircuitBreaker\Contracts\Place;
 use PrestaShop\CircuitBreaker\Contracts\Storage;
 use PrestaShop\CircuitBreaker\Contracts\Transaction;
@@ -17,6 +17,11 @@
  */
 final class SimpleCircuitBreaker implements CircuitBreaker
 {
+    /**
+     * @var Client the client in charge of calling the service
+     */
+    private $client;
+
     /**
      * @var Place the current Circuit Breaker place
      */
@@ -38,7 +43,8 @@ final class SimpleCircuitBreaker implements CircuitBreaker
     public function __construct(
         Place $openPlace,
         Place $halfOpenPlace,
-        Place $closedPlace
+        Place $closedPlace,
+        Client $client
     ) {
         $this->currentPlace = $closedPlace;
         $this->places = [
@@ -47,6 +53,7 @@ public function __construct(
             States::OPEN_STATE => $openPlace,
         ];
 
+        $this->client = $client;
         $this->storage = new SimpleArray();
     }
 
@@ -70,22 +77,22 @@ public function call($service, callable $fallback)
         try {
             if ($this->isOpened()) {
                 if ($this->canAccessService($transaction)) {
-                    $this->moveStateTo(States::HALF_OPEN_STATE, $transaction, $service);
+                    $this->moveStateTo(States::HALF_OPEN_STATE, $service);
                 }
 
                 return \call_user_func($fallback);
             }
 
             $response = $this->tryExecute($service);
-            $this->moveStateTo(States::CLOSED_STATE, $transaction, $service);
+            $this->moveStateTo(States::CLOSED_STATE, $service);
 
             return $response;
         } catch (UnavailableService $exception) {
             $transaction->incrementFailures();
             $this->storage->saveTransaction($service, $transaction);
 
             if (!$this->isAllowedToRetry($transaction)) {
-                $this->moveStateTo(States::OPEN_STATE, $transaction, $service);
+                $this->moveStateTo(States::OPEN_STATE, $service);
 
                 return \call_user_func($fallback);
             }
@@ -120,12 +127,11 @@ public function isClosed()
 
     /**
      * @param string $state the Place state
-     * @param Transaction $transaction the Circuit Breaker transaction
      * @param string $service the service URI
      *
      * @return bool
      */
-    private function moveStateTo($state, Transaction $transaction, $service)
+    private function moveStateTo($state, $service)
     {
         $this->currentPlace = $this->places[$state];
         $transaction = SimpleTransaction::createFromPlace(
@@ -186,9 +192,7 @@ private function canAccessService(Transaction $transaction)
      */
     private function tryExecute($service)
     {
-        $client = new GuzzleClient();
-
-        return $client->request(
+        return $this->client->request(
             $service,
             [
                 'method' => 'GET',

diff --git a/src/SimpleCircuitBreakerFactory.php b/src/SimpleCircuitBreakerFactory.php
@@ -6,6 +6,7 @@
 use PrestaShop\CircuitBreaker\Places\ClosedPlace;
 use PrestaShop\CircuitBreaker\Places\HalfOpenPlace;
 use PrestaShop\CircuitBreaker\Places\OpenPlace;
+use PrestaShop\CircuitBreaker\Clients\GuzzleClient;
 
 /**
  * Main implementation of Circuit Breaker Factory
@@ -22,10 +23,14 @@ public function create(array $settings)
         $halfOpenPlace = HalfOpenPlace::fromArray($settings['half_open']);
         $closedPlace = ClosedPlace::fromArray($settings['closed']);
 
+        $clientSettings = array_key_exists('client', $settings) ? $settings['client'] : [];
+        $client = new GuzzleClient($clientSettings);
+
         return new SimpleCircuitBreaker(
             $openPlace,
             $halfOpenPlace,
-            $closedPlace
+            $closedPlace,
+            $client
         );
     }
 }
diff --git a/src/Transactions/SimpleTransaction.php b/src/Transactions/SimpleTransaction.php
@@ -115,6 +115,8 @@ public static function createFromPlace(Place $place, $service)
      * Set the right DateTime from the threshold value.
      *
      * @param int $threshold the Transaction threshold
+     *
+     * @return void
      */
     private function initThresholdDateTime($threshold)
     {