BUGFIX: Restore Neos.Neos:NodeUri, neos:link.node and LinkingService

[mhsdesign]

Resolves: #4524
Related: #4552

For linking to nodes in Neos (via Neos.Neos:NodeUri, neos:link.node or neos:uri.node) following logic will apply for Neos 9:

If a node is passed as string the base node is required.
Following string syntax is allowed:

• /<Neos.Neos:Sites>/my-site/main
• some/relative/path

Only allowed in Fluid: node://my-node-identifier

Deprecated syntax:

• /sites/site/absolute/path
• ~/site-relative/path
• ~

Not supported syntax:

• ./neos/info
• ../foo/../../bar

Changes:

• handle string node path syntax in Neos.Neos:NodeUri which requires the baseNodeName to be set (9.0 Neos.Neos:NodeUri relative uris? #4524).
• Neos.Neos:NodeUri will like the fluid viewhelpers now also accept the node://my-id syntax
• as partial replacement of \Neos\Neos\TYPO3CR\NeosNodeService::normalizePath the new utility \Neos\Neos\Utility\LegacyNodePathNormalizer::tryResolveLegacyPathSyntaxToAbsoluteNodePath will handle the legacy path syntax
  • instead of reintroducing support of path traversal like /../ via 8.3's \Neos\ContentRepository\Domain\Utility\NodePaths::replaceRelativePathElements we now correctly document that this syntax is not allowed and throw an descriptive error (previously NodePath::fromString would have thrown an error as the node name pattern is not supporting dots).
• the NodeViewHelper now correctly use the baseNodeName/ the documentNode to resolve string nodes
• for shortcuts the link NodeViewHelper (neos:link.node) will initialise the linkedNode variable as the shortcut itself and NOT resolve it. This is the same behaviour in Neos 8.3. Previously shortcut resolving was once earlier part of the uri builder meaning the linkedNode was the actual shortcut, but as Neos 8.3 didnt feature this, we wont need to fix this for fluid.
• the duplicated logic in the NodeViewHelper's resolveNodeAddressFromPath was extracted into \Neos\Neos\Utility\NodeAddressNormalizer::resolveNodeAddressFromPath
• the LinkingService was marked as deprecated and now uses the LinkHelper where possible
• the LinkHelper was simplified to fully work on psr getScheme and getHost instead of regex
• \Neos\Neos\Fusion\Helper\LinkHelper::resolveNodeUri was marked as deprecated as its not usable from within fusion, and it requires the LinkingService
• setting node to null in the fluid viewhelpers and fusion will not lead to link generation of the baseNodeName/ the documentNode but an exception
• the pattern PATTERN_SUPPORTED_URIS in convert uris was simplified as it was previously a union but the character set already matches uuids

Left open points:

• naming of NodeAddressNormalizer
• find alternative for LinkingService::createSiteUri
• why do we evaluate the hidden state in the LinkingService? We dont do it in the new uri builder
• remove return $this->newLinkHelper->hasSupportedScheme($uri); in the LinkingService and add rector migration?
• add behat tests for Neos.Neos:NodeUri

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

[mhsdesign]

Another point is, should we actually introduce support again for these legacy path syntax when the most prominent places flow query, and many places in the configuration doesnt allow it? So having this runtime magic here for Neos.Neos:NodeUri might not be worth it, or at least not if we dont allow it again in the other places

• Neos.Node.nearestContentCollection
  • supports new absolute syntax and relative
  • why do we support absolute paths here at all??
• q(node).find
  • supports new absolute syntax and relative
• q(node).children
  • supports relative syntax

see also: neos/rector#24

[bwaidelich]

No +1 yet, because I failed to review this properly..
Just a little, half unrelated, comment.
Also I wonder: Is this behavior tested somehow?

[mhsdesign]

Also I wonder: Is this behavior tested somehow?

was my last todo:) i introduced a test for 8.3, upmerged that and adjusted it here.
I didnt tests the exceptional cases yet because there is an absorbing throwable storage involved and i need a good plan.

[mhsdesign]

regarding #4886 (comment)
i removed absolute node path functionality from Neos.Node.nearestContentCollection and also tested if .find() worked in 8.3 with ~ and it doesnt. So we indeed dont have to support it here.
Further like in 8.3 only the view-helpers will interpret node:// not the fusion node uri.

I also remember why i deprecated ~ with this change in the LegacyNodePathNormalizer: because nodePaths/ node names are deprecated in general and thus site relative paths are no use as well.

so definitely merge ready

// cc @kitsunet

[kitsunet]

I'll take a last look tomorrow/later and merge

[kitsunet]

The one comment about reusing the regex from basti seems like a good idea but by no means a blocker, thanks, this looks very ncie.

[mhsdesign]

I think this is the wrong name still ... maybe NodePathResolver::resolveNodeAddressFromPath or NodePathUtitlity::retrieveNodeAddressByPath idont know ...
or does *Normalizer fit the usecase here?

cc @kitsunet

[kitsunet]

Yeah I definitely don't like Normalizer here, more like Converter or Resolver given that the method is named resolve...?

Do we want this @internal to avoid people using it (as we probably don't want to keep this forever)?

[mhsdesign]

Jup, added internal and renamed it to NodePathResolver::resolveNodeAddressByPath ... the normalise naming was stolen from 8.3 but doesnt fit anymore ^^

[bwaidelich]

I don't understand the return type. This can either return an AbsoluteNodePath or lead to an exception or it returns null. What is the meaning of null and is that a good abstraction? Maybe we can at least add some @return doc comment explaining the ambiguity?

[bwaidelich]

Ah I see, in the usages you do

tryResolveLegacyPathSyntaxToAbsoluteNodePath($path, ..) ?? $path

wouldn't it make things easier to have some

$absoluteNodePath = AbsoluteNodePath::tryFromString($path);
if ($absoluteNodePath !== null) {
    return $absoluteNodePath;
}

at the beginning of this method and get rid of the nullable return type?

[mhsdesign]

youre right to point this out:) I forgot to write down my line of thought here.

The idea is to separate the legacy behaviour from the more future proof, so we can later just remove the dependency to tryResolveLegacyPathSyntaxToAbsoluteNodePath ... calling AbsoluteNodePath::tryFromString would not be enough as the relative base node behaviour would also need to be handled ...

its a little odd but its all @internal so maybe keep it this way?

Alternatively this could be the api:

public function preprocessLegacyPathSyntax(
        string $path,
        Node $baseNode
    ): string;

[mhsdesign]

its a little weird but for now well keep it okay its internal :)

[mhsdesign]

thank you for the review though ❤️ though now its time to tag the beta:)

[bwaidelich]

I'm absolutely fine with being overruled, but it's a bit demotivating if all my comments are just disregarded. You could have at least added the @return doc comment.
Anyways, not a big deal in this case of course

[kitsunet]

you are right @bwaidelich we had a short call about it, and decided this is ultimately throw away code and we shouldn't bikeshed about it and instead get the beta out. The doc comment would definitely not hurt though.
SORRY! ❤️wasn't meant to ignore you just to get the topic off of our (collective) heads, we discussed the other options but didn't see any single one that was much more pretty and therefore worth changing to.

[bwaidelich]

no worries, no need to be sorry. But thanks for the explanation!