moneyphp / money

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

simplified static constructor #604

Closed jarektkaczyk closed 3 years ago

jarektkaczyk commented 4 years ago

Introducing simplified static constructor without any change in the existing functionalities:

new Money(12345, new Currency('EUR')) == Money::make(12345, 'EUR');

This example is trivial and doesn't really show the value gain. However, in real world often both amount and currency are dynamic. Current constructors are not perfect:

// ugly
Money::{$item->getCurrencyCode()}($item->getAmount())
// redundant Currency
new Money($item->getAmount(), new Currency($item->getCurrencyCode()))

// much more natural
Money::make($item->getAmount(), $item->getCurrencyCode())

Since in most cases currency is just a 3-letter iso code and the object representation is really only a low-level implementation detail, this change should preferably go to the original constructor + something between the lines of:

$currency = is_string($currency) ? new Currency($currency) : $currency;

if (!$currency instance Currency) {
    throw new \TypeError(
        'Argument 2 passed to Money/Money::__construct() must be an instance of Money/Currency or a string'
    ); // or trigger error if we still want to support eol php versions
}

$this->currency = $currency;

Such a change wouldn't be in principle BC breaking, but it could be problematic in some twisted cases using reflection. That is why this safe approach introducing a new static constructor.

PS: seems like cs & static tools are not configured to work on the relevant diff, but the whole codebase. Correct me if I'm wrong and there's anything I should do to make this PR green.

frederikbosch commented 3 years ago

I am not against this per se, but I do not see why it is needed either. @sagikazarmark, how about you?

frederikbosch commented 3 years ago

We will leave this as is. I don't see why not using the constructor.