Doctrine integration

[sagikazarmark]

There are two possible options in my mind.

Option 1

Same as in #327.

Use the money object itself as an embeddable.

Money\Money:
    type: embeddable
    fields:
        amount:
            type: string
    embedded:
        currency:
            class: Money\Currency

Money\Currency:
    type: embeddable
    fields:
        code:
            type: string
            length: 3

Pros:

• Simple
• Easy to integrate

Cons:

• Not really flexible

Option 2

Use a separate embeddable.

<?php

namespace Money\Doctrine;

final class Money
{
    private $amount;
    private $currencyCode;

    /**
     * @return \Money\Money
     */
    public function getValue()
    {
        return new \Money\Money($this->amount, new \Money\Currency($this->currencyCode));
    }

    /**
     * @param \Money\Money $money
     */
    public function setValue(\Money\Money $money)
    {
        $this->amount = $money->getAmount();
        $this->currencyCode = $money->getCurrency()->getCode();
    }
}

And the mapping:

<doctrine-mapping>
    <embeddable name="Money\Doctrine\Money">
        <field name="amount" type="string" />
        <field name="currencyCode" type="string" />
    </embeddable>
</doctrine-mapping>

Then in the entity:

class ObjectWithPrice
{
    /**
     * @var \Money\Doctrine\Money
     */
    private $price;

    public function getPrice()
    {
        return $this->price->getValue();
    }

    public function setPrice(\Money\Money $price)
    {
        $this->price->setValue($price);
    }
}

Since VO equality is not checked based on identity, but value this solution is valid. The advantage is the flexibility gained (less doctrine embed magic), but I am not sure it worth the extra effort.

@pamil I would love to hear your opinion as well.

[frederikbosch]

The second option does not make any sense. If you ask me: 1. use the value object directly, 2. implement a doctrine interface/extend doctrine.

Example: http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/cookbook/custom-mapping-types.html.

[sagikazarmark]

Doctrine type mapping is probably not an option since it allows you to have ONE column.

My concern is that option one includes too much Doctrine magic and reflection whereas in case of option too it's just scalars that it needs to take care of. I am not saying you are not right, just checking the options.

[Padam87]

I like option 1 better. My projects implement it in a similar way. Currency is a custom type, not an embeddable, and I use a decimal field for the amount.

[Padam87]

Option 3

https://github.com/Padam87/AccountBundle/blob/master/Resources/Money/doctrine/Money.orm.xml https://github.com/Padam87/AccountBundle/blob/master/Doctrine/Type/CurrencyType.php https://github.com/Padam87/AccountBundle/blob/master/Doctrine/Type/MoneyType.php

Pros:

• Almost as easy and simple as Option 1
• You get to choose your own storage solution... use the provided types, or register a different one. You want to store the amount as string, not double? Need 4 precision? No problem, swap it out.

Cons:

• The query time conversion is a bit iffy (PHP conversion would be OK by default)

[Padam87]

@geekdevs The bundle takes a very different approach, any chance it would be able to adapt?

[geekdevs]

@Padam87 absolutely, developing something similar to Option1 is in plans for next major version of the bundle. If it's implemented within moneyphp/money - even better, we can reuse it.

[geekdevs]

one thing to remember about storing amount as string is that sorting might be painful:

1, 10, 2, 20, 3, 4

[Padam87]

That is one of the reasons I'm using decimal in my projects, and why I would suggest Option 3, with the custom types + embeddables.

Sometimes (think of analytics) storing the "real" amount is necessary, and decimal allows to do that without the floating point issues...

[sagikazarmark]

Thanks for all the input here.

@geekdevs nice catch about the sorting issue. Using bigint as suggested by @frederikbosch might be an option.

I will try to play with it within a symfony bundle, but since it's a few lines of code (config), maybe we can ship it within this package?

[MichaelGooden]

This is the approach I am using in multiple projects: https://github.com/MichaelGooden/mdg-money-doctrine

Currency is put in as a custom doctrine type (which incidentally didn't suffer from the "BC break" in v3.0.0)

Money is then implemented as an Embeddable using bigint for the amount and the Currency custom type.

You can then do fun stuff like arithmetic in the database:

use Money\Currency;
use Money\Money;

    public function getBalance(UserProxy $userProxy, Currency $currency) : Money
    {
        $qb = $this->repository->createQueryBuilder('t');

        $qb->add('select', 'SUM(t.amount.amount)')
            ->andWhere('t.amount.currency = :currency')
            ->andWhere('t.userProxy = :user')
            ->setParameter('currency', $currency)
            ->setParameter('user', $userProxy);

        $balance = $qb
            ->getQuery()
            ->getSingleScalarResult();

        if (!is_numeric($balance)) {
            $balance = 0;
        }

        return new Money($balance, $currency);
    }

[bendavies]

nice @MichaelGooden, that's very close to what i have.

[sagikazarmark]

Hm, haven't thought about this. It's definitely a pro then for the embeddable solution.

[sovaalexandr]

Prefer option 1 but already had some gotcha. Doctrine embeddable don't fallow value semantics. Money object can be changed from outside if it's an embeddable. Consider case:

$amount = $balance->getAmount();
$report->withAmount($amount);
$entityManager->flush($report);
// Some time later
$entityManager->refresh($balance) // Here report could be also modified, if somebody changed balance amount (Money).
$entityManager->flush(); // Here modified report will be persisted with overwrite of previous data.

And the workaround is like:

public function withAmount(Money $amount) {
    $this->amount = clone $amount;
}

[frederikbosch]

Shall we close this one? It is out of the scope of this library. Maybe inside the scope of this organization. But it should not be an open issue here. Right? I am closing, until someone comes up with a good reason to have it open here.

[guiwoda]

@frederikbosch what's the status on the Doctrine integration library? Can I help you guys with that? I'm contributor in Laravel Doctrine and I have multiple projects mapping Money to Doctrine already.

Cheers!

[frederikbosch]

@guiwoda No idea. I am not using Doctrine. Maybe @sagikazarmark knows?

[sagikazarmark]

@guiwoda Contributions are certainly welcome. We had a few options listed, but unfortunately I didn't really have the time I wanted to spend on it.

A good first step would be doing some research based on this and the linked issues to see what options we considered, what the pros and cons are and decide based on that.

I am not against having multiple solutions supported as they could serve different purposes. Whether we want to ship an integration layer in this or in a separate package is up to discussion. I usually like keeping things separated.

So if you can find some time to pick up this issue that would be really great, many thanks in advance for the offer.

[guiwoda]

Alright, I'm on it!

You can see progress here: https://github.com/guiwoda/doctrine-money

[guiwoda]

Hey @sagikazarmark! I tagged you in an issue in the repo I linked in the comment above. I don't know if what I've done is enough to make it into a library or if you guys expect to have framework integration as well.

README explains the basic way to add mappings to Doctrine, so that should be enough for any usage. Anything else would be extra help for framework specific code (Laravel's ServiceProvider, for example.)

[jkobus]

Any follow-ups @sagikazarmark?

[programarivm]

Hi everybody,

Using bigints is mentioned a couple of times in this thread, and many others out there, as being a natural way to integrate Doctrine with money. However -- please correct me if wrong -- are not bigints inherintly unnecessary and risky?

Here are some facts:

1. Doctrine will convert bigints into strings
2. 64-bit machines handle bigints differently than 32-bit machines
3. Even in 64-bit architectures, a few developers have reported unexpected behavior when dealing with bigints
4. This is a tricky issue across different database vendors
5. Mapping SQL BIGINT to actual PHP integers is an issue
6. Dealing with bigints is all complexity that leads you to implement a BigMoney feature actually

I may be ignoring something, but I wonder why it is encouraged to use bigint when it turns out that it actually violates the KISS principle, increasing the surface of vulnerability of applications using it.

I think that in terms of simplicity, performance, scalability, and most importantly security, bigints should be used only when necessary; for example in astronomy apps. What is the point of using BIGINT as a rule of thumb? Keep in mind that the maximum BIGINT value is 18446744073709551615, and there's about $60 trillion on Earth -- which is $60,000,000,000,000.

For further information, please visit:

• Doctrine bigint from int to string

• How to deal with integer limits?

• Bug #65613 | Bigint is truncated to 32bits even on 64bit machines

• bigint truncated via PDO?

• How much money is in the world?

[frederikbosch]

@programarivm As mentioned on the frontpage of this library: we are using strings because we want to support big integers! This lib includes BigMoney. So that basically makes all your arguments invalid.

Two remain:

1. 64-bit machines handle bigints differently than 32-bit machines: use 64-bit if you want big int.
2. This is a tricky issue across different database vendors: not the responsibility of this library, but of your mapper.

[frederikbosch]

@programarivm See https://github.com/moneyphp/money#features for all features that the lib is offering you. Apparently more than you think ;)

[MichaelGooden]

@programarivm Unfortunately none of your points are valid. All your linked resources are either irrelevant, referring to problems fixed in modern versions or talking about a bug in a specific adaptor for a minor DB engine (that has not even had a stable release in 3 years.)

This library intentionally uses string for storing the integer, which ties in nicely with Doctrine. You can have a look at (one) way of doing it in my library

Your only vaguely relevant point is that machines running 32bit software are going to have a bad day. That said, if you are using some ancient 20 year old 32bit only server processor to handle millions in financials, you must be a bank. God help you.

[frederikbosch]

That said, if you are using some ancient 20 year old 32bit only server processor to handle millions in financials, you must be a bank. God help you.

Haha :+1:

[guiwoda]

Guys, you've been pretty quick to correct @programarivm in his bigint assertion, but I'm still waiting for your feedback on the Doctrine integration package!

[frederikbosch]

@guiwoda Since I do not use Doctrine, I am not the person to provide you with the feedback. I know Mark is on holidays at this moment. So it will have to wait. Maybe give him a @ mention in two weeks or so.

[MichaelGooden]

@guiwoda I use my own :D https://github.com/michaelgooden/mdg-money-doctrine

[Padam87]

I think this issue is too complex to have an universal solution. Embeddable seems like the way to go for the value object, but the field mapping is opinionated, no matter what you choose.

Therefore I strongly suggest this:

<doctrine-mapping>
    <embeddable name="Money\Money">
        <field name="amount" type="money" />
        <field name="currency" type="currency" />
    </embeddable>
</doctrine-mapping>

The library can provide a default money type (which may be a string, bigint or decimal), and a currency type.

Let's say the lib provides a bigint type, and I would prefer a decimal for my system. I could still use this lib with my custom decimal money type, I would just have to override the money type in the doctrine config.

[guiwoda]

I don't understand, I made the embeddable implementation in literally 5 minutes. If embeddables don't fit in your implementation, it will probably take you another 5 minutes to do whatever you see fit. It's not about solving everyone's problems, it's about providing at least one solution!

I've been using Money and Doctrine for live projects, and I've never had any issue at all with the bigint implementation. Decimal doesn't make any sense unless you are actually converting from cents to decimal and back every time.

Can't we at least have one official doctrine package, then iterate over it for other alternative? I'm asking for concensus here, not perfection.

[Padam87]

For you, decimal does not make sense, for me it does, there is probably someone who would prefer to use string... different use cases and opinions.

If you set it to bigint, you won't be able to override it, because doctrine won't allow you to change field types with overrides. If you create a custom type for it (you could even map it to bigint by default), it can be swapped out easily.

[guiwoda]

@Padam87 you don't need to override if you never actually add the wrong mapping at all. If bigint doesn't work for you, then don't add the provided mapping and that's it. You can still use the provided Currency mapping, tho.

I don't really care about your use case. Don't get me wrong, I'm not saying it's not valid. But, as it was pointed out earlier in this thread, this lib already made the choice to use strings and work with BigMoney. This is not about making the most flexible library out there with every possible money representation. I believe an initial bigint representation, aligned with Moneys internal properties, is as straightforward as it gets. It allows for database math (which string doesn't) and I believe the entire planet's gross income combined doesn't overflow a bigint representing cents.

[programarivm]

I'm still surprised that you'll find OK to use bigint arbitrarily without any good reason behind. My point is a general one, it is about sticking with a simplicity mindset when it comes to writing apps or using this or that library, because you want to control complexity in your apps.

To my understanding:

1. Dealing with money is not a necessary condition for using bigints
2. Don't get me wrong, but not all apps need the currency feature

By the way, I found another issue about setting up Doctrine and Money for PHP:

• Doctrine embedded stopped working when upgrading from 1.3 to 3.0

[programarivm]

On a different side note, what about BC Math and GMP vulnerabilities?

• CVE-2016-4537
• CVE-2017-7963

Please educate users by adding a disclaimer in the documentation: use bigints carefully, otherwise your app's vulnerability surface may increase for free.

[MichaelGooden]

@programarivm I find your vendetta most entertaining.

[frederikbosch]

@programarivm If you find a vulnerability, please report it. Your suggestions are not helping at all. Because of a CVE in the past, that does not mean functionality should not be used. We are still using SQL, are we? How many CVEs can you find on that topic? Please bother other people if you do not have anything useful to say.

[sagikazarmark]

@jkobus back from holiday, I will try to find some time next week for it.