Doctrine 2 - ORM
  1. Doctrine 2 - ORM
  2. DDC-3051

PersistentCollection::removeElement() leads to deletion of element, even if it is re-added via add()

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Invalid
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: ORM
    • Security Level: All
    • Labels:
      None

      Description

      PersistentCollection::removeElement() leads to deletion of element, even if it is re-added via add(). This is because internally the element is scheduled for orphan deletion. This leads to unexpected effects, because until the UOW is committed, the relationships are re-established, but after the element is gone.

      Suggested fix would be to add the ability to un-schedule an orphan removal and call this from the add() method.

        Activity

        Hide
        Marco Pivetta added a comment -

        This has been normal behavior for a lot of time now. The correct approach is to either avoid removal of the associated object from the collection or re-persisting the object (manually) after having it removed from the collection.

        Show
        Marco Pivetta added a comment - This has been normal behavior for a lot of time now. The correct approach is to either avoid removal of the associated object from the collection or re-persisting the object (manually) after having it removed from the collection.
        Hide
        Daniel Richter added a comment -

        I worked around by cloning the object, but still I find this unnecessarily inconsistent.

        For example:

        $order = new Order(); //new object working with ArrayCollection internally
        $promotion = new Promotion();
        
        //a sequence of events which might be distributed over different areas of code
        $order->addPromotion($promotion);
        $order->removePromotion($promotion);
        $order->addPromotion($promotion);
        
        $em->persist($order);
        $em->flush();
        
        //at this point $promotion is persisted properly.
        
        

        Compare this to the same sequence of events, but an order that was loaded from the database:

        $order = $repository->find(1); //existing order, working with PersistentCollection internally
        $promotion = new Promotion();
        
        //same sequence of events
        $order->addPromotion($promotion);
        $order->removePromotion($promotion);
        $order->addPromotion($promotion);
        
        $em->flush();
        
        //now the promotion will NOT be persisted, even though $order->getPromotions() will still return it.
        

        To me this violates the idea that Doctrine stays out of your way and lets you work with your domain objects without worrying too much about the persistence details.

        Show
        Daniel Richter added a comment - I worked around by cloning the object, but still I find this unnecessarily inconsistent. For example: $order = new Order(); // new object working with ArrayCollection internally $promotion = new Promotion(); //a sequence of events which might be distributed over different areas of code $order->addPromotion($promotion); $order->removePromotion($promotion); $order->addPromotion($promotion); $em->persist($order); $em->flush(); //at this point $promotion is persisted properly. Compare this to the same sequence of events, but an order that was loaded from the database: $order = $repository->find(1); //existing order, working with PersistentCollection internally $promotion = new Promotion(); //same sequence of events $order->addPromotion($promotion); $order->removePromotion($promotion); $order->addPromotion($promotion); $em->flush(); //now the promotion will NOT be persisted, even though $order->getPromotions() will still return it. To me this violates the idea that Doctrine stays out of your way and lets you work with your domain objects without worrying too much about the persistence details.
        Hide
        Marco Pivetta added a comment -

        Daniel Richter The problem is pretty much the same as creating/removing any element in the object graph.
        OrphanRemoval currently allows removal of items from the object graph, and the assumption is that these items will be "gone" when this happens.

        To have something like what you are suggesting (persist on addition to a collection) we would need another similar (complementary) feature that doesn't affect OrphanRemoval at all.

        OrphanRemoval just works like it should: if you want to propose a complementary functionality, please open a new issue.

        Show
        Marco Pivetta added a comment - Daniel Richter The problem is pretty much the same as creating/removing any element in the object graph. OrphanRemoval currently allows removal of items from the object graph, and the assumption is that these items will be "gone" when this happens. To have something like what you are suggesting (persist on addition to a collection) we would need another similar (complementary) feature that doesn't affect OrphanRemoval at all. OrphanRemoval just works like it should: if you want to propose a complementary functionality, please open a new issue.
        Hide
        Daniel Richter added a comment -

        I appreciate your time and response. I don't want to be complainy or unreasonable, but I'm not sure my point has come across.

        The persist is happening properly, via cascase: persist. No issue there.

        The issue is that the object in question ($promotion in my example) is not an orphan, but still it is removed. I would expect orphan removal to look at the orphan status of objects at the time of the commit, not whether at any point an object has become temporarily detached. I don't think it is reasonable in a large OOP application to keep track of relationship changes during the course of a transaction, it should only matter what the relationships are at the time of the commit.

        My suggestion of simply "unscheduling" the orphan removal seems a simple addition to the PersistentCollection. Schedule on removal, unschedule on addition. This seems consistent and symmetric.
        I would be happy to submit a PR with this addition, I just want to make sure I understand your objection first.

        Thank you for helping me understand.

        Show
        Daniel Richter added a comment - I appreciate your time and response. I don't want to be complainy or unreasonable, but I'm not sure my point has come across. The persist is happening properly, via cascase: persist. No issue there. The issue is that the object in question ($promotion in my example) is not an orphan, but still it is removed. I would expect orphan removal to look at the orphan status of objects at the time of the commit, not whether at any point an object has become temporarily detached. I don't think it is reasonable in a large OOP application to keep track of relationship changes during the course of a transaction, it should only matter what the relationships are at the time of the commit. My suggestion of simply "unscheduling" the orphan removal seems a simple addition to the PersistentCollection. Schedule on removal, unschedule on addition. This seems consistent and symmetric. I would be happy to submit a PR with this addition, I just want to make sure I understand your objection first. Thank you for helping me understand.
        Hide
        Marco Pivetta added a comment -

        Daniel Richter it could work if the PersistentCollection kept track of removed entities.

        You can try with a pull request, but I'm not sure if the feature is going to be accepted by the lead.

        Giving it a try won't hurt though, and adding a PersistOnAdd (or something like that) would eventually also be acceptable in my opinion.

        Show
        Marco Pivetta added a comment - Daniel Richter it could work if the PersistentCollection kept track of removed entities. You can try with a pull request, but I'm not sure if the feature is going to be accepted by the lead. Giving it a try won't hurt though, and adding a PersistOnAdd (or something like that) would eventually also be acceptable in my opinion.

          People

          • Assignee:
            Marco Pivetta
            Reporter:
            Daniel Richter
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: