PersistentCollection#remove() and PersistentCollection#removeElement() behave differently

Testcase and proposed fix at http://github.com/s9e/doctrine2/commit/5a01b50bdbffbfd59c3af6cfc94ddb5793dac329

Slighty changed the patch because removeElement() on the wrapped collection has to be called.

Merged your testcase also, fixed in master.

How about merging the code path differently then? You keep calling the right method on the wrapped collection but you move all the common code from PersistentCollection to its own method instead of duplicating it. Plus, it doesn't prevent you from implementing method-specific routines such as the ones mentionned in the TODOs.

class PersistentCollection
{
    public function remove($key)
    {
        return $this->removeFromCollection('remove', $key);
    }

    public function removeElement($element)
    {
        /*if ( ! $this->initialized) {
            $this->em->getUnitOfWork()->getCollectionPersister($this->association)
                ->deleteRows($this, $element);
        }*/

        return $this->removeFromCollection('removeElement', $element);
    }

    private function removeFromCollection($method, $arg)
    {
        $this->initialize();
        $removed = $this->coll->$method($arg);
        if ($removed) {
            $this->changed();
            if ($this->association !== null && $this->association->isOneToMany() &&
                    $this->association->orphanRemoval) {
                $this->em->getUnitOfWork()->scheduleOrphanRemoval($removed);
            }
        }

        return $removed;
    }
}

dynamic method calls are to slow and this gives no additional benefit.

i just realize that there might be another bug in the removeElement() just that i read the docblock changes yesterday. it returns true and not the element if the operation was successful. I have to reevaluate this...

It's been a long time since I've last heard that dynamic method calls were slow. That may have been true in older versions of PHP (4?) but nowadays the difference is negligible; I've just run a quick informal test showing a ~20ms difference over one million calls on PHP 5.3.3 in CLI .

I think that code deduplication is a strong benefit. In fact, that's because code was duplicated instead of reused that this ticket exists. The next time any code is added to or removed from one of these methods, it will have to be mirrored to the other one or you will run into the same issue.

Wrt the return value, I had noticed earlier that ArrayCollection#removeElement's docblock mentionned returning a boolean, but the Collection interface declares returning the object and that's what the implementation does too, so I assumed its docblock simply needed to be updated. Shouldn't it use

just like PersistentCollection does? Interestingly, it's another case of deduplication.

fixed, again!

I see that PersistentCollection#removeElement has been modified to accomodate the underlying collection's removeElement() potentionally returning a boolean. Is the Collection interface's definition going to be changed so that removeElement() returns a boolean? I think that would be confusing for the end user because we'd then have:

• add() takes an element as argument, returns a boolean
• remove() takes a key as argument, returns an element
• removeElement() takes an element as argument, returns a boolean