thephpleague / omnipay

A framework agnostic, multi-gateway payment processing library for PHP 5.6+
http://omnipay.thephpleague.com/
MIT License
5.94k stars 928 forks source link

[3.0] Money value object #322

Closed barryvdh closed 8 years ago

barryvdh commented 8 years ago

So the de-facto package seems to be this: https://github.com/mathiasverraes/money

Work is happening on the nextrelease branch: https://github.com/mathiasverraes/money/tree/nextrelease

Money has as static 'from string' method to intialise from string, but we should encourage setting an integer. This could also be used for currency.

I've asked for the timeframe for 3.0 is, so we might be able to use that.

barryvdh commented 8 years ago

See https://github.com/mathiasverraes/money/issues/137#issuecomment-173925700 It's close enough I guess :)

sagikazarmark commented 8 years ago

Money has as static 'from string' method to intialise from string

I think @frederikbosch is working on a money parser for greater flexibility, so one can use that as well.

but we should encourage setting an integer

Actually this is not a restriction anymore from an exact type point of view. You can pass the number as a string as well (allows using BCMath and GMP with big numbers), but it still has to be an integer representation.

frederikbosch commented 8 years ago

@sagikazarmark Right, the static method is going to disappear and will be replaced by a parser interface. The parser is accompanied by its sibling formatter interface.

judgej commented 8 years ago

I've noticed the parser does a cast to (string) in at least one constructor. Doing that raised this unexpected issue for us, so I've raised it as an issue

We don't want old issues coming back when we shed legacy code for a third party pachage. Talking of which, I need to get that fix pushed, but was hoping for some feedback first, to make sure I haven't overlooked something important.

barryvdh commented 8 years ago

So I've looked in Money, and the best way to handle this is to just use integers as amount. So instead of '10.00', supply 1000. But that's a pretty BC break. It would be probably best, so we can avoid string/float/integer conversions altogether. And it would be best to use the Currency also, but it would need the numeric code. I've already implemented an external currency list; https://github.com/thephpleague/omnipay-common/commit/af429996b24cb8543d057c05841b2205208af98e So perhaps Money would also do something similar.

barryvdh commented 8 years ago

And the more I think about it, the Money package is probably not really needed. It does add value for parsing strings to float, formatting float to strings and making calculations with the amount. But we just need to store a value, do some validity checks and then return the value again. Depending on the gateway, it needs to be converted again.

So why not:

And perhaps add a method to convert the amount to float/string, using the currency decimals?

sagikazarmark commented 8 years ago

I am not entirely sure about your use case of handling monetary value, but believe me: it is not that simple. :) Let me mention some points from the lifecycle of Money:

This library we are talking about started exactly here: store money as integer. There are several issues with it, that's why we started to store money internally as string. To name a few:

These are all problematic things you have to care about.

aimeos commented 8 years ago

If we only need storing and returning the amount, I would also opt for using strings. I agree with @sagikazarmark that conversion is always accompanied with some problems and in the end, the value is always transfered as string to the payment gateway and never as int, float or something else.

judgej commented 8 years ago

The idea of the money object is the recognition that an amount is more than just a number. The currency is an important part of that - the two go together and should stay together.

For example, you cannot just send that integer to most gateways without formatting it. You cannot format that integer without knowing the currency it represents, and that is driven by the metadata that goes along with that currency.

With a money object, the amount and currency are always kept together. Each gateway then knows exactly what it is dealing with, and it has the tools there to format it as it needs to. Applications passing amounts into OmniPay also then have a consistent interface to use, and don't have to do their own conversions to other formats.

judgej commented 8 years ago

One more thing: if the basket/cart is expanded to offer the flexibility that some gateways offer, then we will need to start doing some money arithmetic (e.g. calculating gross from net+VAT, or line totals from cost*quantity, or just keeping running totals), which is something OmniPay does not do at all at this time. Once we get into that, to make sure it goes smoothly, we need all the help we can by encapsulating that into a functions that already know about rounding, currency arithmetic, sub-units etc.

