Doctrine 2 - ORM
  1. Doctrine 2 - ORM
  2. DDC-2406

Merging of new detached entities with PrePersist lifecycle callback breaks

    Details

    • Type: Bug Bug
    • Status: Awaiting Feedback
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 2.3.2
    • Fix Version/s: None
    • Component/s: ORM
    • Security Level: All
    • Labels:

      Description

      Merging of new detached entities with PrePersist lifecycle callback breaks:

      Code snippet:

          class A
          {
             /**
              *  @ORM\ManyToOne(targetEntity= ...
              *  @ORM\JoinColumn(name=" ...
              */
              protected $b;
              
              public function getB()
              {
                  return $this->b;
              }
              
              public function setB($b)
              {
                  $this->b = $b;
              }
              
              /**
               *
               * @ORM\PrePersist
               *
               * @return void
               */
              public function onPrePersist()
              {
                 if ($this->getB() === null) {
                      throw new \Exception('B instance must be defined);
                 }
                 ....
              }
          }
          
          class B 
          {
          }
          
          $a = new A();
          $b = $em->find('B', 1);
          $a->setB($b);
          $em->persist($a); // works fine as B instance is set
          $em->detach($a);
          
          $a = $em->merge($a) // breaks in onPrePersist
      

      The reason it happens is that the merge operation is trying to persist a new entity created by uow::newInstance($class) without populating its properties first:

       // If there is no ID, it is actually NEW.
          ....
          if ( ! $id) {
              $managedCopy = $this->newInstance($class);
      
              $this->persistNew($class, $managedCopy);
          } else {
      	....
      

      This should happen first for the $managedCopy:

          // Merge state of $entity into existing (managed) entity
          foreach ($class->reflClass->getProperties() as $prop) {
              ....
      

        Activity

        Oleg Namaka created issue -
        Oleg Namaka made changes -
        Field Original Value New Value
        Priority Major [ 3 ] Critical [ 2 ]
        Oleg Namaka made changes -
        Affects Version/s 2.3.2 [ 10324 ]
        Oleg Namaka made changes -
        Component/s ORM [ 10012 ]
        Oleg Namaka made changes -
        Labels merge, prePersist
        Marco Pivetta made changes -
        Priority Critical [ 2 ] Major [ 3 ]
        Hide
        Fabio B. Silva added a comment -

        Benjamin Eberlei, Is this an expected behavior ?

        I mean.. This issue is about dispatch the event before copy the original values into the managed instance.
        But overall, should $em->detach() trigger @PrePersist events ?

        Show
        Fabio B. Silva added a comment - Benjamin Eberlei , Is this an expected behavior ? I mean.. This issue is about dispatch the event before copy the original values into the managed instance. But overall, should $em->detach() trigger @PrePersist events ?
        Hide
        Benjamin Eberlei added a comment -

        Fabio B. Silva he talks about $em->merge() on a detached entity calling pre persist. This should only happen on a NEW entity, not on a DETACHED one.

        Show
        Benjamin Eberlei added a comment - Fabio B. Silva he talks about $em->merge() on a detached entity calling pre persist. This should only happen on a NEW entity, not on a DETACHED one.
        Hide
        Oleg Namaka added a comment - - edited

        I tend to disagree with the statement above about pre persist that should not happen on a detached entity being merged back in. If this event handler contains a business logic that this entity needs to be checked against and the detached entity was modified before the merge operation in a way that invalidates it in the prePersist than I will end up with the invalid entity in the identity map. If the merge operation calls persist it must run the prePersist event handler as well for consistency.

        If there is a logic that prevents persisting invalid entities why should it bypassed in the merge operation?

        Show
        Oleg Namaka added a comment - - edited I tend to disagree with the statement above about pre persist that should not happen on a detached entity being merged back in. If this event handler contains a business logic that this entity needs to be checked against and the detached entity was modified before the merge operation in a way that invalidates it in the prePersist than I will end up with the invalid entity in the identity map. If the merge operation calls persist it must run the prePersist event handler as well for consistency. If there is a logic that prevents persisting invalid entities why should it bypassed in the merge operation?
        Oleg Namaka made changes -
        Status Open [ 1 ] Awaiting Feedback [ 10000 ]
        Hide
        Cory Close added a comment -

        I can confirm that this bug has not been fixed while using doctrine 2.4

        Exactly as Oleg Namaka has described, my organization is trying to use @PrePersist callbacks to enforce validation on new entities.

        However, we use an extensive client side framework that sends json back to our server. Our workflow is then:

        deserializeJson into detached entity
        merge detached entity to get it managed (this will apply our edits to an existing entity, or create a new one if this one is new)
        persist

        However, some entities run into the above problem while using this workflow, so our validation is not run. I can provide more code samples if required.

        Show
        Cory Close added a comment - I can confirm that this bug has not been fixed while using doctrine 2.4 Exactly as Oleg Namaka has described, my organization is trying to use @PrePersist callbacks to enforce validation on new entities. However, we use an extensive client side framework that sends json back to our server. Our workflow is then: deserializeJson into detached entity merge detached entity to get it managed (this will apply our edits to an existing entity, or create a new one if this one is new) persist However, some entities run into the above problem while using this workflow, so our validation is not run. I can provide more code samples if required.
        Hide
        Oleg Namaka added a comment -

        Whose feedback is it awaiting for?

        Show
        Oleg Namaka added a comment - Whose feedback is it awaiting for?
        Hide
        Romaric Drigon added a comment -

        I can confirm this issue.

        My use case (which I guess is pretty standard):

        • I deserialize a new entity (with relationships to already existing ones)
        • I merge it to attach existing entities
        • I persist it

        PrePersist is called on an entity with all properties set to null. Any modifications made inside this entity won't be saved to the DB.

        Show
        Romaric Drigon added a comment - I can confirm this issue. My use case (which I guess is pretty standard): I deserialize a new entity (with relationships to already existing ones) I merge it to attach existing entities I persist it PrePersist is called on an entity with all properties set to null. Any modifications made inside this entity won't be saved to the DB.
        Hide
        Cory Close added a comment -

        Is there any reason the proposed solution can't be implemented? I've patched a local version of Doctrine with the proposed changes and ran it through my application's tests without any issues, so I believe this would fix the problem.

        Show
        Cory Close added a comment - Is there any reason the proposed solution can't be implemented? I've patched a local version of Doctrine with the proposed changes and ran it through my application's tests without any issues, so I believe this would fix the problem.

        This list may be incomplete, as errors occurred whilst retrieving source from linked applications:

        • Request to http://www.doctrine-project.org/fisheye/ failed: Error in remote call to 'FishEye 0 (http://www.doctrine-project.org/fisheye/)' (http://www.doctrine-project.org/fisheye) [AbstractRestCommand{path='/rest-service-fe/search-v1/crossRepositoryQuery', params={query=DDC-2406, expand=changesets[0:20].revisions[0:29],reviews}, methodType=GET}] : Received status code 503 (Service Temporarily Unavailable)

          People

          • Assignee:
            Benjamin Eberlei
            Reporter:
            Oleg Namaka
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated: