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

Support PSR HTTP client #9

Open
garak opened this issue Nov 29, 2023 · 15 comments
Open

Support PSR HTTP client #9

garak opened this issue Nov 29, 2023 · 15 comments

Comments

@garak
Copy link

garak commented Nov 29, 2023

It would be nice to not restrict HTTP client usage to Guzzle, and using a PSR HTTP client instead

@dereuromark
Copy link
Contributor

Looks like this could be included in #8 right?

@yidas
Copy link
Owner

yidas commented Feb 29, 2024

It looks like in #8, the composer is still using Guzzle.

I think it's inconvenient to maintain a PHP client without a mainstream HTTP client package.

@garak
Copy link
Author

garak commented Feb 29, 2024

@yidas you can still include Guzzle of course, but without forcing other people to use it

@yidas
Copy link
Owner

yidas commented Feb 29, 2024

Hi @garak,

As I know, PSR-18 is the definition of the client interface.
May I know more about how you actually want to achieve?

@garak
Copy link
Author

garak commented Feb 29, 2024

I'd like to use this library without being forced to use Guzzle.
This would also improve the quality of the code, making it more SOLID.

@yidas
Copy link
Owner

yidas commented Feb 29, 2024

Got it.
It can be a goal, and once it's implemented we can remove Guzzle dependency.

@yidas yidas added this to the Client without dependency milestone Feb 29, 2024
@alex-kalanis
Copy link

Possible with #8 -> instead of the whole client (facade there) just call inner processings

// ...
# today
$calls = new \yidas\googleMaps\Services(new \yidas\googleMaps\Clients\GuzzleClient(), new \yidas\googleMaps\Services\ServiceFactory(new \yidas\googleMaps\ApiAuth($optParams)));
# via PSR
$calls = new \yidas\googleMaps\Services(new \yidas\googleMaps\Clients\PsrClient($yourPsrClient), new \yidas\googleMaps\Services\ServiceFactory(new \yidas\googleMaps\ApiAuth($optParams)));
// ...

@garak
Copy link
Author

garak commented Feb 29, 2024

Not a good idea: dependencies should be injected, not instantiated (again, to be SOLID)

@dereuromark
Copy link
Contributor

dereuromark commented Feb 29, 2024

I think the main issue remaining is

    "require": {
        ...
        "guzzlehttp/guzzle": "~6.5.6|^7.4.3"
    },

There should be a solution to require the abstract only, not the concrete.
my guess: https://packagist.org/packages/psr/http-client
That's probably what he mainly means.

If we have to major now anyways, with that PR being merged, maybe this is the chance to also clean up that dependency issue

@alex-kalanis
Copy link

@garak : So you just call

class Something
{
    public function __construct(
        protected \yidas\googleMaps\Services $service
    ) {}
}

And set the rest of dependency tree somewhere else... Like in neon config

parameters:
  apiKey: YOUR_API_KEY

services:
  - yidas\googleMaps\ApiAuth(%apiKey%)
  - yidas\googleMaps\Services\ServiceFactory
  - yidas\googleMaps\Clients\PsrClient
  - yidas\googleMaps\Services

I see no problem.

@garak
Copy link
Author

garak commented Feb 29, 2024

@alex-kalanis I was talking about the relationship between \yidas\googleMaps\Services and the HTTP client

@dereuromark
Copy link
Contributor

dereuromark commented Feb 29, 2024

To which I added context that clarifies it further :)

"require": {
    ...
    "psr/http-client": "^1.0.3"
},

and move guzzle to require-dev for test harness (as default)

@garak
Copy link
Author

garak commented Feb 29, 2024

I'm not sure to understand how it was clarified... the only relevant subject here is the constructor of the class receiving the HTTP client

@dereuromark
Copy link
Contributor

No, also the compose require part as outlined in my previous comment here :) Otherwise you gain nothing really.

@garak
Copy link
Author

garak commented Feb 29, 2024

That can be relevant elsewhere, just not relevant here.

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

No branches or pull requests

4 participants