DDC-735: PersistentCollection#remove() and PersistentCollection#removeElement() behave differently #5248

Closed
doctrinebot opened this Issue Aug 7, 2010 · 9 comments

2 participants

@doctrinebot

Jira issue originally created by user s9e:

It all started when I noticed that removing elements from an a collection using removeElement() didn't actually remove them from the database. While debugging that issue, I found that using remove() worked as expected, so I took a look at ArrayCollection#removeElement() and remove(). I remained thorougly confused for a good 5 minutes because the two methods are virtually identical, yet they were behaving differently. That's when I realized that loading a collection from the database gives you a PersistentCollection, not an ArrayCollection.

So here's the bug: PersistentCollection#remove() and PersistentCollection#removeElement() behave differently. remove() marks the collection as changed only if an element is actually removed, then schedules an OrphanRemoval if applicable. removeElement() unconditionally marks the collection as changed and forgets to check for orphanRemoval.

Since both methods are virtually identical, I suggest that they are refactored to use a common code path instead of duplicating their code. The same should be done with ArrayCollection.

@doctrinebot

Comment created by @beberlei:

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

Merged your testcase also, fixed in master.

@doctrinebot

Comment created by s9e:

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;
    }
}
@doctrinebot

Comment created by @beberlei:

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...

@doctrinebot

Comment created by s9e:

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 {@inheritdoc} just like PersistentCollection does? Interestingly, it's another case of deduplication.

@doctrinebot

Comment created by @beberlei:

fixed, again!

@doctrinebot

Issue was closed with resolution "Fixed"

@doctrinebot

Comment created by s9e:

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
@beberlei beberlei was assigned by doctrinebot Dec 6, 2015
@doctrinebot doctrinebot added this to the 2.0-BETA4 milestone Dec 6, 2015
@doctrinebot doctrinebot closed this Dec 6, 2015
@doctrinebot doctrinebot added the Bug label Dec 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment