"getAncestorsGeneric" fails to return the last ancestor

[Xionite]

Since version 2.0, the method Node::getAncestorsGeneric does not return the last ancestor of the set of nodes. In version 1.5.3, the method did not exist and the original method looked like this :

    public function getAncestors($includeSelf = false)
    {
        $ancestors = $includeSelf ? array($this) : array();

        if (null === $this->parent) {
            return $ancestors;
        }

        return array_merge($ancestors, $this->parent->getAncestors(true));
    }

It works fine, since if the current node has no parent, the recursive iteration ends and the current node is returned. Since 2.0, the method getAncestors uses the method getAncestorsGeneric which looks like this :

    protected function getAncestorsGeneric(bool $includeSelf): array
    {
        if (null === $this->parent) {
            return [];
        }

        return array_merge($includeSelf ? [$this] : [], $this->parent->getAncestorsGeneric(true));
    }

This means that if the current node has no parent, an empty array will be returned instead of the current node. This is fine if $includeSelf equals false, but if I have a data set with for example 2 nodes, 1 root and 1 child, the method will ultimately return an empty array despite the fact that $includeSelf equals true at some point.

[BlueM]

I’m afraid this cannot easily be fixed.

The assumed fix for the behavior you describe is: change https://github.com/BlueM/Tree/blob/master/README.markdown?plain=1#L227 to return $includeSelf ? [$this] : [];. Which has the unintended effect of including the root node (which should no longer be the case as of 2.0, cf. https://github.com/BlueM/Tree/blob/master/README.markdown?plain=1#L227), and luckily there are two tests which fail with the change above. A remedy would be to check if the current node is the root node (and if yes, don’t include it in the array to return), but this check is not possible, as currently, a null parent is assumed to identify the root node (see comment https://github.com/BlueM/Tree/blob/master/src/Tree/Node.php#L21), so there is no way to tell apart the “detached” node (the node without parent in your use case) and the root node.

That means that the only real solution would be to give every Node access to the root node (so that each node can test whether it itself is the root node) or to add a bool property named something like $isRoot (and only set for the root node), which could be used for testing if the current node should be included in the return value. Given that detached nodes are an edge case, it looks to me like the fix would introduce quite some overhead which doesn’t look appropriate to me.

What do you think?