Phpspreadsheet: Guzzle 6 support

[tyteen4a03]

This is:

- [ ] a bug report
- [x] a feature request
- [ ] **not** a usage question (ask them on https://stackoverflow.com/questions/tagged/phpspreadsheet or https://gitter.im/PHPOffice/PhpSpreadsheet)

PHPSpreadsheet 1.14 only supports Guzzle 7. As it is only released a few days ago most of us are still on Guzzle 6. Please lower the requirements for Guzzle so we can take advantage of the bugfixes.

[umulmrum]

I second that, especially as 1.14 contains a security fix I'd like to apply. Several other libs don't support Guzzle 7 yet, so that's a blocker. Don't know how hard it is to support both versions though - if it isn't doable, I'd be happy if the fix could be backported to a 1.13.1 release. Thank you very much!

[patrickbrouwers]

From my previous reply:

Is Guzzle 7 a hard requirement for this to work or could we loosen up the requirement to ^6.3.1|^7.0?

Installing in a clean Laravel 7.0 install currently doesn't allow 1.14 to be installed, which I'm afraid will cause a lot of confusing among the Laravel Excel users who want to upgrade to use the backwards compatible fix for the empty enclosures which was fixed in 1.14.

[tyteen4a03]

Looking at the PR itself I don't see anything that would be Guzzle 7 only.

[PowerKiKi]

Guzzle 7 supports PSR-18, but Guzzle 6 does not. This is a problem for us since we rely on PSR-18 in order to be flexible and allow users to change the HTTP client with something else (via \PhpOffice\PhpSpreadsheet\Settings::setHttpClient()).

I don't see an obvious way to still rely on PSR-18 while supporting both Guzzle 6 and 7 out of the box.

I'm thinking the easiest for now is for PhpSpreadsheet to require psr/http-client and not require any implementation. So the user MUST enable WEBSERVICE support by using \PhpOffice\PhpSpreadsheet\Settings::setHttpClient(). And PhpSpreadsheet may use guzzle strictly for testing purpose only (so in require-dev only), or maybe not at all.

I'd merge a PR for that if it also includes documentation and an exception that explain the situation if getHttpClient() cannot return an implementation. Although I believe it would be more constructive to put effort into migrating other libs to Guzzle 7.

edit: that means if you don't need WEBSERVICE, you do nothing (probably the vast majority of users out there). If you need it, but use Guzzle 6, it is up to you to write/find a thin wrapper for PSR-18. If you already are on Guzzle 7, you can configure it easily.

[mfn]

@umulmrum

I second that, especially as 1.14 contains a security fix I'd like to apply

Which one would that be? In the release notes nothing stands out to me at https://github.com/PHPOffice/PhpSpreadsheet/releases/tag/1.14.0?

[tyteen4a03]

Although I believe it would be more constructive to put effort into migrating other libs to Guzzle 7.

Given that Guzzle 7 is less than a week old, I don't think it's reasonable to expect people to be able to finish migrating for at least another half year.

[umulmrum]

@umulmrum

I second that, especially as 1.14 contains a security fix I'd like to apply

Which one would that be? In the release notes nothing stands out to me at https://github.com/PHPOffice/PhpSpreadsheet/releases/tag/1.14.0?

Ah I'm very sorry, I made a total mistake. Just ignore :-)

[Pierstoval]

TL;DR:

• I agree with @PowerKiKi
• The Web feature is totally optional, which is the only reason why Guzzle is needed.
• Users having other HTTP client implementations will have redundant dependencies, and it's especially a problem if they face dependency conflicts with Guzzle.
• PhpSpreadsheet should require PSR-18 instead of a specific implementation, to allow devs to use whatever HTTP client they want.
• Guzzle 7 is young, but as an alternative, Symfony's HTTP client could achieve the same goal and it's been available since Symfony 4.3 (May 2019, so a year now). They both implement PSR-18, so one could be used instead of the other.

---

I've just upgraded PhpSpreadsheet on my project and see a new dependency on Guzzle and I think this is not the best option.

Not because Guzzle is bad (it's not), but because you're forcing users to require Guzzle where it's not particularly needed.

Guzzle implements PSR-18 and therefore PhpSpreadsheet should depend only on the psr/http-client and not create any Client by itself.
Instead, PhpSpreadsheet classes that need an HTTP client should accept a ClientInterface as argument in order to make the HTTP request, and you could still create your own PSR-7 Request objects.

The HTTP part is not a mandatory feature at all, therefore I'd expect it to allow me to provide any HTTP client among the many clients that implement PSR-18.

I would instead put Guzzle (or any other HTTP client implementing PSR-18) into the suggest section of the composer.json, and that's all. The need for a ClientInterface in the current Web class would be sufficient to invite developers to create their own HTTP client.

I would be glad to help refactor the current HTTP feature if you like 🙂

[patrickbrouwers]

Sounds good @Pierstoval ! I think @PowerKiKi already mentioned he would accept a PR to change it, so I would say go ahead! :)

[Pierstoval]

Here you are @patrickbrouwers: #1568 😉