DDC-3730: Embeddable hydrates to object instead of null

[doctrinebot]

Jira issue originally created by user pcnc:

Good afternoon,

When hydrating an Embeddable with nullable attributes the result is an instance of the Embeddable class , this is obviously correct and expected behavior.

If all the attributes are null the hydrator will still return an instance of the class with all of its properties null , even if I persist and flush my Entity with the Embeddable being set as null .

For clarification :

class MyEntity
{
    protected $myEmbeddable;

    public function setMyEmbeddable(MyEmbeddable $myEmbeddable = null)
    {
        $this->myEmbeddable = $myEmbeddable;
    }
    [...]
}

$newEntity = new MyEntity();
$newEntity->setMyEmbeddable(null);

$em->persist($newEntity);
$em->flsuh($newEntity);

Calling $newEntity->getMyEmbeddable() will return an instance of the MyEmbeddable object with all of it's attributes set to null .

I expected $newEntity->getMyEmbeddable() to be NULL .

Can someone clarify is this is expected behaviour ? In case it is , how can I achieve what I'm looking for ?

Best regards

[doctrinebot]

Comment created by eugene-d:

See #1275

[boekkooi]

This is actually really confusing definitely when you combine it with the docs

[afoeder]

I must say I find this rather a bug than an improvement... agree with @boekkooi regarding the docs...

[Ocramius]

Can anyone confirm if this is a dupe of #4670 and #4568?

[afoeder]

well this is #4568; and #4670 is rather a duplicate of #1275, both actually "features" or "improvements".

This here however is a Bug because embeddables whose all properties are nullable and being null hydrate to an empty object when being stored as null. This is misleading (saving != retrieving) and not according to the docs.

[Ocramius]

@afoeder the order of the tickets is scrambled due to the fact that they were imported from Jira in December, heh

[Harrisonbro]

Hi @Ocramius

Has there been any further discussion on this topic? We've just hit into the same problem as described here on our first Doctrine project.

Let me know if there's anything we can do to help — provide usage examples, code samples, discussions, etc.

[Ocramius]

@Harrisonbro as it currently stands, doctrine will not support nullable embeddables. That functionality may be implemented later, by implementing embeddables as hidden one-to-one records.

[Harrisonbro]

OK. Is there any way for us to implement this on a case-by-case basis (eg. by hooking into the hydration process of an embeddable somehow) so we can manually check whether specific embeddables have enough data in the database to be considered 'valid' and therefore hydratable? Obviously we're not keen to allow value objects to be instantiated in an invalid state and then check an isValid method.

[Ocramius]

so we can manually check whether specific embeddables have enough data in the database to be considered 'valid' and therefore hydratable?

Then just use the lifecycle system to (de-)hydrate VOs on your own, no?

[Harrisonbro]

Then just use the lifecycle system to (de-)hydrate VOs on your own,
no?

Do you mean lifecycle callbacks — maybe postLoad — as shown in
http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/events.html#lifecycle-callbacks?
Looks like those only work on entities, not value objects, as far as I
can see? Eg. if I used postLoad an embeddable will already have been
hydrated with invalid data (if the data in the database is all null,
for example). Alternatively, if I move the VO properties onto the entity
directly I’ve lost the nice encapsulation that embeddable so usefully
provides (eg. if I had a Product entity with a SalePrice with 2
properties, value and currency I’d have to move those 2 properties
onto the entity. Whilst I could then have those properties be private
and do an is_null check for those 2 properties before instantiating
and returning the VO from getSalePrice() : SalePrice { … } it does
rather compromise my entity.

I’m almost certainly missing something here, sorry. Rather new to
Doctrine so still learning!

[Ocramius]

@Harrisonbro the idea is to NOT use embeddables there, and use a lifecycle listener to replace fields with embeddables then (manually). Doctrine will not implement nullability for embeddables for now.

[Harrisonbro]

@Harrisonbro the idea is to NOT use embeddables there, and use a lifecycle listener to replace fields with embeddables then (manually). Doctrine will not implement nullability for embeddables for now.

OK, gotcha.

So in the example I gave — a Product entity which wants to use a SalePrice VO with 2 fields, amount and currency — would you suggest simply putting a sale_price_amount and sale_price_currency property on the Product entity, make those private, and then have Product::getSalePrice() : SalePrice first check whether the 2 properties are null before attempting to instantiate and return the VO?

If so, that seems workable and means the entity is responsible for checking if the VO should be instantiated (rather than having the VO able to be ‘invalid’ and have to implement an isValid() method).

Example code of what I mean:

    class Product
    {
        private $sale_price_amount;
        private $sale_price_currency;

        public getSalePrice() : SalePrice
        {
            if (
                is_null($this->sale_price_currency) || 
                is_null($this->sale_price_amount)
            ) {
                return null;
            }

            return new SalePrice(
                $this->sale_price_currency, 
                $this->sale_price_amount
            );
        }
    }

Is that something like what you’re suggesting instead of nullable embeddables?

[Ocramius]

So in the example I gave — a Product entity which wants to use a SalePrice VO with 2 fields, amount and currency — would you suggest simply putting a sale_price_amount and sale_price_currency property on the Product entity, make those private, and then have Product::getSalePrice() : SalePrice first check whether the 2 properties are null before attempting to instantiate and return the VO?

Correct.

Basically, since this is a scenario that Doctrine can't cover right now (because of how RDBMS DDL works), you can just implement it in userland for the few times where it pops up.

[Harrisonbro]

OK great, thanks a lot for the help.

[afoeder]

Thanks, I silently kept up reading your conversation :)
At the moment, my workaround is the following:

class Site
{
    /**
     * @var DomainName
     * @ORM\Embedded(class="DomainName", columnPrefix="domain_")
     */
    private $domainName;

    public function domainName()
    {
        return ((string)$this->domainName === '' ? null : $this->domainName);
    }
}

/**
 * @ORM\Embeddable
 */
class DomainName
{
    /**
     * Note this is only nullable in order to get the whole embeddable nullable (see [1] and [2]
     *
     * @var string
     * @ORM\Column(nullable=true)
     * @see http://doctrine-orm.readthedocs.org/projects/doctrine-orm/en/latest/tutorials/embeddables.html#initializing-embeddables [1]
     * @see https://github.com/doctrine/doctrine2/pull/1275 [2]
     */
    private $name;

    /**
     * Note this is only nullable in order to get the whole embeddable nullable (see [1] and [2]
     *
     * @var string
     * @ORM\Column(name="escaped_name", nullable=true)
     * @see http://doctrine-orm.readthedocs.org/projects/doctrine-orm/en/latest/tutorials/embeddables.html#initializing-embeddables [1]
     * @see https://github.com/doctrine/doctrine2/pull/1275 [2]
     */
    private $escapedName;

    public function __construct($name)
    {
        Assertion::notEmpty($name, 'The domain name must be provided.');
        Assertion::regex($name, '/^(?!www\.)([\pL\pN\pS-]+\.)+[\pL]+$/u', 'The domain name "%s" must be a valid domain name without the www. subdomain, but might have others.');

        $this->name = $name;
        $this->escapedName = static::escapeDomainName($name);
    }

    public function containsSubdomain()
    {
        return substr_count($this->name, '.') >= 2;
    }

    public static function escapeDomainName($name)
    {
        return preg_replace('/\./', '-', $name);
    }

    public function __toString()
    {
        return (string)$this->name;
    }
}

[Harrisonbro]

I like that approach, @afoeder — you still do an is_null check in the entity's getter method (Site::domainName() in your case) but you can still use an embeddable rather than having to hydrate your VOs yourself.

I suppose the major downside of your approach is that you do still have a VO in an inconsistent state, whereas if you don't let Doctrine hydrate the VO as an embeddable you avoid this; a bit more boilerplate & checking code, but you never have a VO in an invalid state.

Really it's just a trade-off between the 2 options. Others reading this should just be aware of the 2 options and their various merits.

[BenMorel]

I would have preferred to comment on #1275, but the present issue has the benefit to be still open.

My 2 cents on the sensitive subject of nullable embedded properties:

• when the Embeddable has at least one non-nullable @Column, and this field is null in the database, there should be no ambiguity and null should be assigned to the embedded property. Otherwise (currently!) you get an empty, invalid value object that has non-nullable properties set to null. IMHO, the current implementation is broken here.
• when all @Column in the Embeddable are nullable, there should be a boolean setting in the @Embedded annotation that controls whether or not you want an empty value object or a null value when all fields are null in the database. Your choice.

Finally, you only have a real problem when you have a fully nullable embeddable, and want to make the distinction between a null property and an empty object. People have suggested to add an extra column in the table, which would work, but would add a ton of complexity for what I think is an edge case. To clarify, I think this edge case should not be supported by Doctrine.

The previous two bullet points, however, I would strongly suggest working on them ASAP. I'll be happy to help, provided that lead developers are happy with the concept.

[Harrisonbro]

Thanks for that clear explanation, @BenMorel. I quite agree with your suggested specification of how Doctrine should behave, and that it should be worked on. This issue is the top priority I'd like to see addressed in doctrine.

I too would be happy to help out with the development and testing of this, if the Doctrine team agree.

[havvg]

We came across the same issue, in our domain a StreetAddress is optional, but if given, it has to contain all fields. All fields on the Embeddable are nullable: true, so the DB is working. The domain is ensuring valid state. So I created that listener to make Doctrine load the objects the way the domain contains objects prior persisting: https://gist.github.com/havvg/602055f1488271f68e5bc82f9a828b4d

Well, it only requires knowledge on the embeddable itself, but easy workaround for now. I hope this helps other developers until the issue will be resolved by Doctrine.

[szepczynski]

@havvg How to use this workaround?

For example I have User entity with embeddable class Gender. Something like that:
https://gist.github.com/szepczynski/d3028eb9f92fd7aadd08a578c7a92ad3

I know that I need the User entity should have registered postLoad listener NullableEmbeddableListener but I have no idea how to register it. Can you provide any example? I guess that I need somewhere call addMapping?

[Evertt]

@BenMorel did you by any chance work on a PR for this? I'd really hope to have this "bug" resolved, but I don't understand doctrine well enough to be able to write a pretty PR for this issue.

[BenMorel]

@Evertt Not yet, and I won't until I get the green light from lead developers. I've invested a lot of time in other pull requests, that have been open for years and are still not merged. I can't waste any more time on this project I'm afraid!

[Evertt]

@BenMorel that's too bad and I totally understand! It's sad that huge projects like these at some point seem to slow down to a point that it just seems frozen.

[BenMorel]

That's the dark side of open source: projects rely solely on the free time developers can invest in them, and at some point they're just too busy on other businesses and/or family life to carry on with developments.

I, too, feel like Doctrine is slowing down; it's just unfortunate that there aren't enough (available) lead developers to keep up the pace with pull requests: many developers are there to offer their help, but without enough consideration from project leaders, it's just wasted brain processing time.

[theofidry]

@BenMorel the pace did slow down a bit last year, but the last months have been quite active :) Also see #6211

[szepczynski]

doctrine 2.x is frozen because doctrine 3 is actively developing (I read somewhere post by @Ocramius)

[Evertt]

@szepczynski except that there's no ETA for doctrine 3 so that could take years for all we know. If it really takes that long it would be nice for improvements to still be added to doctrine 2.

[Ocramius]

If it really takes that long it would be nice for improvements to still be added to doctrine 2.

It would make it a mess to migrate these additions to something completely redesigned. From what I can see in the last dozen releases, doctrine functionality already abundantly covers the 90% of use-case scenarios, so we could even call it "feature complete", if it wasn't for some rough edges that you encounter when you explore more shady features.

[Evertt]

@Ocramius I wouldn't call this issue right here a "shady feature". I think this is a very essential part of the embeddables system.

[Ocramius]

Right, and embeddables have barely been added in 2.5, and are already removed in develop (3.x), as their fundamental internal working mechanisms need to be rewritten

[BenMorel]

@Ocramius Sorry to pollute this thread, but would the transaction object and default lock mode fit in 3.0?

[Ocramius]

@BenMorel most likely, yes

[Evertt]

