Non-breaking PSR-17 + 18 compatibility in Http\Client

[3ynm]

On top of #207

• Restore old request() functionality
• Make sendRequest() public
• Load factories on first use
• PSR 17 and 18 discoveries, fallback if failed
• request() as PSR-17 if instance applies, fallback if not
• PSR 18 and 17 createRequest(), createStream() and createUri() factories
• Delete orphan ResponseInterface reference
• Add deprecation notice for request()
• Add deprecation notice for sendRequest() exceptions

@barryvdh Lacks tests for new functionality... though would be great if you tell me what you think about it

[barryvdh]

As I said in #207, I don't think this is required because the signature on both clients/factories are compatible with each other.

[3ynm]

What about the new methods / sendRequest as public / deprecation notices / not requiring diactoros?

I can add documentation too for those new ways of developing with Omnipay.

[barryvdh]

Restore old request() functionality -> Not required, both factories create the same Request object. If you chain it later, you should still get the same result as adding it with parameters directly Make sendRequest() public -> Why do we need this? Load factories on first use -> I would be okay with this PSR 17 and 18 discoveries, fallback if failed -> The httplug requestfactory is deprecated and the new adapters versions support both httplug + psr18 so not sure if this is required. PSR 18 and 17 createRequest(), createStream() and createUri() factories -> Why do we need this? Delete orphan ResponseInterface reference -> Not sure what you mean exactly Add deprecation notice for request() -> Why? Add deprecation notice for sendRequest() exceptions -> Why?

I want to keep the HttpClient as simple as possible. I've experimented with other methods and more public functions, but when checking the gateways, I've not come across the need to actually get a Request instance to modify it, or to use the Stream/Uri factories. That is why I only expose the request method. There is no direct benefit in using the PSR exceptions directly as far as I can tell, and currently any gateway can just rely on the omnipay-common Exceptions/Client interfaces, without worrying for changes.

[barryvdh]

Alright so there is an issue, because the $body needs to be a StreamInterface in the way I do it, so we actually do need that :( But any library providing the RequestFactory, should probably already support that anyways.

[barryvdh]

See #207 for some of those changes.

[3ynm]

Restore old request() functionality -> Not required, both factories create the same Request object. If you chain it later, you should still get the same result as adding it with parameters directly

OK

Make sendRequest() public -> Why do we need this?

PSR 18 sendRequest is a public (not private) method. It's handy to have it available.

PSR 17 and 18 discoveries, fallback if failed -> The httplug requestfactory is deprecated and the new adapters versions support both httplug + psr18 so not sure if this is required.

I'm pretty sure this way is safer, but no worries about it

PSR 18 and 17 createRequest(), createStream() and createUri() factories -> Why do we need this?

The idea is to provide a more robust HTTP client out of the box with all what a gateway provider might need, and allow the extension of this behavior.

Delete orphan ResponseInterface reference -> Not sure what you mean exactly

It's a use which was not being used

Add deprecation notice for request() -> Why? Add deprecation notice for sendRequest() exceptions -> Why?

It's not expected to break current implementations, but support a more standardized way to build in the library. I would expect to use createRequest directly instead of a wrapper masks the functionality with no benefit to its use. The same with non-standard exceptions.

I want to keep the HttpClient as simple as possible

That's why I introduced those changes and deprecation notices.

I've experimented with other methods and more public functions, but when checking the gateways, I've not come across the need to actually get a Request instance to modify it, or to use the Stream/Uri factories. That is why I only expose the request method.

You can do it that way for sure... but you allow expressiveness and support standardization by exposing the other methods.

There is no direct benefit in using the PSR exceptions directly as far as I can tell, and currently any gateway can just rely on the omnipay-common Exceptions/Client interfaces, without worrying for changes.

Here's the thing, past v/s future. In the past a custom client was needed, because there was no clear standardization of how an HttpClient should behave, now it is. Coming from outside, if you've not used this library and don't know its workings it becomes difficult to grasp what does work as PSRs and what does not, I say everything should work as PSR suggests (as long as it makes sense). Why would you hide the standard PSR methods if you're already using them? A library developer using the PSRs will need access to createStream method, no need to redeclare it.

Adding a defined PSR compatible implementation for this library is breaking 'Dependency injection principle', my idea was to bring the best of both worlds, allowing new PSRs workings for new developments, but also not requiring changes for anything that is already developed.

V4 of this library could arrive at any time, even never, but providing an elegant and simple (because you don't need to learn anything new) http client for future developments is well worth it.

[barryvdh]

But the only people using the Client are the gateway developers so it's not meant for end users. For v4 we could perhaps drop our own HttpClient altogether and just rely on the PSR classes. But this version (v3) was created to be as easy as possible to update from to v2, just by updating the HTTP client. If you want to actually make it clean, the PSR client/factories should be inject in the gateways, but that's a bigger change.

[3ynm]

but that's a bigger change.

it's a new feature, not a breaking change

[3ynm]

... people extends AbstractGateway, sometimes you need to access to those features, or re-implement in your own end

[3ynm]

@barryvdh I don't want to be a pain in the ass neither... no worries if you don't find sense in my recommendations

[barryvdh]

No worries, I like the feedback ;) But I find it hard to find a balance between flexibility, ease of use and purely cleanest. There are hundreds of gateways and I try to avoid breaking as much as possible. And the PSR stuff is all pretty new still. The hard dependency on Guzzle 3 was extremely annoying so that's why we want to use our own interface. Similar to what Symfony does with it's own client interface. I think the current way is okay for gateways ( I don't hear of any problems with the current approach and a lot have been upgraded), so my main focus now is making it as easy for end users to use with the best support for http clients, but also keep the legacy (httplug) to a minimum (as they deprecated it themselves)

[3ynm]

@barryvdh I just wanted you to notice that lazy loading the factories makes much more sense with its functions exposed to the public. I'm not insisting on the issue, but actually you don't need to lazy load if you're just calling 1 function (request) always.