Support SagePay v3

[judgej]

This plugin supports v2 at the moment. V3 is not fundamentally different in the protocol, but provides additional features that can be very useful, such as:

• Handling the basket as XML.
• Handling the customer as XML.
• Better surcharge support (on a per-transaction basis).
• More metadata support (various details about customers, the services and products etc.)

There are other additions too. Some notable features have been removed from V3, such as the SagePay Simulator (I find that a very handy feature to get things working, so that's a shame), so I think it would help if both versions are supported.

Because the supported features and data differ, maybe this should be implemented as a new service? We have SagePay_Direct, and SagePay_Server, so maybe SagePay_Difrect_3 and SagePay_Server_3? Not sure though, it feels cumbersome that way. Maybe just a parameter to specify the API version, then have lots of checks against any features you try to use and that are not supported in that version.

[judgej]

Also there are additional messages and callbacks associated with SagePay's PayPal integration.

SagePay Server supports an iframe mode, so the remote form can appear to the end user to be on the application's site. That needs to be turned on with a parameter. It makes sense for iframe support to be flagged and handled by OmniPay/Common, assuming that support is not already there.

[greydnls]

I would opt for having separate gateways for V2 and V3, rather than a API Version parameter. I think it leads to a cleaner implementation, and less confusion.

[judgej]

Yes, that does feel safer.

[robjmills]

:+1: for v3 support. This is now the stable protocol version used by Sagepay.

[judgej]

It has a tonne of additional features and metadata that can be submitted too, but I suspect most of it will be out-of-scope for OmniPay, as the aim is just to pull out the basic features that are common between the various gateways. The stuff that is working for 2.x will need just minor changes for 3.x as it is largely backwards-compatible; the new features are bolted on top of the old.

I might give this a go, depending on spare time (never got enough of that). Need to get my Helcim driver released first, and that requires unit tests.

[robjmills]

Things like the tokenisation support would be great, but I can see they might be out of scope here. That aside, I can see no reason why someone would choose to offer 2.23 (for example) over 3.0

[judgej]

Agreed. Does OmniPay have a standard convention for handling tokens? I am not sure it has. It should be part of the CreditCard IMO, and validation of a card needs to look for the presence of a card OR a token, which may also vary for each action. The Helcim token has two parts - the outer 8 digits of the card, and a UUID token. Other gateways (SagePay?) may just need the token, or perhaps the token and the last four digits of the card.

It would be great to have one agreed interface to cover them all, perhaps get/setToken(), and get/setTokenContext() for the additional card details, unless those card details should not actually be stored (or maybe it can be stored along with the token as one string that can be split by the driver when it needs the two parts).

[bgallagher]

Any update on this?

I've only been looking at SgaePay integration for a couple of days - but it appears that the Omnipay SagePay gateway does not support tokenising the card?

[judgej]

Just a heads-up from SagePay:

January 12th 2015 - we will be disabling older payment systems (v2.0-2.23 Form transactions) on the test gateway July 31st 2015 - support for older payments systems (v2.0-2.23 Form) will be removed

So within a couple of days, you will not be able to test installations of SagePay using this driver. That will make new installations pretty much impossible to develop.

By the middle of the year (2015), all production installations must have been migrated to V3.

I'm happy to start some work on this soon, so let me know if you want to help, or are already doing this. I have some outstanding PRs to submit and stuff to tidy up - been away from this for a good month.

@bgallagher If you want tokenisation, raise it as a separate ticket, It will be easier to track it that way. I am wondering if it needs to be raised against Omnipay-Common to provide some shared support for the tokenisation feature that seems to be increasingly common with many gateways?

[robjmills]

@judgej is VSP Form currently supported by this package? The readme only mentions Direct and Server.

[judgej]

No, I don't believe VSP Form is supported. Some OmniPay packages support form-based authentication (e.g. Stripe) so I would think it is in scope to include it.

[robjmills]

With the upcoming changes with Sagepay I think it would be massively useful to get this package up to full v3 compatibility for Form, Direct and Server asap. I'd be willing to help where I can as it's definitely something i'd use.

[judgej]

Yep, definitely. I think the best way will be to start with the minimum to take it up to 3.x so that it functions as it does now. There will probably be very little change to do that, because 3.x is largely backwards compatible with 2.x

Once that is stable and gives people an easy migration path, starting on new features could be done in a branch. It's not my project, so I don't have the last say in that, but it's what I would recommend.

@kayladnls when you say "separate gateways", presumably that would still be a part of this driver? We could extend this driver to include the new gateways (with the version number in the names), or start an entirely new driver that inherits from this, or put this driver to bed after duplicating it (that may be useful if there is a lot of V2.x dead wood to remove, but I suspect there isn't).

[judgej]

@bgallagher There is an active branch that looks like it is tackling the payment by token. You might want to check it out:

https://github.com/wearecodesquare/omnipay-sagepay/commit/c18764b598511ae32888039e236811d945634092

[greydnls]

@judgej Yes, very much we need to get this driver updated if it will no longer work as of July.

I would create new gateways in this driver, and once those are stable, we can tag a new version of this driver. Likely it'll end up being a new major version, and at that time we can deprecate the old 2.x drivers.

[judgej]

I'm wondering how this is going to be structured. Instead of putting a "3" into all the protocol V3 classes, creating an big confusing list, I think we can do this using a namespace. I just don't know how that fits in with the whole approach to OmniPay.

So the \Omnipay\SagePay\Message class would contain all the messages for protocol 2.23 Then protocol 3.00 can have all its classes in namespace \Omnipay\SagePay3.

The first release of that would have more-or-less a one-to-one copy of the 2.23 message classes (all extending 2.23 classes, with a few tweaks). As time goes on, the protocol 3.00 functionality can be extended with additional features, without having the 2.23 classes intermingled with them. Features include XML baskets, XML customer records and various additional metadata support, PayPal, surcharges, and so on.

A step further would be a completely new 3.00 driver, but even that could still extend the 2.23 driver to avoid duplicating code (have we totally dismissed this idea?)

I've looked through the various other drivers to see if any others have gone through a major protocol change, and I can't see any that have gone through the same thing as a guide.

[greydnls]

Given that the 2.23 version will no longer work after july, I'm thinking that it might be a better idea to update this driver entirely, then. Once that's done, we'll tag a new major version and Omnipay/Sagepay v3.0 will support the V3 API, and older versions will support the V2 API, for anyone who is still using that until July.

That way it will stay in a consistent namespace. It will also light a fire under me to get all of the other drivers tagged with a new version.

Just as an FYI, with the next major version of Omnipay/Common and the other drivers well be moving to the League namespace. I also plan on doing some work to break up the credit card class so we will have a Customer entity, which will help with any drivers that allow E-Check payments, and also tokenized billing.

So, how does that sound @judgej

[judgej]

That sounds like a plan. Two years ago it would have made sense to split them up, but I think this makes more sense now.

Splitting customer and card is a great idea, especially when a card is essentially optional when a token is used. The league namespace change will be a bit of a short-term pain, but things evolve so we'll just run with it.

I've forked the current master to play with protocol 3.00 this week. Whether anyone else is doing the same thing, I'm not sure. I don't want to be duplicating work, or stepping on toes, but it is something I do need. Just shout if you see it heading off in an obscure direction.

[darylldoyle]

@judgej have you made any progress with this? I'd be willing to get my hands dirty and help get this up to V3 in my spare time!

Another thing I've come across when using V3.00 is that there is no simulator available. Meaning test accounts have to be set up to test anything. What do you think the best way of getting around this for unit tests is?

[judgej]

@darylldoyle I've got a branch here:

https://github.com/academe/omnipay-sagepay/tree/protocol3

The changes are not major, as the V3.00 protocol is largely compatible with the V2.34 protocol. I've trying not to introduce any of the new features of V3.00 into this branch, no matter how tempting.

The security ID check done in the callback I believe is different - it introduces a few additional fields into the md5 check. However, the "how to convert to protocol 3.00" docs do not even hint that this is the case, but the SDK does. I've added those fields, but need to do a full round-trip test on the SagePay test instance to make sure that works. Just not had the time yet.

The callback is only used for SagePay Server (AFAIK) and not SagePay Direct. Although many e-commerce sites seem to use SagePay Direct because it is simpler without the callback, I am not sure they are aware of the more stringent PCI requirements. SagePay Server, with the callback FTW in my books :-)

The SagePay Server form can be used in an iframe so users feel they are remaining on the site, and that would make a nice addition to this driver, but it's not in here yet. As an aside, I was talking to a colleague about this today. Knowing how coding works, I would always feel safer being sent offsite to a well-known gateway to enter my credit card details, than I would entering them directly into an online shop. The shop managers seem to think people want the complete opposite, and must not be allowed to leave the shop site under any circumstance. I don't know what the average non-developer really thinks though.

What's the best way to do this? You could fork this branch, or I could just add you as a collaborator.

tl;dr: So far as I am concerned, what is in that branch now should work with v3.00. However, some end-to-end tests need to be run with both gateways and the callback ("notification") functionality.

[judgej]

The tests are a funny one. I've added a method to this driver to get access to the raw HTTP messages, so they can be recorded for the mock messages. The unit tests can tell us only so much, but I feel there is something missing. The integration tests, or round-trip tests are not something that are easy to automate, and there are so many subtleties in the interaction with the real gateway that can throw a spanner in the works, that the unit tests never feel complete to me. For example, one I found the other day: an "OK REPEAT" response returned with an error message. There are no tests that confirm how this should be handled.

[judgej]

If it is any use, I have some additional documentation and network flow diagrams here:

https://github.com/judgej/omnipay-sagepay/tree/patch-1/docs

[judgej]

The real-life callback (notification) handler is non-trivial, which is why it is not something that can be tested quickly. It needs the transaction to be storable so it can be shared between the user session (in the browser) and the callback check. That would normally be a long-term storage such as a database table in production, could be any kind of storage for testing - a file, shared memory, whatever. A CSV file makes sense, I guess.

[darylldoyle]

@judgej Lovely, I'll fork your branch then and see what I can do. I'll also try to find some time tomorrow at work to run some end-to-end tests on what's there.

I must say, I usually lean towards Direct so most of my knowledge is there but I do implement Server from time to time so will help with that where I can/if it's needed.

On another note, Sagepay are adding V3.00 support to the simulator apparently, but I wouldn't bet on it being there any time soon.

[robjmills]

Are there any plans to implement the iframe integration within this package? This is definitely something i'd use but i'm not totally clear how, from an Omnipay perspective, this should be approached. I had a glance at a bunch of other omnipay integrations and none seem to have something similar.

[judgej]

Right now, this thread and change is just about moving what we have now to V3, without any additional functional changes. Supporting the iframe approach would be great, but I think out of scope for this particular issue.

OmniPay pretty much handles the absolute messaging core of the gateways it supports. There is an awful lot more than most people expect to wrap around it - forms, data validation, business logic, saving of the transactions cross-session (between the front end and callbacks, and "silient" posts that may happen days after the initial authorisation request. Personally I think this all needs to be documented first, so people really understand how these gateways really work (I've seen too many implementations with wide-open security issues for comfort to believe that everyone truly believes how these things hang together). iframe forms are one area to improve on, local forms that post direct to the gateway are another - it's all there for the future, but where it will reside, I don't know (i.e. whether it needs more helper functions in the common package to help generate redirects, forms, hidden form items etc. or whether all that front-end stuff can be handled by additional packages.)

So yeah, but not here, right now :-) Do raise it in the groups and maybe raise a separate ticket if nobody else has, to collect ideas. Someone may already have done all this stuff, and just not released it or made it readily available.

[judgej]

I should have a little time this week to work on this. Most of the changes have been made to a branch, and I just need to fix up the documentation and extend a few of the tests. It would be great to see V3.00 as the default protocol.

Edit: got sidetracked, as usual, but still working in it. I am aiming to put my V3 branch into production while identifying and final tweaks. That way I would be more confident that the pull request is not going to break sites.

[simnom]

Just in passing, we've pushed one site it a staging environment and one into production using the Server integration with the v3 protocol. Seems to be running fine in both instances.

Agree with previous comments on this that tagging this as a v3 release makes more sense. Would allow anyone still using the 2.23 protocol to grab that release if they need to (no idea why they would) - maybe that could/should be reflected in the installation details.

[judgej]

Thanks @simnom that's awsome to know.

@kayladnls how do tags like this work? Would it just be information for the end developer, or can it be used on their composer.json as a selection mechanism?

If we wanted to keep v2.23 until it finally goes offline later this year, would we create a branch from master now, label it as v2.23 and leave it there, as an end-of-line branch (with the potential for security fixes if needed)? Then protocol_v3 would be merged into master, and master labelled with the "SagePay-v3.00" label?

We do need to make sure packagist does not get confused, because it does look at the github labels to get the release version of the package (which is not the same as the gateway protocol version). This is something that I guess should be a general rule across all the drivers - protocols change, and sites need to be able to keep up-to-date with the protocol they have coded for, without an advanced one slipping in unexpectedly.

[simnom]

If it's released as a v3 tag with the v2 tags still in place then Packagist shouldn't be a problem, you could just pull in "omnipay/sagepay": "~2.0" or "omnipay/sagepay": "~3.0" as required giving the flexibility to pull in either to a project. Not sure how that would work from a master/branch point of view although a tag is just a pointer to a commit hash and isn't necessarily branch specific.

[judgej]

This relevant question popped up on the composer lists:

https://groups.google.com/d/topic/composer-dev/zZBmTteaQZI/discussion

The suggestion from Jordi there is that once master moves to 3.x then a 2.x branch is made and that sits there for anyone still using it.

So, are we starting with the assumption that the major version of this package (the github release version) will be the same as the major version of the SagePay API? 2.x.y releases of this package would always be for 2.z versions of the API, and 3.x.y releases of this package would be for 3.z versions of the API?

I would propose we stop at just the major version, to leave us with the flexibility to make major/bugfix releases using semver conventions without being locked onto the minor release number of the API. SagePay doesn't use semver, so trying to follow it would be problematic anyway.

How does that sound? It would mean making a branch now, and calling that V2 (just the major version) then merging in branch protocol_v3 and making the next release from master as 3.0.0

[judgej]

Finally managed to test this on a live system. It worked fine with no other changes to the application. Tested (composer.json):

"repositories": [
    {
        "type": "vcs",
        "url": "https://github.com/academe/omnipay-sagepay"
    }
],
"require": {
    "omnipay/sagepay": "dev-protocol3",
    ...
},

[judgej]

I'm happy this is done now. There are plenty of protocol v3.00 features the driver can be extended to make use of, but those are for another day :-)