-
-
Notifications
You must be signed in to change notification settings - Fork 135
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
BUGFIX: Fix hidden state evaluation #3867
BUGFIX: Fix hidden state evaluation #3867
Conversation
The Neos 8.3 LinkingService contained this logic ``` $action = $node->getContext()->getWorkspace()->isPublicWorkspace() && !$node->isHidden() ? 'show' : 'preview'; ``` ensuring that disabled nodes can still be previewed. In Neos 9 a no route matched error will be thrown. To restore the old behaviour we evaluate the hidden state
…uri builder behaviour This was accidentally wrongly migrated. Regarding > Why do we evaluate the hidden state here? We dont do it in the new uri builder. We dont evaluate the hidden state in the new uri builder because we only have the node address at hand. The 8.3 logic ``` $action = $node->getContext()->getWorkspace()->isPublicWorkspace() && !$node->isHidden() ? 'show' : 'preview'; ``` Ensured if youre routing to a hidden live node then a preview uri will be built. This is mostly unlikely going to happen and building the preview uri in this case is possibly not even the desired behaviour One exception, in the Neos.Ui requires this behaviour: neos/neos-ui#3867
…uri builder behaviour This was accidentally wrongly migrated. Regarding > Why do we evaluate the hidden state here? We dont do it in the new uri builder. We dont evaluate the hidden state in the new uri builder because we only have the node address at hand. The 8.3 logic ``` $action = $node->getContext()->getWorkspace()->isPublicWorkspace() && !$node->isHidden() ? 'show' : 'preview'; ``` Ensured if youre routing to a hidden live node then a preview uri will be built. This is mostly unlikely going to happen and building the preview uri in this case is possibly not even the desired behaviour One exception, in the Neos.Ui requires this behaviour: neos/neos-ui#3867
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.
Thanks for fixing!
The amount of domain logic in the controller makes me a little unsure, but in general it makes sense to me..
Will we be able to cover this with a test to prevent this from breaking again in the future?
|
||
$nodeAddressInBaseWorkspace = NodeAddress::create( | ||
$nodeAddress->contentRepositoryId, | ||
$workspace->baseWorkspaceName ?? WorkspaceName::forLive(), |
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.
If the selected workspace has no base workspace – shouldn't that be an error?
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.
i this is like one of these if ($siteNode) {}
calls that are just there to prevent an error than to run into a fatal error.
If some logic is changed that we for example work on the live workspace directly we should also still be able to preview it ... but i think youre right ... for that case $workspace->baseWorkspaceName ?? $workspace->workspaceName should be enough;)
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.
Adjusted:)
…ction-for-disabled-nodes-in-base-live-workspace
remove logic of falling back to live workspace, but use the original workspace then instead.
Co-authored-by: Bastian Waidelich <[email protected]>
The Neos 8.3 LinkingService contained this logic
ensuring that disabled nodes can still be previewed. In Neos 9 a no route matched error will be thrown.
To restore the old behaviour we evaluate the hidden state
What I did
How I did it
How to verify it