[HTTP] Allowing external Http clients

[Guikingone]

Thanks for stopping by to let us know something could be better!

PLEASE READ: If you have a support contract with Google, please create an issue in the support console instead of filing on GitHub. This will ensure a timely response.

Is your feature request related to a problem? Please describe.

Not related to an issue.

Hi everyone 👋

I'm trying to tweak this library for specific usages and I'm facing an issue when using symfony/http-client, for now, this library use Guzzle as the main HTTP client.

That's a good solution but could it be possible to not depends hardly on it and rather just use the interfaces provided by PSR-18?

The idea here is to allow every HTTP client that follow the interfaces to be used in this library (for example, MeiliSearch does it: https://github.com/meilisearch/meilisearch-php), this way, as developers, we can easily wrap something around it without using Guzzle 🤔

Thanks again for the feedback and have a great day 🙂

Describe the solution you'd like

Describe alternatives you've considered

Additional context

[bshaffer]

@Guikingone this is a great idea! We will look into it. When this library was built, PSR-7 did not exist yet. We can definitely open this up now though!

[Guikingone]

@bshaffer Great news 🎉

Let me know if I can help, thanks again for the feedback and have a great day 🙂

[norberttech]

@bshaffer would you be open to a PR that makes this repo compliant with PSR-18 known also as http-client?
I was going to suggest that change in https://github.com/googleapis/gax-php/ but since it has auth as a dependency that is bringing guzzle anyway changes need to start here.

I would try to fully replace the current Guzzle implementation with PSR interfaces, which would obviously break most of the integrations since Guzzle would no longer be provided by this package (new major release would be required).

To make the transition easier I can bring http-discovery that would look for any available Http implementations and that would try to use them in case when HTTP client is not provided.

Finally, when auto-discover fails I would throw an exception that would recommend the user to install one of available HTTP Client implementations.

Since PSR Http Message and PSR Cache are already in place, I think moving to Http Client PSR makes perfect sense.

[bshaffer]

@norberttech we are not really interested in this change. If it could be done in a non-breaking way then maybe, but we don't want our auth libraries / clients to throw exceptions requiring users to chose their HTTP client.

Potentially, we could require Guzzle, and if the user really didn't want to use it, I believe that's possible with composer's "replace" keyword

[norberttech]

Ok, let me elaborate on this one.

1) Replace Guzzle with HttpClient PSR interface

Let's first tackle replacing Guzzle with a more generic HttpClient interface.
The biggest question is whether it would be considered a BC break from this library BC promise point of view or not.

Since Guzzle is implementing \Psr\Http\Client\ClientInterface it might not be considered a BC break but it's not my call.

In general HttpClient interface gives SDK users full freedom and flexibility on what HTTP client they are willing to use so I hope we all agree this is a good change.

So going back to your question:

If it could be done in a non-breaking way then maybe,

I believe this can be implemented in a way that would still allow this lib to automatically instantiate the guzzle http client and use it.

2) Remove guzzle dependency

Now a harder one, but let's start from the beginning and let me explain my motivations behind that approach.

In general, I consider dependencies as something that I should be always extremely careful and humble about. I do not see myself in a position to decide for other developers what kind of libraries they should use, especially when I can give that decision to the users. But this is my logic and motivation and I understand that other people might prefer different approaches.

Ok, Let's look at both approaches (with and without guzzle) from more technical point of view then:

a) Guzzle as a dependency

Let's look at the dependencies:

guzzlehttp/guzzle: 7.*

guzzlehttp/promises: ^1.5.3 || ^2.0.1
guzzlehttp/psr7: ^1.9.1 || ^2.5.1
psr/http-client: ^1.0
symfony/deprecation-contracts: ^2.2 || ^3.0

guzzlehttp/promises:

empty

guzzlehttp/psr7:

guzzlehttp/promises: ^1.5.3 || ^2.0.1
guzzlehttp/psr7: ^1.9.1 || ^2.5.1
psr/http-client: ^1.0
symfony/deprecation-contracts: ^2.2 || ^3.0

