diff --git a/src/Simple/NodeBuilder.php b/src/Simple/NodeBuilder.php index ed53598..c5e1a75 100644 --- a/src/Simple/NodeBuilder.php +++ b/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) diff --git a/src/Tree.php b/src/Tree.php index d0ca6d7..9ab4d62 100644 --- a/src/Tree.php +++ b/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); } } diff --git a/tests/factory.phpt b/tests/factory.phpt new file mode 100644 index 0000000..f7d2dbb --- /dev/null +++ b/tests/factory.phpt @@ -0,0 +1,46 @@ + []))->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)); +})(); + + diff --git a/tests/nodes.phpt b/tests/nodes.phpt index 55c60ec..e08a46e 100644 --- a/tests/nodes.phpt +++ b/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).'); +})(); diff --git a/tests/setup.php b/tests/setup.php index fabbb02..a1f5d22 100644 --- a/tests/setup.php +++ b/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; + } +} \ No newline at end of file diff --git a/tests/tree.phpt b/tests/tree.phpt new file mode 100644 index 0000000..9649847 --- /dev/null +++ b/tests/tree.phpt @@ -0,0 +1,65 @@ + 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 () { + // +})(); +