BUGFIX: Handle nodes with missing UriPathSegments in uriPathProjection

[mficzel]

Use nodeAggregateId as fallback for missing uriPathSegments in uriPathProjection.

This is necessary as the fallback to "" can create empty pathes for documents without uriPathSegment which is later on confused with root urls of the site. Falling back to the nodeAggregate id which is by definition unique and url-safe ensures that all documents get a valid url by the projection that can be adjusted later.

Empty uriPathSegments are usually avoided by validation and autocreation from the title but those mechanisms do not work for tethered nodes. This is even more problematic as this cannot be fixed later since the full uriPath is only created oCreate and laster on only the last segment of the path is adjusted without ever reevaluating the parentPath.

In addition this pr adds a nodeName to the site node in multiple test cases as this is actually required and only worked in the past because the empty uriPathSegment behaved like a site node.

Resolves: #5413

Upgrade instructions

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

[mficzel]

I wonder wether the site node detection that is currently done in handCreateNodeAggregateWith is wise because a
site node is basically defined as a named node that is directly below a rootNode.

[bwaidelich]

I don't fully understand, why some seemingly unrelated tests needed adjustments, but the change itself makes sense to me, thanks!

[mficzel]

The definition of site node (with path „“) used in the uriPathProjection is that it is a child of a rootNode and has a nodeName.

The adjusted tests only worked in the past because an empty uriPathSegment was treated implicitly like a site node. Since we now have fallback for empty uriPathSegments the tests that relied on that now need a proper site Node.

[mhsdesign]

we discussed an alternative in line 273 but it doesnt really work :D

                $uriPath = $this->nodeTypeManager->getNodeType($parentNode->getNodeTypeName())?->isOfType(NodeTypeNameFactory::NAME_SITE)
                    ? $uriPathSegment
                    : $parentNode->getUriPath() . '/' . $uriPathSegment;

[mficzel]

Still think resilience against missing uri path segments is better than adding an even tighter bond to the Neos NodeTypes. Even if this would work ;-)

The fallback way makes uriPathes more like a soft concern (which it is technically anyways) and allows routing of documents in cases where it is missing for any reason.

Only thing that would speak against that would be a case where the node-identifier based fallback does harm anything but i do not see a way for this to happen.

[mhsdesign]

It would preferably not necessary to use the node identifier here as that would now lead to odd uris containing a uuid which is not humanreadable. But then again - hey - at least there is an uri now and it can be edited and changes are picked up. Alternatively i suggested using the NodeName which is definitely available as a first choice over the node aggregate id as this will be kindof readable but to be honest this is such an edgecase it might not be worth discussing over this tiny detail.

A total alternative direction would be to introduce some kind of special path segment in the projection which says "hey" im just a placeholder please dont route me. But we agreed its more important to have an actual uri first than to have no uri until the reaction fixes it.

This whole issue shows that the node creation dialog of the ui is actually missing an important feature: To allow to specify the initial properties of the tethered nodes ... and fun fact the Neos ESCR core api cannot do this at the current point - there is no "initial node properties for tethered node" option, which we might yet have to introduce.

Either way - even with the thought out creation dialog extension we cant ensure that the uriPathSegment is set always (well we could via command hooks - but do we want to? related: #4581).

So it definitely makes sense to build the projection a little more forgiving.

---

Note that there are some more inconsistencies we found like that an empty ("") uriPathSegment on creation does not yet replaced with the node aggregate id to avoid problems like this.

And also the projection does not consider the case when the property is manually set to empty ("") or unset via propertyValuesToUnset.

neos-development-collection/Neos.Neos/Classes/FrontendRouting/Projection/DocumentUriPathProjection.php

Line 537 in b1168c4

$uriPathSegments[array_key_last($uriPathSegments)] = $newPropertyValues['uriPathSegment'];

Technically we would need to adopt and enforce the same behaviour as started with this pr: use the node aggregate id as fallback. Currently we dont do so and run in odd states. Either way this cleanup will be out of scope of this current bugfix and might not even be that critical as the Neos Ui regularly prevents the uripath segment to be unset or empty.
We can always fix that later in the future and advice affected users to replay.

[mficzel]

@mhsdesign adjusted the code to handle "" as uriPathSegments and also later setting of uriPathSegments to "" or null.

The general the rule for the uriPathSegment is now ($newPropertyValues['uriPathSegment'] ?? '') ?: $event->nodeAggregateId;