barryvdh commented 8 years ago

Yes but for Omnipay, rounding/subunits aren't really a concern, because no actual changes are made. Omnipay just uses the amount parameter to store the value and retrieve it again.

Some gateways use integers, other floats/strings, so some conversion is always needed, it only differs what we take as base value.

And yes, the Amount and currency are bound, but most gateways don' require the amount formatted with currency right? And Money doesn't seem to just transform an integer to a float, based on the currency, but also adds the currency sign. The other way around, developer don't usually input amounts with the currency sign, right?

So now we have to methods:

With not just store as integer from the first place? If we use Money, I would argue that we would still best provide an integer as the input. But then it doesn't have much added value, or I'm missing something important.

The currency in Omnipay is currently already combined, so could be something like this:

public function getAmountFloat()
{
    return $this->getAmount() / $this->getCurrencyDecimalFactor();
}

@sagikazarmark: There are several issues with it, that's why we started to store money internally as string

Yes, you're storing it as a string internally, but you are forcing an integer as input (in cents). That's what I'm suggesting here also.

aimeos commented 8 years ago

Your valid point is that a number is nothing without its currency. A number alone simply has no meaning and the gateways always need the number and the associated currency code.

But payment gateways expect numbers to be always in the same format (e.g. 1000.00) and not according to the language/country conventions the currency is used (e.g. "1.000,00 €" in German speaking countries). Thus, we still vote for storing currency and together with the ISO currency code, but both as strings and avoid conversion by accepting strings as parameters.

We've seen the problems of tax calculation ourselves and I think that shouldn't be done in Omnipay. Just keep it simple because as soon as you calculate taxes, you will have to store values with more precision than two decimals and the problem of rounding errors will still occur (the +-1 cent problem).

judgej commented 8 years ago

I'm not concerned about the currency symbol when formatting. It is more about whether "100" needs to be sent as "10000", "100.00" or "100". ALL of those are a valid representation of £100 in different gateways (though that last one is an edge-case).

Okay, so putting tax caclulations aside, the basket does already multiple item costs by the amount to get the line total. And at least one gateway does need the basket total to be included, which MUST add up to the total of the lines.

judgej commented 8 years ago

Just curious what is the main objection to using a money class? Is it felt to be too big a BC break, or an unnecessary complication? I'm not talking about any particular library - just the concept of passing the handling of money and currency into the hands of a package that focuses on money.

For me, it is an abstraction that fits well with the ways other libraries are moving, and I've had too many very close calls due to PHP float handling and rounding, which I believe highlights it's more complex than just a few scalar values.

sagikazarmark commented 8 years ago

Yes, you're storing it as a string internally, but you are forcing an integer as input (in cents).

Yes we do, but keep in mind that we do this to allow integer type overflow. So the representation might be integer, but the type cannot always be.

My point was that there are many problematic things with money handling, which you might not want to care about in Omnipay.

barryvdh commented 8 years ago

Money accepts an integer and returns one. So when we want to allow a string to be used as input, this needs to be converted anyways. (But money has a parser for that, which may be more or less reliable for our use case. If we want to output a string, we would still need to convert it ourself. And without calculations, the money object can be pretty much replaced with the integer directly.

If developers want to use Money, they still scan, and just pass the amount to Omnipay.

If you mean we could just pass a Money object instead of amount and currency, that would be okay with a simple instanceof check and getting the actual value.

barryvdh commented 8 years ago

But I've tried to use the money library in Omnipay, just wasn't happy with the results because of the mentioned reasons. But feel free to make a PR to make the idea more clear.

aimeos commented 8 years ago

@judgej For me it seems like an unnecessary complication of something that should be best stored in two strings. If you wrap a small class around with two get/set() methods, it would be OK but still without additional value in my point of view. The valid representations you are mentioning are gateway specific. We shouldn't try to offer all kind of representations but only a standard one (100.00) and leave it up to the Omnipay driver to do the right thing. Payment gateways ALWAYS need the basket total that should be charged. All other details (product prices, shipping, etc.) are only nice to see at the payment gateway but unnecessary. If you leave the price and tax calculation to the gateway, the sum will differ by one cent in many cases (see PayPal).

Furthermore, Omnipay shouldn't offer a basket implementation by default. That's not the purpose of the common payment gateway interface. It's OK to offer that as additional library but Omnipay itself shouldn't depend on that.

judgej commented 8 years ago

I am talking about the basket object used to provide metadata to the payment gateway in support of the payment, not anything to do with the "basket" used in the application as a part of the customer's shopping experience. Not sure if I made that clear. As such, it is very much in OmniPay's realm to handle how that gets sent to the gateway.

Thanks for your feedback :-) I do worry a bit if drivers are given a string and told to sort out any reformatting themselves. The more of that kind of thing that gets spread around the drivers, the more various money issues will be replicated in different places in different ways, IMO.

sagikazarmark commented 8 years ago

Money accepts an integer and returns one. So when we want to allow a string to be used as input, this needs to be converted anyways.

I am kind of confused what we really are talking about. Money object accepts string and integer as well. It returns string to be consistent about return types. As soon as you pass PHP_INT_MAX, you cannot represent your value as integer. You either need float or string. Money handles all these conversion stuff when possible, in every other cases, it throws an exception for example.

For me, using a custom representation seems to be a bigger complication:

  1. Using Money:

Money->Omnipay->Money

  1. Using custom money object

Custom Money->Omnipay->Custom Money

(but in this case you have to handle all the conversion stuff Money already does)

  1. Using string/integer

String/Integer->Conversion/Overflow check/Rounding/Etc->Omnipay->String/Integer

Maybe I don't fully understand your use case, but from a public API point of view, using a Money VO seems to be a better choice than using string/integer. But that's my point of view.

frederikbosch commented 8 years ago

Whether Omnipay is going to use the Money package as originally created by Mathias Verraes, frankly I don't care. But one should take the right things into account before making decisions. What to take into account.

  1. It must not be possible to change the value of an amount. Driver may not change the value. It is only used to forward it to respective gateway.
  2. The representation of the value might differ per gateway. It should be possible to format the amount as such that it can be accepted by the gateway.
  3. It is possibly wise to store the amount as integer. When people do calculations with the amount, either inside or outside the Omnipay package, they must learn that using floats is very risky. The fact that Omnipay might not be doing calculations with the amount is irrelevant.
  4. An amount will be forwarded to the gateway together with the currency. That is quite logical because an amount only has meaning with a currency. So there multiple reasons to store an amount together with the currency.

What do these considerations learn us?

  1. We need an immutable object that protects the amount to be changed: a value object that represents Money. So please, so no setters.
  2. We need a formatter class that formats the amount to the format as demanded by the gateway.
  3. The value object should only accept integers when constructing the object, should probably use an integer internally and when you need the amount (get) it should return an integer.
  4. The value object should consist of a value and a currency.

Now, before deciding which package you are going to require, one should agree on the requirements. But if you ask me, I think it would be wise to use the Money package that is already there.

aimeos commented 8 years ago

@frederikbosch We object against using integers as base representation due to what @sagikazarmark said about overflows (more worse than using floats). A formatter class for a few formats doesn't help too. The rest is simply a matter of style and can be implemented as the majority will like it most.

frederikbosch commented 8 years ago

@aimeos I dont agree with any of your arguments.

judgej commented 8 years ago

@frederikbosch I agree with your analysis, with perhaps the exception of this (maybe):

The value object should only accept integers when constructing the object

The conversion between data types and formats is the thing that can be problematic, so it makes sense to be able to create the object from other formats too. Whether that is built into the money class or are separate helper functions, is less important. What is important is that there is one place where this is done, instead of thirty drivers all doing it their own way. The application will have amounts as integers, strings, floats, with or without decimals or decimal points, and IMO they need to be able to throw it at OmniPay (through an object or helper function) without the need to reinvent this.

I guess where I am coming from is not just a standard mechanism for making payments, but a reliable, consistent and trusted mechanism.

barryvdh commented 8 years ago

I think there is a fair bit confusion about the scope of this issue. For example, the issues with string vs integer vs float; there are 2 issues:

  1. How to store them internally. Integers + float can be stored as strings internally.
  2. If we use decimals by default (eg. 10.00) or integers (whole numbers) for the cents (eg. 1000).

What I mean to say is that a lot of issues with floats/rounding can be prevented by using integers as input (either as actual integer, or a string representing an integer). You don't have precision problems, because you use whole numbers. Only when converting to float, you go from integers to a float (factor 100 usually), so that shouldn't lead to rounding problems.

Current getAmount in in AbstractRequest. (v2)

/**
* Validates and returns the formated amount.
*
* @throws InvalidRequestException on any validation failure.
* @return string The amount formatted to the correct number of decimal places for the selected currency.
*/
public function getAmount()
{
   $amount = $this->getParameter('amount');
   if ($amount !== null) {
       // Don't allow integers for currencies that support decimals.
       // This is for legacy reasons - upgrades from v0.9
       if ($this->getCurrencyDecimalPlaces() > 0) {
           if (is_int($amount) || (is_string($amount) && false === strpos((string) $amount, '.'))) {
               throw new InvalidRequestException(
                   'Please specify amount as a string or float, '
                   . 'with decimal places (e.g. \'10.00\' to represent $10.00).'
               );
           };
       }
       $amount = $this->toFloat($amount);
       // Check for a negative amount.
       if (!$this->negativeAmountAllowed && $amount < 0) {
           throw new InvalidRequestException('A negative amount is not allowed.');
       }
       // Check for a zero amount.
       if (!$this->zeroAmountAllowed && $amount === 0.0) {
           throw new InvalidRequestException('A zero amount is not allowed.');
       }
       // Check for rounding that may occur if too many significant decimal digits are supplied.
       $decimal_count = strlen(substr(strrchr((string)$amount, '.'), 1));
       if ($decimal_count > $this->getCurrencyDecimalPlaces()) {
           throw new InvalidRequestException('Amount precision is too high for currency.');
       }
       return $this->formatCurrency($amount);
   }
}
/**
 * Get the payment amount as an integer.
 *
 * @return integer
 */
public function getAmountInteger()
{
    return (int) round($this->getAmount() * $this->getCurrencyDecimalFactor());
}

So, if we say, from v3 onwards, input the amount as smalles unit ('cents'), we could use Money in something like this: (So

/**
  * Validates and returns  amount as integer.
  *
  * @throws InvalidRequestException on any validation failure.
  * @return string The amount in smallest unit possible (eg. 'cents')
  */
 public function getAmount()
 {
     $amount = $this->getParameter('amount');

     if ($amount !== null) {
         $currency = new \Money\Currency($this->getCurrency() ?: 'USD');
         $money = new Money($amount, $currency);

         // Check for a negative amount.
         if (!$this->negativeAmountAllowed && $money->isNegative()) {
             throw new InvalidRequestException('A negative amount is not allowed.');
         }
         // Check for a zero amount.
         if (!$this->zeroAmountAllowed && $money->isZero()) {
             throw new InvalidRequestException('A zero amount is not allowed.');
         }

         return $money->getAmount();
     }
 }

 /**
  * Validates and returns the formatted amount as a float.
  *
  * @throws InvalidRequestException on any validation failure.
  * @return string The amount formatted to the correct number of decimal places for the selected currency.
  */
 public function getAmountFloat()
 {
     $amount = $this->getAmount();
     if ($amount !== null) {
         return (string) ($amount / $this->getCurrencyDecimalFactor());
     }
 }

So you can see, we don't need to modify or calculate anything. We could roughly do the same without converting to Money:

/**
 * Validates and returns  amount as integer.
 *
 * @throws InvalidRequestException on any validation failure.
 * @return string The amount in smallest unit possible (eg. 'cents')
 */
public function getAmount()
{
    $amount = $this->getParameter('amount');

    if ($amount !== null) {
        if (filter_var($amount, FILTER_VALIDATE_INT) === false) {
            throw new InvalidRequestException('Amount must be an integer');
        }
        // Check for a negative amount.
        if (!$this->negativeAmountAllowed && $amount < 0) {
            throw new InvalidRequestException('A negative amount is not allowed.');
        }
        // Check for a zero amount.
        if (!$this->zeroAmountAllowed && $amount == 0) {
            throw new InvalidRequestException('A zero amount is not allowed.');
        }
        return (string) $amount;
    }
}

/**
 * Validates and returns the formatted amount as a float.
 *
 * @throws InvalidRequestException on any validation failure.
 * @return string The amount formatted to the correct number of decimal places for the selected currency.
 */
public function getAmountFloat()
{
    $amount = $this->getAmount();
    if ($amount !== null) {
        return (string) ($amount / $this->getCurrencyDecimalFactor());
    }
}
frederikbosch commented 8 years ago

@judgej I agree with you on the fact that Omnipay should offer an api that allows multiple methods to create an amount.

While I feel that people should learn to use an integer for money instead of float, it's maybe not realistic to require that. Having multiple methods for creating a money object, would be very helpful.

However, that process (parsing) should be separated from the resulting value object. You could create a parser per method, like FloatToMoneyParser and DecimalStringToMoneyParser. That would be in line SRP. A second way could be to create an amount factory, with methods like.

barryvdh commented 8 years ago

I would just stick with 1 input (integer), and 2 output methods (integer and float)

frederikbosch commented 8 years ago

@barryvdh But in that method you are not using money as a value object. You use it as validation tool. Then the third example gives less overhead, and would be prefered over the second with money. I thought you were considering using Money as value object, being the class that is used for amounts and passed through all methods of Omnipay, whether it is common or driver. Too bad, I hope that you will reconsider.

barryvdh commented 8 years ago

Well yes, that's a different case. But how would you see that? A setMoney attribute where you can provide a money attribute, and it will set the amount and currency parameter? I'm in favor of that, wouldn't even need to require the money package by default.

frederikbosch commented 8 years ago

@barryvdh That is easy.

$request = $gateway->purchase([
    'amount' => new Money(500, new Currency('USD'))
]);
frederikbosch commented 8 years ago

@barryvdh And if I may do more suggestions

$request = $gateway
   ->purchase(new Money(500, new Currency('USD')))
   ->withDescription('..')
   ->withReturnUrl('..')
   ->withNotifyUrl('..')
;
barryvdh commented 8 years ago

So would that differ much from what we have now?

$request = $gateway->purchase([
    'amount' => 500,
    'currency' => 'USD',
]);

As said, we could add a setter like this:

public function setMoney($value)
    {
        if (!($value instanceof Money)) {
            throw new InvalidRequestException('Not a valid Money object.');
        }

        $this->setAmount($value->getAmount());
        $this->setCurrency($value->getCurrency()->getCode());
    }
$request = $gateway->purchase([
    'money' => new Money(500, new Currency('USD'))
]);

Which would me most useful for when the application is already using the Money object.

I do like the chained setup also :)

@sagikazarmark @frederikbosch So regardless of the Money class itself, do you suggest we use the integer value (as string) as source, and convert to float when needed? (So the opposite of what we do now)

barryvdh commented 8 years ago

So this is what I mean, in a PR: https://github.com/thephpleague/omnipay-common/pull/79

frederikbosch commented 8 years ago

@barryvdh If you choose to encapsulate it into an own defined value object, the internal value is not an issue any more. It can be abstracted away easily!

With that object, the api consumer constructs it like this.

$request = $gateway->purchase([
    'amount' => Money::fromInteger(512)
]);
$request = $gateway->purchase([
    'amount' => Money::fromFloat(5.12)
]);
$request = $gateway->purchase([
    'amount' => Money::fromDecimalString('5.12')
]);

Whereas a driver maintainer, will do something similar as this.

$data['amount'] = $this->amount->toInteger();
$data['amount'] = $this->amount->toFloat();
$data['amount'] = $this->amount->toDecimalString();

And now nobody cares how you internally store the amount within the value object. Moreover, you can add methods to your own Money object that are related to the Omnipay package. So let's do that, and go one step further.

Now the api consumer does the following.

$request = $gateway->purchase([
    'amount' => Money::fromInteger(512, 'EUR')
]);

And the driver maintainer does the following.

if ($this->amount->isCurrencyAvailableWithin(['EUR', 'USD']) === false) {
  throw new InvalidArgumentException('Sorry this driver cannot process this currency');
}

That does makes sense, right? Maybe Omnipay does not need all the methods from moneyphp. But it sure needs encapsulating money related behaviour into a value object.

sagikazarmark commented 8 years ago

Well, yes, we saw this whole thing from completely different points of view.

I think using Money library would only make sense if you make it part of the public API. That will restrict the user to pass in a valid Money object. All of the conversion/representation issues would just magically go away.

Also, using the Money object internally is quite easy I think.

If you want to allow string/integer values to be passed, then there are many issues you have to face with and Money would probably be an unnecessary complication (as @frederikbosch said above, it would just validate the amount).

Also, @barryvdh I am not sure I understand why you validate the input in a getter. Input should be validated at the injection point and should agressively react if invalid data is passed (like good little Exception to make the developer happier).

Update: ah, @frederikbosch mentioned almost the same things I did. ;)

judgej commented 8 years ago

That's what I thought we were talking about too - using an object to handle the amount all the way through. If the money object handles its own validation, then all OmniPay would need to worry about was that an object of the correct interface was being passed in, and that's it. The object would be a lump of monetary value, all valid and clean, and with a currency.

The amount can be a third party money object, or it could be a much simpler OmniPay interface (OmniPay only needs to care about getting values out of the object in a limited number of formats, at this stage at least) so an adapter to a money library would provide the functionality behind it.

@frederikbosch Spot on my thoughts. I'm already doing this on a new non-omnipay gateway. The gateway only needs to ask for the minor units and the currency code, and that is all the money interface consists of. An adapter then puts a lot more functionality into the object used to define the amount, allowing it to be set from a string, or an int, or a float etc.

sagikazarmark commented 8 years ago

@judgej Exactly my thoughts!

barryvdh commented 8 years ago

@sagikazarmark the input validation is from the already existing code, I just modified that to show the changes.

I do agree that the amount could probably be better of in it's own class, instead of part of a big parameter bag, although is does pretty much the same, but not isolated.

I'm not so sure of providing multiple input formats though.

sagikazarmark commented 8 years ago

I'm not so sure of providing multiple input formats though.

That's my point: having a single class enforces the user to pass a strict format.

barryvdh commented 8 years ago

That was more a response to the 'Money::fromDecimalString' example :)

But I'll see, something in line with the Credit Card etc could work, if string/integer, convert to value object, otherwise use that directly. And create some validation/output of formats there.

judgej commented 8 years ago

Here is a practical example that I hope helps to demonstrate the idea.

I wrote this Amount class last year for a gateway. It has some fancy ways of initialising it to suite different use-cases:

// All £9.99
$amount = Amount::GBP()->withMajorUnit(9.99); // Or "9.99"
$amount = Amount::GBP()->withMinorUnit(999);
$amount = new Amount(new Currency('GBP'), 999);

The gateway itself is not interested in how it is initialised or where it came from - it is just interested in this interface

Now, I just wrote an adapter for moneyphp\money. It is very simple and could arguably have been done using injection rather than extending, but it's just a simple adapter.

That adapter now allows me to create an amount object using everything that Money\Money provides, for example:

$amount = new \Academe\SagePayMsg\Money\Money(999, new \Money\Currency('GBP'));

That offloads all the storage, conversion, validation etc. into a third-party library. I'm not locked into that library either - it took ten minutes to add this one, so could add other quickly too. For a gateway to be usable in many applications and frameworks, it is important to be able to do this - to have options to work around conflicts and be able to update support libraries, maybe with a simple built-in solution too, but ultimately interfaces that make it easier to plug in alternatives (all of which are optional - suggested by composer but not required).

The same approach, IMO, should or could apply to other areas, such as the PSR-7 messages, address classes, basket, payment type (card, token, null etc.)

I realise this is obvious stuff to some of you, so apologies if I'm teaching granny to such eggs, but it's important to be on the same page. The fact that I could offload several hundred lines of code and several classes out of my package to a third party using a half-dozen-line adapter, demonstrates how powerful using classes appropriately, instead of lists of scalars, can be.

frederikbosch commented 8 years ago

@barryvdh Providing multiple ways of instantiating a Money object is only a solution to something that is apparently a problem. I would prefer a strict format too.

And maybe it is a problem. The users of Omnipay are very broad group. People that are using Active Record are probably using the same decimal string format in their models that Omnipay is now using. Changing that to a value object might cause confusion for them. But maybe providing multiple ways of instantiating the value object helps them. You can better forsee this than I do.

The solution of @judgej is a valid alternative. In the end of the day, Omnipay is only interested in a very few ways to get the amount and currency. A contract for that by using an interface is a good solution for it.

sagikazarmark commented 8 years ago

Mentioning here as well: Money object is final as of 3.0, so writting adapters might not be so easy, unless we are talking about some kind of wrappers.

barryvdh commented 8 years ago

I do agree that while it may not be ideal, it would probably be more difficult to upgrade when we don't accept their stored input. But we could offer it as a deprecated option, so it can be removed in the future.

So something in the lines of https://github.com/thephpleague/omnipay-common/commit/a82d250b5da94f4814057f6c82f376354b7ca29f ?

$request = $gateway->purchase([
    'amount' => Amount::fromDecimal(5.12, 'EUR'),
]);

$request = $gateway->purchase([
    'amount' => 512,
]);

$request = $gateway->purchase([
    'amount' => ['value' => 512, 'currency' => 'EUR'],
]);

$request = $gateway->purchase([
    'amount' => new Amount(512, 'EUR'),
]);

$request = $gateway->purchase([
    'amount' => new Money(500, new Currency('USD'))
]);

(And the currency is optional, defaults to USD)

One thing to think about is that we're now binding the amount to a currency. This is probably a good thing, but it doesn't allow to set a global currency for the gateway (like before, with default parameters), only combined with the amount. And it isn't possible to NOT set a currency, but this is perhaps also a good thing..

frederikbosch commented 8 years ago

@barryvdh When you define an Amount object, you will have the freedom to construct it as you like: with, without or optional currency. But if the currency is defined, it is defined together with amount, which makes sense.

I think an amount object is not a bad idea. Then I would not even allow the money object. Money users like me can perfectly convert our Money objects to Amount.

$request = $gateway->purchase([
    'amount' => new Amount($price->getAmount(), $price->getCurrency()->getCode())
]);

I don't see a problem with the above.

barryvdh commented 8 years ago

Makes sense. I've updated to PR.

I'll see what the other say about the currency parameter.

greydnls commented 8 years ago

This has been resolved in the PR @barryvdh and it looks like there's agreement here. Closing this issue.

barryvdh commented 8 years ago

Also related: http://verraes.net/2016/02/type-safety-and-money/ Where handling Bitcoin might be a valid concern for this.

Still not really sure if floats or integers should be the base input. Perhaps scan the existing gateways to see what is more common.

sagikazarmark commented 8 years ago

As an internal representation, I would stick with integer as the common representation. Can save you a lot on rounding, but it adds some complexity.