[3.0] Currency fallback to that of passed in Money object?

[judgej]

Just running this past the project to see if is something I should add.

Omnipay 3.x will accept a Money\Money object as the amount. A Money\Money object will always have a currency.

There is a separate currency parameter that can be set and read independently.

My proposal is that if (a) the currency parameter is not set, and (b) the amount is a Money\Money object, then getCurrency() should return the currency of amount.

[barryvdh]

That would be a bigger BC break, right?

[barryvdh]

I'm actually not really sure if it's smart to allow Money to be set directly, as we're depending on the library then. If we don't expose it directly, we can just change it internally.

I do think we might want to add an AmountInteger to make it easier (and skip to formatting)

[barryvdh]

See https://github.com/thephpleague/omnipay-common/commit/cda8afc670d93e51d4a5d0676732206a230e5d4c for the commit that removes the setting of Money and makes it private. Allowing integer in https://github.com/thephpleague/omnipay-common/pull/179

[judgej]

I'm not sure about this approach. Being able to throw an amount object into the package, and not caribng about how the driver is going to format it, makes things a lot more robust. Similarly, the driver not caring about whether you started with an integer, a float, a string, minor units. major units etc, and having to make assumptions about what was being intended, also helps to make things more robust. IMO the amount (in the itembag items too) needs to support a money interface (of some sort) that takes out all the guesswork, and is best-of-breed to keep on top of rounding errors.

[barryvdh]

And what about a separate setMoney method which uses that object directly and sets the currency also?

[judgej]

I'm not sure if that solves the problem or just moves it.

I'm trying to understand the idea behind this. Is what you are trying to do, to avoid using that specific third-party package (moneyphp/money) in any interfaces, so users of Omnipay are not locked into using that library, or at least discouraged from using the money package in a way that may cause them a BC break if Omnipay decides to move to a different library? If so, a setMoney() would probably not help with that aim.

So would it be better to define an interface for an Omnipay money class, with a connector for whatever money library people want to use (with moneyphp/money perhaps provided out of the box)? Omnipay could still use whatever it wanted to use internally, but any money amounts passed in or out, could be handled by the Omnipay Money interface. The interface would cover simple functions such as currency, amounts as minor units or formatted values. I suspect a factory would be involved here too, for creating money objects with the connector the application wants to use, but I'm really not very familiar with how factories are generally set up for cases like this.

[barryvdh]

I made it private to avoid future BC breaks indeed. If it's just the userland function, it might be better, because that's easier to change (eg. if the gateways don't use it, only the common package needs to be upgraded).

The logic behind the split from amount -> money was your question about the currency. Does it make sense that setting the amount will also change the currency? I just want it to be as clear as possible for uses to expect to be able to use. With a setMoney method we could internally use the Money for amount and currency, and perhaps even validation exception if the currency/amount is already set.

So instead of setAmount which accepts all the things, just setAmount(string $string), setAmountInteger(int $value) and setMoney(Money $money). I'm not really sure if there are others to be aware of.

[barryvdh]

See https://github.com/thephpleague/omnipay-common/pull/180