9.0 Discuss Node.nodeName

[bwaidelich]

Currently Node::nodeName is nullable because nodes don't require an explicit name.
This is error prone because it can lead to invalid node paths (see https://neos-project.slack.com/archives/C3MCBK6S2/p1685647511878619).

I would suggest to make the name default to the node aggregate id (and to rename the field to just "name" while we're at it):

[mhsdesign]

Okay we discussed a lot of points regarding the nodeName, but we agreed to keep the behavior as is for now.

I would like to close this, but maybe first further elaborate why we wont make it nullable? (i dont have all the points in my head anymore ^^)

[mhsdesign]

Also related: #4313 (comment)

Where i wrote down that there is currently no future in sight were we totally get rid of the node name.

[bwaidelich]

Thanks for all the discussions and documentations. I'll close this one for now, we have the comments for reference

[mhsdesign]

I think i had this idea a few times to attach the tethered node name to the NodeAggregateClassification, and just to document for me that its a dead end ill write it down here :D

Currently code like the following doesnt look that nice due to the outside made invariant check:

if ($childNode->classification->isTethered()) {
    assert($childNode->name !== null); // it's tethered!
    $actualTetheredChildNodes[$childNode->name->value] = $childNode;
}

And phpstan doesnt support yet this invariant:

Sadly phpstan doenst allow us to annotate this behavior ...

https://phpstan.org/writing-php-code/narrowing-types#custom-type-checking-functions-and-methods

And also the feature like in @return doesnt work here https://phpstan.org/blog/phpstan-1-6-0-with-conditional-return-types:

/** @var ($this->classification is TETHRED ? string : null) */

playground: https://phpstan.org/r/7c491f32-89df-4b4b-a218-00c2b1889933

So i thought why not turn the NodeAggregateClassification into a php class with enum like behaviour and the node name attached if its tethered?

final class NodeAggregateClassification implements \JsonSerializable
{
    const CLASSIFICATION_REGULAR = 'regular';

    const CLASSIFICATION_ROOT = 'root';

    const CLASSIFICATION_TETHERED = 'tethered';

    /** @var array<string, self> */
    private static array $instances;

    private function __construct(
        public readonly string $value,
        public readonly ?NodeName $tetheredNodeName = null
    ) {
    }

    public static function from(string $value, ...?): self
    {
        return $instances[$value] ??= match ($value) {
            self::CLASSIFICATION_ROOT => self::CLASSIFICATION_ROOT(),
            self::CLASSIFICATION_REGULAR => self::CLASSIFICATION_REGULAR(),
            self::CLASSIFICATION_TETHERED => self::CLASSIFICATION_TETHERED($nodeName ... ????),
            default => throw new \InvalidArgumentException('Invalid node aggregate classification "' . $value . '", must be one of the defined constants.', 1719739959),
        };
    }

    public static function CLASSIFICATION_ROOT(): self
    {
        return new self(self::CLASSIFICATION_ROOT);
    }

    public static function CLASSIFICATION_REGULAR(): self
    {
        return new self(self::CLASSIFICATION_REGULAR);
    }

    public static function CLASSIFICATION_TETHERED($nodeName ???): self
    {
        return new self(self::CLASSIFICATION_TETHERED, $nodeName );
    }

    public function isRoot(): bool
    {
        return $this->value === self::CLASSIFICATION_ROOT;
    }

    public function isRegular(): bool
    {
        return $this->value === self::CLASSIFICATION_REGULAR;
    }

    public function isTethered(): bool
    {
        return $this->value === self::CLASSIFICATION_TETHERED;
    }

    public function getValue(): string
    {
        return $this->value;
    }

    public function equals(self $other): bool
    {
        return $this->value === $other->value;
    }
}

That seems as long fun until you have to think about if a classification equals another if they have a different node name? Should the instance cache also cache CLASSIFICATION_TETHERED and how to deal with the node name? And how should the main constructor from look like? should one always pass a $possibleNodeName and just hope that classification and node name match?
Another thing is, how to create a CLASSIFICATION_TETHERED instance without nodename just to check if two classifications equal?? (we do this in tests)

In the end i came to the conclusion that this behaviour is best kept part of the node.

And maybe - as there is a feature request for this: phpstan/phpstan#10463 - we will get phpstan support some point.

in the meantime the compromise is that we just document it inline and assert the correct behaviour see #4469