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

TASK: Adjust document uri projection to soft removals #5485

Conversation

mhsdesign
Copy link
Member

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

…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!
Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

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

Copy link
Member

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'

Copy link
Member Author

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.

Copy link
Member Author

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`.
Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@kitsunet kitsunet merged commit 581a32e into neos:9.0 Mar 6, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants