moneyphp / money

PHP implementation of Fowler's Money pattern.
http://moneyphp.org
MIT License
4.61k stars 440 forks source link

Default currency #20

Closed marcospassos closed 10 years ago

marcospassos commented 11 years ago

Hello,

I think the Money library should have a default currency just as DateTime has date_default_timezone. Without this, I need to inject a provider or the default currency in everywhere that I need to use the Money type:

class Product
{
    /**
     * @Column(type="money")
     */
    protected $price;

    public function __construct()
    {
        $this->price = new Money(0);
    }
}

// throw new UnspecifiedCurrencyException()
$price = new Money(15);

// Initialization
Money::setDefaultCurrency('USD');

$price = new Money(15);
assert(strval($price->getCurrency()), 'USD');

/*
 * To discuss:
 * throw new CurrencyAlreadyDefinedException();
 */
Money::setDefaultCurrency('BRL');  

If you agree, I would be happy to contribute with this feature.

mathiasverraes commented 11 years ago

Hi Marco,

First, I think that you accidentally touch upon an interesting thing in your constructor: Zero EUR is the same as Zero USD. I made a pull request for that: https://github.com/mathiasverraes/money/pull/21 (It's work in progress, it needs test for all operations, and the implementations) I think that should solve the problem of instantiating Products when you don't know the Currency yet.

(As a side note, a better practice is to always pass a price to the Product constructor. After all, a Product without a price is not a valid Product. Eg: new Product($productName, $productCode, $price) )

Can you explain why you want a default Currency? Isn't the Currency always dependent on context? I'm hesitant to introduce a default currency, because what would you propose is global state, and is a source for bugs. You can indeed prevent those bugs with a DefaultCurrencyAlreadyDefinedException, but that feels like fixing a hack with a hack.

If you want an application wide default Currency, you could do something like this:

class MyDefaultCurrency extends Currency
{
    public function __construct()
    {
        parent::__construct('BRL');
    }
}

new Money(15, new MyDefaultCurrency);

Thoughts?

marcospassos commented 11 years ago

Hello Mathias,

First fo all, thanks so much for your comments. You bring up some really good points.

I agree that product should be constructed with valid required values, but it's not possible using Symfony's Form Component, so I've to set using the setters.

About the default currency, some implementations (like Python) allows defining a default currency, but I agree you suggestion sounds better, but highly coupled. About the context, in my specific case I need allow just one currency per tenant (configured at setup time).

sa-tasche commented 11 years ago

The default currency should be the local currency, set in the php.ini (or user.ini). So it's probaly a good idea to have a util class for currency/money and a "DefaultMoney" class which uses this util class.

marijn commented 11 years ago

You assume that the user is in the same region as the machine…?

sa-tasche commented 11 years ago

No. But it's obvious for me that the user want to use his currency in her own shop. And that's the point.

marcospassos commented 11 years ago

@sascha-tasche I disagree. In multitenant applications you need to set the default currency according with user shop configuration. So, brazilians for example, would like to set Reais (R$) as default currency. So, it must be not static.

mathiasverraes commented 11 years ago

I agree that product should be constructed with valid required values, but it's not possible using Symfony's Form Component,

The short answer would be "don't use Symfony Forms" :-)

The long answer is: decouple the forms from the model. It's bad practice. You can do that for example by building the forms using a GoF Command, then use that Command to pass instructions to your entities. Or you could use a ViewModel or just plain arrays -- basically anything that sits between your model and your UI is better than coupling.

In any case, by using a zero Money without a Currency, you can effectively initialize Products with a 0 price, and leave it up to the client code to add a price later, using a config parameter or a user preference to assign the Currency.

Having a global, or .ini configuration, still feels like a bad solution. If you have a single currency environment, (eg in each tenant needs to pick a single currency for the whole shop), then maybe you don't need Money and simple integers will do?

sa-tasche commented 11 years ago

@mathiasverraes @marcospassos (Sorry for my bad english) I meant something similar to $locale = ini_get(DEFAULT_LOCALE); But I found only intl.default_locale in php.ini and that's from a PECL extension.

My second idea was to using something like setlocale(LC_MONETARY, YOUR_DEFAULT_LOCALE); $formatted_money = money_format(....)

But at this time, I use my third solution: I allow to use a \Closure. So if it's set, the Money/Currency object calls it and so the user can use her own solution/extention (like NumberFormatter or native PHP functions like money_format() or what ever she want. But as a fourth solution it's possibly better to use the own 'CurrencyFormatter' interface. I prefer this idea.

To sum up: First basic idea: I have a Currency class which knows the number/currency format. And a Money class which adds his number to its format -- similar to \sprintf(). Second basic idea: I have a mechanism which knows my default currency and all my supported currencies.

@marcospassos You see hopefully, that I mean the same thing. With the difference that I don't want to using a new config file but the PHP's user.ini. Your right, that it's a bad idea to using a static file or to rely on the existens of an user.ini. I think this is the reason why you doesn't understand me. But I didn't considered that I can not explicitly use user.ini.

mathiasverraes commented 10 years ago

Closing, global defaults lead to all kinds of things, and better code is not one of them.