Zero value line item in ItemBag

[lukeholder]

Paypal does not accept zero value line items in the item bag:

See item (4) here:

https://developer.paypal.com/docs/classic/express-checkout/integration-guide/ECCustomizing/#setting-order-details-on-the-paypal-review-page

Should it be the responsibility of this package to remove zero value items, or the application adding things to the paypal item bag?

[judgej]

Personally I would make it the responsibility of the gateway driver. The application should just fill the cart and not have to worry about peculiarities of the gateway.

It is similar to IP addresses - many gateways will only accept an IPv4 IP address being passed to it for logging. Many sites now run IPv6. So the gateway driver, if it only accepts IPv4, just needs to silently ignore what it has been passed if it does not meet the required format. Again, the application has what it has, and should not need to adapt the data differently to each gateway.

All in theory, at least.

[delatbabel]

On the other hand, the ItemBag functionality is pretty much core Omnipay. I don't see it as the responsibility of each driver to over-ride core functionality if it's not absolutely essential so actually I see this as the application responsibility.

The best thing to do would probably be to add a note under the Quirks section of the class docblocks for the gateway classes to indicate that PayPal does not support zero value items in the itembag.

[lukeholder]

I understand, but this driver extends the itembag class so in theory it should be possible to put the gateway specific logic in that class right?

[judgej]

It depends on how you look at it. The core OmniPay is handing the itembag over to the gateway driver and saying, "do what YOU want to do with it". There is no overriding going on here. If the application wants to put zero-value items in, and the driver wants to ignore those items, then that is entirely consistent with the way OmniPay works IMO.

Are we really talking about overriding the itembag functionality for a specific driver, or is this just about how the driver formats the basket for the gateway? Want to make sure we aren't talking cross-purposes.

[barryvdh]

I think you can put it in the ItemBag, but when the gateway processes it for PayPal, it should remove the zero items.

[lukeholder]

My thoughts were that one would do:

$itemBag->add([
'quantity' => 0,
//...
]);

And the PayPal item bag class would have a overridden add function that ignores zero value items.

[barryvdh]

Why not here? https://github.com/thephpleague/omnipay-paypal/blob/ec7fc864ae0699dd832cd727da26286a5354a35e/src/Message/RestAuthorizeRequest.php#L242

 foreach ($items as $n => $item) {
    if ($item->getQuantity() > 0) {
        $itemList[] = array(
            'name' => $item->getName(),
            'description' => $item->getDescription(),
            'quantity' => $item->getQuantity(),
            'price' => $this->formatCurrency($item->getPrice()),
            'currency' => $this->getCurrency()
        );
    }
}

[lukeholder]

That's makes more sense, yes. Should I make a pull request with this feature?

Also there are some other validation rules for itembag; like the payment amount not being smaller than the item total. I assume the same principle that the gateway drivers has the responsibility to clean up the data and raise validation errors before trying to send the request to the gateway?

[leith]

This issue is about zero-value items - while this could be zero quantity, I feel it's probably more likely to be zero price (you have 1 quantity of a zero-price item), so if you're going to add a check somewhere, it's liable to be something like $item->getQuantity() * $item->getPrice() > 0 (or separate comparisons).

The suggestion from @barryvdh only applies to the Rest gateway, not the Express gateway (and it's the express gateway documentation this issue is looking at). @lukeholder's suggestion makes more sense to me, since the PayPal driver is already overriding the ItemBag class, specifically to override the ItemBag::add() method.

The only thing about that is it does make it trickier to audit. Would it be better to leave them in the PayPalItemBag, but have an additional flag on the PayPalItem to indicate it shouldn't be submitted to the gateway? Or is there a way to add some kind of note to the submitted details so that there's something at checkout to indicate that they are purchasing free items as well, they're just not shown?

That or I guess @barryvdh's suggestion should also include the same loop for the AbstractRequest here: https://github.com/thephpleague/omnipay-paypal/blob/ec7fc864ae0699dd832cd727da26286a5354a35e/src/Message/AbstractRequest.php#L303