search

thephpleague / omnipay-sagepay

Sage Pay driver for the Omnipay PHP payment processing library
MIT License
54 stars 78 forks source link

Attempt to create basket parameter to show line items in sagepay panel -... #8

Closed benjam-es closed 8 years ago

benjam-es commented 10 years ago

... requires tax to be implemented on common itembag

Signed-off-by: Ben James in@benjam.es

greydnls commented 9 years ago

Hi, this looks great, and many apologies in the delay on reviewing it. Can you write some tests to cover the new functionality? I'll gladly merge after that.

judgej commented 9 years ago

I notice that $item->getName() does not escape any ":" characters that may be in the name. The colon is a field separator.

Having said that, now we are on protocol v3.00, I would recommend we move straight to the XML basket. That basket format has a lot more flexibility (there are many additional metadata items that can be stored in it, such as shipping details, delivery details, hotel booking details), and does not suffer from field length limits like the old format did. If future versions of OmniPay have support more complete baskets, then this driver will be ready to take advantage of it.

Here is the basket I wrote some time ago on another package, to give you an idea of what it looks like:

https://github.com/academe/SagePay/blob/master/src/Academe/SagePay/Model/BasketAbstract.php

I wonder if the ItemBag of omnipay-common needs a more specific implementation to cater for this additional metadata where required, i.e. basket/cart-level data that is not a cart item? Or maybe each driver can extend the ItemBag to add its own functionality - a merchant site could use that if it wanted to use those features, or just the ItemBag for the lowest-common denominator basket with features that would work across all drivers..

judgej commented 8 years ago

The XML basket, from Sage Pay v3.00, is a lot more flexible and is one now adopted by this driver since Sage Pay v3.00 became the lowest supported version. Being able to support these additional features (e.g. tax at a minimum) is great, and I would love to see it.

However while methods such as $item->getTax() do not exist in the core OmniPay basket item, and there seems to be no way to add custom fields to it, then I'm not sure how this can work.

judgej commented 8 years ago

PR #67 now covers this old-style basket. The newer XML basket was added first, and then it was realised the XML basket is great for the front end (shows items to the end user) but does not integrate with Sage accountancy packages like the old style basket. So PR #67 layered the old style basket over the top of that.

I'll close this PR as no longer needed, but if you spot any problems in the current basket implementation - do shout. Thank you.