Skip to content
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

[GH-4568] A new attempt at nullable embeddables based on Luis work. #8022

Open
wants to merge 4 commits into
base: 2.8.x
Choose a base branch
from

Conversation

beberlei
Copy link
Member

@beberlei beberlei commented Feb 15, 2020

This is a new attempt at solving #4568 which becomes a more present requirement to solve because of typed properties, see #8014

Its based of @lcobucci work on #1275, which was closed. The implementation of that PR put the burden on the UnitOfWork and introduced additional work in the hotpath. This new PR moves the logic into the ReflectionEmbeddedProperty.

TODO:

  • Add tests for the mapping drivers
  • Add more tests for the embeddable part, also nested embeddables.

Related #8015

@michaeldnelson
Copy link

I really hope this or something like it gets merged. I know people are calling this an edge case, but I deal with it all-the-time. It would clean up a lot of code for me. Fingers crossed, and thanks.

@rdotter
Copy link

rdotter commented Nov 25, 2020

@beberlei is this still a state that can be continued and do you need help on this PR?

And do you think these changes can still be in Doctrine version 2? :-)

@Padam87
Copy link
Contributor

Padam87 commented Feb 19, 2021

@michaeldnelson

I agree, this is definitely not an edge case.

I think every developer who works on a commercial app will encounter the problems around dealing with money.
Fowlers money pattern is an idustry standard when it comes to implementing a good system.
Money is best represented as a VO, so the natural thing to do is to handle it as an Embeddable in Doctrine.
Money(0, 'EUR') and null are not the same.

I have to deal with this all the time, and each and every time I check back if the original PR is merged. This started in 2015.

@Padam87
Copy link
Contributor

Padam87 commented Feb 22, 2021

Anyone currently waiting / in need of this feature, this is probably your best bet:

(This example is using brick/money Money value objects)

Create the nullable version of your VO.

/**
 * This is a stop gap solution for a common doctrine problem; embedded objects cannot be nullable.
 * See issues and PRs linked below.
 *
 * This VO acts as a proxy, which resolves into a \Brick\Money\Money object, or null
 *
 * @see https://github.com/doctrine/orm/pull/1275
 * @see https://github.com/doctrine/orm/pull/8022
 *
 * @ORM\Embeddable()
 */
class NullableMoney
{
    /**
     * @ORM\Column(type="decimal_object", precision=28, scale=2, nullable=true)
     */
    private ?BigDecimal $amount = null;

    /**
     * @ORM\Column(type="currency", nullable=true)
     */
    private ?Currency $currency = null;

    public function __construct(?Money $money = null)
    {
        $this->amount = $money ? $money->getAmount() : null;
        $this->currency = $money ? $money->getCurrency() : null;
    }

    public function __invoke(): ?Money
    {
        if ($this->amount === null || $this->currency === null) {
            return null;
        }

        return Money::of($this->amount, $this->currency);
    }
}

NOTE: Brick\Money\Money is final, so I had to recreate it, you might not need to, just simply extend and implement __invoke (or any other method name you would like)

    public function getMoney(): ?Money
    {
        return ($this->money)();
    }

    public function setMoney(?Money $money): self
    {
        $this->money= new NullableMoney($money);

        return $this;
    }

This way, doctrine always hydrates a NullableMoney object, which is esentially a proxy for the real VO.

The goal was keep encapsulation, so the entities wouldn't get over polluted with amount-currency pairs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants