Implement PSR-17 + PSR-18

[barryvdh]

See #204 (comment)

• Diactoros nyholm/psr7 is requires as Guzzle/psr7 doesn't yet provide a PSR-17 request factory

Slightly breaking changes:

• Client is now final
• Typehint is removed for the request factory, streamFactory added
• Minimum PHP version is 7.1
• A PSR-18 client is now required, but most php-http adapters should also support that now.

This should only affect people who are extending our Client. If they are providing a PSR-18/Httplug Client it should still work. Also the Requestfactory should support both the Httplug Requestfactory (deprecated) and PSR-17 RequestFactory.

[barryvdh]

cc @sagikazarmark Does this make sense to you?

[barryvdh]

@HACKTIVISTA I've added some things based on your PR:

• Add StreamFactory (but private)
• Lazily use the factories

And made the Body param work + test it.
I want to drop diactoros when Guzzle (and other popular clients) provide the Factories by default, to avoid more difficult installs.

[3ynm]

About

omnipay-common/src/Common/Http/Client.php

Lines 89 to 92 in 06cde89

	if ($body !== null && $body !== '' && is_string($body)) {
	$body = $this->getStreamFactory()->createStream($body);
	$request = $request->withBody($body);
	}

I think it's ok to accept body as string, but not accepting an stream is somewhat unexpected, don't you think? You can copy-paste this.

        if (!empty($body) && (is_string($body) || $body instanceof StreamInterface)) {
            if(is_string($body)) {
                $body = $this->getStreamFactory()->createStream($body);
            }
            $request = $request->withBody($body);
        }

[barryvdh]

Yeah we could allow a I guess.

[barryvdh]

Done, I only convert when it's a string, let the error fall through the factory otherwise.

[judgej]

Thanks, I'll take a look tomorrow - doing something very similar with Xero this evening, so I should have a bit more experience on this by the morning :-)

[judgej]

Should these all be type-hinted? I'm not sure if a later PHP breaks this by requiring the typehint to be optional e,g. ?StreamFactoryInterface and that's the reason for leaving optional parameters unhinted? Just discovering this myself.

Suggested change

	public function __construct($httpClient = null, $requestFactory = null, $streamFactory = null)
	public function __construct(HttpClientInterface $httpClient = null, RequestFactoryInterface$ requestFactory = null, StreamFactoryInterface $streamFactory = null)

[barryvdh]

The reason they are not typehinted, is that current versions use HttpPlug instead of PSR Http Client, so that would be a breaking change.

[barryvdh]

We could perhaps also deprecate this HttpClient and create a new PsrHttpClient with proper typehinting, and use that by default (starting with v3.1)

[3ynm]

@barryvdh here my version if you want some inspiration: https://gist.github.com/hacktivista/63cdd322e3764793f7885efdf91da76a

You could save it in the namespace Omnipay/Common so it becomes Omnipay/Common/HttpClient instead of Omnipay/Common/Client/Client

[judgej]

Can I just take a step back from this and ask a silly question?

I'm a bit confused about where discovery should sit. The idea behind factory discovery is that no specific implementation needs to be baked into a package, so it can use any implementation of some common HTTP functions that meet a standard (PSR) interface. That makes sense, and means the package (omnipay common) needs those factory implementations.

Now, the discovery is another beast. The documentation of the concepts behind these is pretty poor IMO, in that they don't suggest where they should sit. If they sit at the framework level, then the framework developer or framework user chooses just which discovery service they will use (e.g. http-interop, php-http, which I think have the same roles). The framework instantiate the factories then passes them into the packages that need them. That makes sense too, though it makes the factories a little more cumbersome to use without the framework around them.

So, we then have a hybrid. If I understand correctly, that means the package can accept factories if it is supplied with them, but will discover factories for itself if not. To do that teh package developer will have chosen a discovery package to use and that will be baked in, though I guess option (it is only used if factories are not instantiated outside of the package).

So - am I correct in that understanding? If so, is Omnipay going for the hybrid option, with php-http as the chosen fallback discovery package? Sorry for a will of text - I just need to understand how it all hangs together, and reading up widely has not actually touched on how these areas should be approache.

[barryvdh]

To be honest, I'm not really sure either.

Ideally, we (Omnipay Common) wouldn't need an Http class and we would just reference the PSR-18 Client Interface in the gateways. Same would go for the factories, the gateways could just reference that.

In a framework, I guess the PSR-18 Client + factories would be autowired or something, so that the correct one gets resolved. But for non-framework, you would have to define them and pass them to a constructor or something.

The problem right now is that there is no autowiring / dependency resolving in Omnipay, it's just passing the HttpClient and the HttpRequest, always, see

omnipay-common/src/Common/GatewayFactory.php

Lines 80 to 88 in c4567f7

	public function create($class, ClientInterface $httpClient = null, HttpRequest $httpRequest = null)
	{
	$class = Helper::getGatewayClassName($class);
	if (!class_exists($class)) {
	throw new RuntimeException("Class '$class' not found");
	}
	return new $class($httpClient, $httpRequest);

That wouldn't be a problem if the PSR-18 Client alone would be enough to actually make a request, but to make a request you need the PSR-18 client, + a PSR-7 Request. And for that you would need at least a PSR-7 factory and a StreamFactory. So do we want to bother the Gateways with those specific details or not? Currently, that is all abstracted in our HttpClient, so gateways don't need to bother with the requests specifics.

Now the question, where does discovery belong. I do agree that if you want to follow DI principles, you'd rather not have that in this class, but perhaps a HttpClientFactory or a static method to set the default HttpClient.

Currently you would need to do something like this to set your own Client stuff:

use Omnipay\Omnipay;
use Omnipay\Common\Http\Client;

$client = new Client($psrClient, $requestFactory, $streamFactory);

// Setup payment gateway
$gateway = Omnipay::create('Stripe', $client);
$gateway->setApiKey('abc123');

But we could make it something universal:

use Omnipay\Omnipay;
use Omnipay\Common\Http\Client;

// Use own Http Client
$client = new Client($psrClient, $requestFactory, $streamFactory);
Omnipay::setHttpClient($client);

// Setup payment gateway
$gateway = Omnipay::create('Stripe');
$gateway->setApiKey('abc123');

Then we could use Omnipay::getHttpClient() to either get the defined version, or use discovery there to detect the factories/clients. And we could properly typehint the class (if we make a new one)

[judgej]

Switching to PSR-17+PSR-7 kind of changes the emphasis from configuring the client, to configuring the message. The client becomes a dumb "message pusher" - it's only got one method in the API anyway - sendRequest().

The way I have tackled it in a Xero package - wrongly or right as I am still experimenting - is like you describe. A PSR-18 client can be passed in, but it if isn't, then it will be auto-discovered. However, auto discovery uses a package I have chosen, since discovery of discovery libraries starts to go a bit inception. here my client is a decorator for the PSR-17 client, which can be passed in or also auto-discovered. This client can then modify the requests as they come through 9that would be middleware I guess) but being OAuth1 with token renewal, it also needs to invisibly generate new renewal messages and that involves a URL factory discovery too. This is a library I need for a project next week, but also I'm trying to learn about how PSR-7/PSR-17/PSR-15/PSR-18 hangs together in a practical way.

I'm rambling again, but it seems it is a good way to go for flexibility. If a layer could be built on top fo that to support the existing interfaces for Omnipay drivers, but also expose the underlying PSR-factories and client for new or updated drivers to use. I don't think the drivers should mess about with any discovery though - they should just say, "give me the PSR client" or "give me the PSR request factory". They are simple to use after that (though I have found including the PSR URI factory is essential to do this - not sure if there was an oversight in one of the PSRs allowing UriInterface or strings in some PSRs, but only UriInterface in others).

---

Just as a principle, even if a single discovery package was chosen for Omnipay Common, I think it:

• Should be a suggested install only, with this being well documented. The chosen discovery package may not be compatible with others that the developer may have installed.
• Should not be required, i.e. have ways to pass all this stuff in during instantiation if a framework wants to (message factory, client, uri factory).

[barryvdh]

Removing Discovery now would be a big breaking change, as people would need to update their code to make Discovery work.

[judgej]

Agreed, I'm not suggesting removing it. Just make it an option to install. The idea would be to use discovery built in to Omnipay Common, requiring an installation of a package, or do your own discovery or hard wiring and pass your factories in. In time, we can support multiple discovery packages with no breaking changes.

[barryvdh]

Right, for Guzzle7 we should probably merge this sometime.

[sagikazarmark]

cc @sagikazarmark Does this make sense to you?

Probably a bit late to the discussion, but yeah, makes sense 😄

[matthewnessworthy]

@barryvdh what would be required to get this merged and tagged? is there something I can help with?

[barryvdh]

Not sure if we need to do anything still. Was kinda hoping that guzzle/psr7 would provide the PSR-17 implementations, but that is only in 2.x and not yet released.

Do you have a breaking issue for now? I think Omnipay should still work with the new Guzzle etc, right?

[matthewnessworthy]

Guzzle 7 was not working for me in the 3.1@dev branch ( thephpleague/omnipay#615 (comment) )

[Axent96]

It looks a bit similar with #204 && #262
Any plans for progress here?