Orm: orphan removal removes children, not orphans

[stijn-at-work]

Upgrade from doctrine/orm 2.5.14 to 2.6.0 in our Symfony based application caused the removal of our EnvironmentAccesses records. Before, the orphanRemoval=true annotation worked as expected and only removed orphans. After upgrade the orphanRemoval annotation removed all children, not only orphans.

User parent

    /**
     * @ORM\OneToMany(targetEntity="AccountsBundle\Entity\EnvironmentAccess", mappedBy="user", cascade={"persist", "remove"}, orphanRemoval=true)
     * @Expose
     * @Type("array<AccountsBundle\Entity\EnvironmentAccess>")
     * @var AccountsBundle\Entity\EnvironmentAccess[]
     */
    private $environmentAccess;

EnvironmentAccess child

    /**
     * @ORM\ManyToOne(targetEntity="AccountsBundle\Entity\User", inversedBy="environmentAccess")
     * @ORM\JoinColumn(name="user_id", referencedColumnName="id")
     */
    private $user;

[Ocramius]

What is the code causing the issue looking like?

Are any of the removed records removed and re-added to the association, by chance? If so, this is expected behaviour.

An example of how this can be triggered is needed in order to write an integration test.

See
https://github.com/doctrine/doctrine2/tree/2.6/tests/Doctrine/Tests/ORM/Functional/Ticket for examples of what would be needed here.

[stijn-at-work]

There are no User records or EnvironmentAccess records removed and re-added.

The bug is gone when we remove 'orphanRemoval=true' from environmentAccess attribute.

Our hypotheses: a closer look at our (legacy) code shows that we have added both cascade="remove" and orphanRemoval=true instead of choosing for one of these mutually exclusive strategies.
Choosing for cascade="remove" strategy removes the bug.
Choosing for orphanRemoval=true does not though (!?).

EnvironmentAccesses are removed when we make a post or patch request to a User or EnvironmentAccess. e.g. POST [environment].localhost.com:8000/api/oauth/v2/token

[Ocramius]

@stijin-at-work that doesn't help us find the bug. What we need is a reproducible test scenario that works in isolation. See https://github.com/doctrine/doctrine2/tree/2.6/tests/Doctrine/Tests/ORM/Functional/Ticket for examples.

[Padam87]

@Ocramius I can confirm the bug exists. Will investigate.

[Padam87]

@Ocramius I believe the bug was introduced by the piece of code linked above.

At this point $orgValue is just an empty persistent collection. But thanks to this change it triggers a DELETE FROM table WHERE owner_id = x statement.

This only causes problems when orpanRemoval=true because of this part: https://github.com/doctrine/doctrine2/blob/0b7d878cd340fed70e22404daa9656bb06cec74a/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php#L35

I would advise everyone to temporarily revert back to 2.5, because this can cause data loss.

[Padam87]

6976

[Padam87]

https://github.com/symfony/symfony/issues/25937

[Steveb-p]

Looks like this hit me in my own app when updating to ORM 2.6. I've got a collection that is replaced by it's copy during form processing, and pre-existing entities are removed from database after redirect. It looks to me that replacing PersistentCollection instance with ArrayCollection triggers this effect, but will investigate further.

EDIT:

If I replace PersistentCollection with ArrayCollection in my setter, UnitOfWork's getScheduledCollectionUpdates() and getScheduledCollectionDeletions() report as above. Otherwise, if I simply check Collections against each other they appear as expected.

[Padam87]

@Steveb-p You should never replace collections, only manipulate them with add / remove.

This always was the preferred usage, but so far it had not caused any problems.

I linked the commit above which caused this change, but so far no one has acknowledged it as a BC break.

If you have a live system, revert back to 2.5 until you change your code to avoid data loss.

[Steveb-p]

@Padam87 Well, now I know :)
It didn't have any side-effects until this change. I couldn't find any explicit notices that mentioned that one should not replace Collection instances. In my case, I was using Collection's filter method to remove elements which went over a certain threshold and saved the result, overwriting the previous Collection, so it seemed like a good idea at the time ;)

[Padam87]

@Steveb-p Yeah, I didn't know before this either, it is not general knowledge at all.

I see it as a huge problem, because there are many ways a collection might get replaced. Your case is a perfect example, even a simple filter creates a new collection.

[Mvannes]

+1 in being affected by this issue, or at least very very closely related.

Mapping in the Entity 1

    /**
     * @ORM\ManyToOne(
     *     targetEntity="Entity",
     *     inversedBy="preferred_entity"
     * )
     * @ORM\JoinColumn(name="entity_id", referencedColumnName="id")
     */
    private $entity;

And mapping in entity 2

     /**
     * @ORM\OneToMany(
     *     targetEntity="PreferredEntity",
     *     mappedBy="entity",
     *     orphanRemoval=true
     * )
     */
    private $preferred_entity;

If the $entity field in Entity 1 is changed, orphan removal is triggered because the collection on the entity 2 side will be empty.

Reverting to a ~2.5 version fixes this.

[Padam87]

I don't think this BC break is gonna get acknowledged ever. It has been a year.

Revert to 2.5, and fix the usage of your collections.

The important thing is that you should not recreate or override your collections (orphaning a collection will trigger a delete statement for all entries), you must use the same object.

BAD

    public function setUsers($users): self
    {
        $this->users = new ArrayCollection(); // or []

        foreach ($users as $user) {
            $this->addUser($user);
        }

        return $this;
    }

GOOD

    public function setUsers($users): self
    {
        $this->users->clear();

        foreach ($users as $user) {
            $this->addUser($user);
        }

        return $this;
    }

[Mvannes]

Yeah damn, saw the november comments and thought it was a very active issue still. But it's actually way older.
We'll be fixing it in the above way eventually. Luckily it only affects the Entity I was creating today in this exact way due to my specific use case of switching relations around a bit.
Should be a fun afternoon to fix in our codebase. Thanks for the examples @Padam87

[damienfa]

Is this the same issue ?
https://stackoverflow.com/questions/47037493/manytomany-relation-orphanremoval
I think it's really well explained this way.

[Marco-Kawon]

upvote ...

Caused data-loss on a production environment for about 2 months. 12 Entities affected. Was working fine for years until we upgraded to doctrine/orm 2.6 in February. Not even our tests catched it, because the entity still returned the relations with valid database IDs and data, but they were deleted already from database. Only after reload entity again from database, we noticed data is not there anymore.

Thanks to @Padam87 for pointing me to the right direction

[ianef]

This is also causing me a lot of grief, I've upgraded 2 projects to 2.6 recently and had to remove the orphanRemoval from all my entities. I use JSON deserialisation extensively in my projects.

I find it hard to understand why this issue has not seen any positive action from the maintainers. Loss of data is a very serious issue, ignoring this is not exactly a responsible approach and potentially leaves developers open to financial harm through loss of customers data.

[albe]

We stumbled across this too.

What would work at least for the "easy" case of just replacing the collection with a new collection containing the same elements (or cloning), would be to add following code at https://github.com/doctrine/orm/blob/2.6/lib/Doctrine/ORM/UnitOfWork.php#L745:

if ($orgValue->toArray() === $actualValue->toArray()) {
    continue;
}

What would be the drawback of this? The equality check on the arrays should be relatively fast, no? Would that be viable @Ocramius ?