Non-breaking PSR-17 + 18 compatibility in Http\Client #208
Conversation
- 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
|
As I said in #207, I don't think this is required because the signature on both clients/factories are compatible with each other. |
|
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. |
|
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 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 |
|
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. |
|
See #207 for some of those changes. |
OK
PSR 18 sendRequest is a public (not private) method. It's handy to have it available.
I'm pretty sure this way is safer, but no worries about it
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.
It's a
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.
That's why I introduced those changes and deprecation notices.
You can do it that way for sure... but you allow expressiveness and support standardization by exposing the other methods.
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. |
|
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. |
it's a new feature, not a breaking change |
|
... people extends AbstractGateway, sometimes you need to access to those features, or re-implement in your own end |
|
@barryvdh I don't want to be a pain in the ass neither... no worries if you don't find sense in my recommendations |
|
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) |
|
@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. |
On top of #207
@barryvdh Lacks tests for new functionality... though would be great if you tell me what you think about it