Uploaded image for project: 'Doctrine 2 - ORM'
  1. Doctrine 2 - ORM
  2. DDC-1775

Notify strategy listener is not attached for new entities

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.1
    • Fix Version/s: 2.2.3
    • Component/s: ORM
    • Security Level: All
    • Labels:
      None

      Description

      New entities with Notify strategy for changes and with GeneratedValue(strategy="AUTO") never get the onPropertyChanged listener attached to them. That happens because of the logic in the UOW::scheduleForInsert($entity) method. The last condition in it "isset($this->entityIdentifiers[$oid])" is never true because the identifier is not set and therefore the code that attaches the PropertyChangedListener in addToIdentityMap is never run.

        Activity

        deatheriam Oleg Namaka created issue -
        deatheriam Oleg Namaka made changes -
        Field Original Value New Value
        Description For entities with Notify strategy of changes tracking new entities with GeneratedValue(strategy="AUTO") never get the onPropertyChanged listener attached to them. That happens because of the logic in the UOW::scheduleForInsert($entity) method. The last condition in it "isset($this->entityIdentifiers[$oid])" is never true because the identifier is not set and therefore the code that attaches the PropertyChangedListener in addToIdentityMap is never run. New entities with Notify strategy for changes and with GeneratedValue(strategy="AUTO") never get the onPropertyChanged listener attached to them. That happens because of the logic in the UOW::scheduleForInsert($entity) method. The last condition in it "isset($this->entityIdentifiers[$oid])" is never true because the identifier is not set and therefore the code that attaches the PropertyChangedListener in addToIdentityMap is never run.
        Hide
        guilhermeblanco Guilherme Blanco added a comment -

        This is intentional to me.
        According to code and documentation, PropertyChanged is supposed to operate over updates, never over inserts.

        I experimented changing the code to also notify about changed during new and the consequences were very drastic. Internally, propertyChanged creates entityChangesets that implies an UPDATE.

        Marking ticket as won't fix.

        Show
        guilhermeblanco Guilherme Blanco added a comment - This is intentional to me. According to code and documentation, PropertyChanged is supposed to operate over updates, never over inserts. I experimented changing the code to also notify about changed during new and the consequences were very drastic. Internally, propertyChanged creates entityChangesets that implies an UPDATE. Marking ticket as won't fix.
        guilhermeblanco Guilherme Blanco made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Assignee Benjamin Eberlei [ beberlei ] Guilherme Blanco [ guilhermeblanco ]
        Resolution Won't Fix [ 2 ]
        Hide
        deatheriam Oleg Namaka added a comment - - edited

        In that case the Notify strategy is partially broken:
        Category entity:

        ..............
        
          *\Entity\Category
         *
         * @ORM\Table(name="category")
         * @ORM\Entity
         */
        class Category implements TimestampableInterface
         
            /**
             * Caption
             *
             * @var string $caption
             * @ORM\Column(name="caption", type="text", nullable=true)
             */
            protected $caption;
        	
            /**
             * Added At
             *
             * @var \DateTime $addedAt
             *
             * @ORM\Column(name="added_at", type="datetime", nullable=true)
             */
            protected $addedAt;
        
            public function setCaption($caption)
            {
                if ($this->caption != $caption)  {
                    $this->_onPropertyChanged('caption', $this->caption, $caption);
                    $this->caption = $caption;
                }
            }
        	
            public function setAddedAt($addedAt)
            {
                $oldValue = $this->addedAt;
        		$this->addedAt = $addedAt;
                if ((is_null($oldValue) || ($oldValue->getTimestamp() != $this->addedAt->getTimestamp()))) {
                    $this->_onPropertyChanged('addedAt', $oldValue, $this->addedAt);
                }
            }
        
            protected function _onPropertyChanged($propName, $oldValue, $newValue)
            {
                if ($this->_listeners) {
                    /** @var $listener \Doctrine\Common\PropertyChangedListener */
                    foreach ($this->_listeners as $listener) {
                        $listener->propertyChanged($this, $propName, $oldValue, $newValue);
                    }
                }
            }
        
            public function addPropertyChangedListener(PropertyChangedListener $listener)
            {
                $this->_listeners[] = $listener;
            }
        	
        ..............	
        
        

        OnFlush event handler:

        ..............
        
                foreach ($uow->getScheduledEntityInsertions() as $entity) {
                    if ($entity instanceof TimestampableInterface) {
                        $entity->setUpdatedAt(new \DateTime('now'));
                        $entity->setAddedAt(new \DateTime('now'));
                        $uow->recomputeSingleEntityChangeSet($em->getClassMetadata(get_class($entity)), $entity);
                    }
                }
        		
        ...............		
        

        Code that uses the entity:

                $cat = new \Entity\Category();
                // @todo this is a workaround until the http://www.doctrine-project.org/jira/browse/DDC-1775 is resolved
                $cat->addPropertyChangedListener($this->_em->getUnitOfWork());
                $cat->setCaption('Please explain');
                $this->_em->persist($cat);
                $this->_em->flush();
        

        If there is no explicit call to the addPropertyChangedListener method, the caption field gets saved but the $addedAt remains null after flush. The entity does not have the attached listener so UnitOfWork does not know anything about the update that happened in the OnFlush event, and the recomputeSingleEntityChangeSet method skips entities with Notify strategy.

        Show
        deatheriam Oleg Namaka added a comment - - edited In that case the Notify strategy is partially broken: Category entity: .............. *\Entity\Category * * @ORM\Table(name= "category" ) * @ORM\Entity */ class Category implements TimestampableInterface /** * Caption * * @ var string $caption * @ORM\Column(name= "caption" , type= "text" , nullable= true ) */ protected $caption; /** * Added At * * @ var \DateTime $addedAt * * @ORM\Column(name= "added_at" , type= "datetime" , nullable= true ) */ protected $addedAt; public function setCaption($caption) { if ($ this ->caption != $caption) { $ this ->_onPropertyChanged('caption', $ this ->caption, $caption); $ this ->caption = $caption; } } public function setAddedAt($addedAt) { $oldValue = $ this ->addedAt; $ this ->addedAt = $addedAt; if ((is_null($oldValue) || ($oldValue->getTimestamp() != $ this ->addedAt->getTimestamp()))) { $ this ->_onPropertyChanged('addedAt', $oldValue, $ this ->addedAt); } } protected function _onPropertyChanged($propName, $oldValue, $newValue) { if ($ this ->_listeners) { /** @ var $listener \Doctrine\Common\PropertyChangedListener */ foreach ($ this ->_listeners as $listener) { $listener->propertyChanged($ this , $propName, $oldValue, $newValue); } } } public function addPropertyChangedListener(PropertyChangedListener $listener) { $ this ->_listeners[] = $listener; } .............. OnFlush event handler: .............. foreach ($uow->getScheduledEntityInsertions() as $entity) { if ($entity instanceof TimestampableInterface) { $entity->setUpdatedAt( new \DateTime('now')); $entity->setAddedAt( new \DateTime('now')); $uow->recomputeSingleEntityChangeSet($em->getClassMetadata(get_class($entity)), $entity); } } ............... Code that uses the entity: $cat = new \Entity\Category(); // @todo this is a workaround until the http://www.doctrine-project.org/jira/browse/DDC-1775 is resolved $cat->addPropertyChangedListener($ this ->_em->getUnitOfWork()); $cat->setCaption('Please explain'); $ this ->_em->persist($cat); $ this ->_em->flush(); If there is no explicit call to the addPropertyChangedListener method, the caption field gets saved but the $addedAt remains null after flush. The entity does not have the attached listener so UnitOfWork does not know anything about the update that happened in the OnFlush event, and the recomputeSingleEntityChangeSet method skips entities with Notify strategy.
        deatheriam Oleg Namaka made changes -
        Resolution Won't Fix [ 2 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Hide
        beberlei Benjamin Eberlei added a comment -

        Changing computeScheduleInsertsChangeSets() would solve this, but it would also mean that the notifier gets injected more than once.

            private function computeScheduleInsertsChangeSets()
            {
                foreach ($this->entityInsertions as $entity) {
                    $class = $this->em->getClassMetadata(get_class($entity));
        
                    $this->computeChangeSet($class, $entity);
        
                    if ($entity instanceof NotifyPropertyChanged) {
                        $entity->addPropertyChangedListener($this);
                    }
                }
            }
        

        I think injecting in scheduleForInsert() is ok, but we have to look at potential problems also

        Show
        beberlei Benjamin Eberlei added a comment - Changing computeScheduleInsertsChangeSets() would solve this, but it would also mean that the notifier gets injected more than once. private function computeScheduleInsertsChangeSets() { foreach ($ this ->entityInsertions as $entity) { $class = $ this ->em->getClassMetadata(get_class($entity)); $ this ->computeChangeSet($class, $entity); if ($entity instanceof NotifyPropertyChanged) { $entity->addPropertyChangedListener($ this ); } } } I think injecting in scheduleForInsert() is ok, but we have to look at potential problems also
        beberlei Benjamin Eberlei made changes -
        Workflow jira [ 13626 ] jira-feedback [ 14106 ]
        beberlei Benjamin Eberlei made changes -
        Workflow jira-feedback [ 14106 ] jira-feedback2 [ 15970 ]
        Hide
        beberlei Benjamin Eberlei added a comment -

        Fixed

        Show
        beberlei Benjamin Eberlei added a comment - Fixed
        beberlei Benjamin Eberlei made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Assignee Guilherme Blanco [ guilhermeblanco ] Benjamin Eberlei [ beberlei ]
        Fix Version/s 2.2.3 [ 10196 ]
        Fix Version/s 2.2.1 [ 10194 ]
        Resolution Fixed [ 1 ]
        beberlei Benjamin Eberlei made changes -
        Workflow jira-feedback2 [ 15970 ] jira-feedback3 [ 19438 ]

        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={expand=changesets[0:20].revisions[0:29],reviews, query=DDC-1775}, methodType=GET}] : Received status code 503 (Service Temporarily Unavailable)

          People

          • Assignee:
            beberlei Benjamin Eberlei
            Reporter:
            deatheriam Oleg Namaka
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: