Fix Sortable Update and Delete handling

lib/Gedmo/Sortable/SortableListener.php

@@ -169,9 +169,12 @@ class SortableListener extends MappedEventSubscriber
     {
         $uow = $em->getUnitOfWork();
         
-        $oldData = $uow->getOriginalEntityData($object);
-        $oldPosition = $oldData[$config['position']];
-        $newPosition = $meta->getReflectionProperty($config['position'])->getValue($object);
+        $changeSet = $uow->getEntityChangeSet($object);
+        if (!array_key_exists($config['position'], $changeSet)) {
+            return;
+        }
+        $oldPosition = $changeSet[$config['position']][0];
+        $newPosition = $changeSet[$config['position']][1];
         
         // Get groups
         $groups = array();
@@ -193,12 +196,29 @@ class SortableListener extends MappedEventSubscriber
         }
         
         // Set position to max position if it is too big
-        $newPosition = min(array($this->maxPositions[$hash] + 1, $position));
+        $newPosition = min(array($this->maxPositions[$hash] + 1, $newPosition));
         
         // Compute relocations
-        $relocations = array();
-        $relocations[] = array($hash, $meta, $groups, $oldPosition, $newPosition, -1);
-        $relocations[] = array($hash, $meta, $groups, $newPosition, -1, +1);
+        /*
+        CASE 1: shift backwards
+        |----0----|----1----|----2----|----3----|----4----|
+        |--node1--|--node2--|--node3--|--node4--|--node5--|
+        Update node4: setPosition(1)
+        --> Update position + 1 where position in [1,3)
+        |--node1--|--node4--|--node2--|--node3--|--node5--|
+        CASE 2: shift forward
+        |----0----|----1----|----2----|----3----|----4----|
+        |--node1--|--node2--|--node3--|--node4--|--node5--|
+        Update node2: setPosition(3)
+        --> Update position - 1 where position in (1,3]
+        |--node1--|--node3--|--node4--|--node2--|--node5--|
+        */
+        $relocation = null;
+        if ($newPosition < $oldPosition) {
+            $relocation = array($hash, $meta, $groups, $newPosition, $oldPosition, +1);
+        } elseif ($newPosition > $oldPosition) {
+            $relocation = array($hash, $meta, $groups, $oldPosition + 1, $newPosition + 1, -1);
+        }
         
         // Apply existing relocations
         $applyDelta = 0;
@@ -212,10 +232,8 @@ class SortableListener extends MappedEventSubscriber
         }
         $newPosition += $applyDelta;
         
-        // Add relocations
-        foreach ($relocations as $relocation) {
-            call_user_func_array(array($this, 'addRelocation'), $relocation);
-        }
+        // Add relocation
+        call_user_func_array(array($this, 'addRelocation'), $relocation);
         
         // Set new position
         $meta->getReflectionProperty($config['position'])->setValue($object, $newPosition);
@@ -238,6 +256,11 @@ class SortableListener extends MappedEventSubscriber
         // Get hash
         $hash = $this->getHash($meta, $groups, $object);
         
+        // Get max position
+        if (!isset($this->maxPositions[$hash])) {
+            $this->maxPositions[$hash] = $this->getMaxPosition($em, $meta, $config, $object);
+        }
+        
         // Add relocation
         $this->addRelocation($hash, $meta, $groups, $position, -1, -1);
     }
@@ -247,7 +270,7 @@ class SortableListener extends MappedEventSubscriber
         foreach ($this->relocations as $hash => $relocation) {
             $config = $this->getConfiguration($em, $relocation['name']);
             foreach ($relocation['deltas'] as $delta) {
-                if ($delta['start'] > $this->maxPositions[$hash]) {
+                if ($delta['start'] > $this->maxPositions[$hash] || $delta['delta'] == 0) {
                     continue;
                 }
                 $sign = $delta['delta'] < 0 ? "-" : "+";

tests/Gedmo/Sortable/Fixture/Node.php

@@ -6,7 +6,7 @@ use Gedmo\Mapping\Annotation as Gedmo;
 use Doctrine\ORM\Mapping as ORM;
 
 /**
- * @Entity
+ * @Entity(repositoryClass="Gedmo\Sortable\Entity\Repository\SortableRepository")
  */
 class Node
 {

tests/Gedmo/Sortable/SortableTest.php

@@ -74,9 +74,112 @@ class SortableTest extends BaseTestCaseORM
         $this->assertEquals('Node1', $nodes[0]->getName());
     }
     
-    public function testInsertUnsortedList()
+    public function testShiftForward()
     {
-        $this->assertTrue(true);
+        $node2 = new Node();
+        $node2->setName("Node2");
+        $node2->setPath("/");
+        $this->em->persist($node2);
+        
+        $node = new Node();
+        $node->setName("Node3");
+        $node->setPath("/");
+        $this->em->persist($node);
+        
+        $node = new Node();
+        $node->setName("Node4");
+        $node->setPath("/");
+        $this->em->persist($node);
+        
+        $node = new Node();
+        $node->setName("Node5");
+        $node->setPath("/");
+        $this->em->persist($node);
+        
+        $this->em->flush();
+        
+        $this->assertEquals(1, $node2->getPosition());
+        $node2->setPosition(3);
+        $this->em->persist($node2);
+        $this->em->flush();
+        $this->em->clear();
+        
+        $repo = $this->em->getRepository('Sortable\\Fixture\\Node');
+        $nodes = $repo->getBySortableGroups(array('path' => '/'));
+        
+        $this->assertEquals('Node1', $nodes[0]->getName());
+        $this->assertEquals('Node3', $nodes[1]->getName());
+        $this->assertEquals('Node4', $nodes[2]->getName());
+        $this->assertEquals('Node2', $nodes[3]->getName());
+        $this->assertEquals('Node5', $nodes[4]->getName());
+    }
+
+    public function testShiftBackward()
+    {
+        $node = new Node();
+        $node->setName("Node2");
+        $node->setPath("/");
+        $this->em->persist($node);
+        
+        $node = new Node();
+        $node->setName("Node3");
+        $node->setPath("/");
+        $this->em->persist($node);
+        
+        $node2 = new Node();
+        $node2->setName("Node4");
+        $node2->setPath("/");
+        $this->em->persist($node2);
+        
+        $node = new Node();
+        $node->setName("Node5");
+        $node->setPath("/");
+        $this->em->persist($node);
+        
+        $this->em->flush();
+        $this->assertEquals(3, $node2->getPosition());
+        
+        
+        $node2->setPosition(1);
+        $this->em->persist($node2);
+        $this->em->flush();
+        $this->em->clear();
+        
+        $repo = $this->em->getRepository('Sortable\\Fixture\\Node');
+        $nodes = $repo->getBySortableGroups(array('path' => '/'));
+        
+        $this->assertEquals('Node1', $nodes[0]->getName());
+        $this->assertEquals('Node4', $nodes[1]->getName());
+        $this->assertEquals('Node2', $nodes[2]->getName());
+        $this->assertEquals('Node3', $nodes[3]->getName());
+        $this->assertEquals('Node5', $nodes[4]->getName());
+    }
+
+    public function testDelete()
+    {
+        $node2 = new Node();
+        $node2->setName("Node2");
+        $node2->setPath("/");
+        $this->em->persist($node2);
+        
+        $node3 = new Node();
+        $node3->setName("Node3");
+        $node3->setPath("/");
+        $this->em->persist($node3);
+        
+        $this->em->flush();
+        
+        $this->em->remove($node2);
+        $this->em->flush();
+        $this->em->clear();
+        
+        $repo = $this->em->getRepository('Sortable\\Fixture\\Node');
+        $nodes = $repo->getBySortableGroups(array('path' => '/'));
+        
+        $this->assertEquals('Node1', $nodes[0]->getName());
+        $this->assertEquals('Node3', $nodes[1]->getName());
+        $this->assertEquals(0, $nodes[0]->getPosition());
+        $this->assertEquals(1, $nodes[1]->getPosition());
     }
     
     public function testGroupByAssociation()