omnipay
omnipay copied to clipboard
thephpleague
[3.0] Extend ItemBag/Items to support more gateway features
The OmniPay\Common\Item class holds a single item in the basket. It has a limited number of properties that can be set, and that puts some of the gateways to shame.
For example, the Item does not allow tax to be set, so there is no tax breakdown of items in the basket. The Sage Pay basket supports pages of metadata that can be sent in the basket, and that can be incredibly useful when reconciling payments with other systems.
I'll do some research and see how other systems do it. I think a list of the common features would be good, but expanded a bit beyond what is supported at the moment. Expanding the field list a little now would help standardise these additional field names (e.g. netPrice, grossPrice, tax, taxRate, with perhaps intellegent derivations of some of these fields if only some are provided).
After that, a means to push in arbitrary fields that may just be needed for one specific gateway or application, would be great. A means for the gateway to "declare" what these additional fields are, would help a lot, but I'm not sure what form that would take.
On that note of derived fields, e.g. only netPrice and tax being supplied, then the gateway asking for price or grossPrice, then that would be a lot safer if a currency or money class were used for these. At the moment, OmniPay does NO money calculations, so rounding errors are not something that needs to be dealt with (on the whole). For now, at least.
Too true. There is an example of it somewhere, but that's hard to find. IIRC the example just passes the items in as a nested array, and the core uses that to initialise the Item and ItemBag objects, so it's not clear you can create your own ItemBag or Items.
I'm not 100% sold on the idea of getting Omnipay to do money calculations. It's purpose is to be a payment gateway abstraction, not necessarily to handle all basket/shopping cart functionality.
I worry that we'd be opening the door to start supporting coupon/discount/etc etc etc and I don't think that's the road we'd like to go down.
That said, the Item class we currently have does allow for custom parameters to be get/set for use, if necessary.
From what I can see, you cannot add custom fields to the Item object. It can be initialised using an array, but only values in that array for which there is a an explicit setter will be accepted, and all others discarded.
Like it or not, calculations on the basket amounts are done now. The Item accepts a "price" for an item, and a quantity. The total for that line (which some gateways need) must be calculated. Some baskets also need a total line, so that total needs to be calculated too.
This isn't anything to do with cart functionality as needed by the shop, but is purely about being able to provide metadata to the gateway to capture the details of the transaction. Many customers need this for reconciling with their accounts systems later, and some benefit from the additional metadata that some gateways are able to accept in their basket data.
I think this can be solved as simply as this: https://github.com/thephpleague/omnipay-common/pull/80/files#diff-a39894a7b3ba84e701a49cf16186cc4fR21
By making setParameter a public method it gives more flexibility for additional meta data to be set.
Yes, I did notice last night your changes allow any parameter name to be set, even if there are no setters for it. The public parameter setter is a great move. I suspect it was never done like this in the past because it could be abused - gateways adding any old parameters they like without checking what the "standard" parameters should be first. So this can lead to a consistency problem across the drivers if we aren't careful, but hopefully the common tests will pick up many of these.
I think better documentation for what's available can help alleviate that. Beyond that, I feel like the onus is on the user to understand the tool they're using. If we provide reliable methods to assign standard things and they abuse setParameter instead the fault isn't on Omnipay.
If the gateway authors abuse it, then OmniPay will start to get a reputation for being unreliable. That was my point. The very end users either put in the right data and it works, or the wrong data and it doesn't - at least that's what we hope for.
Can the Item accept any custom parameter? That leaves flexibility open for gateways to use additional optional data of they want to. It would be good if there were a way for a gateway to register what additional data an item can accept, much like any gateway can support any custom parameter it likes by extending the Message class with getters and setters. But I'm not sure how that could be done, since the Item would need to be somehow instantiated in the context of the gateway driver.
As long as the parameters are strings only, I am totally with you. One of the current problems is that there are gateway drivers that expect objects as parameters and this will fail in environments like ours.
Hitting this issue with ONEPAY - it seems like basket items are mandatory (at least one item is needed). Each item must also have a stock ID and a VAT property. I can extend the Item class just for this gateway, with a fallback setting the stock ID to a sequential number and the VAT to zero if not using the extended ItemBag.
We're also hitting this problem with SagePay requiring a SKU code in order for automatic reconciliation in Sage. At the moment there doesn't seem to be a way at all for the site to supply this information to the Item for the SagePay driver to process. Have any conclusions been reached on this front?
