brick / money

A money and currency library for PHP
MIT License
1.61k stars 96 forks source link

update "Using an ORM" example for better typing #44

Closed bendavies closed 2 years ago

bendavies commented 3 years ago

update "Using an ORM" example for better typing

It is important to use string as the property type with bigint, as this is the type that will be returned by the RDBMS. If int is used, Doctrine will always compute a diff in the price field, issuing a useless update from "100" (string) to 100 (int).

BenMorel commented 3 years ago

Hi, I'm not sure what you're trying to achieve here: int is big enough for most purposes (up to ~40,000,000 USD / EUR / ...) so it's perfectly fine as an example, and people are clever enough to use a larger field if need be.

Also, (string) $price->getMinorAmount() is not a good idea, because getMinorAmount() may return a decimal number, if the scale of the Money exceeds the default scale of the currency!

bendavies commented 3 years ago

hi @BenMorel !

Thanks for the reply, i'll try explain my rational: PHP max int size: 2147483647 (32bit), 9223372036854775807 (64 bit) postgres max column sizes: 2147483647 (int), 9223372036854775807 (bigint)

so as you say, using int in postgres is good for up to +-$21,474,836.47. a unacceptably small value for most systems (at least mine), so we must use bigint.

now, if we use bigint as the field type, the value will come back from postgres as a string, and hydrated by doctrine as a string.

if your property is specified as an int:

/** @ORM\Column(type="bigint") */
private int $price;

upon hydration, the value will be cast from a string to int, BUT, doctrine's internal stored presentation will be a string, so on flush, doctrine will see that your $price value is not the same as the value that came back from postgres, (string) 100 !== (int) 100, so it will perform a useless update.

so, to work around this we must use a string column type

/** @ORM\Column(type="bigint") */
private string $price;

Also, (string) $price->getMinorAmount() is not a good idea, because getMinorAmount() may return a decimal number, if the scale of the Money exceeds the default scale of the currency!

can you show me how this might occur please? I thought this library would always keep the scale in line with the currencies scale, 2 in the case if USD. (FYI my system never works in fractions of a cent.)

to summarize:

  1. i must use bigint in postgres
  2. i must use string in php, to prevent doctrine making useless update queries.

Many thanks!

bendavies commented 3 years ago

hey @BenMorel

sorry for the ping - any thoughts on the above? I could use the help!

Many thanks!

BenMorel commented 2 years ago

@bendavies Getting back to this, sorry for the late reply.

Also, (string) $price->getMinorAmount() is not a good idea, because getMinorAmount() may return a decimal number, if the scale of the Money exceeds the default scale of the currency!

can you show me how this might occur please?

Sure:

$money = Money::of('1.2345', 'USD', new CustomContext(4));
echo $money->getMinorAmount(); // 123.45

TBH, I'm not sure what we're trying to do here: I think the example in the README is a good guideline for a lot of use cases; yours may be slightly different, and it's OK to adapt your persistence to support larger amounts, but I'm not sure whether we should add Doctrine-specific oddities to the README, especially as even though it mentions "an ORM such as Doctrine", the example is generic enough so that it could apply to any ORM!

bendavies commented 2 years ago

@bendavies Getting back to this, sorry for the late reply.

Also, (string) $price->getMinorAmount() is not a good idea, because getMinorAmount() may return a decimal number, if the scale of the Money exceeds the default scale of the currency!

can you show me how this might occur please?

Sure:

$money = Money::of('1.2345', 'USD', new CustomContext(4));
echo $money->getMinorAmount(); // 123.45

Thanks for that.

Since in my system, I know we are never dealing with fractional cents, I think I can safely go ahead here and slightly modify my example to this?

class Entity
{
    /** @ORM\Column(type="bigint") */
    private string $price;

    /** @ORM\Column(type="string", length=3) */
    private string $currencyCode;

    public function getPrice() : Money
    {
        return Money::ofMinor($this->price, $this->currencyCode);
    }

    public function setPrice(Money $price) : void
    {
        $this->price = (string) $price->getMinorAmount()->toInt();
        $this->currencyCode = $price->getCurrency()->getCurrencyCode();
    }
}

(I've just added >toInt())

This way, i'll get a RoundingNecessaryException if for some reason, fractional cents have crept in.

Thanks again, closing here.

BenMorel commented 2 years ago

You're on the safe side, provided that you're running PHP 64-bit; otherwise toInt() will fail after 20 million dollars ;-)