Rely on PSR-18 and Guzzle/psr7 only instead of full Guzzle. #1568

Pierstoval · 2020-07-03T21:41:57Z

This is:

- [x] a bugfix
- [ ] a new feature

Checklist:

Changes are covered by unit tests
Code style is respected
Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
CHANGELOG.md contains a short summary of the change
Documentation is updated as necessary (note from PR author: this feature isn't yet documented)

Why this change is needed?

Fixes #1562, check the issue for more details

Pierstoval · 2020-07-03T21:46:08Z

src/PhpSpreadsheet/Settings.php

-    /**
-     * Set the HTTP client implementation to be used for network request.
-     */
-    public static function setHttpClient(ClientInterface $httpClient): void


I removed this because some HTTP clients implementing PSR-18 might be scoped (the Symfony one can be, for instance), so it's safer to inject it at runtime instead of keeping a client in the memory

might be scoped

What to you mean be that ? And what difference does it make for PhpSpreadsheet if an HTTP client is scoped ?

When using Symfony HTTP Client, it can be scoped, meaning it can have a base uri, default headers, etc., which might not be suitable for PhpSpreadsheet usage. Some HTTP clients can also contain state: history, cookies, etc., so keeping its state in the memory can have side-effects, such as leaking state, etc.

Pierstoval · 2020-07-03T21:46:31Z

src/PhpSpreadsheet/Settings.php

+        if (class_exists(Client::class)) {
+            // Support for Guzzle
+            return new Client();
+        } elseif (class_exists(HttpClient::class)) {
+            // Support for Symfony HttpClient
+            return HttpClient::create();


Let's provide support for at least 2 clients, just in case 😉

This won't do. You removed dependencies on implementation in composer.json, but you re-add add them here in code. All references to specific implementations must disappear.

Well, these are optional dependencies, since class_exists() is used, so if the class doesn't exist, it will fall back to an exception, telling the user to either inject an http client, or install one of the two supported implementations.

I forgot to do it in the PR, but I was about to add a suggest section in composer.json to invite users to install either symfony/http-client or guzzlehttp/guzzle to benefit from the WEBSERVICE feature.

This is kinda like automatic feature-flag depending on what composer packages are installed in the project

Pierstoval · 2020-07-03T21:47:19Z

composer.json

@@ -57,7 +57,8 @@
        "markbaker/complex": "^1.4",
        "markbaker/matrix": "^1.2",
        "psr/simple-cache": "^1.0",
-        "guzzlehttp/guzzle": "^7.0"
+        "psr/http-client": "^1.0",
+        "guzzlehttp/psr7": "^1.6"


guzzlehttp/psr7 is safe, compared to guzzlehttp/guzzle, because it only contains the tools to create the Request object, which would be cumbersome if we had to create it directly in PhpSpreadsheet.

If we do the PSR way, we hat to do it all the way. That means dropping all references to Guzzle and instead rely on https://www.php-fig.org/psr/psr-17/ to create Request object.

Even though I agree on your point, there are some kind of maintainability issues with it: this means that PhpSpreadsheet would have to have its own factory, plus its own Request class, plus its own Response class for testing. This is reinventing the wheel at the cost of maintaining it for one single feature in PhpSpreadsheet.

Here, relying on guzzlehttp/psr7 is safer than the entire Guzzle stack, because guzzlehttp/psr7 depends only on ralouphie/getallheaders version 2 and 3, making it compatible with all packages that might already have it in their dependency tree. For instance, Drupal needs it since 8.8, Slim needs it too, they depend on its v3. There are very few packages that need its v2, and yet guzzle's PSR-7 implementation is still compatible with it anyway, so it's safe even with really old apps.

Seems really fair in terms of balance between maintainability and dependency tree 😉

If you like to go all along with PSRs, then we shouldn't at all require any PSR-18 implementation and instead recreate an entire Http Client in PhpSpreadsheet, but I think you agree that relying on external implementations by using interfaces is safer for PhpSpreadsheet maintainers burden 😉

PowerKiKi

There are a few things that need changing/explaining before this can be merged.

PowerKiKi · 2020-07-06T00:42:36Z

composer.json

@@ -57,7 +57,8 @@
        "markbaker/complex": "^1.4",
        "markbaker/matrix": "^1.2",
        "psr/simple-cache": "^1.0",
-        "guzzlehttp/guzzle": "^7.0"
+        "psr/http-client": "^1.0",
+        "guzzlehttp/psr7": "^1.6"


If we do the PSR way, we hat to do it all the way. That means dropping all references to Guzzle and instead rely on https://www.php-fig.org/psr/psr-17/ to create Request object.

PowerKiKi · 2020-07-06T00:44:47Z

src/PhpSpreadsheet/Calculation/Web.php

@@ -18,7 +19,7 @@ class Web
     *
     * @return string the output resulting from a call to the webservice
     */
-    public static function WEBSERVICE(string $url)
+    public static function WEBSERVICE(string $url, ?ClientInterface $client = null)


The method signature must not change, because in most cases the user is not directly calling this method, instead the calculation engine is. And the calculation engine cannot provide extra parameters such as this new one.

PowerKiKi · 2020-07-06T00:45:44Z

src/PhpSpreadsheet/Settings.php

-    /**
-     * Set the HTTP client implementation to be used for network request.
-     */
-    public static function setHttpClient(ClientInterface $httpClient): void


might be scoped

What to you mean be that ? And what difference does it make for PhpSpreadsheet if an HTTP client is scoped ?

PowerKiKi · 2020-07-06T00:46:57Z

src/PhpSpreadsheet/Settings.php

+        if (class_exists(Client::class)) {
+            // Support for Guzzle
+            return new Client();
+        } elseif (class_exists(HttpClient::class)) {
+            // Support for Symfony HttpClient
+            return HttpClient::create();


This won't do. You removed dependencies on implementation in composer.json, but you re-add add them here in code. All references to specific implementations must disappear.

PowerKiKi · 2020-07-06T00:48:34Z

src/PhpSpreadsheet/Settings.php

-    /**
-     * Set the HTTP client implementation to be used for network request.
-     */
-    public static function setHttpClient(ClientInterface $httpClient): void


This should probably be kept as:

public static function setHttpClient(ClientInterface $httpClient, RequestFactoryInterface $requestFactory): void

Because both parameters must be set, in a coherent way, in order for WEBSERVICE to work.

PowerKiKi · 2020-07-06T00:57:05Z

tests/PhpSpreadsheetTests/Calculation/Functions/Web/HttpClientMock.php

+use Psr\Http\Message\RequestInterface;
+use Psr\Http\Message\ResponseInterface;
+
+class HttpClientMock implements ClientInterface


This entire class should be replaced by PHPunit mock mechanic in order to also test that the method is called exactly once.

I don't see any value in checking the number of times the client is tested.

I could change the mock to remove the internal response when sendRequest() is used, this will ensure it's used only once.

Also, using phpunit mocks has a performance impact. Here, having a custom mock is costless

patrickbrouwers · 2020-07-18T10:10:44Z

@Pierstoval are you planning to improve your PR based on @PowerKiKi 's comments?

Pierstoval · 2020-07-19T11:31:18Z

Thanks for taking the work back 👍

Pierstoval mentioned this pull request Jul 3, 2020

Guzzle 6 support #1562

Closed

Rely on PSR-18 and Guzzle/psr7 only instead of full Guzzle.

eac0db0

Pierstoval commented Jul 3, 2020

View reviewed changes

PowerKiKi requested changes Jul 6, 2020

View reviewed changes

PowerKiKi closed this in 7cb4884 Jul 19, 2020

Pierstoval deleted the guzzle branch July 19, 2020 11:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rely on PSR-18 and Guzzle/psr7 only instead of full Guzzle. #1568

Rely on PSR-18 and Guzzle/psr7 only instead of full Guzzle. #1568

Pierstoval commented Jul 3, 2020

Pierstoval Jul 3, 2020 •

edited

Loading

PowerKiKi Jul 6, 2020

Pierstoval Jul 6, 2020

Pierstoval Jul 3, 2020

PowerKiKi Jul 6, 2020

Pierstoval Jul 6, 2020

Pierstoval Jul 3, 2020

PowerKiKi Jul 6, 2020

Pierstoval Jul 6, 2020 •

edited

Loading

PowerKiKi left a comment

PowerKiKi Jul 6, 2020

PowerKiKi Jul 6, 2020

PowerKiKi Jul 6, 2020

PowerKiKi Jul 6, 2020

PowerKiKi Jul 6, 2020

PowerKiKi Jul 6, 2020

Pierstoval Jul 6, 2020

patrickbrouwers commented Jul 18, 2020

Pierstoval commented Jul 19, 2020

Rely on PSR-18 and Guzzle/psr7 only instead of full Guzzle. #1568

Rely on PSR-18 and Guzzle/psr7 only instead of full Guzzle. #1568

Conversation

Pierstoval commented Jul 3, 2020

Why this change is needed?

Pierstoval Jul 3, 2020 • edited Loading

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

Pierstoval Jul 6, 2020 • edited Loading

Choose a reason for hiding this comment

PowerKiKi 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

patrickbrouwers commented Jul 18, 2020

Pierstoval commented Jul 19, 2020

Pierstoval Jul 3, 2020 •

edited

Loading

Pierstoval Jul 6, 2020 •

edited

Loading