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

[3.0] PSR-7 Messages #320

Closed
barryvdh opened this issue Jan 13, 2016 · 20 comments
Closed

[3.0] PSR-7 Messages #320

barryvdh opened this issue Jan 13, 2016 · 20 comments
Labels

Comments

@barryvdh
Copy link
Member

So as discussed in #274, we should probably move to PSR-7 instead of Guzzle request/responses. And probably also instead of the http foundation.

Pros:

  • A PSR standard, so should be stable enough
  • Easily replaceable HTTP clients

Cons

  • Not so pretty interface
  • Lot of work to replace
  • Missing some easy helpers

So what to do? Decorators and factories have been suggested. One solution could be to mimic the old httpclient / httprequest, or at least the most common functionality. Examples:

Decorated ServerRequestInterface
Currently the request is mostly used to get request (post) and query (get) variables. This isn't as pretty in PSR-7 ($request->getQueryParams()[$key]), could be $request->query($key, $default). Same goes for Request and perhaps a referrer/IP from the headers.

Decorated ResponseInterface
The response is pretty basic and also not pretty. $response->json() and $response->xml() could be added back.

HttpClient

  • createRequest($method, $uri, $headers, $body)
    Currently creates a Guzzle Request, with send() method. Could be changed to return a PSR-7 request, which can be sent with the client
    • sendRequest($psr7request), could return a decorated response object.
    • get($url, $headers), post($uri, $headers, $body) etc, could return a PSR-7 request, or could be sent directly (like in v2) and return a decorated response.

These helpers would probably work for 90% of the time, the rest of the time the original PSR message could be used.

@barryvdh barryvdh mentioned this issue Jan 13, 2016
12 tasks
@judgej
Copy link
Member

judgej commented Jan 13, 2016

$response->json() and $response->xml() would certainly cover a lot of of the gateways. Others can provide their own $response->body() decoders (I've seen CSV and name/value pairs used, at least).

@barryvdh
Copy link
Member Author

Hmm, so okay, not really anymore.
I think I'd rather go different path, to more plain PSR-7 support, without decorating too much.

See the PR in thephpleague/omnipay-common#74

Just a createRequest($method, $uri, $headers, $body), send(RequestInterface $request) and request($method, $uri, $headers, $body).

Simplest would be just $client->request('GET', $url), which imho would be simple enough..

Also, instead of decorating the respones, better just a Omnipay\Common\json_decode($request) and xml_decode($request) function.

@barryvdh
Copy link
Member Author

Now leaning more towards http://php-http.readthedocs.org/en/latest/index.html
With default guzzle adapter and the httpcommonmethods.

@judgej
Copy link
Member

judgej commented Jan 20, 2016

What's the word on the street about HTTPPlug? Is there a feeling it may become a PSR? I like the idea.

Whether we decorate/extend PSR-7 messages, or provide help functions to make dealing with vanilla PSR-7 messages a little simpler, I personally have no preference to. The aim, IMO, is to avoid having to make each gateway duplicate the same features (naturally, each in slightly different ways, and with their own issues to work through). So I'm easy on that one.

@greydnls
Copy link
Contributor

I'd actually not heard of HTTPPlug before and I'm not 100% sure how I feel about that. I am a fan of pulling in PSR-7, though. A lot of apps are going that way and it will make us easy and agnostic to use.

I've been using PSR-7 at work for some time with Zend-Diactoros. You're right @barryvdh about less than pretty interface (and very confusing at first, IMO), but I think it's worth using and taking it upon ourselves to make it pretty when/if we need to.

We've already decided to bump the Guzzle version we're using for V3, so we can just go straight to Guzzle 6.

@greydnls greydnls added the V3.0 label Jan 22, 2016
@barryvdh
Copy link
Member Author

barryvdh commented Feb 3, 2016

So what kind of interface do you suggest? Remove the get() and post() helpers, and just createRequest($method, $url, $headers, $body), sendRequest($request) and one that combines the two (request($method, $url, $headers, $body) which sends directly)?

@greydnls
Copy link
Contributor

greydnls commented Feb 9, 2016

What's the benefit of having them separated like that? createRequest, sendRequest and request? Why not a single function that creates and sends?

@barryvdh
Copy link
Member Author

barryvdh commented Feb 9, 2016

That was to allow extra modifications to the request object, but not sure if we actually need that.

@barryvdh
Copy link
Member Author

barryvdh commented Feb 9, 2016

For example, for Authorization headers. You could want to do something like this, if you create a function or use a PSR-7 library:

$request = $client->createRequest('GET', 'http://example.com/purchase');
$request = $this->addOAuthHeaders($request);
$client->sendRequest($request);

@judgej
Copy link
Member

judgej commented Feb 9, 2016

A question on the HTTP client - is OmniPay 3.0 going to be locked to a version of Guzzle, like OmniPay 2.x is? I thought the idea was to be HTTP client agnostic, with PSR-7 messages being a part of enabling that? The fact that Guzzle baked into 2.x now, is holding back lots of people from using OmniPay in some frameworks, so I'm not sure why we would want to go down that route again.

@barryvdh
Copy link
Member Author

barryvdh commented Feb 9, 2016

No, because the gateways rely on our HTTP interface and the psr objects. We can create a Guzzle7 or some other lib, as long as it uses PSR7. We just supply a default client with zero configuration, so we could change it later on.

@judgej
Copy link
Member

judgej commented Feb 9, 2016

Not sure I understand which bit is "no". Is Guzzle just going to be a suggest library that OmniPay 3.0 can use out of the box, and not a composer require? If it is required, then it is more than a default HTTP client - it locks the whole of OmniPay to Guzzle, and with the rate Guzzle and frameworks are changing, that causes all sorts of conflicts. Or maybe I'm misunderstanding?

@barryvdh
Copy link
Member Author

barryvdh commented Feb 9, 2016

We require guzzle, but its not a public api, so we can change it to a new/other library.

@judgej
Copy link
Member

judgej commented Feb 9, 2016

But somebody using OmniPay can't change it? That's my point. If Guzzle of a certain version is required, then that sets Guzzle for the whole environment that OmniPay is used in. That throws away the main advantage of PSR-7 in that no specific HTTP client or version should be required. ANY PSR-7 client could be used.

@barryvdh
Copy link
Member Author

barryvdh commented Feb 9, 2016

So what do you suggest, no default client?

@judgej
Copy link
Member

judgej commented Feb 9, 2016

An OmniPay client with an adapter for Guzzle.

@barryvdh
Copy link
Member Author

barryvdh commented Feb 9, 2016

That could make it more difficult to install. People need to require an extra package for the client, instantiate their own client and pass it on to Omnipay etc.

@judgej
Copy link
Member

judgej commented Feb 9, 2016

Yes sure, if they want to use their own client. If they don't pass in their own client, then OmniPay can check if Guzzle is installed, then use its own if it is.

@barryvdh
Copy link
Member Author

I kind of understand you're issue. But currently Guzzle is by far the most used library for this.

We should avoid making a hard dependecy out of Guzzle, but I don't think we should leave this completely open. If Guzzle upgrades, it would be reasonably to suspect that they will follow the PSR-7 standard still, so we can update the library.

The only problem would be people who instantiate our Guzzle Http client on their own.

So options are:

  1. Provide a black box, without any options and means to change Guzzle, so we can swap Guzzle out for v7 or something else. (== not flexible, but good so people don't rely on some specific function, but could lead to trouble with proxies or exotics setups)
  2. Don't create a default HTTP adapter and don't include Guzzle, let the developer require Guzzle (or something else in the future?).
  3. Let Omnipay create the default HTTP driver and note in the docs that for using a custom driver requires developers to require Guzzle themselves (eg. we can remove it). When Guzzle 6 becomes a problem, we can either remove Guzzle for something else of allow both v6 an v7.

Option 3 is what we sort of do now (create a default client with zero config), if we just make that clear in the docs, I think it shouldn't really be a problem imho.

@barryvdh
Copy link
Member Author

Implemented

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

No branches or pull requests

3 participants