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

BUGFIX: Fix hidden state evaluation #3867

Conversation

mhsdesign
Copy link
Member

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

What I did

How I did it

How to verify it

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
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Oct 14, 2024
…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
neos-bot pushed a commit to neos/neos that referenced this pull request Oct 14, 2024
…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
@bwaidelich bwaidelich changed the title TASK: Fix hidden state evaluation BUGFIX: Fix hidden state evaluation Oct 15, 2024
@bwaidelich bwaidelich added Bug Label to mark the change as bugfix and removed Task labels Oct 15, 2024
Copy link
Member

@bwaidelich bwaidelich left a 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(),
Copy link
Member

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?

Copy link
Member Author

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;)

Copy link
Member Author

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]>
@mhsdesign mhsdesign merged commit fe52b16 into neos:9.0 Oct 16, 2024
4 of 5 checks passed
@mhsdesign mhsdesign deleted the bugfix/fix-redirectToAction-for-disabled-nodes-in-base-live-workspace branch October 16, 2024 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
9.0 Bug Label to mark the change as bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants