!!! TASK: 9.0 Set nodeName to null for regular created nodes

[mhsdesign]

WAIT BEFORE MERGE OF #3569 - as some tests would be cool ;)

resolves #3605

The ui currently tries to generate random (not particular "safe" as in unique) nodeNames. We dont need that anymore and it should go.

Also we currently dont account for $node->nodeName being null. And we should better check for the classification #3515 (comment)
What I did

How I did it

How to verify it

[mhsdesign]

Well this is not totally correct as a tethered
node is not guaranteed to have a nodeName (see command) why don’t we actually just check if the classification of the node is tethered?

[nezaniel]

Tethered nodes always have node names (see NodeCreation::createTetheredWithNode), so if a node does not it is guaranteed to not be tethered.
Tethered is also one of the classifications, so a tethered node will always be classified as tethered.

The difference is that the classification is projection state, e.g. if the node type configuration changed in the meantime, this information might be outdated, while asking the NodeTypeManager will be always in sync with the configuration - while not necessarily reflecting the content graph state.

That said I'd go with the classification as this is more consistent with the rest I suppose

[nezaniel]

As for the NodeInfoHelper: I think we should remove or at least deprecate the isAutocreated method and check the classification instead

[mhsdesign]

Yeah its used once in the helper itself internal (to provide the info for the neos ui)

the other usage is:

neos-ui/Classes/Domain/Model/Changes/AbstractStructuralChange.php

Line 190 in a08a6cb

if (NodeInfoHelper::isAutoCreated($node, $subgraph)) {

[bwaidelich]

With this change applied, many more occurrences of {node.path} will lead to an exception. Are we aware of that fact and willing to ignore it? (not a rhetoric question)

[mhsdesign]

Yes I thought that was the plan as discussed in the weeklies?
nodeName and path cannot be relied upon.

but we can discuss this of course again … @skurfuerst what do you say?

[bwaidelich]

Yes I thought that was the plan as discussed in the weeklies?
nodeName and path cannot be relied upon.

IIRC we agreed that both are deprecated and that ContentSubgraphInterface::retrieveNodePath() can throw an exception. I don't want to restart that discussion.

My point was just to make aware that this change will increase the chances of exceptions at rendering time. Personally I'm fine with that because I try to avoid node paths already

[mhsdesign]

Hmm. Legacy projects can be migrated and the path logic would still work for existing nodes because they have names of course. For newly created nodes the use of nodepath would partially break your website. But the adjustment here of the developer to migrate away from nodenames is critical as otherwise nodes created via a custom import won’t work in this universe and everyone would be forced again to fill the node name with random stuff. -> this is not what we want as otherwise we wouldn’t need to make the nodename nullable. (Especially true for open source packages using nodepath)

Additionally the nodepath was deprecated for ages (since neos 4 or 5?) and I think even announced at the con?

Another point is that the now removed todo in line 100 says that our current nodename generation logic is not safe. We would need to look in all dimensions and check if the nodename is already occupied. And I rather not have this logic in place.

I can imagine two things:

Don’t generate nodeNames from the ui(this pr)

or allow a flag to opt in the old behaviour-> it’s important to be opt in so the ecosystem will adjust to empty node names (or they could do it via aop)

[bwaidelich]

Legacy projects can be migrated and the path logic would still work for existing nodes

Good point!

[mhsdesign]

an hack for everyone to restore the old behavior - which is highly discouraged, would be to use aop (this might break at anytime):

@Flow\Around("Neos\Neos\Ui\Domain\Model\Changes\AbstractCreate->getName()")

return $original ?: uniqid('node-', false);

[mhsdesign]

Aaaand much scarier than path might be if people rely on nodeName for anchor / id / fragment generation on a site.

But the same arguments apply here as above.

The other choice would be that we have to guarantee for nodeName to be set if the escr is used for the Neos.Neos cms.

[mhsdesign]

The fact that nodeName will be nullable must have been also discussed ages ago ^^

the with 4.3 introduced \Neos\ContentRepository\Domain\Projection\Content\TraversableNodeInterface::getNodeName is nullable and the non-nullable original getName was deprecated.

see pr neos/neos-development-collection#2430

In theory people might not really have noticed this of course, and i assume no one currently handles the null case for getNodeName