psr/http-client:

empty

symfony/deprecation-contracts

empty 

guzzlehttp/guzzle: 6.*

guzzlehttp/promises: ^1.0
guzzlehttp/psr7: ^1.9
symfony/polyfill-intl-idn: ^1.17

guzzlehttp/promises:

empty

guzzlehttp/psr7:

psr/http-message: ~1.0
ralouphie/getallheaders: ^2.0.5 || ^3.0.0

symfony/polyfill-intl-idn:

symfony/polyfill-intl-normalizer: ^1.10
symfony/polyfill-php72: ^1.10

symfony/polyfill-intl-normalizer:

empty

symfony/polyfill-php72:

empty

And we are talking here about versions 6 and 7 (8 is currently under development).

---

It's not that bad, however, any of those dependencies might be conflicting with other dependencies in a project where this lib (or any other lib that is using this one) is used.

But now let's think about people who are building their systems on top of let's say Symfony, which provides its own HTTP client that is compatible with PSR-18 and could be easily used to work as an HTTP client for this library.
Or about folks that just want to use nyholm/psr7 or many other PSR-18 implementations.

Here is the first concern.

Why do you think it's a good idea to push guzzle into those people's system dependencies even when they have no intention of using guzzle?

Putting aside that it's just redundant code, that impacts the size of deployable artifacts, it's also another thing they need to maintain and follow security upgrades, that creates more potential conflicts with other dependencies.

Not to mention that when sticking to specific implementation you should make it as broad as possible, so for example, with guzzle, you already need to support guzzle 6 and 7, and potentially 8 when it's released.

b) psr/http-client with php-http/discovery as a dependency

Now let's take a look at the approach where we remove guzzle as a dependency and replace it with PSR's for Http Client/Message/Factory adding php-http/discovery

"psr/http-client": "^1.0",
"psr/http-factory": "^1.0",
"php-http/discovery": "^1.19",

psr/http-client

psr/http-message

psr/http-factory

psr/http-message

php-http/discovery

empty

---

IMO this looks much better, google auth library is now not pushing any conflicting dependencies to library users' projects.
No conflicts, no security concerns, and no additional maintenance related to supporting different versions of default HTTP client.

Ok, but what happens when someone installs this SDK without having any HTTP client?

The following code:

<?php

use Http\Discovery\Psr18Client;

require __DIR__ . '/vendor/autoload.php';

$client = new Psr18Client();

$request = $client->createRequest('GET', 'https://flow-php.com');
$response = $client->sendRequest($request);

var_dump((string) $response->getBody());

will throw the following exception:

Http\Discovery\Exception\NotFoundException: No PSR-18 clients found. Make sure to install a package providing "psr/http-client-implementation". Example: "php-http/guzzle7-adapter".

and what the user needs to do to fix that?

composer require php-http/guzzle7-adapter

That's all.
Even for completely unexperienced users that should be pretty straightforward.
So literally the whole upgrade/installation/migration is telling users to do:

-composer require google/auth
+composer require google/auth php-http/guzzle7-adapter

Http Discovery - automated installation of HTTP client

HTTP discovery can be set up in a way where it can automatically install HTTP client during composer require google/auth when no client is detected by simply allowing composer to use php-http/discovery plugin which then makes it completely transparent for the end user.

All we would need to do is add two more dependencies to google/auth (that are virtual packages, not real dependencies):

"psr/http-client-implementation": "*",
"psr/http-factory-implementation": "*"

Then, whoever will do composer require google/auth allowing composer to use http-php/discovery plugin will automatically get HTTP client dependency (which we can configure to by default fallback to guzzle).

So long story short, if we would like to take this approach, the only difference for the end user that we would like to spare deciding which HTTP client he should use would be to allow composer to use http-php/discovery plugin which makes it even less problematic from a potential migration point of view.

---

In general, this is a huge amount of work. Before even committing to anything I would need to go through all Google php libs that depend on google/auth and check how to approach this without causing any BC breaks.
But I think it's a step in the right direction that might inspire other SDK authors to move away from a specific http client implementation in favor of more generic PSR common interfaces.
That's why I'm willing to invest my time in this, of course only as long as you guys are even interested.

If I didn't manage to convince you, don't worry, and feel free to just close this issue.
But in that case, I would appreciate if you could explain to me your point of view since I might be missing something in my logic.

[gsteel]

Leaving it up to consumers to pick their preferred implementations is absolutely the way to go IMO. It's rare for a project not to already have an http client hanging around, either by default or by choice - same for factory and message implementations. It's also important to the ecosystem to promote usage of the PSRs for greater interoperability and fewer dependency conflicts

[bshaffer]

@norberttech

In general, this is a huge amount of work.

I don't think this would require a huge amount of work - you can already provide your own "http client" implementation. It's not well documented, and it uses GuzzleHttp\ClientInterface instead of PSR-18, but if a class follows the same implementation as in Guzzle6HttpHandler, it can be used instead... something like:

use Psr\Http\Client\ClientInterface;
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\ResponseInterface;

class Psr18HttpHandler
{
    public function __construct(private ClientInterface $client)
    {
    }

    /**
     * Accepts a PSR-7 request and an array of options and returns a PSR-7 response.
     *
     * @param RequestInterface $request
     * @param array<mixed> $options
     * @return ResponseInterface
     */
    public function __invoke(RequestInterface $request, array $options = []): ResponseInterface
    {
        // as "$options" aren't supported in PSR-18, either set them in the $request or $client, 
        // or don't support them.
        return $this->client->sendRequest($request);
    }

    public function async(RequestInterface $request, array $options = [])
    {
        // I'm not sure how "async" works in PSR-18...
        // This may need to be a hack that just returns a promise wrapped around "sendRequest".
        // As this method is NOT used in the auth library, you do not even need to implement it if you're 
        // passing the credentials in directly
    }
}

This can be provided when fetching a token:

$credentials = ApplicationDefaultCredentials::getCredentials($scope);
$httpClient = new Psr18HttpHandler($client);
$token = $credentials->fetchAuthToken($httpClient);

... or when you create the client library (this will use your handler for auth requests only):

use Google\Cloud\Compute\V1\Client\InstancesClient;
$instanceClient = new InstancesClient([
    'credentialsConfig' => ['authHttpHandler' => $httpClient]
]);

You can also use a custom http handler for each RPC call by configuring the rest transport (but this will require that the async method is defined):

$instanceClient = new InstancesClient([
    'transport' => 'rest',
    'transportConfig' => ['rest' => ['httpClient' => $httpClient]],
]);

[norberttech]

I don't think this would require a huge amount of work - you can already provide your own "http client" implementation. It's not well documented, and it uses GuzzleHttp\ClientInterface instead of PSR-18, but if a class follows the same implementation as in Guzzle6HttpHandler, it can be used instead... something like:

Even if that would be better documented I don't think it's a reasonable approach, duck typing is nice but it's not the most reliable way of defining extension points (http client interface is an extension point from an sdk point of view). Honestly, I would try to discurage people from taking that approach.
Not to mention that from a lib maintainer point of view, it just adds complexity into BC policy because once you make it a thing, you need to either document it somewhere or make sure that every new contributor is aware of this implicit contract existance. Properly defined, common interface is much more predicatble, reliable and more developer friendly approach.

But again, I'm not trying to force anyone to do anything, if my proposition is not something that you guys are interested in because current solution is just good enough and proposed changes in your eyes are not worth that effort (or for any other reason), that's perfectly fine with me.

[bshaffer]

duck typing is nice but it's not the most reliable way of defining extension points

We could add a Google\Auth\HttpHandler\HttpHandlerInterface. This would define the interface and essentially be an adapter. Then if we supported the Psr18HttpHandler class, this would be the same support as we have for PSR-6