search

thephpleague / omnipay

A framework agnostic, multi-gateway payment processing library for PHP 5.6+
http://omnipay.thephpleague.com/
MIT License
5.95k stars 928 forks source link

[3.0] Roadmap #274

Closed barryvdh closed 8 years ago

barryvdh commented 9 years ago

Sometimes I see Omnipay V3 being mentioned in the comments, so perhaps best to keep track of it somewhere. So what are the actual changes?

API cleanup:

Timeline: End 2015?

But as it's probably a big work to update all gateways also, we should probably only update if needed.

Imho, the most interesting part to upgrade would be PSR-7, because it would reduce dependancy with Symfony/Guzzle, as we could use that to: A. construct the request to gateways. B. construct the request back to the developer.

cfreear commented 9 years ago

:+1: for 5.5 and psr-7 so that devs can choose their own (psr-7 compatible) http clients.

Further feedback on the 5.5 thing would be beneficial to make sure there isn't an issue for some consumers and their hosts not providing 5.5...

EDIT: API cleanup: I think renaming transactionId and transactionReference would be a good idea to reduce confusion as to what they each represent.

greydnls commented 9 years ago

All that you listed is correct. I want to decouple our dependance on Guzzle and Symfony and PSR-7 would be a good way to do that.

Also, adding better support for Bank Accounts, which requires breaking up the Customer object from the Card object.

Better documentation for each of the gateways. I figure as we update gateways, we can write documents as we go so that they each have better documentation.

judgej commented 9 years ago

Was there mention of a vendor name change for composer? I kind of like omnipay and don't see any advantage of changing that. But I may have just misheard or misunderstood. Maybe it was the namespace root name? Can you put me out of my confused misery? ;-)

greydnls commented 9 years ago

@judgej all League packages have the league namespace. This will be part of V3.

FQNS would be: League/Omnipay/GatewayName/ClassName

judgej commented 9 years ago

The cart - can we do something with that to allow it to be extended when needed? At the moment it is kind of embedded in middle of omnipay-common and you cannot get to it before it is instantiated and passed to the gateway driver.

Use-case is that for some gateways I would like to be able to extend the parameters that the gateway supports, for a richer set of basket details for gateways that support it. This can assist the service owner with audit trails and reconciling accounts further down the line.

In fact, I wonder if this is a wider issue? Supposing OmniPay used whatever DI container was available for its own internal services. Could that be used as a way to rewire the internals of OmniPay where-ever the need arose, perhaps to put in an extended cart in place of the default one? DI containers may be a step too far for V3, or maybe not?

judgej commented 9 years ago

Thanks - writing up the "Version 3" section for my OmniPay talk for PHPNE on Tuesday, so that's useful to know.

greydnls commented 9 years ago

Is it being recorded? I'd love to see!

judgej commented 9 years ago

It isn't normally, but I'll ask if anyone would like to do the camera work. I'll put all the slides and notes up on github anyway.

judgej commented 9 years ago

@cfreear I personally thing transactionId and transactionReference are fine. I do realise there is at least one gateway that uses these exact same names for opposite purposes, but I suspect that will happen somewhere along the line whatever names are chosen. That is also mainly an issue for the gateway driver developer, and should not concern the users of the drivers too much.

I think this is a documentation issue. The difference between those two fields should be explained in a big orange callout somewhere near the top of the documentation. That's my 2p.

judgej commented 9 years ago

Sorry not trying to hijack this thread - last post for today ;-) This is something for V3 that has been mentioned a few times:

I would say there may be a few more classes in there: we have CC details, customer personal details and address, billing address, shipping address, potentially emails for all of those (most gateways distinguish between billing/shipping/customer emails). So maybe CreditCard, Customer, PostalAddress.

maxnet commented 9 years ago

Talking about transactionReference. Think that when receiving a RedirectResponse getting and saving the transactionReference to pass it to CompletePurchaseRequest later on should be made a hard requirement.

Right now some modules fallback to insecure behavior that gets the transactionReference from a GET parameter that is normally set by the gateway upon redirecting the customer back to the website, but which the customer can modify (and make it point to a different previous transaction that did succeed, while cancelling the current one).

