Using PSR-18 interface instead of hard Guzzle dependency

[bbaga]

I was wondering whether you'd be willing to drop the hard dependency in Guzzle and make the HTTP client interchangeable. I know Guzzle is one of the (if not THE) most popular HTTP clients out there, but I'm not a big fan of the fact that most libraries will have a dependency on some specific version of Guzzle and it can be a pain in the neck to align the versions so everybody can be happy.

Looking at the code, I didn't see anything that wouldn't be possible with the PSR-18 interface and I believe the switch could be implemented so it requires only a minimal "migration" on the end-user side - users would have to install guzzle (and a guzzle -> PSR-18 adapter) explicitly.

I'd like to use this library for its Query/Mutation classes (I don't really care for the Client class), but I'd prefer to not bring Guzzle in as a dependency.

I'd be happy to do the work and submit a PR, but before I start, I wanted to see if there is any chance of it being accepted.

[mghoneimy]

Hi @bbaga ,

Let me take a look at the code tonight and let you know what I think tomorrow. But until I do I have a concern I'd like to hear your thoughts on. Do you think that this change will be backwards incompatible? Will the package need to be bumped up one major version after this?

[mghoneimy]

I took a look at the code. The change can definitely be made to replace the hard dependency on GuzzleHttp to become a dependency the PSR-19 inerface, and it should be an easy one.

Modifications needed:

1. Modify the constructor to inject the client to be used to make the request
2. Modify the method invoked on the client class to use sendRequest() instead of the magical method post()
3. Remove GuzzleHttp from the composer.json file

I love your suggestion and the direction of removing unnecessary dependencies from packages in general is something I'm all in form. My only concerns remains that if we do this we'll need to bump up the package to version 2.0 because of the backwards incompatible changes that will be made to the constructor signature.

Because of this, I'd rather if we find a middle point where we'd implement the changes that would make the package ready to integration with any PSR-18 compliant client (Modification number 2 in my list) but at the same time keep the constructor signature the way it is (Modification number 1).

I propose adding an optional parameter of a client of type PSR-18 ClientInterface to the client constructor interface. If the optional parameter is provided, then the client passed is used, else the constructor would instantiate a new GuzzleHttp client and use it as it normally did.

The disadvantages of this is that it wouldn't solve your problem, guzzle would still be in the composer dependencies. The advantages though is that the package is now not dependent on the GuzzleClient, but rather on the Psr\ClientInterface, and when the package is ready for a major release, this change would be squeezed in with it, removing GuzzleHttp from the package forever.

I'm open to hearing your thoughts on all this, maybe you can convince me that the client change is enough to bump the package one major version up.

[bbaga]

You just posted your update while I was typing this:

Given that guzzle would be removed from required and added to suggest instead. This change probably would warrant a major version bump as the current mainline's users may (rightfully) assume that the package will continue to work as it did before, which may or may not be the case depending on their composer.json/composer.lock

I believe we could leave the client's interface backward compatible though.

I'm cool with your plan, do you have a timeline/backlog for v2 release? 😉

EDIT: I don't have strong arguments for why you should release a new major version other than I'd like this project more if it didn't depend on guzzle 🤷‍♂️

[mghoneimy]

I just wanted to note that to me, it seems like the constructor signature change is more of a backward compatibility problem than the composer file as we'll need to the users to start injecting their preferred HTTP client choice to the GraphQL\Client now opposed to earlier when the package used Guzzle by default. Let me know if I'm missing something here.

As for my v2 timeline, I have some tech debt to get rid off, some behaviors to modify how they work, and a few enhancements to implement, but I don't have a specific timeline tied to that. All I can say with regards to the timeline is that I have a lot of personal stuff going on in my life now and I'd rather not open up working on a new major release in the near future.

I'd rather if we maintain backward compatibility for now and release this change as a minor version. I feel like the changes you've proposed are changes in the right direction either ways so if you're open to doing that, let's start a pull request to get things moving.

[bbaga]

What I had in mind for the constructor is that we could add a new optional $client parameter for the PSR-18 client in the constructor, then when:

• $client !== null , use it as the client
• $client === null && class_exists(\GuzzleHttp\Client::class), set a guzzle psr-18 adaptor class as the client
• else: throw an exception telling the users that they either have to provide a PSR-18 client or install the guzzlehttp package

I'll start working on the backward compatible version in the next couple days 🤞

[mghoneimy]

Ah gotcha! I like this suggestion, it makes more sense than what I proposed earlier.

Will wait for your pull request when it's ready ✌️

[mghoneimy]

Hard dependency on Guzzle will be totally removed in the next major version release because it is backwards incompatible

[Doqnach]

That is a very welcome change! Is there any timeline on this, since I'm currently working on a project that will hopefully use this package and would benefit greatly from this decoupling.

[mghoneimy]

@Doqnach I don't have a strict timeline tied to this, but it's going to be included in the next major release as this change will lead to some backward incompatibly. Generally though, I would expect to have some time to work on the next major release within the next 2-3 months.
Hope this helps!

[scorgn]

Laravel 8 just came now out which now uses Guzzle ^7.0.1. Guzzle 7 implements PSR-18, so making this change would open up the possibility of using Guzzle 7 as well as Laravel 8.

I imagine a lot of people who use this package use it with Laravel, just based on the compatibility with Laravel (past versions) and the large amount of PHP developers who use Laravel. Would we expect to see changes that allow for Guzzle ^7.0.1 to support Laravel 8, or will that be bypassed in place of using the PSR-18 interface? Both would be breaking changes - the difference is one could be implemented much faster with minimal code change. I'm wondering if we can release a new version with Guzzle 7 while continuing to work on using the PSR-18 interface.

In any case, I can help with the development of this feature.

[mghoneimy]

@ShawnCorrigan I'm not exactly against supporting Guzzle7, I just don't think it's worth the major version bump. I just posted my thoughts about #36 in a comment. If we are able to resolve how we can merge that pull request without bumping the package up to the next major version, then I'm fine doing that while work is getting done on replacing guzzle dependencies with PSR-18 interfaces.

I'm always open to contributions from the community, so thank you for the pull request! I took a quick look #38 but it's a big change and will need thorough review. Will allocate more time to doing this later this week.