Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guzzle 6 support #1562

Closed
tyteen4a03 opened this issue Jun 30, 2020 · 10 comments
Closed

Guzzle 6 support #1562

tyteen4a03 opened this issue Jun 30, 2020 · 10 comments

Comments

@tyteen4a03
Copy link

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
Copy link

umulmrum commented Jul 2, 2020

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
Copy link
Contributor

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
Copy link
Author

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

@PowerKiKi
Copy link
Member

PowerKiKi commented Jul 3, 2020

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
Copy link

mfn commented Jul 3, 2020

@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
Copy link
Author

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
Copy link

umulmrum commented Jul 3, 2020

@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
Copy link

Pierstoval commented Jul 3, 2020

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
Copy link
Contributor

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

@Pierstoval
Copy link

Here you are @patrickbrouwers: #1568 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

6 participants