-
-
Notifications
You must be signed in to change notification settings - Fork 102
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: |
Coming late to the discussion, here’s my analysis: First, we need to accept that the Secondly, with the main methods for object creation being static, proper dependency injection to configure a global default behavior becomes impossible. Solution 1: Inheritance Approach The quickest solution without a major refactor would be to introduce inheritance as an extension point. This allows users to provide custom currency providers tailored to their use cases. The library itself could leverage this to introduce new types of currencies, such as the historic ISO 4217 currencies discussed here: #93 (comment). Here’s a proof of concept: #94. Solution 2: Composition Approach Alternatively, we could go all-in on a composition-based design, which I believe offers more flexibility in the long run. Here's a conceptual outline: /** Currency */
interface CurrencyInterface
{
public function getCode(): string;
public function getFractionalDigits(): int;
}
class CurrentIsoCurrency implements CurrencyInterface
{
public function getCountry(): string {}
}
class HistoricIsoCurrency implements CurrencyInterface
{
public function getWithdrawalDate(): \DateTimeImmutable {}
}
class CryptoCurrency implements CurrencyInterface
{
// No specific methods
}
/** Currency Providers */
interface CurrencyProviderInterface
{
public function getByCode(string $code): ?CurrencyInterface;
}
class IsoCurrencyProvider implements CurrencyProviderInterface
{
public function getByCode(string $code): ?CurrencyInterface {}
public function getByCountry(string $country): ?CurrentIsoCurrency {}
}
class CryptoCurrencyProvider implements CurrencyProviderInterface
{
public function getByCode(string $code): ?CurrencyInterface {}
}
class MultiCurrencyProvider implements CurrencyProviderInterface
{
/** @var CurrencyProviderInterface[] */
private array $providers;
public function getByCode(string $code): ?CurrencyInterface {}
}
/** Money */
interface MoneyInterface
{
public function getCurrency(): CurrencyInterface;
public function getAmount(): int;
public function add(MoneyInterface $money): MoneyInterface;
public static function of($amount, $currencyCode): self;
}
trait MoneyTrait
{
public function getCurrency(): CurrencyInterface;
public function getAmount(): int;
public function add(): int {}
}
class CurrentIsoMoney implements MoneyInterface
{
use MoneyTrait;
public static function of($amount, $currencyCode): self
{
return (new IsoCurrencyProvider())->getByCode($currencyCode);
}
}
class CryptoMoney implements MoneyInterface
{
use MoneyTrait;
public static function of($amount, $currencyCode): self
{
return (new CryptoCurrencyProvider())->getByCode($currencyCode);
}
} With this composition model, you get much more flexibility. For example: CurrentIsoMoney::of(1000, 'USD');
$currentIsoMoney = CurrentIsoMoney::of(2000, 'EUR');
$conversion = fn(MoneyInterface $baseMoney, MoneyInterface $targetMoney) => 1;
$conversion(HistoricIsoMoney::of(3, 'DEM'), CryptoMoney::of(2, 'BTC'));
// Because of composition, `getCountry()` doesn’t exist on a CryptoMoney instance.
// This avoids the need for UnsupportedCurrencyException due to poor abstraction.
CryptoMoney::of(2, 'BTC')->getCountry(); This approach allows us to handle various currency types—ISO, historic, crypto—without forcing unrelated methods (e.g., |
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: