Doctrine 2 - ORM
  1. Doctrine 2 - ORM
  2. DDC-735

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

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0-BETA3
    • Fix Version/s: 2.0-BETA4
    • Component/s: ORM
    • Security Level: All
    • Labels:
      None

      Description

      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.

        Activity

        Hide
        s9e added a comment -
        Show
        s9e added a comment - Testcase and proposed fix at http://github.com/s9e/doctrine2/commit/5a01b50bdbffbfd59c3af6cfc94ddb5793dac329
        Hide
        Benjamin Eberlei added a comment -

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

        Merged your testcase also, fixed in master.

        Show
        Benjamin Eberlei added a comment - Slighty changed the patch because removeElement() on the wrapped collection has to be called. Merged your testcase also, fixed in master.
        Hide
        s9e added a comment -

        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;
            }
        }
        
        Show
        s9e added a comment - 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; } }
        Hide
        Benjamin Eberlei added a comment -

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

        Show
        Benjamin Eberlei added a comment - 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...
        Hide
        s9e added a comment -

        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.

        Show
        s9e added a comment - 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.
        Hide
        Benjamin Eberlei added a comment -

        fixed, again!

        Show
        Benjamin Eberlei added a comment - fixed, again!
        Hide
        s9e added a comment -

        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
        Show
        s9e added a comment - 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

          People

          • Assignee:
            Benjamin Eberlei
            Reporter:
            s9e
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: