search

helpscout / helpscout-api-php

PHP Wrapper for the Help Scout API
MIT License
99 stars 62 forks source link

Leverage Psr18Client when possible #311

Closed nicolas-grekas closed 11 months ago

nicolas-grekas commented 1 year ago

Since #309 was received with praise, I decided to spend a few hours on the topic, so here we are: this PR allows switching to a PSR-18 client.

It does so without breaking compatibility with existing deployment, since using PSR-18 is opt-in.

1st commit is #309 2nd commit is #310

The implementation leverages Psr18Client from php-http/discovery to provide a smooth experience.

The user-facing side of this PR is ApiClientFactory::createClient(), which gets a 2nd argument to allow injecting a PSR-18 implementation. This enables nicely integrating the SDK with e.g. the Symfony profiler toolbar.

When Guzzle isn't installed but Symfony is (and no client is explicitly passed to ApiClientFactory::createClient()), RestClientBuilder will instantiate a retryable HttpClient from Symfony.

miguelrs commented 1 year ago

@nikolas-grekas Again, thanks for all this work! ❤️

At first sight, this looks good to me 👍 Still, we will need to take some time to discuss it and review it properly.

If this sounds good to you, I'll put this PR and the previous one on hold for a moment, and we will review them as a whole. I know the previous PR is independent, but it makes sense to dedicate some time to the whole thing and then release them together as well. I can't give you an exact estimate, but I'll try to make room for it soon!

nicolas-grekas commented 1 year ago

Sure! Thanks for considering :)

miguelrs commented 11 months ago

Hey @nicolas-grekas, I'm sorry we took so long to get back to you! We've been busy with some big product launches, and this one went under the radar 😓

We did discuss these changes internally a while back, and we decided we should not merge them, at least for now.

The reason is basically that we were already using HTTPlug in the past, and we removed it in https://github.com/helpscout/helpscout-api-php/pull/70 back in 2018, in favor of just using Guzzle. This was for simplicity for our customers, who sometimes don't have great dev skills, or at least don't have experience with PHP/Composer/etc...

Also, even if we wanted to go back to that approach, it'd require a major version release, because it'd break BC for anyone not explicitly requiring Guzzle, or even worse, for anyone requiring Guzzle ^6.3 in particular.

Furthermore, our understanding is you could rather easily fork this project (you probably already have :), given it's not a highly active repo. Please let me know if that's not the case.

Anyway, if we do have some other substantial changes to make a major version release in the future, we will reconsider going back to this approach, because we do agree it's good to maximize flexibility, without adding a lot of complexity...

Thanks again for your efforts though! ❤️

nicolas-grekas commented 11 months ago

Thanks for considering and for your reply!

nicolas-grekas commented 11 months ago

(i didn't fork - forking is too high maintenance costs ;) )