!!! TASK: Deprecate setting NodeNames via cr commands

[mhsdesign]

Resolves: #5050

Upgrade instructions

The parameter $nodeName is not accepted anymore in the main factory via CreateNodeAggregateWithNode::create and CopyNodesRecursively.

Please adjust your code to use the additional with*-er instead.

$command = CreateNodeAggregateWithNode::create(...);

if ($nodeName) {
    $command = $command->withNodeName($nodeName);
}

If you set their value previously simply to null, you can just omit setting this argument.

Review instructions

Checklist

• Code follows the PSR-2 coding style
• Tests have been created, run and adjusted as needed
• The PR is created against the lowest maintained branch
• Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
• Reviewer - The first section explains the change briefly for change-logs
• Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

[mhsdesign]

$succeedingSiblingNodeAggregateId might also be worth having a with* for to have extra space for documentation :D My guess is that this will not be used directly and the next option $initialPropertyValues is super important and having a null in between is me :D

implemented via: https://github.com/neos/neos-development-collection/commit/30e35d598703039f0b9c67776a0e4fce4feb90cb.patch

[kitsunet]

agreed

[bwaidelich]

succeeding sibling is a common requirement – you only don't need it if you want the new node to be the first child (or if you don't care about the position)

[nezaniel]

I definitively prefer named arguments over withers...

[mhsdesign]

alternatively we can put succeedingSiblingNodeAggregateId at the end (after initialProperties) or just omit this change from this change ... it was just a thought which came up here: #5082 (comment)

i think we definitely want to mark setting the nodeName as deprecated as it is infact a deprecated feature.

[nezaniel]

imho node names become a deprecated feature once Neos starts resolving sites by id, not name.
I'd agree to make nodename a wither, but not succeeding sibling id since this is a common use case (I'm literally using this at this moment in the starship)

[mhsdesign]

Okay fair enough, in that case i was maybe too quick ... anyways i reverted the change to introduce withSucceedingSiblingNodeAggregateId. This part is about the node names ;)

[bwaidelich]

I like, thanks. Just wondered why we'd need CreateNodeAggregateWithNodeAndSerializedProperties::withInitialPropertyValues().

I definitively prefer named arguments over withers...

I agree, we should use named arguments especially for command creation more to make that a common concept. It makes the code easier to reason and allows for skipping optional parameters.
Re nodeName I think it makes sense to have the "wither" because it allows us to document that deprecated behavior more clearly

[bwaidelich]

Why was this added? initialPropertyValues can be set in the constructor already!?

[mhsdesign]

It was an inconsistency here its just to align the behaviour fully to CreateNodeAggregateWithNode. This serialised command is internal but i thought why not.

[bwaidelich]

Let me rephrase my question:
Why do we need a wither for parameters that are already part of the constructor?
I have the same question re CreateNodeAggregateWithNode. I traced it back to some special requirement in the NodeCreationCommands of Neos.UI.

IMO it's not the best idea to let those downstream dependencies control the architecture of the API (instead it should just collect all details before creating the command instances). But obviously this is not a big issue at all and it might even be useful for other, similar. requirements

[mhsdesign]

Kinda true i see. CreateNodeAggregateWithNode::withInitialPropertyValues probably always just existed for this particular case.

Annnnnd - trommel wirbel bitte 🥁 - you introduced it *g

neos/contentrepository-development-collection@a73c8c7

Anyway with my neos ui adjustments and the new NodeCreationCommands of Neos.UI we can actually get rid of this with*er and do it differently youre right.

So should i do it. Would be up for it.

[mhsdesign]

But no, that is super impractical actually. Because not everything can be set by the main constructor, we would have to write this:

$newFirst = CreateNodeAggregateWithNode::create(
    $this->first->workspaceName,
    $this->first->nodeAggregateId,
    $this->first->nodeTypeName,
    $this->first->originDimensionSpacePoint,
    $this->first->parentNodeAggregateId,
    $this->first->succeedingSiblingNodeAggregateId,
    $newInitialPropertyValues
);

if ($this->first->nodeName !== null) {
    $newFirst = $newFirst->withNodeName($this->first->nodeName);
}

if ($this->first->tetheredDescendantNodeAggregateIds !== null) {
    $newFirst = $newFirst->withTetheredDescendantNodeAggregateIds($this->first->tetheredDescendantNodeAggregateIds);
}

would still be fine as its abstracted away in the Neos.Ui for the users, but we are creating so many intermediate instances and there is a possibility of bugs if you forget as user to call any with*er to transfer state.

idk.

[bwaidelich]

Annnnnd - trommel wirbel bitte 🥁 - you introduced it *g

I can be - trommel wirbel bitte 🥁 - wrong

[bwaidelich]

But as written above, I think it's not an issue. I just wondered why you had added it in this case.

What we could consider (for a separate PR): whether we want to create more generic withers for those cases. i.e:

    private function with(
        public WorkspaceName $workspaceName = null,
        public NodeAggregateId $nodeAggregateId = null,
        public NodeTypeName $nodeTypeName = null,
        public OriginDimensionSpacePoint $originDimensionSpacePoint = null,
        public NodeAggregateId $parentNodeAggregateId = null,
        public SerializedPropertyValues $initialPropertyValues = null,
        public NodeAggregateId $succeedingSiblingNodeAggregateId = null,
        public NodeAggregateIdsByNodePaths $tetheredDescendantNodeAggregateIds = null,
    ) {
        return new self(
            $workspaceName ?? $this->workspaceName,
            $nodeAggregateId ?? $this->nodeAggregateId,
            $nodeTypeName ?? $this->nodeTypeName,
            $originDimensionSpacePoint ?? $this->originDimensionSpacePoint,
            $parentNodeAggregateId ?? $this->parentNodeAggregateId,
            $initialPropertyValues ?? $this->initialPropertyValues,
            $succeedingSiblingNodeAggregateId ?? $this->succeedingSiblingNodeAggregateId,
            $this->nodeName,
            $tetheredDescendantNodeAggregateIds ?? $this->tetheredDescendantNodeAggregateIds,
        );
    }

We did so in other places and IMO it works out nicely.
And in the future we might be able to replace the redundant implementation with some clone with syntax

[mhsdesign]

But as written above, I think it's not an issue. I just wondered why you had added it in this case.

just for consistency. So if someone is using this low level stuff on purpose it should not be too hard to use, but youre right its YAGNI - i also doubted if i should add it and rolled a dice :D

[mhsdesign]

Removed it ;)

[kitsunet]

Note here, my problem with deprecating NodeNames overall (as I understand Bernhard) is still the relative addressing, with non unique names, ala (q(node).children('main')) which the aggregate identifier cannot solve as it must be globally unique.

[mhsdesign]

true, but for now we only deprecate it for non tethered nodes (maybe we have to do this more clearly?)

we could for example keep findNodeByPath deprecated, but introduce findTetheredNode which effectively does the same but will only return nodes that are of classification tethered and otherwise null.

What do you think?