judgej commented 9 years ago

@maxnet Ouch. There are too many examples of gateway plugins that do that type of thing in other proejects, often in the notify handler just returning "yeah, whatever" while not actually checking the security hashes or matching them against the stored transaction.

I think OmniPay needs a standard storage interface that perhaps drivers could use. That storage would be indexed by the transactionId (plus some other values for multiple captures against payments), the gateway status and transaction state (i.e. overall status of where it is at), plus custom fields for the application use (e.g. I store all message data in all directions, which is invaluable for tracing issues later). I started a package that could do this, but kind of got stuck (probably because I was thinking along the lines of a bolt-on, and not something that could be required by a driver, enforcing its use and so making sense having a contract define in OmniPay Core) and needed to move on.

The idea would be similar to PSR-7 - "I want to send and receive HTTP messages, so give me the tools to do so". In this case it would be the tools to store, retrieve and amend transactions in non-volatile storage. Is that along the kind of lines you were thinking?

barryvdh commented 9 years ago

Yeah I agree, part of the problem is that is just isn't really clear how a gateway is supposed to behave. Partially also because of differences in the gateways.

judgej commented 9 years ago

Should we be taking these suggestions off to separate issues, and give them a "3.0 Ideas" label?

I think this storage one is a great idea. Having the right interface available for drivers that do want to use it, could work really work well. Not all drivers would need it, as they don't involve access across multiple sessions, or involve the user's browser at all (i.e. direct APIs) but those that do, really should use the storage to keep checking things have not been changed.

Providing an implementation of the storage interface would also take away a lot of the guesswork/learning curve that people have to go through when integration some gateways.Ready-made adapters, e.g. for eloquent, would soon be a thing.

barryvdh commented 9 years ago

Or extend the interface, eg. to verify a complete Purchase, you would need to provide the original reference as parameter.

maxnet commented 9 years ago

The idea would be similar to PSR-7 - "I want to send and receive HTTP messages, so give me the tools to do so". In this case it would be the tools to store, retrieve and amend transactions in non-volatile storage. Is that along the kind of lines you were thinking?

Yes, that would do too.

Having a database, might also be useful to support gateways that require you to poll transaction status periodically. (with ING iDeal Advanced they send you an e-mail telling you off, if you did create a transaction, but your software never polled its status afterwards. Can happen if customer does not click link to get redirected back to site. They do not do notifications).

maxnet commented 9 years ago

Or extend the interface, eg. to verify a complete Purchase, you would need to provide the original reference as parameter.

That's currently already possible (see Mollie), but providing the reference should be made a hard requirement IMO, instead of security being optional and undocumented.

judgej commented 9 years ago

Or extend the interface, eg. to verify a complete Purchase, you would need to provide the original reference as parameter.

Some already do this, such as SagePay's CompleteAuthorize::send(). However, you can completely skip that stage and return a SUCCESS regardless of whether the TransactionReference (containing the hashes) match what has been sent in the notification. Someone not wanting to store the transaction may be tempted to do that. They may also just do exit("OK\r\nRedirectUrl=http://example.com/paid-successfully"); in the notify handler and skip OmniPay totally, but we can't help them ;-) What we want to do, is make it easier to do it the right way to encourage it.

barryvdh commented 9 years ago

Making it a hard requirement would make it obvious it needs to be stored somewhere (and should be noted that session doesn't suffice) but perhaps we should investigate to see if there are gateways wo don't support it. Perhaps all just support it and it wouldn't be a problem

Storage driver could also work, but not sure if that makes it easy enough when people store custom information or account for gateway differences.

maxnet commented 9 years ago

Storage driver could also work, but not sure if that makes it easy enough when people store custom information or account for gateway differences.

Just stick all the custom information in a TEXT field as json?

judgej commented 9 years ago

@maxnet yes, that's what I do. Usually I have about five or six columns exposed as indexed/searchable columns - transactionId, amount, overallStatus, gatewayStatus, errorMessage and a few others, and then everything else goes into a TEXT column. ORMs like eloquent can silently handle all the JSON encoding and decoding so you don't even know it's there, and don't need to know how the storage is structured. An adapter between a flexible enough interface and storage can handle all the necessary mapping, and maybe the JSON encoding can be done there depending on the framework it is in. The overallStatus tracks what happens to the transaction as far as the application is concerned - did it register, is it waiting for a notification, has it been aborted, did it get rejected, is it complete with a successul payment etc. The driver statuses do not generally reflect this in a consistent way. For example, SagePay returns "OK" when a transaction has been successfully registered, and it returns "OK" when a payment has been successfully authorised. That can be kind of confusing, because that same status means two different things, while an overallStatus provides some context to that.

It is something I've wanted to see anyway for a while, just to take away the laborious task of having to decide what to store each time on different gateways, and how to store it on different frameworks - there is nothing out there to just pick up and run with.

judgej commented 9 years ago

I think the storage needs for all the drivers can be pretty much standardised. There are only a few pieces of information necessary for the functionality (hashes to check, authorisation status to pass out from a notification handler), and the rest is just custom fields. I've seen gateway plugins that tell the application whether the payment was authorized solely by sending the user to a success or fail URL from the notification handler, avoiding having to store this detail. Makes the hair on the back of neck stand up. shiver.

Also some gateways allow the user to change the amount both on the way to get authorized, and on the way back with the result - and they don't include the amount in the hash with the result. double shiver. Unless the gateway driver checks the amount actually authorised is what it expected to be authorised, then there is trouble in store there. That's why the amount in the transaction is an important column.

judgej commented 9 years ago

Any thoughts about using a value object for the monetary values? That object could encapsulate all the necessary validation, formatting, would have a defined currency etc. For backwards compatibility, the old methods could remain, but would just feed into the money value object. Whether it would be built into OmniPay, or provided by a third-party package, or offered as an interface plus an adapter for a recommended third party package(s) with the option to drop others in, I'm not sure.

Personally, I've had too many hairy dealings with the amount values being passed in to remain comfortable with it remaining as a string.

A money package can know a lot more about a currency then we would want to get involved in. As well as the number of decimal digits, it would know what fractional units are allowed, whether a particular currency should be rounded up or down, or rounded to the nearest odd or even value.

greydnls commented 9 years ago

@judgej If we're going to use anything for Money, I'd prefer to use this: https://github.com/mathiasverraes/money Thoughts?

lord2800 commented 9 years ago

I'm personally not a fan of Omnipay making storage decisions (re: transaction ids/refs/messages) for me--any reason not to depend on an interface and then have a few well-known implementations?

judgej commented 9 years ago

That seems to be the one that all money roads lead to. The next release is a complete rewrite, and looks much lighter: https://github.com/mathiasverraes/money/tree/nextrelease

That library does store money amounts in the currency's lowest denominator (e.g. pennies and cents) which is what OmnIPay used to do before it was changed to strings/floats. However, using floats in OmniPay has not been an issue because we don't do calculations on the amounts. But in building some baskets, calculations need to be done to provide the totals lines (e.g. SagePay non-XML basket), so maybe not using floats is not such a good idea.

ivank commented 9 years ago

I can't wait for the guzzle / psr7 in this update! Guys have you considered https://github.com/sebastianbergmann/money for money representation? (That's the guy who made phpunit) It looks a bit more "stable" and thought out.

judgej commented 9 years ago

@ivank This money package has some nice practical features for converting between different formats and units. I do like it.

I think, so long as Omnipay provides its own interface for what it expects of a Money object, then simple adapters can be written for any money package. This is similar to the PSR-7 approach - rather than land on Guzzle for communications and lock the package into Guzzle, just accepting a PSR-7 interface is enough for the end developer to use any HTTP communications package they like,

barryvdh commented 8 years ago

So, a concrete problem arises:

So we can't use Omnipay with Symfony 3..\

Upgrading to Guzzle 6 will require that all gateways are converted to use PSR-7 messages, which is gonna take some time..

judgej commented 8 years ago

I assume once PSR-7 messages are being used, then any PSR-7 client can be used to do the comms, perhaps with an appropriate connector? I've been trying out that approach on a Particle (IoT) API to see how practical it is and what I can learn from it, and it seems to be possible. That's not to say it will not be a lot of work. And that's assuming I understand just what the aims of moving to PSR-7 really is about.

