Add support for the SurchargeXML parameter in server purchase requests

[Kyoushu]

When attempting to pass the SurchargeXML parameter in a server purchase request, I noticed it wasn't making it through to SagePay.

http://www.sagepay.co.uk/support/12/36/protocol-3-00-surcharge-xml

This PR includes the parameter and provides a getter and setter to allow it to be modified after initialize() has been called.

[judgej]

For consistency, the field key should be "surchargeXml" rather than "SurchargeXML". The get/setSurchargeXml methods are fine.

Ultimately it would be great to handle surcharges as data and have the XML constructed in this driver (with a surcharge object). Looking at how surcharges are supported by other gateways would be a good start to see how it could be supported across all the drivers that may have surcharges.

In the meantime, access to the raw XML field like this is great, and doesn't close any doors to normalising it later.

[Kyoushu]

I only named it "SurchargeXML" because of the SagePay documentation. I'll change it to "surchargeXml" and see if it still works.

I seems I also need to learn how to use CodeSniffer.

[judgej]

There is the SagePay field name - which has to use the letter case that SagePay requires - and the OmniPay keys for the data. OmniPay has its own conventions, and its own reserved names for each of these fields. The consistency just helps when writing tests and auto-generating code and data.

So far as I know, surchargeXml does not clash with any other fields on other gateways, so it will be fine to use. If many other gateways support surcharges like this, then we may introduce surcharge or similar at the core level, with a Surcharge object to hold the charges, and then each gateway can convert that to the format it needs (XML in this case).

That's my line of thinking, anyway. The cart/basket already operates in a similar way.

Here is a surcharge object I wrote for another project some time ago: https://github.com/academe/SagePay/blob/master/src/Academe/SagePay/Model/SurchargeAbstract.php which may make it a little clearer. Now forget it, as nothing has been decided :-)

[Kyoushu]

I like that abstraction, it's a much nicer way to work with the data.

As it stands, my latest commit is passing tests, and SagePay is accepting and using the renamed "surchargeXml" parameter in my functional tests.

[jrattue]

Hi @judgej,

As this is passing and adds a handy new feature, can this PR be accepted?

[Kyoushu]

I'm currently having to run production code against a fork of this library. Could somebody let me know if there are any plans to merge this PR?

[judgej]

Good question - thanks for your patience. I'll have a word with @kayladnls and see what we can do. There are just so many things going on at the moment, that it is difficult to keep up. I have forks of many projects running in production myself, and that's fine in the short term, but does just add another task to keep it up-to-date. What did we do in the past before github?

[Kyoushu]

Thanks @judgej. We have quite a few sites using this library, so I expect we'll be implementing more features as SagePay introduces them in the future.

[judgej]

It's a hard one to decide where the line is between what OmniPay offers, and what is an extension. The aim of OmniPay is to normalise all the gateways to the lowest common denominator (sort of), so too many custom features on gateways are going to make it more difficult to normalise those features that may be shared by an increasing number of gateways in the future. On the other hand, some things - such as surcharges like this - are hard to plug into OmniPay as an add-on. It probably needs more hooks, and possibly a container of some sort so you can swap out core classes for your own extended classes more easily. But for now, it makes sense to put these features into the separate gateway drivers, assuming that's the way the project is going.

[Kyoushu]

I agree, it is getting to the point where we need something like an event dispatcher, or a way of attaching lambda functions.

[Kyoushu]

I've created a fork of omnipay/omnipay-common to see how much work it would take to implement symfony/event-dispatcher in gateway classes.

https://github.com/Kyoushu/omnipay-common/commit/b05ba7aa2b28ea27dbd39ff0e269b9c55de95930

It seems to me that the bulk of the work would be adding triggers to the transaction methods (i.e. purchase(), authorize(), refund() etc.)

[Kyoushu]

Opened an enhancement issue in omnipay/common

https://github.com/thephpleague/omnipay-common/issues/68

[judgej]

It has always been an objective to have as few dependencies as possible on specific frameworks (even 3.0 is trying to get rid of Guzzle for any generic PSR-7 messaging library, which does include Guzzle as an option). It's all about portability these days - bear that in mind for what would likely be accepted, but go for it to see where it leads :-) Format event dispatching is new to me, so can't wait to see how it works.

[judgej]

Will try and look at this soon. It's been a long time, I know.

[judgej]

Just trying to get a splurge of development and PRs done this week. A quick question on use: this surchargeXml parameter does not do any interpreting of the parameter - it is just a string that it is given on to Sage Pay as supplied. Is that correct? Helper functions to construct the XML can be added later, but are not necessary at this stage when dealing with what is essentially just a configuration string.

The surcharge XML parameter is supported on the server purchase, but not the server authorize. Sage Pay supports it with PAYMENT, AUTHENTICATE and DEFERRED (OmniPay supports the first and last of these as Purchase and Authorize). I think it would be better to move it to the ServerAuthorizeRequest class.

This parameter is also supported by the Direct API, so moving it to the DirectAuthorizeRequest class would be even better. That way it can be used for Server Authorize+Purchase and Direct Authorize+Purchase.

If those could be done on your branch, I'll commit this PR. Let me know if any of that is not clear (or I'm just being silly). The short version is: move your new functionality from ServerPurchaseRequest to DirectAuthorizeRequest, then the surcharge XML will be usable in ALL the transaction types that support it. The core data is all put together in DirectAuthorizeRequest::getBaseAuthorizeData()

[judgej]

Sorry that took so long to merge. I've expanded the field so it works for all transaction types and both gateways, to be a little more complete.