!!! TASK: Refactor `Node` property mapper to use new NodeAddress #5068

mhsdesign · 2024-05-18T17:47:20Z

Solves: #4873

This is an intermediate step after !!! FEATURE: Overhaul node uri building before we fully move to the new node address serialisation in the Neos Ui.
Everything was refactored here that does not require adjustments in the Neos Ui (and the workspace module was also not really touched).

avoid using the legacy node address instance in the code. The NodeAddressFactory is only used at the places where we still depend on the legacy format, but we create already new node address instances via createCoreNodeAddressFromLegacyUriString
removed the $contentStreamId field from the legacy NodeAddress already (as we can otherwise not transform a new node address to this one). Also this prevents the $contentStreamId field from accidentally being used again in the migration time.
removed the the unused legacy node address isInLiveWorkspace method.
The currently unused method Neos.Node.serializedNodeAddress(node) will return the new node address format
where previously Node $node was used to trigger property mapping (DataSourceController and WorkspaceController) we now pass the node as string and use the legacy node address factory to handle the legacy format manually. A followup will adjust everything to work with the new node address.
the unused linkinservice will use the new node address string format if a string is passed as $node
the Node property mappers were adjusted to use the new node address and not depend on the global active request handler
lint Neos.Workspace.Ui/Classes with phpstan

Upgrade instructions

This change is breaking for existing Neos 9 users as the node being property mapped will require a new serialised input.

public function someAction(Node $node): {}

In Neos 8.3 you could call this action with the node like

?node=/sites/neosdemo/features/multiple-columns@user-admin;language=en_US

in Neos9-dev it was

?node=user-admin__eyJsYW5ndWFnZSI6ImVuX1VTIn0%3D__995c9174-ddd6-4d5c-cfc0-1ffc82184677

and now it must be the node address in json NodeAddress::toJson()

?node=%7B%22contentRepositoryId%22%3A%22default%22%2C%22workspaceName%22%3A%22live%22%2C%22dimensionSpacePoint%22%3A%7B%22language%22%3A%22es%22%2C%22country%22%3A%22ar%22%7D%2C%22aggregateId%22%3A%22node-1%22%7D

to build such a uri you can simply use:

$myActionUri = new Uri($this->uriBuilder->uriFor(...));
$myActionUriWithNode = UriHelper::uriWithAdditionalQueryParameters(
    $myActionUri,
    ['node' => NodeAddress::fromNode($node)->toJson()]
);

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

…ly instead of property mapping

see neos#5043

bwaidelich

I just left some comments on first flyover, but I don't understand all the implications yet

bwaidelich · 2024-06-30T11:23:35Z

Neos.Neos/Classes/Controller/Backend/ContentController.php

@@ -150,14 +150,15 @@ public function uploadAssetAction(Asset $asset, string $metadata, string $node,
        $contentRepositoryId = SiteDetectionResult::fromRequest($this->request->getHttpRequest())
            ->contentRepositoryId;
        $contentRepository = $this->contentRepositoryRegistry->get($contentRepositoryId);
-        $nodeAddress = NodeAddressFactory::create($contentRepository)->createFromUriString($nodeAddressString);
+        // todo legacy uri node address notation used. Should be refactored to use json encoded NodeAddress


The TODO is related to line 149, right?

jip a followup will change the whole neos ui and neos.neos to work on the new format we agreed upon.

bwaidelich · 2024-06-30T11:24:06Z

Neos.Neos/Classes/FrontendRouting/NodeAddress.php

- *  in contentStreamId $contentStreamId
- *
- * It is used in Neos Routing to build a URI to a node.
- *


Why did you remove those?

because its just @deprecated and will be removed soon.
The statements are not true anymore: It is used in Neos Routing to build a URI to a node.

Right, thanks for clarifying, I briefly confused the two NodeAddresses.. Good that we are getting rid of one :)

bwaidelich · 2024-06-30T11:25:33Z

Neos.Neos/Classes/FrontendRouting/NodeAddress.php

@@ -42,8 +32,9 @@
    /**
     * @internal use NodeAddressFactory, if you want to create a NodeAddress
     */
+    /** @phpstan-ignore-next-line its all just temporary */


Could you elaborate the comment a bit? Why do we need it?

i dindt want to remove the param yet as it would break the neos ui now (we construct it over there) this is just intermediate

bwaidelich · 2024-06-30T11:26:09Z

Neos.Neos/Classes/FrontendRouting/NodeAddress.php

    public function __construct(
-        public ContentStreamId $contentStreamId,
+        ?ContentStreamId $_contentStreamId,


I assume, you just kept this to keep the PR smaller? But is it necessary to rename the parameter?

its just a dummy stub, and _ indicates its unused. the class will be obliterated into nothing.

bwaidelich · 2024-06-30T11:28:35Z

Neos.Neos/Classes/FrontendRouting/NodeAddress.php

    public function __toString(): string
    {
        return sprintf(
-            'NodeAddress[contentStream=%s, dimensionSpacePoint=%s, nodeAggregateId=%s, workspaceName=%s]',
-            $this->contentStreamId->value,
+            'NodeAddress[dimensionSpacePoint=%s, nodeAggregateId=%s, workspaceName=%s]',


I hope that we don' relay on this string representation (=> do we really need it, what about using __debugInfo() instead?) – and if we keep it: Why did you decide to move the workspace name to the end?

we probably dont even need the __toString anymore at this time ... but i kept it ... it will all go away. Its legacy.

bwaidelich · 2024-06-30T11:30:11Z

Neos.Neos/Classes/Service/Controller/DataSourceController.php

     * @throws NeosException
     */
-    public function indexAction($dataSourceIdentifier, Node $node = null): void
+    public function indexAction($dataSourceIdentifier, string $node = null): void


you removed the param annotation – I would instead suggest to keep it and add some docs instead – what is this $node string expected to contain?

as per my main description:

where previously Node $node was used to trigger property mapping (DataSourceController and WorkspaceController) we now pass the node as string and use the legacy node address factory to handle the legacy format manually. A followup will adjust everything to work with the new node address.

OK, sorry, missed that one!

bwaidelich · 2024-06-30T11:30:35Z

Neos.Neos/Classes/Service/Controller/DataSourceController.php


        $this->view->assign('value', $values);
    }

+    private function deserializeNodeFromLegacyAddress(?string $stringFormattedNodeAddress): ?Node
+    {
+        if (!$stringFormattedNodeAddress) {


Suggested change

if (!$stringFormattedNodeAddress) {

if ($stringFormattedNodeAddress === null) {

mhsdesign

regarding the why of this change, i tried to put it into words in my main description above but i would also be happy to elaborate on that or have a quick huddle.

mhsdesign · 2024-06-30T11:44:44Z

Neos.Neos/Classes/Controller/Backend/ContentController.php

@@ -150,14 +150,15 @@ public function uploadAssetAction(Asset $asset, string $metadata, string $node,
        $contentRepositoryId = SiteDetectionResult::fromRequest($this->request->getHttpRequest())
            ->contentRepositoryId;
        $contentRepository = $this->contentRepositoryRegistry->get($contentRepositoryId);
-        $nodeAddress = NodeAddressFactory::create($contentRepository)->createFromUriString($nodeAddressString);
+        // todo legacy uri node address notation used. Should be refactored to use json encoded NodeAddress


jip a followup will change the whole neos ui and neos.neos to work on the new format we agreed upon.

mhsdesign · 2024-06-30T11:45:33Z

Neos.Neos/Classes/FrontendRouting/NodeAddress.php

- *  in contentStreamId $contentStreamId
- *
- * It is used in Neos Routing to build a URI to a node.
- *


because its just @deprecated and will be removed soon.
The statements are not true anymore: It is used in Neos Routing to build a URI to a node.

mhsdesign · 2024-06-30T11:46:15Z

Neos.Neos/Classes/FrontendRouting/NodeAddress.php

@@ -42,8 +32,9 @@
    /**
     * @internal use NodeAddressFactory, if you want to create a NodeAddress
     */
+    /** @phpstan-ignore-next-line its all just temporary */


i dindt want to remove the param yet as it would break the neos ui now (we construct it over there) this is just intermediate

mhsdesign · 2024-06-30T11:46:51Z

Neos.Neos/Classes/FrontendRouting/NodeAddress.php

    public function __construct(
-        public ContentStreamId $contentStreamId,
+        ?ContentStreamId $_contentStreamId,


its just a dummy stub, and _ indicates its unused. the class will be obliterated into nothing.

mhsdesign · 2024-06-30T11:47:35Z

Neos.Neos/Classes/FrontendRouting/NodeAddress.php

    public function __toString(): string
    {
        return sprintf(
-            'NodeAddress[contentStream=%s, dimensionSpacePoint=%s, nodeAggregateId=%s, workspaceName=%s]',
-            $this->contentStreamId->value,
+            'NodeAddress[dimensionSpacePoint=%s, nodeAggregateId=%s, workspaceName=%s]',


we probably dont even need the __toString anymore at this time ... but i kept it ... it will all go away. Its legacy.

mhsdesign · 2024-06-30T11:48:35Z

Neos.Neos/Classes/Service/Controller/DataSourceController.php

     * @throws NeosException
     */
-    public function indexAction($dataSourceIdentifier, Node $node = null): void
+    public function indexAction($dataSourceIdentifier, string $node = null): void


as per my main description:

where previously Node $node was used to trigger property mapping (DataSourceController and WorkspaceController) we now pass the node as string and use the legacy node address factory to handle the legacy format manually. A followup will adjust everything to work with the new node address.

kitsunet

There is one little code suggestion from bwaidelich I would think should be merged, the rest seems fine to me.

mhsdesign · 2024-07-01T11:44:38Z

Okay in that case id merge it ;)

github-actions bot added the 9.0 label May 18, 2024

mhsdesign force-pushed the task/nodeAddressFlowPropertyMapper branch 2 times, most recently from 2053ce7 to 106cde2 Compare June 16, 2024 09:03

mhsdesign added 4 commits June 16, 2024 11:25

TASK: Adjust controllers to use legacy node address notation explicit…

97c9445

…ly instead of property mapping

TASK: Adjust LinkingService to not use legacy node address

ba79300

TASK: Adjust node property mappers to use new node address

6d72c8a

TASK: Adjust Neos.Node.serializedNodeAddress to use new node address

931304f

mhsdesign force-pushed the task/nodeAddressFlowPropertyMapper branch from 106cde2 to 931304f Compare June 16, 2024 09:25

mhsdesign changed the title ~~!!! TASK: Provide pure NodeAddress property converter~~ !!! TASK: Refactor Node property mapper to use new NodeAddress Jun 16, 2024

TASK: Remove ContentStreamId from legacy NodeAddress

0f90905

see neos#5043

mhsdesign mentioned this pull request Jun 16, 2024

Remove $node->subgraphIdentity->contentStreamId #5043

Closed

12 tasks

mhsdesign closed this Jun 23, 2024

mhsdesign reopened this Jun 23, 2024

mhsdesign marked this pull request as ready for review June 23, 2024 20:12

mhsdesign requested review from kitsunet, mficzel and grebaldi June 24, 2024 15:56

bwaidelich reviewed Jun 30, 2024

View reviewed changes

mhsdesign commented Jun 30, 2024

View reviewed changes

kitsunet approved these changes Jul 1, 2024

View reviewed changes

mhsdesign merged commit 9b79f68 into neos:9.0 Jul 1, 2024
19 checks passed

mhsdesign mentioned this pull request Oct 16, 2024

9.0 Discussion Node property mapping in controllers #4873

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

!!! TASK: Refactor `Node` property mapper to use new NodeAddress #5068

!!! TASK: Refactor `Node` property mapper to use new NodeAddress #5068

mhsdesign commented May 18, 2024 •

edited

Loading

bwaidelich left a comment

bwaidelich Jun 30, 2024

mhsdesign Jun 30, 2024

bwaidelich Jun 30, 2024

mhsdesign Jun 30, 2024

bwaidelich Jul 1, 2024

bwaidelich Jun 30, 2024

mhsdesign Jun 30, 2024

bwaidelich Jun 30, 2024

mhsdesign Jun 30, 2024

bwaidelich Jun 30, 2024

mhsdesign Jun 30, 2024

bwaidelich Jun 30, 2024

mhsdesign Jun 30, 2024

bwaidelich Jul 1, 2024

bwaidelich Jun 30, 2024

mhsdesign left a comment

mhsdesign Jun 30, 2024

mhsdesign Jun 30, 2024

mhsdesign Jun 30, 2024

mhsdesign Jun 30, 2024

mhsdesign Jun 30, 2024

mhsdesign Jun 30, 2024

kitsunet left a comment

mhsdesign commented Jul 1, 2024

	if (!$stringFormattedNodeAddress) {
	if ($stringFormattedNodeAddress === null) {

!!! TASK: Refactor Node property mapper to use new NodeAddress #5068

!!! TASK: Refactor Node property mapper to use new NodeAddress #5068

Conversation

mhsdesign commented May 18, 2024 • edited Loading

bwaidelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsdesign left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitsunet left a comment

Choose a reason for hiding this comment

mhsdesign commented Jul 1, 2024

!!! TASK: Refactor `Node` property mapper to use new NodeAddress #5068

!!! TASK: Refactor `Node` property mapper to use new NodeAddress #5068

mhsdesign commented May 18, 2024 •

edited

Loading