barryvdh commented 8 years ago

That is the idea, we don't rely on Guzzle message interfaces, but PSR-7 Message interfaces. We also don't really need the http foundation responses, but can return PSR-7 responses.

Once Guzzle6 updates to Guzzle7, we should be able to migrate without replacing all gateways, if they respect the interface. (Or to any other PSR-7 compatible http client)

judgej commented 8 years ago

What I've found is that there is some handy stuff that Guzzle does with the messages in the background, such as adding headers automatically. That is low-level stuff that needs to be handled by the application using the PSR-7 messages. I don't think pushing that to all the gateways is the way to go, as it spreads too much HTTP details around the drivers. A central OmniPay-PSR7 implementation is probably the way to go, offering the messages (maybe as connectors to a strict PSR-7 implementation) but adding a layer on top (is that a decorator?) to implement the stuff that PSR-7 does not do, but which we know in practice all the gateways will need.

I'm probably not explaining myself very well, but hopefully it makes sense :-)

barryvdh commented 8 years ago

You mean for creating a Request/Response object?

Perhaps so, we have to create those objects somewhere anyways, and if we use guzzle/psr7, we're still tied to Guzzle. Not sure if that should be a decorator or a factory or something else.

// Factory
$request = Omnipay\RequestFactory::get('http://my-domain.com', ['a' => 123]);
$request = Omnipay\RequestFactory::post('http://my-domain.com', $headers, $body);

$response = Omnipay\ResponseFactory::string($body);
$response = Omnipay\ResponseFactory::redirect($url);

Should we open a new ticket for this and evaluate some options?

makasim commented 8 years ago

In payum I introduced a http client interface and guzzle as default implementation.

There is tiny factory to build an http client faster

judgej commented 8 years ago

Yes, that's right. guzzle/psr7 Request and Response classes seem to be a pretty "pure" implementation of PSR-7, with little that goes beyond the PSR. I expect they could be used pretty much directory, so an OmniPay Request object could probably extend.a Guzzle PSR-7 Request object and be used pretty much directly.

Also not sure the best way - factory, connector, decorator or a mix of them all. It's knowing where to draw the lines that is unclear without some examples. For example, headers will be needed for some GET requests, because the headers can carry authentication tokens. Now, whether the gateways should be dealing with headers directly, or passing "Bearer" tokens to OmniPay to put the headers in, I really don't know.

It has lots of helpers for creating streams, so you wouldn't use those without a connector of some sort if you don't want to be tied in.

judgej commented 8 years ago

I like the general idea of keeping the generation of the PSR-7 messages separate from the sending and receiving of PSR-7 responses though. It feels right that building data, and then using it over HTTP, are two very different things that should be kept apart.

judgej commented 8 years ago

Bridge, Connector - I feel like I have to go back to school to relearn the subtleties between them and what they do...

delatbabel commented 8 years ago

As long as we can pass arbitrary headers in, and in addition cater for gateways (First Data Webservices) that require server certificate + client key + password for establishment of the SSL connection. For the current gateway I had to drop out of guzzle and do it directly in cURL.

barryvdh commented 8 years ago

There is also a pretty popular implementation here: https://github.com/zendframework/zend-diactoros

Guzzle psr-7 Request: __construct($method, $uri, array $headers = [], $body = null,$protocolVersion = '1.1' ) Diactoros Request: __construct($uri = null, $method = null, $body = 'php://temp', array $headers = [])

Which could be just a function in the AbstractRequest, right? Or possibly abstract it to a request factory, but that's probably overkill. As long as people rely on the PSR interface, not the actual implementation, it should be easy to swap out.

public function createRequest($method, $uri, $headers = [], $body = null) {
    return new \GuzzleHttp\Psr7\Request($method, $uri, $headers, $body);
    return new \Zend\Diactoros\Request($uri, $method, $body, $headers);
    return new RequestFactory::createRequest($method, $uri, $headers, $body);
}```
judgej commented 8 years ago

Yes. I personally think PSR-7 is too low level for the gateways - it puts too much burden on the developer to understand HTTP to a low level (e.g. PSR-7 knows absolutely nothing about how to format the body of a POST request to send data - we don't want 30 implementations of that done by 30 different developers). How I would see it, is that OmniPay would provide an implementation, or a series of implementations that implement an interface, which does more-or-less what Guzzle or Diactoros does. Behind the scenes they would pass as much as they can to Guzzle or whatever, and they would offer some additional helpers such as addBearerToken, addPostParameters etc.

So I think OmniPay should rely on the PSR-7 interface only, when dealing with the messages created by Guzzle and friends, but provide more than that (extended PSR-7 messages) to the gateway drivers. A gateway driver could deal with PSR-7 messages if it wants, but PSR-7+OmniPay features would be much nicer to deal with.

That is just mu conclusion having played with PSR-7 messages over the last few weeks - it is so low level, that you really need more if you are not going to get bogged down in HTTP intricacies.

@delatbabel if we are extending PSR-7 messages, then the ability to pass in and manipulate any header, URL parameter, etc. will be built right in. Use it if you need to, use the higher-level OmniPay handy methods if not (which hopefully will be the majority of cases).

judgej commented 8 years ago

Taking this as an example that method generates PSR-7 messages to send to an Arduino device. These are plain GET, PUT, DELETE messages, some involving uploading files, and also involving several different types of authentication (OAuth and device tokens). That's all standard stuff - nothing special - but look at all those headers and stuff that needs to be handled in there. That is the kind of thing that dealing with pure PSR-7 messages will force you to do. That is why this stuff, IMO, should be offered by OmniPay as decorated/extended PSR-7 messages so all that stuff can be wrapped up out of the way.

Passing a PSR-7 request message to a HTTP client and sending it - that's the easy bit. Creating the PSR-7 request in the first place is the hard part that needs more help.

barryvdh commented 8 years ago

We'll have to check for common headers/patterns, but to be honest, it shouldn't be too hard for gateway developers to configure a couple of headers.. And gateways might have slight differences in implementations anyways..

Another thing, should we aim for an easy upgrade path, or don't care? Should we rename the package to league/omnipay, dump the omnipay-common package and just put the Common stuff in the League\Omnipay package? I don't really see the use for the global 'require all gateways' package..

barryvdh commented 8 years ago

Let's continue the PSR/Request stuff in #320

barryvdh commented 8 years ago

Shall we also simplify the getting of the Gateway, since we now have the ::class operator?

Instead of Omnipay::create('Stripe') and converting/matching the gateway name, just:

use Omnipay\Stripe\StripeGateway;
Omnipay::create(StripeGateway::class);

And also remove the stuff about finding/listing all gateways from composer etc.

aimeos commented 8 years ago

No, that would prevent creating gateways by configuration!

barryvdh commented 8 years ago

Why? Omnipay::create('Omnipay\Stripe\StripeGateway'); would also work, so you can store that instead of Stripe

aimeos commented 8 years ago

This makes it more difficult for users to get it right. We have experienced a lot of users failing on such small things which only increases the amount of support requests. Please always try to keep things as simple as possible.

barryvdh commented 8 years ago

Using the class name = the simplest. Your IDE can autocomplete it and you don't have to guess. Right now it uses some automatic transformation to move to the Omnipay namespace. They should also just use any namespace they want.

aimeos commented 8 years ago

Think about the users, not only the developers. In Aimeos shop owners can configure the Omnipay gateways using some settings like the gateways name (currently e.g. "Stripe"). They are no developers and know nothing about namespaces and maybe haven't seen an IDE in their entire life.

For the developer point of view (mine) it doesn't make things easier either. The factory Omnipay:create() should abstract from the concrete instantiation and shouldn't bother me with implementation details. The concrete class name is such an implementation detail which can change, the string "Stripe" should not.

Implementation of the factory would be easier for sure if the concrete class name is given but than you don't need the factory either ...

barryvdh commented 8 years ago

Omnipay is meant for developers, not for end-users. If you want simple names, just map them in your back-end to the real names. Stripe is just as much imlementation detail, it can only map to Omnipay\Stripe\Gateway, so can't change either. The point of the factory is (in my opininion) to inject the proper dependancies.