MDL-39801 navigation_node::remove fails if first child does not have a key

diff --git a/lib/navigationlib.php b/lib/navigationlib.php
index 7e1576b..58201a8 100644 (file)
--- a/lib/navigationlib.php
+++ b/lib/navigationlib.php
@@ -899,7 +899,7 @@ class navigation_node_collection implements IteratorAggregate {
         $child = $this->get($key, $type);
         if ($child !== false) {
             foreach ($this->collection as $colkey => $node) {
-                if ($node->key == $key && $node->type == $type) {
+                if ($node->key === $key && $node->type == $type) {
                     unset($this->collection[$colkey]);
                     break;
                 }

diff --git a/lib/tests/navigationlib_test.php b/lib/tests/navigationlib_test.php
index 430ec86..2b1f676 100644 (file)
--- a/lib/tests/navigationlib_test.php
+++ b/lib/tests/navigationlib_test.php
@@ -60,6 +60,8 @@ class navigation_node_testcase extends basic_testcase {
 
         $this->node = new navigation_node('Test Node');
         $this->node->type = navigation_node::TYPE_SYSTEM;
+        // We add the first child without key. This way we make sure all keys search by comparision is performed using ===
+        $this->node->add('first child without key', null, navigation_node::TYPE_CUSTOM);
         $demo1 = $this->node->add('demo1', $this->inactiveurl, navigation_node::TYPE_COURSE, null, 'demo1', new pix_icon('i/course', ''));
         $demo2 = $this->node->add('demo2', $this->inactiveurl, navigation_node::TYPE_COURSE, null, 'demo2', new pix_icon('i/course', ''));
         $demo3 = $this->node->add('demo3', $this->inactiveurl, navigation_node::TYPE_CATEGORY, null, 'demo3',new pix_icon('i/course', ''));
@@ -251,8 +253,14 @@ class navigation_node_testcase extends basic_testcase {
         $this->assertInstanceOf('navigation_node', $this->node->get('remove2'));
         $this->assertInstanceOf('navigation_node', $remove2->get('remove3'));
 
+        // Remove element and make sure this is no longer a child.
         $this->assertTrue($remove1->remove());
+        $this->assertFalse($this->node->get('remove1'));
+        $this->assertFalse(in_array('remove1', $this->node->get_children_key_list(), true));
+
+        // Remove more elements
         $this->assertTrue($this->node->get('remove2')->remove());
+        $this->assertFalse($this->node->get('remove2'));
         $this->assertTrue($remove2->get('remove3')->remove());
 
         $this->assertFalse($this->node->get('remove1'));