-
-
Notifications
You must be signed in to change notification settings - Fork 225
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
TASK: Adjust document uri projection to soft removals #5485
TASK: Adjust document uri projection to soft removals #5485
Conversation
…led twice With neos#5357 this CANNOT happen, this test is just a leftover and isNodeExplicitlyDisabled is not a necessity anymore.
@@ -80,6 +81,7 @@ public function getByIdAndDimensionSpacePointHash( | |||
if ($this->cacheEnabled && isset($this->getByIdAndDimensionSpacePointHashCache[$cacheKey])) { | |||
return $this->getByIdAndDimensionSpacePointHashCache[$cacheKey]; | |||
} | |||
// todo AND removed = 0 -> we should not allow building uris to removed nodes! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has to be discussed!
Currently matching and resolving is not behaving the same for disabled nodes:
As shown by this test:
Then No node should match URL "/david-nodenborough/earl-document/leaf"
And The node "leaf-mc-node" in content stream "cs-identifier" and dimension "{}" should resolve to URL "/david-nodenborough/earl-document/leaf"
so matching is expected to work always, but i think this is wrong! And i dont want to introduce this for soft removed nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be utterly broken if we introduce it for soft deleted nodes. it must free the url.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh look:
Note: This will not exclude disabled nodes in order to allow the calling side to make a distinction (e.g. in order to display a custom error)
This is from the docblock at \Neos\Neos\FrontendRouting\Projection\DocumentUriPathFinder::getByIdAndDimensionSpacePointHash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which for example the shortcut resolver uses to throw a custom error about the target being disabled.
I guess we need split behavior where disabled does resolve and removed doesnt'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or we just dont do that for now as this introduces just additional complexity. There are a lot of cases where disabled nodes cannot be worked with like when using back references or references but the nodes are hidden and thus wont be displayed in fusion live rendering. My point is, its fairly obvious that linking should not work and editors who want to find out why not will not have access to the stack trace and have to find out either way by looking at the tree, and why make it complex just to make it easy for developers.
I think we should have a clean api to the outside here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay found it the resolving (not matching!) of disabled nodes was done via #4363 to make the redirect handler work.
So we sadly cannot remove the complexity to return disabled nodes and have to split decisions for soft removed nodes.
Contraire resolving disabled nodes: neos#4363 We forbid that for removed nodes.
…PointHash` filters nodes by disabled and removed state they must not be matched against a live uri!
…d uri projection tests This is a leftover from the workspace aware node adjustments
The method `getByIdAndDimensionSpacePointHash` will for example now return soft removed `DocumentNodeInfo`'s to make this a stable api this needs to get an overhaul. Neos takes already care to handle all tasks like shortcut resolving or uri matching. In case certain things need to be made we should provide them as a service to be able to swap out the underlying implementation and to not expose things like `DocumentNodeInfos`.
…ri-projection-to-soft-removals
…ri-projection-to-soft-removals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you
Upgrade instructions
Review instructions
Checklist
FEATURE|TASK|BUGFIX
!!!
and have upgrade-instructions