Add tests and fix some bugs

Author: lazychaser

README.markdown

@@ -136,7 +136,7 @@ Query can be filtered out from the root node using scope `withoutRoot`:
 $nodes = Category::withoutRoot()->get();
 ```
 
-Deleting nodes as simple as before:
+Deleting nodes is as simple as before:
 
 ```php
 $node->delete();
@@ -293,3 +293,5 @@ children?
 When you create your table's schema and use `NestedSet::columns`, it creates foreign
 key for you, since nodes are connected by `parent_id` attribute. When you hard delete
 the node, all of descendants are cascaded.
+
+In case when DBMS doesn't support foreign keys, descendants are removed manually.

composer.json

@@ -14,10 +14,15 @@
         "illuminate/support": "4.0.x",
         "illuminate/database": "4.0.x"
     },
+
+    "require-dev": {
+        "phpunit/phpunit": "3.7.*"
+    },
+
     "autoload": {
         "psr-0": {
             "Kalnoy\\Nestedset": "src/"
-        }
+        },
     },
     "minimum-stability": "dev"
 }

phpunit.php

@@ -0,0 +1,11 @@
+<?php
+
+include __DIR__.'/vendor/autoload.php';
+
+$capsule = new \Illuminate\Database\Capsule\Manager;
+$capsule->addConnection(array('driver' => 'sqlite', 'database' => ':memory:'));
+$capsule->setEventDispatcher(new \Illuminate\Events\Dispatcher);
+$capsule->bootEloquent();
+$capsule->setAsGlobal();
+
+include __DIR__.'/tests/models/Category.php';

phpunit.xml

@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <phpunit backupGlobals="false"
          backupStaticAttributes="false"
-         bootstrap="vendor/autoload.php"
+         bootstrap="phpunit.php"
          colors="true"
          convertErrorsToExceptions="true"
          convertNoticesToExceptions="true"

src/Kalnoy/Nestedset/NestedSet.php

@@ -18,7 +18,7 @@ static public function columns(Blueprint $table, $primaryKey = 'id')
         $table->integer(Node::RGT);
         $table->unsignedInteger(Node::PARENT_ID)->nullable();
 
-        $table->index([ Node::LFT, Node::RGT, Node::PARENT_ID ], 'nested_set_index');
+        $table->index(array(Node::LFT, Node::RGT, Node::PARENT_ID), 'nested_set_index');
 
         $table
             ->foreign(Node::PARENT_ID, 'nested_set_foreign')

src/Kalnoy/Nestedset/Node.php

@@ -4,8 +4,7 @@
 use \Illuminate\Database\Query\Builder;
 use Exception;
 
-class Node extends Eloquent 
-{
+class Node extends Eloquent {
     /**
      * The name of "lft" column.
      *
@@ -94,7 +93,7 @@ public function descendants()
     {
         $query = $this->newQuery();
 
-        return $query->whereBetween('lft', array($this->getLft() + 1, $this->getRgt()));
+        return $query->whereBetween(static::LFT, array($this->getLft() + 1, $this->getRgt()));
     }
 
     /**
@@ -162,7 +161,8 @@ public function scopeWithoutRoot($query)
      */
     public function path()
     {
-        if (!$this->exists) {
+        if (!$this->exists) 
+        {
             throw new Exception("Cannot query for path to non-existing node.");
         }
 
@@ -216,7 +216,7 @@ public function prepend(Node $node)
      */
     public function appendTo(Node $parent)
     {
-        return $this->insertAt($parent->attributes[static::RGT], $parent);
+        return $this->checkTarget($parent)->insertAt($parent->attributes[static::RGT], $parent);
     }
 
     /**
@@ -228,7 +228,7 @@ public function appendTo(Node $parent)
      */
     public function prependTo(Node $parent)
     {        
-        return $this->insertAt($parent->attributes[static::LFT] + 1, $parent);
+        return $this->checkTarget($parent)->insertAt($parent->attributes[static::LFT] + 1, $parent);
     }
 
     /**
@@ -264,14 +264,15 @@ public function before(Node $node)
      * @param  self::BEFORE|self::AFTER $dir
      *
      * @return Node
+     * @throws Exception If $node is not a valid target.
+     * @throws Exception If $node is the root.
      */
     protected function beforeOrAfter(Node $node, $dir)
     {
-        if (!$node->exists) {
-            throw new Exception("Cannot insert node $dir node that does not exists.");  
-        }
+        $this->checkTarget($node);
 
-        if ($node->isRoot()) {
+        if ($node->isRoot()) 
+        {
             throw new Exception("Cannot insert node $dir root node.");
         }
 
@@ -280,18 +281,38 @@ protected function beforeOrAfter(Node $node, $dir)
         return $this->insertAt($position, $node->parent);
     }
 
+    /**
+     * Check if target node is saved.
+     *
+     * @param  Node   $node
+     *
+     * @return Node
+     * @throws Exception If target fails conditions.
+     */
+    protected function checkTarget(Node $node)
+    {
+        if (!$node->exists || $node->isDirty(static::LFT)) 
+        {
+            throw new Exception("Target node is updated but not saved.");
+        }
+
+        return $this;
+    }
+
     /**
      * Insert node at specific position and related parent.
      *
      * @param  int $position
      * @param  Node $parent
      *
      * @return Node
+     * @throws Exceptino If node is inserted in one of it's descendants
      */
     protected function insertAt($position, Node $parent)
     {
-        if ($this->exists && $this->getLft() < $position && $position < $this->getRgt()) {
-            throw new Exception("Cannot insert node into itself.");
+        if ($this->exists && $this->getLft() < $position && $position < $this->getRgt()) 
+        {
+            throw new Exception("Trying to insert node into one of it's descendants.");
         }
 
         $height = $this->getNodeHeight();
@@ -317,17 +338,23 @@ protected function insertAt($position, Node $parent)
      */
     public function fireModelEvent($event, $halt = true)
     {
-        if ($event === 'creating' or $event === 'updating' and $this->isDirty(static::LFT)) {
-            if (!$this->updateTree()) {
+        if ($event === 'creating') 
+        {
+            if (!isset($this->attributes[static::LFT]) || !$this->updateTree())
+            {
                 return false;
             }
         }
 
-        if ($event === 'deleting' and $this->isRoot()) {
-            return false;
+        if ($event === 'updating' && $this->isDirty(static::LFT))
+        {
+            if (!$this->updateTree()) return false;
         }
 
-        if ($event === 'deleted' and !$this->softDelete) {
+        if ($event === 'deleting' && $this->isRoot()) return false;
+
+        if ($event === 'deleted' && !$this->softDelete) 
+        {
             $this->processDeletedNode();
         }
 
@@ -345,7 +372,8 @@ protected function updateTree()
         $rgt    = $this->attributes[static::RGT];
         $height = $rgt - $lft + 1;
 
-        if ($this->exists) {
+        if ($this->exists) 
+        {
             $oldLft = $this->original[static::LFT];
             $oldRgt = $this->original[static::RGT];
 
@@ -358,7 +386,8 @@ protected function updateTree()
                 // Node is going down.
                 // Other nodes are going up on its place.
                 static::shiftNodes(-$height, $oldRgt + 1, $lft - 1, $this);
-            } else {
+            } else 
+            {
                 $shiftAmount = $lft - $oldLft;
 
                 // Node is going up.
@@ -382,11 +411,19 @@ protected function updateTree()
      */
     protected function processDeletedNode()
     {
+        $lft = $this->getLft();
+        $rgt = $this->getRgt();
+
+        // DBMS with support of foreign keys will remove descendant nodes automatically
+        $this->descendants()->delete();
+
         // We cannot use getNodeHeight because it always return 2 for non-existing
         // nodes.
-        $height = $this->attributes[static::RGT] - $this->attributes[static::LFT] + 1;
+        $height = $rgt - $lft + 1;
+
+        $this->shiftNodes(-$height, $rgt + 1, null, $this);
 
-        $this->shiftNodes(-$height, $this->attributes[static::RGT] + 1, null, $this);
+        unset($this->attributes[static::LFT], $this->attributes[static::RGT]);
     }
 
     /**
@@ -438,13 +475,9 @@ protected function revealNodes($shiftAmount)
      */
     static protected function shiftNodes($amount, $from, $to, $instance = null)
     {
-        if ($amount == 0 || $from === null && $to === null) {
-            return;
-        }
+        if ($amount == 0 || $from === null && $to === null) return;
 
-        if ($instance === null) {
-            $instance = new static;
-        }
+        if ($instance === null) $instance = new static;
 
         $query  = $instance->newQueryWithDeleted()->getQuery();
         $method = $amount > 0 ? 'increment' : 'decrement';
@@ -453,13 +486,14 @@ static protected function shiftNodes($amount, $from, $to, $instance = null)
         static::shiftNodesColumn($query, static::LFT, $method, $amount, $from, $to);
         static::shiftNodesColumn($query, static::RGT, $method, $amount, $from, $to);
 
-        // Update parent of the instance to support inserting multiple nodes into
-        // single parent.
-        if (isset($instance->relations['parent'])) {
-            $parent = $instance->relations['parent'];
+        // Update all ancestors (if any) of the instance to support inserting 
+        // multiple nodes into single parent.
+        while (isset($instance->relations['parent'])) 
+        {
+            $instance = $instance->relations['parent'];
 
-            $parent->shiftColumn(static::LFT, $amount, $from, $to);
-            $parent->shiftColumn(static::RGT, $amount, $from, $to);
+            $instance->shiftColumn(static::LFT, $amount, $from, $to);
+            $instance->shiftColumn(static::RGT, $amount, $from, $to);
         }
     }
 
@@ -480,13 +514,9 @@ static protected function shiftNodesColumn(Builder $query, $column, $method, $am
         $query->wheres = array();
         $query->setBindings(array());
 
-        if ($from === null) {
-            $query->where($column, '<=', $to);
-        } elseif ($to === null) {
-            $query->where($column, '>=', $from);
-        } else {
-            $query->whereBetween($column, array($from, $to));
-        }
+        if ($from === null) $query->where($column, '<=', $to);
+        elseif ($to === null) $query->where($column, '>=', $from);
+        else $query->whereBetween($column, array($from, $to));
 
         return $query->$method($column, $amount);
     }
@@ -502,9 +532,7 @@ static protected function shiftNodesColumn(Builder $query, $column, $method, $am
      */
     protected function shift($amount, $from, $to)
     {
-        if ($from === null && $to === null || $amount == 0) {
-            return;
-        }
+        if ($from === null && $to === null || $amount == 0) return;
 
         $this->shiftColumn(static::LFT, $amount, $from, $to);
         $this->shiftColumn(static::RGT, $amount, $from, $to);
@@ -545,7 +573,8 @@ protected function reveal($shiftAmount, $from, $to)
     {
         $lft = $this->getLft();
 
-        if ($from <= $lft && $lft <= $to) {
+        if ($from <= $lft && $lft <= $to) 
+        {
             $this->attributes[static::LFT] += $shiftAmount;
             $this->attributes[static::RGT] += $shiftAmount;
         }
@@ -564,9 +593,7 @@ protected function reveal($shiftAmount, $from, $to)
      */
     static protected function hideOrRevealNodes($from, $to, $shiftAmount = 0, $instance = null)
     {
-        if ($instance === null) {
-            $instance = new static;
-        }
+        if ($instance === null) $instance = new static;
 
         $query   = $instance->newQueryWithDeleted()->getQuery();
         $grammar = $query->getGrammar();
@@ -577,9 +604,11 @@ static protected function hideOrRevealNodes($from, $to, $shiftAmount = 0, $insta
         // and "revealing" is putting nodes back in database but in new position based on shiftAmount
         $updateValues = array();
 
-        if ($shiftAmount == 0) {
+        if ($shiftAmount == 0) 
+        {
             $updateValues[static::LFT] = $query->raw('-'.$grammar->wrap(static::LFT));
-        } else {
+        } else 
+        {
             $shiftAmountStr = $shiftAmount > 0 ? '+'.$shiftAmount : $shiftAmount;
 
             $updateValues[static::LFT] = $query->raw('-'.$grammar->wrap(static::LFT).$shiftAmountStr);
@@ -624,9 +653,7 @@ public function newCollection(array $models = array())
      */
     public function getNodeHeight()
     {
-        if (!$this->exists) {
-            return 2;
-        }
+        if (!$this->exists) return 2;
 
         return $this->attributes[static::RGT] - $this->attributes[static::LFT] + 1;
     }
@@ -647,14 +674,12 @@ public function getDescendantCount()
      * Behind the scenes node is appended to found parent node.
      *
      * @param int $value
+     * @throws Exception If parent node doesn't exists
      */
-    public function setParentId($value)
+    public function setParentIdAttribute($value)
     {
-        if ($this->isRoot()) {
-            throw new Exception("Cannot change parent of root node.");
-        }
-
-        if ($this->attributes[static::PARENT_ID] != $value) {
+        if (!isset($this->attributes[static::PARENT_ID]) || $this->attributes[static::PARENT_ID] != $value) 
+        {
             $this->appendTo(static::findOrFail($value));
         }
     }