Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate/Fix/Rethink NodeIdentityConverterAspect for 9.0 #5069

Open
mhsdesign opened this issue May 19, 2024 · 5 comments
Open

Migrate/Fix/Rethink NodeIdentityConverterAspect for 9.0 #5069

mhsdesign opened this issue May 19, 2024 · 5 comments
Labels

Comments

@mhsdesign
Copy link
Member

mhsdesign commented May 19, 2024

subissue of #3216

I dont understand exactly what the NodeIdentityConverterAspect thing does or what it should do in 9.0, but as we dont implement any __contextNodePath logic in 9.0's EventSourcedFrontendNodeRoutePartHandler and seeing that the tests pass after its removal makes me think it has become obsolete.

If there is something i dont know, please let me know otherwise i think it can be removed?

PersistenceManager::convertObjectToIdentityArray was introduced 2011 with:

NodeIdentityConverterAspect introduced 2012 with:

NodeIdentityConverterAspect was adjusted to return array now as the return type was enforced now:

whyyyy does Route::resolves even call convertObjectsToIdentityArrays?

... And why with raw node objects?

This can easily happen in case you build uris using the uri builder or from fluid/fusion:

$uriBuilder->uriFor('redirectTo', ['node' => $nodeInBaseWorkspace], 'Backend', 'Neos.Neos.Ui');

In Neos 8.3 the Node - not being a doctrine entity (NodeData is the entity) - is unknown to the persistence manager:

Tried to convert an object of type "Node" to an identity array, but it is unknown to the Persistence Manager.

Thus we AOP the method and serialise it into an array with __contextNodePath. The old Node property mapper in Neos 8.3 would deserialise it here again.

The original intention or the conversion of doctrine entities seems to be the following:

Request arguments are serialized in Fluid forms in order to generate the
hidden referrer fields that are required to "replay" the last request in
case of an validation error. If a request argument is a complex type,
the value of the __referrer[arguments] hidden field gets huge which
poses 3 major issues:

  1. you'll have to transfer a lot of unnecessary data
  2. the URI for redirects will grow to a point where the browser dies
  3. if an object is not serializable, you get serialization errors
@kitsunet
Copy link
Member

we rely on Neos\Flow\Persistence\AbstractPersistenceManager->convertObjectToIdentityArray in some low level places, think you have a node in a Scope("session") object or so. That method can only convert ORM objects though so the aspect wraps around and converts nodes itself into something that can later be unserialized. A serialized NodeAddress shoudl be fine I think.

@mhsdesign
Copy link
Member Author

and converts nodes itself into something that can later be unserialized. A serialized NodeAddress shoudl be fine I think.

... but we dont support string nodeaddress to node in the long run? #4873

@mhsdesign
Copy link
Member Author

so it was for holding a Node in a session?

like

/**
 * @Flow\Scope("session")
 */
class MySessionThing {
        protected Node $myNode = null;

        public function setNode(Node $item) {
                $this->myNode = $item;
        }

        public function getMyNode() {
                return $this->items;
        }
}

in Neos 9 id say one must use:

/**
 * @Flow\Scope("session")
 */
class MySessionThing {
        protected NodeAddress $myNode = null;

        public function setNode(NodeAddress $item) {
                $this->myNode = $item;
        }

        public function getMyNode() {
                return $this->items;
        }
}

@mhsdesign
Copy link
Member Author

well ... as we relaxed our decision over at #4873 (comment) and it looks like well keep a node property mapper, we should probably keep this hack as well ... and make sure that it works.

Imo for that we have to return the node address as string directly and NOT in an array as value of contextPath!

@mhsdesign
Copy link
Member Author

Christian and me concluded that for routing nodes this is not necessarily required as we currently and will ever serialise the NodeAddress manually. If passed as instance we could confirm that __contextPath will be part of the uri.
Also this logic is needed for forwarding requests and serialising for that all arguments.

@mhsdesign mhsdesign changed the title Remove NodeIdentityConverterAspect Migrate/Rethink NodeIdentityConverterAspect for 9.0 Jun 16, 2024
@mhsdesign mhsdesign changed the title Migrate/Rethink NodeIdentityConverterAspect for 9.0 Migrate/Fix/Rethink NodeIdentityConverterAspect for 9.0 Jun 16, 2024
@mhsdesign mhsdesign added the 9.0 label Jun 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants