New rule: forbid replacing a persistent Doctrine collection field instead of mutating the collection

[stof]

When a field is a toMany relation, Doctrine relies on a special Collection implementation to track changes.

A common mistake is a write a setter that replaces the collection entirely instead of writing into the existing collection. This will make things that your collection field went from an empty list to its new content when checking for changes (as it lost all tracking for the existing content), which will do very bad things for the database data (in the best case, it triggers an error due to duplicate data in a unique index. In the worse case, it silently corrupts your data).

It would be great if phpstan-doctrine could prevent such mistakes for people using it.

[VincentLanglet]

Hi @stof, just found this issue and I had some questions. If I understand correctly

    public function setFoo(Collection $foo): self
    {
        $this->foo = $foo;

        return $this;
    }

shouldn't be implemented this way in an entity with a ToMany relation.

How would you write it then ? Does it mean we have to write:

    public function setFoo(Collection $foo): self
    {
        $this->foo->clear();
        foreach ($foo as $newFoo) {
            $this->foo->add($existingFoo);
        } 

        return $this;
    }

Or I misunderstood ?

[stof]

@VincentLanglet your suggested code would indeed work (modulo the wrong variable name used in the snippet, which would be caught by phpstan), but would still not be optimal if some items are both in the existing collection and the new one as you would still delete them from the join table for clearing and then adding them again. The best implementation is to compare both collections, to add the missing items and remove the extra items.