[RFC] Add CurrencyInterface and CurrencyLoader

[giosh94mhz]

I've shared with you my fork, since I really think this will improve the library usability (i.e. integration is existing systems).

First and foremost: the implementation of the Money class is great! Invariant Final Value Object are great for money, and prevent silly update-by-reference errors.

The main limitation I found is the Currency class. The current limitation are:

• all currencies are pre-loaded, no matter how or when you use it (see my issue Provide a way to do lazy-loading of currencies #94);
• currencies include all(?) and only ISO code, so you cannot use "imaginary currencies" (gold, credit, HyruleRupees, ... you said in another PR that can be done, but I can't see how probably)
• currency only property is the code, and... well... that's not very useful.

The solution seems simple at first: replace or wrap (not extend!) the default currency, and provide my decorated version!

But this is not possibile, for multiple reasons:

• Money class depends on Currency implementation, not to abstraction, and so there is no way of providing new behaviour (no Dependency inversion);
• Money class allocate Currency instances directly (Multiple Responsibilities);
• it is bound to a specific set of currencies, since only so I cannot even add currency if I use this implementation (not Open/Closed).

I know, that you did this to provide Immutable Value Object instances, and this is great when it comes to money, but for Currency, the Immutability condition is not required (as long you don't change the currency code, of course); as in the original Value Object Pattern, value Object only require that object are compared by value, immutability is just a good heuristic.

So, what have I done to the library?

• Currency is a final implementation of CurrencyInterface;
• Money depends on CurrencyInterface;
• Money class use an instance of CurrencyLoader to allocate currency (preconfigured with ISO code for backward compatibility);

All tests still pass and it is fully backward compatible. The only drawback I can think of is that if an application use a custom implementation of Currency, it should not change the currency code at runtime if there are Money instances.

To avoid BC I left the following things which I'm not "proud" of:

• use Currency+CurrencyInterface instead of Currency+ISOCurrency (or DefaultCurrency);
• Money::registerLoader automatically initialize with ISOCurrencyLoader;
• config file currencies.php is included twice in ISOCurrencyLoader and ISOCurrencies;
• Currency::getName is deprecated for code, and should be reused to return the ISO name.

I hope to hear your feedback, and maybe the feedback from the community.

Related PR:

• Sharing is caring (iso4217) #71 for iso loader implementation
• Provide a way to do lazy-loading of currencies #94 lazy loading
• change: self usages in static usages for extendability reasons #53 where you and @sagikazarmark discussed this same patch. [edited ticket number]

[sagikazarmark]

I have various feelings about this.

First of all: you can absolutely create imaginary currency types. There is no limitation on that. The fact that only an ISOCurrency list is available in the repository does not mean you can't. In fact, this is the reason why we introduced AvailableCurrencies: to allow you to provide a custom list of currencies.

Secondly: there is a good reason why the Money is not aware of any currency loading at all. It is a value object, there should not be more responsibility in that class.

Last, but not least: Currency is a value object too, thus introducing an interface breaks the rule IMHO. Not just because you can implement a custom one, but because the currency should not have anything else, but the data (code) which identifies it. Why do you need an interface then?

Lastly: I can hardly see the extra features this PR adds. I mean, what is that you CAN'T do without this PR with the current implementation?

Currency auto-loading? That is your responsibility and should not be in the core. And as I proved earlier, you can do it, ISO is not hard coded.
Custom currency implementations? As said above, it breaks the Value Object being of the Currency.

For the record: I've been trying to implement something similar earlier (meaning my custom money implementation) with interfaces, loaders and the like, but then I found @mathiasverraes's package and I found it much more better. The point is that I think it tries to be very strict about how money should be handled and reduces extensibility to a minimum level which is fine IMO.

[giosh94mhz]

Thank you @sagikazarmark for your fast feedback! I try to reply to all you comment.

Imaginary currency can be created, but there are limitations AFAIK. To add it I can just do new Currency("GOLD") or the shortcut Money::GOLD(123); this can be done even if that currency isn't part of the current domain model. The interface AvailableCurrencies as it is now, seems ok to validate a currency against a set, but not to "get" a currency from a valid set, or throw.

I agree that Money library should not know about loading. In fact I think that currency should be always passed in the constructor. The only reason I've introduced this loading login is to allow "Money::CURRENCY" syntax, which is cool and provide backward compatibility, even if it's not SOLID OOP.

I think that Currency should be value object (that's why the method equals is defined), but it should not be forced to only have the "code" property. For example, in my domain I have currency name, number of decimal precision, and a conversion rate based on default currency (ok, the latter is too specific and should be refactored out :). Also, these informations are saved on a backend (i.e. Doctrine), so always duplicating force the application to have and use adapters. This is also the main reason why I think that having a loader may be good: you can load all the currency at once, or load only the one use need (e.g. from DB).

So, If I use my own implementation of currency I always need to do something like this:

$money = new Money(123, $myCurrency->getCode());

or define my custom Money and Currency with duplicated interface, and for each method do:

    public function doSomething()
    {
        $newMoney = $this->wrappedMoney->doSomething();
        $currency = $myCurrencyLoader->load($newMoney->getCurrency()->getCode());
        return new MyMoney($newMoney, $currency);
    }

...or similar.

So, in the end.

I love too the way this library is written, and that's why I've opened this PR in the first place. I was expecting this kind of feedback. What I say is this: with this PR, the library can be used the same way as before, with Money and Currency used as strict ValueObject. In the case of "complex" system where having the currency code is not enough, you can provide your own Currency class with extra information and carry it along with the pure money value. Also, by having a clear (even if misplaced, though) hook point to set the list of available currencies.

All I say obviously don't apply to Money, of course. If someone need to extend both Money and Currency, then this library is not what he's looking for.

[sagikazarmark]

Actually you confused me a little bit.

In fact I think that currency should be always passed in the constructor.

Isn't it right now?

The only reason I've introduced this loading login is to allow "Money::CURRENCY"

Can't you do this currently?

be forced to only have the "code" property

That's where I disagree. The point of currency is to provide information about the money value and nothing more. Any further information should be stored elsewhere, where you can actually use the Currency object as the key.

I have currency name, number of decimal precision

I admit that this is related to the currency. However the point of the Currency object is to let you identify. So this kind of data should be stored elsewhere. (Since you mentioned doctrine: Currency is an association, not an entity). At least this is the case now, but we can argue about it.

Also, these informations are saved on a backend (i.e. Doctrine), so always duplicating force the application to have and use adapters.

Sorry, what? 😄 I have my own doctrine types and extensions for money which work just fine.

you can load all the currency at once, or load only the one use need (e.g. from DB).

That's again something that is out of scope. If I were you, I would create a factory wrapper which contains all your custom knowledge to load currencies and any further custom logic. Also, check the ISOCurrencies, which itself does the currency loading and can check if it is in your custom set.

It seems to me that (almost) everything you want is already in the library. That extra which is not should be implemented as custom logic IMHO.

[giosh94mhz]

Mmm probably my explanation was a bit fuzzy :)

In fact I think that currency should be always passed in the constructor.

Isn't it right now?

The only reason I've introduced this loading login is to allow "Money::CURRENCY"

You can now both initialize using new Money(123, new Currency("EUR")) and Money::EUR(123). The former is more formal and IMO the way to go, the latter is a convenient shortcut, but imply the knowledge of how a Currency is allocated. Since the Money class use __callStatic to create magic methods, then there should be a static component which know how to allocate an object. In the current implementation the allocation is left to a simple new statement with a fixed class.

That's where I disagree. The point of currency is to provide information about the money value and nothing more. Any further information should be stored elsewhere, where you can actually use the Currency object as the key.

Well, for the same reason I think that Currency should be an interface. I mean, If the only information useful from a currency was the code, then it would have become a string property in money. But the class already provide information about CurrencyPair, ISOCurrencies, ... it seem so natural to allow different currencies implementation.

Since you mentioned doctrine: Currency is an association, not an entity

Let's go a bit off-topic. I don't get this. I have many tables with tuple of columns: money and currency. Money is just a "string" and currency is an association to a Currency entity. But to use Doctrine, I cannot just pass "EUR" I should pass the persisted Currency. Are you doing another easier way?

Also, these informations are saved on a backend (i.e. Doctrine), so always duplicating force the application to have and use adapters.

That's definitely not clear, sorry :) As I explained before, with Doctrine I need to pass reference to persisted entities, so the Currency class cannot be used. This mean that if I use Money class as is, I need an Adapter, an abstraction or some EntityListeners to replace the correct instance before persist. This is about doctrine, but I think that if someone save on CSV (shame on you :) is the same.

If I were you, I would create a factory wrapper which contains all your custom knowledge to load currencies and any further custom logic.

This is the first thing I have though, but consider this two examples

class Factory {
    public function createMoney($amount, $currency) {
        return new Money($amount, $currency->getCode());
    }
    public function createCurrency($code) {
        return $this->loadOrCreateFromSomewhere($code);
    }
}

$eur = $factory->createCurrency("EUR");
$money = $factory->createMoney(123, $eur);
$money->getCurrency()->equals($eur); // ok cool
$eur->customMethod(); // whatever
$money->getCurrency()->customMethod(); // undefined method error

Still I have the same problem if not using my custom currencies inside Money, unless I also use a custom money which reimplement all Money method. But this is overkill: this library is small, and if I need to adapt all the classes, then it is better to use another library (or a fork).

It seems to me that (almost) everything you want is already in the library. That extra which is not should be implemented as custom logic IMHO.

Maybe I simply don't see it... maybe you have some example?

[sagikazarmark]

In the current implementation the allocation is left to a simple new statement with a fixed class.

This is only a problem because you want custom currency implementations.

it seem so natural to allow different currencies implementation.

Why not a money interface then? I think currently the approach is to have Currency as a Value Object, thus an interface is not acceptable here. If @mathiasverraes agrees to change that... However, there are still problems with your Currency implementation: for example equals should only accept an object of the same type, not an interface. Typehinting is a perfect solution there, a type check would not be.

ISOCurrencies, ... it seem so natural to allow different currencies implementation.

ISOCurrencies? Where?

I understand it seems easier to have all those information in a Currency object, but it simply does not belong there. Again: Value Object. Short definition: an object representing a simple entity (aka. value in this case). Currency itself (not the object) belongs to the Money (not the object). It really matters if you pay 200 something for an item: dollars or bitcoins? That's why currency belongs to the money. But name? It does not. Imagine the scenario: you are developing a multi language e-commerce site. Do you really want to put the name into the Currency object. Or just the identifier which gives you the proper translation of the currency name. This is just an example, where it is not a good idea, but also shows why it is better to have Currency as a value object.

But to use Doctrine, I cannot just pass "EUR" I should pass the persisted Currency. Are you doing another easier way?

IIRC @mathiasverraes puts the currency code into the price field and parses the string into a Money object using a DBAL type.

This is the first thing I have though, but consider this two examples

You are overcomplicating things 😄

use Money\AvailableCurrencies;
use Money\Currency;
use Money\Money;

class MoneyFactory
{
    private $currencies = [];

    public function __construct()
    {
        // load currencies

        // example
        $this->currencies['EUR'] = [
            'name'         => 'Euro',
            'reversedName' => 'oruE',
            'whatever'     => 'anything',
        ];
    }

    public function createMoney($amount, $currencyCode)
    {
        $currency = new Currency($currencyCode);

        // optionally check if it is available:
        // $currency->isAvailableWithin($this);

        return new Money($amount, $currency);
    }

    /**
     * {@inheritdoc}
     */
    public function contains(Currency $currency)
    {
        return isset($this->currencies[$currency->getCode()]);
    }

    public function getCurrencyName(Currency $currency)
    {
        if ($currency->isAvailableWithin($this)) {
            return $this->currencies[$currency->getCode]['name'];
        }
    }
}

Usage:

$factory = new MoneyFactory;
$money = $factory->createMoney(123, 'EUR');
$currencyName = $factory->getCurrencyName($money->getCurrency());

The good thing is that you can inject this factory into any of your custom logic (formatting, storage) and you can use it there. So that these kinds of custom data do not have to travel with the VALUE object, since it is not value, but some kind of metadata. If you are really concerned about separating your code, you can have your own Metadata (AvailableCurrencies with some extensions) class (which handles loading and is injected into your Factory) so that you have the metadata and factory logic at two different places.

[giosh94mhz]

ISOCurrencies? Where?

I mean the currencies.php file and the like...

But name? It does not. Imagine the scenario: you are developing a multi language e-commerce site. Do you really want to put the name into the Currency object. Or just the identifier which gives you the proper translation of the currency name.

You got a point here. If I think of Money as something like a-primitive-type-php-should-have-provided it sounds good: use the Currency as an simple type and leave the rest to the domain (i.e. formatter, available currency contraint, etc...).

The good thing is that you can inject this factory into any of your custom logic (formatting, storage) and you can use it there. So that these kinds of custom data do not have to travel with the VALUE object, since it is not value, but some kind of metadata.

So, you are suggesting to have services which provide property and method of currencies, instead of providing property and method along with the currency. The latter seem more convenient in my domain, but maybe is not the way to go...

I will think about it more carefully tomorrow... with fresh mind!

[sagikazarmark]

I mean the currencies.php file and the like...

It has nothing to do with the Currency object. It is just there, because it is the most common use case.

So, you are suggesting to have services which provide property and method of currencies, instead of providing property and method along with the currency.

Exactly. The implementation itself is up to you. Imagine it like Doctrine's metadata....

[sagikazarmark]

I would close this for now.

[giosh94mhz]

Ok, as you prefer.

By the way @sagikazarmark, some time as passed and I can give you some more feedback. I really loved your approach and used in other situation of my project (e.g. tipical Status[id, name, description]), but still using the CurrencyInterface for Money. Maybe this is just my preference :) Anyway, thank you for your valuable feedback.