Custom currencies

[BenMorel]

Part of the list of things I want to work on before tagging a release, as mentioned in #2. Comments welcome.

The library ships with ISO currencies, but allows you to add custom currencies (bitcoins, etc.). This is done through a class called DefaultCurrencyProvider; Money uses it to resolve a currency code such as USD to a Currency instance, to get its default scale among other things.

My issue with the current implementation is that it's a singleton: if your code adds a custom currency, it affects third-party code using Money as well, and vice versa. ISO currencies can't be overridden, but conflicts between custom currencies are possible.

The only way I can see around this, would be to not allow currency codes in Money factory methods, and force a Currency instance to be provided.
This used to be this way, but this was very unfriendly to use. You want to be able to write:

Money::of('9.90', 'USD');

and not:

Money::of('9.90', ISOCurrency::of('USD'));

So at some point, you have to resolve the currency code from somewhere, and I could not find a better idea than making this a global, singleton configuration.

Now that I'm thinking about it, another option could be to allow a currency code string to be provided for ISO currencies only; custom currencies would need to be provided as a Currency object. This would remove the currency provider bloat entirely. Thoughts?

Side question: the Currency object has this thing called the default fraction digits (default scale, for example 2 for USD/EUR/etc.); do cryptocurrencies have a somewhat official, default scale like traditional currencies?

[jkuchar]

allow a currency code string to be provided for ISO currencies only

Seems good to me. 👍 It prevents anybody to break 3rd party code. It is still very simple to extend (create new currency).

If someone wants to introduce to create money in his own currency from string, he can always introduce his own global factory, this way he can't break anything.

function money($amount, $currencyString) {
    // 1) look up currency and assign into $currency
    return Money::of($amount, $currency);
}

// usage of custom global factory
$moneyInMyCurrency = money(16, 'PRG');

[BenMorel]

I just realized that if we follow this route, we won't be able to use parse() with custom currencies, not sure if this is a problem. This won't be possible, for example:

Money::parse('BTC 1.234000');

or we have to accept an optional CurrencyProvider instance as a second parameter.

[jkuchar]

That sounds like a serious issue if someone use this for serialization. (I do not) Do we want to support that at all? Does not seems that useful to me and there is a serious complexity involved in getting this right. (e.g. providing factory with currency list, which can create and reconstruct monies)

[BenMorel]

Serialization (using serialize(), at least) is not an issue, as we will serialize everything: the Currency object and its properties, the amount with its scale etc. unserialize() will reconstruct an equivalent object.

As for parse(), it was designed as a convenient way to recover a Money from its string representation.

TBH I've only used it in unit tests so far.

It's not a big deal to drop it, or at least restrict it to ISO currencies for now.

[jiripudil]

FWIW, I guess I'd be fine even with Money::of('9.90', ISOCurrency::of('USD'));, at least it adds clarity. In the code I work with (mainly e-commerce), there are not many places where currencies are instantiated directly; usually they are fetched from the database and there is a single authority that resolves them, e.g. a Doctrine Type object. But that's just one use case and it'd surely be a step back in terms of convenience.

Restricting code resolution to ISO currencies sounds like a good way to go 👍

Ad side question: they do, for example Bitcoin is divisible to 8 places; Ethereum has a scale of 18, apparently

[BenMorel]

Personally I store my currencies as 3-letter strings in the DB, so the convenience of accepting a string for ISO currencies is nice. We'll do this then, strings for ISO only, and force objects for non-ISO currencies.

Ad side question: they do, for example Bitcoin is divisible to 8 places; Ethereum has a scale of 18, apparently

Interesting, thanks!

[VasekPurchart]

I tend to agree with @jiripudil here, I think Money::of('9.90', ISOCurrency::of('USD')); is fine, because most of the time it would be more likely Money::of('9.90', $country); and I think the sooner the actual string converts to the ISOCurrency instance, the better, because you have a type check closer to where the string is originating from. I think this is broader than the DBAL type, you should actually manipulate the string values as seldom as possible, usually just in "infrastructure" code as the types, or other types of serializers (when you need to send/get them in an API etc.). I have a lot of experience with these, because I have been using Enums based on same premise for a long time now.

As for the Money::parse('BTC 1.234000');, I think this is quite convenient, but personally I would also not cringe on it, but I think this should either be working for all currencies (even custom, if they are supported), or not be there at all.

[BenMorel]

Honestly I would miss Money::of('9.90', 'USD') if it wasn't there.

Your reasoning is correct and I do use DBAL types as well, but more sparsely since a few years ago, when I profiled a slow app and realized that the CPU time was mostly spent on hydrating properties that were only rarely used. Now I'm mostly using a "proxy" approach, where the property contains the raw value (amount = int, currencyCode = string), and the getter reconstructs the object only when requested.

As for the Money::parse('BTC 1.234000');, I think this is quite convenient, but personally I would also not cringe on it, but I think this should either be working for all currencies (even custom, if they are supported), or not be there at all.

I may remove parse() for the time being. Note that for of(), I do think it makes sense to provide support for currency code strings only for ISO currencies, as these are the only official currencies in use today. I personally don't mind that other currencies cannot be provided as strings.

[VasekPurchart]

when I profiled a slow app and realized that the CPU time was mostly spent on hydrating properties that were only rarely used

This is interesting, maybe have a look at consistence/consistence-doctrine#7? I was actually solving this last week and testing on a lot of data. This is using an PostLoadListener, because I need to have per field additional parameter (the resulting instance's class), so using only a DBAL type is unfortunately not possible. But when you look at the timings, it seems that the overhead in the implementation after optimization is nearly negligible (as seen from the last test where I "skipped" the execution).

But in any way as noted in the referenced issue, I think that in most cases, when this comes into focus as being a problem, a lot of time its the case that Doctrine is used in a way, that is not suited for.

Anyway the particular implementation does not matter from my point of view, because even if it was lazy-evaluated I would want it to be abstracted away from the other code, so that again everywhere only ISOcurrency is expected.

[BenMorel]

This was several years ago and the culprit was Zend_Date (ZF1). I had a Doctrine type for this class, and realized after a while that initialization of this class was very heavy.

It's definitely cheaper to instantiate your enums inconditionally, because they do not require a lot of code to be run. Money would be only slightly more expensive, but still, parsing a BigDecimal, parsing a Currency, doing some consistency checks and all, for a type that might not be used for sure, is a waste of resources IMHO. Mapping Doctrine types to proxy classes generated specially for your objects is another solution. But we're digressing :)

[BenMorel]

Summary: methods accepting Currency|string will only accept ISO currency codes from now on. Custom currencies will be provided as objects only.