New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CurrencyProvider? #30
Comments
Hi, this has been discussed to some extent in #5. I know this is a common requirement, however as you guessed, I’m against any type of global state... Maybe your best bet is to create a MoneyFactory that would replicate the I'm actually thinking that this MoneyFactory could be provided by the library; you’d only have to implement a CurrencyProvider that takes a currency code and returns a class MyCurrencyProvider implements CurrencyProvider
{
public function getCurrency($currencyCode): Currency
{
switch ($currencyCode) {
case 'BTC':
return new Currency(...);
// ... other currencies as required
}
// handle ISO currencies as normal
return ISOCurrencyProvider::getInstance()->getCurrency($currencyCode);
}
}
$factory = new MoneyFactory(new MyCurrencyProvider);
$money = $factory->of('0.0123', 'BTC'); // use this instead of Money::of() Would this work for you? |
Yeah! That's pretty good. MoneyFactory could easily be dependency injected etc. I like it. |
I actually gave it a shot, but I feel like this MoneyFactory does not bring much value: final class MoneyFactory
{
/**
* @var CurrencyProvider
*/
private $currencyProvider;
/**
* @param CurrencyProvider $currencyProvider
*/
public function __construct(CurrencyProvider $currencyProvider)
{
$this->currencyProvider = $currencyProvider;
}
public function of($amount, $currency, ?Context $context = null, int $roundingMode = RoundingMode::UNNECESSARY) : Money
{
if (! $currency instanceof Currency) {
$currency = $this->currencyProvider->getCurrency($currency);
}
return Money::of($amount, $currency, $context, $roundingMode);
}
public function ofMinor($minorAmount, $currency, ?Context $context = null, int $roundingMode = RoundingMode::UNNECESSARY) : Money
{
if (! $currency instanceof Currency) {
$currency = $this->currencyProvider->getCurrency($currency);
}
return Money::ofMinor($minorAmount, $currency, $context, $roundingMode);
}
} Sure, it allows you to do: $money = $factory->of('0.0123', 'BTC'); But is it much better than this? $money = Money::of('0.0123', $currencyProvider->getCurrency('BTC')); |
Another option could be to pass the CurrencyProvider to $money = Money::of('0.0123', 'BTC', null, RoundingMode::UNNECESSARY, $currencyProvider); |
Another advantage to the factory is that you could specify a different default context or rounding mode for all monies being created - for example I think I'd want to always use HALF_UP rounding for all the monies in a particular part of my app. $factory = (new MoneyFactory)
->withCurrencyProvider($provider)
->withRoundingMode(RoundingMode::HALF_UP);
$money = $factory->of('0.0123', 'BTC'); |
Interesting idea, though using a default rounding mode potentially means that you change the This may work, but I'm not sure if I see a value in using a default rounding mode! |
Fair enough - I can always just set up the factory idea in my own project if it's not shipped with the library 🙂 |
I pushed a proof of concept here: 62f7eea Comments welcome. |
Nice. I like it. Did you consider the |
I did, actually. Would you expect this to return a new instance? |
🤔 I don't think it would need to, as a factory. I think |
@BenMorel @francislavoie What do you think about this api
or we can create AggregateCurrencyProvider and use something like that
and use AggregateCurrencyProvider in currency class, but we will need ISOCurrencyProvider in constructorby default |
@Big-Shark That's creating a global context, which is usually bad ( You'll want to pass your CurrencyProvider around instead, probably using your DI container. |
@BenMorel yes, but when we will use |
@Big-Shark There is no way around the fact that you cannot have The only possible solution is a factory and DI. And if you need to instantiate a custom currency from within a value object, you need to refactor it so that the VO method accepts a Currency instance instead of a currency code, or takes the factory as a parameter. |
@BenMorel Maybe we can add CryptoCurrencyProvider in this repository too? I think we can add maybe one hundred most popular cryptocurrencies. |
@Big-Shark The problem with crypto-currencies is that they may come and go, there is no stable, official list of currencies like the one provided with the ISO organization! |
@BenMorel Yeah, I understand this, but usually, people use 10 or maybe 20 crypto currencies, but we can add 100 most popular cryptocurrencies, and I think this will be enaughf for the first time, but if some bode need more, he can create PR. |
I faced the same issue with crypto currency. How about making CurrencyInterface for Currency class and remove the final? In that case, anybody will have the opportunity to extend Currency class and make their own providers and redeclare the __construct to avoid redundant Currency declaration. |
I added a Composer script to edit the {
"scripts": {
"post-install-cmd": [
"@add-btc-to-brick-money"
],
"post-update-cmd": [
"@add-btc-to-brick-money"
],
"add-btc-to-brick-money": [
"sed -i '' \"s/return \\[/return \\[\\n 'BTC' => \\['BTC', 0, 'Bitcoin', 8\\],/\" vendor/brick/money/data/iso-currencies.php"
], NB: |
We have some currencies like BTC which are treated "equally" to other currencies in our database -- it would be nice if instead of needing to make a
new Currency
when making aMoney
, I could instead configure a currency provider globally (extending the ISO one) so I could instead just doMoney::of('0.123', 'BTC')
.Of course I feel dirty recommending any global state, but I'm not sure how else it could be implemented 🤔
The text was updated successfully, but these errors were encountered: