Consider making Omnipay do more of the work

[eileenmcnaughton]

trafficstars

The advantage of Omnipay is it's possible to re-use the same code across multiple processors. However, Sagepay makes that quite tricky because is supports multiple methods and requires the integrator to have a pretty full understanding of them.

I'm wondering if Omnipay could respond more similarly to other processors by taking a queue from the context.

For example after calling

$gateway->purchase();

there is no getToken function to retrieve the json string that I could use to call repeatPurchase. However, it would be fairly easy for this function to be added & for it to return a json string that could be stored & later passed to repeatPurchase as a 'token' Further the purchase'function could actually call repeatPurchase if it is called with a token consistent with what repeatPurchase requires. Doing this would make it possible to use the same code for sagePay without understanding then inner workings (it would still be necessarly to know the initial call uses purchase`` rather than createCard'``` with ['action' => 'purchase'] as we are currently doing for other gateways but it would be close to compatible and that could be addressed too.)

I can try to work up a patch if you are open to it

[eileenmcnaughton]

Also another gotcha is that passing in an empty 'card' causes it to fail so

if ($card)

could check if any params are actually set....

[judgej]

Yes, that makes a lot of sense.

So:

• The JSON transactionReference it also returned by getToken().
• The purchase() method calls repeatPurchase() (similarly for authorize) if the token is set.

Is that the gist of it?

There are also some unusual flags to "set token" and "store token" that need to be used in the request if yout want to be able to reuse a payment for repeat payments. I think "set token" allows a token that has been previously stored to be used, and "store token" sets a token for reuse in the current transaction. You need to kind of juggle the two, but setting "store token" to yes as a default has some security implications, in that every single transaction effectively becomes a repeatable transaction, which you don't really want unless the user has excplicitely agreed to it.

I'm a little rusty, so need to reed up on what the latest is, expecially since SagePay was purchased, and who knows what direction it will be going. I get the impression there is a tonne of technical debt in this gateway, and it will either have the tiniest of incremental changes, or will be replaced completely with a new system.

[eileenmcnaughton]

@judgej yep - that's pretty much the gist of it - I'll take a look since you seem open to it - I did put up #158 which is a bit tangential but part of the same thing

[judgej]

You caught that heavy-lifting word "assumption" on #158 ;-)

[eileenmcnaughton]

@judgej do you have any thoughts about the travis issues on #158 ?

[judgej]

Just looking at that, and much of it (as is often the case with travis) is above my head and I just end up poking it and seeing what happens. This looks like it could be worth trying: https://github.com/sebastianbergmann/php-code-coverage/issues/834#issuecomment-736337016

[judgej]

Also Barry is using php4snapshot here, which may help: https://github.com/thephpleague/omnipay-payfast/blob/master/.travis.yml However, I think the problem is only in December 2020's PHP releases, so it could be other drivers have not actually hit this problem yet, but will.

[eileenmcnaughton]

I looked at making it call repeatPurchase rather than purchase if cardReference is set but it seemed to work better to simply make purchase & authorize adapt their behaviour if it is a repeat. The code that figures that out is actually lower in the chain that they extend so it was pretty available to them. On the other hand repeatPurchase & repeatAuthorize don't share methods the others use so I felt that deprecating rather than changing them would work.

Also the task of determining which to call would need to be done in the gateway class - and it would not pick up . be able to respond if the setTransactionReference function were called after the initial construct.

[eileenmcnaughton]

I also opened this to raise the way 'confirm' is handled https://github.com/thephpleague/omnipay-common/issues/245

[eileenmcnaughton]

@judgej I'll look at the remaining bits & pieces in a couple of weeks if you get a chance to check out the PRs I've opened so far

[judgej]

Okay, thanks. I'll try to find some time this weekend. Just been unfeasibly busy this week.

[eileenmcnaughton]

@judgej fair enough ! (I'll be AFK from 19 to 27 Jan myself)