Right, and embeddables have barely been added in 2.5, and are already removed in develop (3.x), as their fundamental internal working mechanisms need to be rewritten

@Ocramius I'm not sure I understand you right. Do you mean they will come back in 3.x after their internal working mechanisms have been rewritten?

[Ocramius]

@Evertt yes, but likely as completely rewritten/redesigned.

[Harrisonbro]

@Ocramius Are there ways we can help the development of Doctrine, either v2 or v3? It's a tool we all use so would love to,support development if a can.

(Sorry to pollute this thread but not sure where else to write.)

[Ocramius]

@Harrisonbro https://github.com/doctrine/doctrine2/milestones/3.0

Let's please stop going further OT. If you have a question, make a new issue.

[dbu]

FTR: there is a small 3rd party library that provides a listener for setting embedded entities that are all null to null (the gist that was discussed above): https://github.com/tarifhaus/doctrine-nullable-embeddable

[alcaeus]

@BenMorel With all due respect, I have to go r/quityourbullshit on you here:

I've invested a lot of time in other pull requests, that have been open for years and are still not merged. I can't waste any more time on this project I'm afraid!

For Doctrine 2, there are 26 pull requests from you: 2 Open, 2 Closed without merge (one was fixed differently, one would introduce a lot of pain with future pull requests), and 22 merged.
For DBAL, there are 15 pull requests from you: 1 open, 2 Closed without merge, and 12 merged. A quick peek into other repositories (common, annotations, bundle, etc.) shows merged pull requests only.

Feel free to point out pull requests that you are waiting to get merged, but please don't say stuff like that without backing it up when other people sacrifice lots of free time to get you free software. Thank you.

[BenMorel]

@alcaeus Thanks for investing your time investigating my contributions to this repository.

You have already wonderfully pointed out my unmerged pull requests, you just forgot to mention their opening date:

• Introduce a Transaction object dbal#634 : opened 3 years ago
• Add a default lock mode to the EntityManager #949 : opened 3 years and 6 months ago

I did invest quite a lot of time on these two pull requests, and since 2 years I am getting next to no feedback, despite multiple attempts to draw attention from the team.

Yes, I had PRs merged as well (did I ever say I didn't?), but the lack of feedback on these last two blocked my motivation to contribute further to this project for now.

[...] please don't say stuff like that without backing it up when other people sacrifice lots of free time to get you free software.

I hope I have backed it up to your taste, and rest assured that I know quite well what it's like to sacrifice some time on free software.

Cheers.

[alcaeus]

@BenMorel the way you made it sound was that people completely disregarded your work, which they don't. That's why I took the time to look through your contributions to see what's going on.

As for the pull request you mentioned, (please keep in mind that I'm not an ORM guy) it looks like it's a fairly large pull request that touches transaction logic in the DBAL, affecting most of what it (and thus ORM) does. Pull requests like that take time to review and evaluate. I'm not trying to make excuses for other people here, I'm just hoping you can bear with the people maintaining it. That said, there is currently a development push going for 3.0, so maybe there's an opportunity to get those pull requests merged.

Open source projects need contributors to move forward and evolve, especially when maintainers have little or in some cases no time for the project. However, writing pull requests is just a small part of that work - most of it is looking at issues, figuring out what's going on, evaluating pull requests and making sure your user base is not going to burn you at the stake because you messed up. That can be very time consuming and tiring, so unfortunately, large pull requests are often the first to stay open simply because of the effort it takes to review and merge them.

[ryall]

@BenMorel I agree with you. The Doctrine mantra I keep hearing seems to be along the line of "Well, it kind of works in most cases and changing things is hard so let's not do it".

They say a picture says a thousand words and I think this picture sums up my feelings about Doctrine right now:

Disclaimer: Owners, don't take offense - it's meant in a lighthearted way and is just my personal opinion. I know you work hard on this, and for that I thank you. I just disagree a bit with the general negativity that I personally see towards any major changes. I know you're working on 3.0 but Symfony and others are leaving you in the dust. If there's too much work, consider giving other contributors more rights or bumping the major version more often so that there can be BC breaks.

[Ocramius]

@ryall I think that's the nail on the coffin then.

Here's the exit:

Closing and locking.