orm icon indicating copy to clipboard operation
orm copied to clipboard

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

Open beberlei opened this issue 5 years ago • 8 comments

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 https://github.com/doctrine/orm/pull/8015

beberlei avatar Feb 15 '20 13:02 beberlei

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.

michaeldnelson avatar Aug 17 '20 04:08 michaeldnelson

@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? :-)

rdotter avatar Nov 25 '20 13:11 rdotter

@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 avatar Feb 19 '21 02:02 Padam87

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.

Padam87 avatar Feb 22 '21 22:02 Padam87

Hey, I just ran into the issue this PR is trying to solve, and this seems to be the only open PR/issue about nullable embeddables. Therefore I kindly want to ask what the state of this is. Is there any chance this will get merged (despite the fact that it is quite old)? Or is there an alternative PR I have missed, that is more recent? My workaround for now is that I don't use embeddables if they are nullable, which is not really nice...

danrot avatar Aug 05 '24 20:08 danrot

Hope this will be merged at some point

rela589n avatar Nov 19 '24 10:11 rela589n

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. If you want to continue working on it, please leave a comment.

github-actions[bot] avatar Feb 18 '25 03:02 github-actions[bot]

Hi, @beberlei ,

Thank you for all the effort made during doctrine orm development. This is indeed powerful tool used by millions, yet sometimes people do not fully understand the cost behind it.

Regarding the current PR, I see that you have tried to implement nullable embeddables - a very needed feature for ORM, yet something didn't work out as expected as it's never been merged.

From the point of community, I would like to earnestly ask to continue on this PR to complete this feature. It will indeed make our software better, as it communicates architectural design much better, and I really hope for this feature to be completed at some point.

Thank you for the time once again, and have a nice day :slightly_smiling_face:

Yours sincerely, Yevhen

rela589n avatar Feb 21 '25 10:02 rela589n

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. If you want to continue working on it, please leave a comment.

github-actions[bot] avatar May 23 '25 03:05 github-actions[bot]

@github-actions I think it would be a pity if this does not make it into the product 🙂

danrot avatar May 23 '25 06:05 danrot

@greg0ire is there a new attempt in the pipe?

sukei avatar Jul 04 '25 08:07 sukei

Sorry, it was closed because I deleted the branch. Feel free to reopen against another branch.

greg0ire avatar Jul 04 '25 14:07 greg0ire