9.0 Discussion Node property mapping in controllers

[mhsdesign]

in neos 8 one could leverage the property mapping to write a controller like this

public function someAction(NodeInterface $node): {}

and pass as node its fully qualified serialised representation: the old context path?node=/sites/neosdemo/features/multiple-columns@user-admin;language=en_US.

In Neos 9 this is not that easily doable as the NodeAdress (the closest to its old context path) is not fully qualified see #4564. The correct way would be:

public function indexAction(string $node) {
    $siteDetectionResult = SiteDetectionResult::fromRequest($this->request->getHttpRequest());
    $contentRepository = $this->contentRepositoryRegistry->get($siteDetectionResult->contentRepositoryId);
    $nodeAddress = NodeAddressFactory::create($contentRepository)->createFromUriString($node);
    $workspace = $contentRepository->getWorkspaceFinder()->findOneByName(
        $nodeAddress->workspaceName
    );
    $subgraph = $contentRepository->getContentGraph()->getSubgraph(
        $workspace->currentContentStreamId,
        $nodeAddress->dimensionSpacePoint,
        VisibilityConstraints::withoutRestrictions()
    );
    $node = $subgraph->findNodeById($nodeAddress->nodeAggregateId);

But due to a hack in the NewNodeConverter - accessing the bootstraps getActiveRequestHandler - we can make that magically work:

public function indexAction(Node $node) {

When i stumbled upon the NewNodeConverter, based on its comment, i assumed that its only used for fusion uncached serialisation. Thats why i opened this issue #4732 (and pr #4734) to remove fusion dependency to the node property mapper and the hack.

But it seems i only thought, being initially unaware of the hack, that the old property mapping style would and should not work. By the fact that we dont use it i was even more convinced. It seems there was until now no real discussion and decision?.

[mhsdesign]

Copied from #4734 (comment)
With this PR Neos and Fusion will completely work without the property mapper #4734

In my opinion its super important that people dont accidentally use the property mapper with the new nodes.
Allowing mapping would render all of our strictness and new explicitness useless as it would allow again magical nodes in controllers:

public function nodeAction(Node $node): {}

which will already now confusion for example about the nodes format when serialised ...

Lets to it rather explicit this time please :D

[mhsdesign]

@kitsunet wrote:

Allowing mapping would render all of our strictness and new explicitness useless as it would allow again magical nodes in controllers:

I think that is a core expectation though, countless projects will have controllers expecting a node and I don#t see why for the time being as the propertyMapper IS still the suggested and only way to do controller marshalling build into Flow, this should work

[mhsdesign]

In the previous weekly (9.2.) we discussed (bastian, christian, denny, me) that we DO NOT want to support someAction(Node $nodeIdentity).
The consent was that mapping entities magically was never a good idea.

Instead to still make it simpler to get a node in a controller we decided that we want to introduce a fully qualified NodeIdentity. By also encapsulating the ContentRepositoryId it becomes less tedious to transform, and its representation is irrelevant of the domains host.

Similarly as bastian put it here #4564 (comment), the introduction of the NodeIdentity with the overhauled routing we can allow patterns like these:

public function someAction(string $nodeIdentity): {
   $nodeIdentity = NodeIdentity::fromJsonString($nodeIdentity);
   // ...
}

// with little property converter, almost no gain
public function someAction(NodeIdentity $nodeIdentity): {
  $node = $this->someService->findNodeByIdentity($nodeIdentity);
}

[mhsdesign]

Actually today when christian Bastian and me were discussing #5072 we came to revisit our decision here. While sometimes thinking that we know whats better for the user we should not go to far. This might be such case. That means we will still support property mapping Nodes and auto converting them from their fully qualified json serialised NodeAddress. Though we might call that deprecated and suggest better alternatives.

Initially i was very much against this node property mapper because of the mentioned hack that we access the bootstrap's active request handler. But with introduction that the node address also contains its content repository this will not a problem anymore and the parameter will contain everything the property mapper needs to know.

[mhsdesign]

But how to we want to actually determine the correct visibility for the subgraph?
currently we do it based on the workspacename

$subgraph = $contentRepository->getContentGraph($nodeAddress->workspaceName)->getSubgraph(
    $nodeAddress->dimensionSpacePoint,
    $nodeAddress->workspaceName->isLive()
        ? VisibilityConstraints::frontend()
        : VisibilityConstraints::withoutRestrictions()
);

but if i remember correctly this will be solved with bastians cr privileges?

[mhsdesign]

Resolved via #5068