Tree::linkChildren fixed

+ fixes wrong node used for condition to call $onParentUnlinked
+ fixes an issue with node keys by providing a way to calculate them instead
+ more edge case tests

src/Simple/NodeBuilder.php

@@ -4,7 +4,6 @@
 
 namespace Dakujem\Oliva\Simple;
 
-use Dakujem\Oliva\Exceptions\ExtractorReturnValueIssue;
 use Dakujem\Oliva\Exceptions\InvalidInputData;
 use Dakujem\Oliva\Exceptions\InvalidNodeFactoryReturnValue;
 use Dakujem\Oliva\MovableNodeContract;
@@ -46,7 +45,7 @@ public function node(
         foreach ($children as $key => $child) {
             // Check for consistency.
             if (!$child instanceof MovableNodeContract) {
-                throw (new InvalidInputData('The children must be movable node instances.'))
+                throw (new InvalidInputData('Each child must be a movable node instance (' . MovableNodeContract::class . ').'))
                     ->tag('child', $child)
                     ->tag('key', $key)
                     ->tag('parent', $node)

src/Tree.php

@@ -75,21 +75,29 @@ public static function unlink(
      * Attaches a bunch of nodes to a parent,
      * establishing both the child-to-parent link and adding the child to the parent's children list.
      *
-     * Does NOT remove the original children, collisions may occur.
+     * Does NOT remove the original children of the parent node ($parent).
      *
      * The callable $onParentUnlinked may be used to process cases where the original node's parent is unlinked.
+     *
+     * The call does not use the keys in the given list of children.
+     * However, those may be used via the $key callable, in fact, any valid keys may be returned.
      */
     public static function linkChildren(
         MovableNodeContract $parent,
         iterable $children,
         ?callable $onParentUnlinked = null,
+        ?callable $key = null,
     ): MovableNodeContract {
-        foreach ($children as $key => $child) {
+        foreach ($children as $index => $child) {
             if (!$child instanceof MovableNodeContract) {
                 throw new NodeNotMovable($child);
             }
-            $originalParent = self::link($child, $parent, $key);
-            if (null !== $parent && null !== $onParentUnlinked) {
+            $originalParent = self::link(
+                node: $child,
+                parent: $parent,
+                key: null !== $key ? $key($child, $index) : null,
+            );
+            if (null !== $originalParent && null !== $onParentUnlinked) {
                 $onParentUnlinked($originalParent, $child);
             }
         }

tests/factory.phpt

@@ -0,0 +1,46 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Dakujem\Test;
+
+use Dakujem\Oliva\Exceptions\InvalidNodeFactoryReturnValue;
+use Dakujem\Oliva\MaterializedPath;
+use Dakujem\Oliva\Recursive;
+use Dakujem\Oliva\Simple\NodeBuilder;
+use Dakujem\Oliva\Simple\TreeWrapper;
+use Tester\Assert;
+
+require_once __DIR__ . '/setup.php';
+
+// Test some edge cases
+(function () {
+    $testFactory = function (callable $badNodeFactory): void {
+        Assert::throws(function () use ($badNodeFactory) {
+            (new TreeWrapper($badNodeFactory, fn() => []))->wrap(['item']);
+        }, InvalidNodeFactoryReturnValue::class);
+
+        Assert::throws(function () use ($badNodeFactory) {
+            (new NodeBuilder($badNodeFactory))->node('item');
+        }, InvalidNodeFactoryReturnValue::class);
+
+        Assert::throws(function () use ($badNodeFactory) {
+            (new MaterializedPath\TreeBuilder($badNodeFactory, fn() => []))->build(['item']);
+        }, InvalidNodeFactoryReturnValue::class);
+
+        Assert::throws(function () use ($badNodeFactory) {
+            (new Recursive\TreeBuilder($badNodeFactory, fn() => 1234, fn() => null))->build(['item']);
+        }, InvalidNodeFactoryReturnValue::class);
+    };
+
+    // These factories do not return a valid Node instance (node contract).
+    $testFactory(fn() => null);
+    $testFactory(fn() => 'foo');
+    $testFactory(fn($data) => new NotNodeAtAll($data));
+    $testFactory(fn($data) => new NotMovable($data));
+
+    // The following can be uncommented to test that the failing tests actually do not fail... lol.
+//    $testFactory(fn($data) => new Node($data));
+})();
+
+

tests/nodes.phpt

@@ -6,6 +6,8 @@ namespace Dakujem\Test;
 
 use Dakujem\Oliva\Exceptions\AcceptsDebugContext;
 use Dakujem\Oliva\Exceptions\Context;
+use Dakujem\Oliva\Exceptions\InvalidInputData;
+use Dakujem\Oliva\Exceptions\InvalidNodeFactoryReturnValue;
 use Dakujem\Oliva\Node;
 use Dakujem\Oliva\Simple\NodeBuilder;
 use Dakujem\Oliva\Simple\TreeWrapper;
@@ -265,4 +267,20 @@ require_once __DIR__ . '/setup.php';
 })();
 
 
+// Test some edge cases of Path::fixed
+(function () {
+    Assert::throws(function () {
+        $builder = new NodeBuilder(fn(mixed $data) => new Node($data));
+        $builder->node('root', [
+            'foo',
+        ]);
+    }, InvalidInputData::class, 'Each child must be a movable node instance (Dakujem\Oliva\MovableNodeContract).');
+
+    Assert::throws(function () {
+        $builder = new NodeBuilder(fn(mixed $data) => new NotMovable($data));
+        $builder->node('root', [
+            'foo',
+        ]);
+    }, InvalidNodeFactoryReturnValue::class, 'The node factory must return a movable node instance (Dakujem\Oliva\MovableNodeContract).');
+})();
 

tests/setup.php

@@ -101,3 +101,67 @@ public static function wikiTree(): Node
         return $root;
     }
 }
+
+class NotNodeAtAll
+{
+    public function __construct(
+        public mixed $data
+    ) {
+    }
+}
+
+class NotMovable implements TreeNodeContract
+{
+    public function __construct(
+        public mixed $data
+    ) {
+    }
+
+    public function children(): iterable
+    {
+        // foo
+        return [];
+    }
+
+    public function parent(): ?TreeNodeContract
+    {
+        // foo
+        return null;
+    }
+
+    public function hasChild(int|string|TreeNodeContract $child): bool
+    {
+        // foo
+        return false;
+    }
+
+    public function child(int|string $key): ?TreeNodeContract
+    {
+        // foo
+        return null;
+    }
+
+    public function childKey(TreeNodeContract $node): string|int|null
+    {
+        // foo
+        return null;
+    }
+
+    public function isLeaf(): bool
+    {
+        // foo
+        return true;
+    }
+
+    public function isRoot(): bool
+    {
+        // foo
+        return true;
+    }
+
+    public function root(): TreeNodeContract
+    {
+        // foo
+        return $this;
+    }
+}

tests/tree.phpt

@@ -0,0 +1,65 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Dakujem\Test;
+
+use Dakujem\Oliva\Exceptions\NodeNotMovable;
+use Dakujem\Oliva\Node;
+use Dakujem\Oliva\Simple\NodeBuilder;
+use Dakujem\Oliva\Tree;
+use Tester\Assert;
+
+require_once __DIR__ . '/setup.php';
+
+// Test tree manipulation edge cases
+(function () {
+    $nodeWithUnmovableChild = new Node('A', ['one' => new Node('B'), 'two' => new NotMovable('impostor')]);
+    $nodeWithUnmovableParent = new Node('A', parent: new NotMovable('impostor'));
+
+    Assert::throws(function () use ($nodeWithUnmovableChild) {
+        Tree::unlinkChildren($nodeWithUnmovableChild);
+    }, NodeNotMovable::class, 'Encountered a non-movable node while manipulating a tree.');
+
+    Assert::throws(function () use ($nodeWithUnmovableParent) {
+        Tree::unlink($nodeWithUnmovableParent);
+    }, NodeNotMovable::class, 'Encountered a non-movable node while manipulating a tree.');
+
+    Assert::throws(function () use ($nodeWithUnmovableParent) {
+        Tree::link($nodeWithUnmovableParent, new Node(null));
+    }, NodeNotMovable::class, 'Encountered a non-movable node while manipulating a tree.');
+
+    Assert::throws(function () use ($nodeWithUnmovableChild) {
+        Tree::reindexTree($nodeWithUnmovableChild, null, null);
+    }, NodeNotMovable::class, 'Encountered a non-movable node while manipulating a tree.');
+})();
+
+(function () {
+    $proxy = new NodeBuilder(fn(mixed $data) => new Node($data));
+
+    $one = $proxy->node('A', $childrenOfOne = [
+        $b = $proxy->node('B'),
+        $c = $proxy->node('C'),
+    ]);
+    $two = $proxy->node('X', $childrenOfTwo = [
+        $y = $proxy->node('Y'),
+        $z = $proxy->node('Z'),
+    ]);
+
+    $shouldBeTwo = Tree::link($y, $one);
+    $shouldBeOne = Tree::link($y, $two);
+    Assert::same($two, $shouldBeTwo);
+    Assert::same($one, $shouldBeOne);
+
+    $counter = 0;
+    Tree::linkChildren($one, $childrenOfTwo, function ($parent) use (&$counter, $two) {
+        Assert::same($two, $parent);
+        $counter += 1;
+    });
+    Assert::same(2, $counter);
+})();
+
+(function () {
+    //
+})();
+