!!! BUGFIX: Prevent collection setters from overwriting collections

[albe]

Overwriting a collection will lead to all related entities being deleted as of Doctrine ORM 2.6+, if the relation has orphanRemoval, as do all *ToMany relations to non-aggregate roots in Flow.

This is marked breaking, because if you have setters for collections in your domain models that simply replace the collection property, this will now throw an exception (to avoid leading to possible data loss). You should replace your collection setters with code like this:

/**
 * @ORM\OneToMany(mappedBy="parent")
 * @var Collection<MyEntity>
 */
protected $myCollection;

public function __constructor()
{
    $this->myCollection = new ArrayCollection();
}

public function setMyCollection(Collection $newCollection)
{
    foreach ($this->myCollection as $child) {
        if (!$newCollection->contains($child)) {
            $this->myCollection->removeElement($child);
        }
    }
    foreach ($newCollection as $child) {
        if (!$this->myCollection->contains($child)) {
            $this->myCollection->add($child);
        }
    }
}

i.e. only initialize the collection in your constructor and never replace it in a method, only add/removeElement to it.

Related to #2159

[kdambekalns]

Travis trigger…

[kdambekalns]

@bwaidelich @albe This is the lkind of important breaking bugfix we should consider now - because if it's too bad for 5.3 it should definitely go into the next major, IMHO.

[bwaidelich]

I can't really judge how invasive it is in practice and thus whether this should go into 5.3. I fully trust you guys here. The CI doesn't though, functional tests fail atm :)

[albe]

Now the test exactly shows what will break and how. We do not follow those semantics of "don't replace a collection" in our own test entities.

Edit: So the question is as Karsten said - we need to decide if we want to put this into 5.3+ - if not, let's get this into master

[sorenmalling]

If in doubt, leave it out. To be fair, it's a different release, made by a different team of RM, so we will be putting a responsibility on them (but still us as a whole) and more support for a version that they/we don't want. We can take full responsibility for it in 7.0 and future at least

[albe]

So, should we retarget 7.0 and merge this in @bwaidelich ? I guess @sorenmalling is somewhat right. Plus, as I think I said - we could still backport this after the release iff we do find it sensible

[bwaidelich]

So, should we retarget 7.0 and merge this in @bwaidelich ?

I'd say: retarget for master. I hope that we are really close to tag the 7.0 release and I don't want to add any more potentially breaking changes to it

[mficzel]

If we have to make this change at some point it better be now.

[albe]

Yeah, shifting this onto 8.0 will be... well, a bit of a let-down if we do think we should help our developers not fall into this trap too easily. Technically, it's even our fault that they are effectively opt-into this behavior (via our orphanRemoval policy) - and there's no escape hatch (can't "unset" orphanRemoval on non-ARs... - see discussion in https://github.com/neos/flow-development-collection/issues/1127)

Edit: Assuming we won't agree to have this as a BUGFIX in 5.3+ of course

[albe]

Okay, so after talking with @bwaidelich again and going through this, trying to add a test, we found that this would for example also break the new Account with the transient Roles property. So we'd need to check reflection here (😬) or somehow otherwise pass through non-entity collections. All in all this is too hot a change to rush now, so we agreed to proceed as follows:

• retarget onto 5.3 again
• add a test that clearly covers the ill-behaviour
• possibly put the exception throwing behind a feature flag and enable it by default in 7.1 only