Migrate Guzzle to Symfony Http Client #45

boherm · 2023-11-06T10:54:46Z

Questions	Answers
Description?	Migrate Guzzle to Symfony Http Client. We can use Guzzle Client if we really want, to knew more about this, you can read the `README.md` file. Tests are updated too (for php versions: 7.2, 7.3, 7.4, 8.0, 8.1, 8.2): - Coding standard - Commons tests for the library -Guzzle Client implementation tests - Symfony Http Client (all versions: ^5.4\|^6) implementation tests
Type?	refactor
BC breaks?	yes
Deprecations?	no
Fixed ticket?	Fixes PrestaShop/PrestaShop#32855
Sponsor company	PrestaShop SA
How to test?	You can read the Readme to tests this new implementation and also the Guzzle implementation too.

boherm · 2023-11-06T11:13:40Z

edit: PR merged! So this one are open to reviews!

CI is failing because of cs-fixer.
Before merge this PR, we need #46.

matks

One thing that puzzles me

src/Client/SymfonyHttpClient.php

matks

Looks good to me, how do we test?

M0rgan01 · 2023-11-07T13:28:44Z

composer.json

@@ -14,7 +14,8 @@
        }
    ],
    "require": {
-        "guzzlehttp/guzzle": "^7.3"
+        "php": ">=7.2.5",
+        "symfony/http-client": "^5.4"


will the transition to v 6 require modifications? I ask this because we are soon going to switch to symfony 6 in the core

In fact, if we move to Symfony Http Client v6, we need to drop PHP 7.2 version for circuit-breaker too.
I think that could be done in another PR in the future.

Yes I didn't think I would do it on the same PR, but it's potentially something to do in the future

Maybe we can allow multiple versions? 🤔

Suggested change

"symfony/http-client": "^5.4"

"symfony/http-client": "^5|^6"

Does the contract change that much on the http client itself?

@jolelievre, not that much, your suggestion can work properly, I will change that today 😉

M0rgan01 · 2023-11-07T13:34:53Z

src/Client/SymfonyHttpClient.php

+    {
+        if (isset($options['method'])) {
+            if (!in_array($options['method'], self::SUPPORTED_METHODS)) {
+                throw UnsupportedMethodException::unsupportedMethod($options['method']);


Having a test that checks for the error could be a good thing

You're right, and maybe for the GuzzleClient too, what do you think? 🤔

Yes, also 👍

boherm · 2023-11-07T14:43:51Z

@matks Hum, isn't easy to test this.
I think that the best way to test is to follow this steps:

Clone my fork and checkout the #32855-symfony-http-client branch.
Install composer link (to easily link the library to your cloned version).
From your PrestaShop folder: composer link ../path/to/cloned-repository
Then look at the circuit-breaker readme to instantiate a new factory and client with some configuration.
Make sure that by default the client is SymfonyHttpClient and not Guzzle.
Play with url that exist or not and see if the fallback callback work!
Try the same with GuzzleClient.

(At the end, don't forget to make a composer unlink ../path/to/cloned-repository.)

jolelievre

Thanks @boherm

Just a few comments, and also you need to modify the GuzzleClient among other things we still have a use statement here

circuit-breaker/src/Client/GuzzleClient.php

Line 32 in ded5440

use GuzzleHttp\Client as OriginalGuzzleClient;

on a class that is potentially not present

I had something in mind, although I'm not sure it's the good way to do this, to remove the use statement and instead the GuzzleClient must act as factory and:

use the FQCN of the guzzle original client via a hard coded string value \GuzzleHttp\Client
use class_exists to check if the class is available, throw an exception if it's not
if it exists you can do something like $className = '\GuzzleHttp\Client'; $client = new $className($options)

I think if we want the code to be completely uncoupled from the guzzle dependency it's a good way to remove all dependencies in the code, what do you think?

jolelievre · 2023-11-07T18:40:32Z

composer.json

@@ -14,7 +14,8 @@
        }
    ],
    "require": {
-        "guzzlehttp/guzzle": "^7.3"
+        "php": ">=7.2.5",
+        "symfony/http-client": "^5.4"


Maybe we can allow multiple versions? 🤔

Suggested change

"symfony/http-client": "^5.4"

"symfony/http-client": "^5|^6"

jolelievre · 2023-11-07T18:40:48Z

composer.json

@@ -14,7 +14,8 @@
        }
    ],
    "require": {
-        "guzzlehttp/guzzle": "^7.3"
+        "php": ">=7.2.5",
+        "symfony/http-client": "^5.4"


Does the contract change that much on the http client itself?

jolelievre · 2023-11-07T18:46:33Z

src/Client/SymfonyHttpClient.php

+            // Symfony Http Client not support "method" passed in options array.
+            unset($options['method']);
+            // For mock purpose, we can pass a handler in options array in a Guzzle way.
+            if (isset($options['handler']) && $options['handler'] instanceof MockHttpClient) {


This way of mocking the client/handler was a bit nasty But mostly it's really based on an option that's purely and only meaningful for Guzzle we shouldn't handle a similar handler option if it's not handled by the symfony client

There's probably a smarter and cleaner way to handle tests and mock for a symfony client, one that is not inspired from guzzle but more adapted to Symfony client, maybe even a client option that overrides the OriginalSymfonyHttpClient::create would be a bit better

jolelievre · 2023-11-07T18:53:39Z

tests/AdvancedCircuitBreakerFactoryTest.php

@@ -64,7 +64,7 @@ public function testCircuitBreakerWithDispatcher(): void
            ->getMock()
        ;

-        $localeService = 'file://' . __FILE__;
+        $localeService = 'https://prestashop-projects.org/';


Why changing the local accessible URL to an external one? The tests were independent from a tier service so far, what's the advantage of changing that, if prestashop-projects.org is don the tests ould fail without a valid reason

After some tests on my side, we can only access to an external url with Symfony Http Client 😅

jolelievre · 2023-11-07T18:57:50Z

tests/AdvancedCircuitBreakerTest.php

@@ -108,7 +105,7 @@ public function testSimpleCall(): void
            return false;
        });
        $this->assertSame(State::CLOSED_STATE, $circuitBreaker->getState());
-        $this->assertEquals(0, $mock->count());
+        $this->assertEquals(1, $mock->getRequestsCount());


I know the mock is not the same anymore so it behaves differently but the modified values seem a bit off compared to the initially tested feature 🤔 Are we sure we're still validating correctly the circuit breaker behaviour..

jolelievre · 2023-11-07T19:07:31Z

tests/Client/GuzzleClientTest.php

+    protected function setUp(): void
+    {
+        // Skip tests if GuzzleHttp\Client is not installed
+        if (!class_exists('GuzzleHttp\Client')) {


OK but then it means that we actually never test the guzzle client right? 🤔
Maybe we should still keep guzzle in the require-dev in composer so that we can still run tests in the CI

Changes done!

matthieu-rolland

It's qa-dev aproved 👍

I applied your PR on circuit-breaker in a prestashop, called an url after making sure I was using your PR, and it works.
The timeout also still works as usual => the fallback answer is given when timeout is exceeded
ps_distributionapiclient is still working correctly as well with your PR on circuit-breaker

boherm marked this pull request as draft November 6, 2023 11:14

boherm force-pushed the #32855-symfony-http-client branch from 1af76d5 to ae4d29e Compare November 6, 2023 14:11

boherm marked this pull request as ready for review November 6, 2023 14:12

matks reviewed Nov 7, 2023

View reviewed changes

src/Client/SymfonyHttpClient.php Outdated Show resolved Hide resolved

boherm force-pushed the #32855-symfony-http-client branch from ae4d29e to ded5440 Compare November 7, 2023 13:21

matks approved these changes Nov 7, 2023

View reviewed changes

M0rgan01 reviewed Nov 7, 2023

View reviewed changes

jolelievre previously requested changes Nov 7, 2023

View reviewed changes

boherm force-pushed the #32855-symfony-http-client branch from ded5440 to c302553 Compare November 8, 2023 11:44

Add Symfony Http Client and put Guzzle in suggest in composer.json

9076d6d

boherm force-pushed the #32855-symfony-http-client branch from c302553 to 1b2a6c2 Compare November 10, 2023 08:18

Use Symfony Http Client by default in all factories

2757706

boherm force-pushed the #32855-symfony-http-client branch 7 times, most recently from 2670573 to f6ce2f8 Compare November 10, 2023 09:00

boherm added 2 commits November 10, 2023 10:02

Update tests to use Symfony Http Client instead of Guzzle Client

8092bbb

Update docs

a38e635

boherm force-pushed the #32855-symfony-http-client branch from f6ce2f8 to a38e635 Compare November 10, 2023 09:02

boherm requested review from M0rgan01, jolelievre and matks November 10, 2023 09:08

matthieu-rolland approved these changes Nov 14, 2023

View reviewed changes

matks approved these changes Nov 17, 2023

View reviewed changes

matthieu-rolland reviewed Nov 20, 2023

View reviewed changes

matthieu-rolland merged commit 1ff44e3 into PrestaShop:develop Nov 20, 2023
26 checks passed

boherm deleted the #32855-symfony-http-client branch November 20, 2023 09:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Guzzle to Symfony Http Client #45

Migrate Guzzle to Symfony Http Client #45

boherm commented Nov 6, 2023 •

edited

Loading

boherm commented Nov 6, 2023 •

edited

Loading

matks left a comment

matks left a comment

M0rgan01 Nov 7, 2023

boherm Nov 7, 2023

M0rgan01 Nov 7, 2023

jolelievre Nov 7, 2023

jolelievre Nov 7, 2023

boherm Nov 8, 2023

M0rgan01 Nov 7, 2023

boherm Nov 7, 2023

M0rgan01 Nov 7, 2023

boherm commented Nov 7, 2023

jolelievre left a comment

jolelievre Nov 7, 2023

jolelievre Nov 7, 2023

jolelievre Nov 7, 2023

jolelievre Nov 7, 2023

boherm Nov 8, 2023

jolelievre Nov 7, 2023

jolelievre Nov 7, 2023

matthieu-rolland left a comment •

edited

Loading

Migrate Guzzle to Symfony Http Client #45

Migrate Guzzle to Symfony Http Client #45

Conversation

boherm commented Nov 6, 2023 • edited Loading

boherm commented Nov 6, 2023 • edited Loading

matks left a comment

Choose a reason for hiding this comment

matks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boherm commented Nov 7, 2023

jolelievre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthieu-rolland left a comment • edited Loading

Choose a reason for hiding this comment

boherm commented Nov 6, 2023 •

edited

Loading

boherm commented Nov 6, 2023 •

edited

Loading

matthieu-rolland left a comment •

edited

Loading