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

FEATURE: Fusion renderingMode semantic correct inBackend #4396

Closed
mhsdesign opened this issue Jul 10, 2023 · 9 comments · Fixed by #4505
Closed

FEATURE: Fusion renderingMode semantic correct inBackend #4396

mhsdesign opened this issue Jul 10, 2023 · 9 comments · Fixed by #4505
Labels

Comments

@mhsdesign
Copy link
Member

mhsdesign commented Jul 10, 2023

current inBackend state

currently in 8.3 and 9.0-dev we check if a node is in the backend by checking if the workspace is not live.

In Neos 8.3 it looks like node.context.inBackend
and for Neos 9.0-dev of July 2023 we currently use Neos.Node.inBackend(node).
The edit preview modes are at this time not implemented in Neos 9 (see #4086).

public function inBackend(Node $node): bool
{
    $contentRepository = ...;
    $workspace = $contentRepository
        ->getWorkspaceFinder()
        ->findOneByCurrentContentStreamId($node->subgraphIdentity->contentStreamId);
    return !$workspace->workspaceName->isLive();
}

The assumption that nodes in non-live workspaces should render in backend mode is understandable but not fully semantically correct.
It currently happens to be that way that one cannot directly edit the live workspace via Neos Ui or that workspaces cannot be rendered in frontend mode, but it should not be fundamentally impossible.

problems workspace determined check inBackend = !workspace.live

The workspace based rendering logic works well for when editing nodes in the Ui and rendering live nodes only for frontend.

But the downside is that the external preview mode will still be treated as inBackend.

It's not possible to render every workspace for frontend.
Without any content element wrapping and possible backend notes.

Also, the node uris are not correctly rendered see: #2347

The current workaround in 8.3 is to check if the preview is in user-admin or in its base workspace, as the "Show preview" will always show the node in its base workspace (see NodeInfoHelper::createRedirectToNode):

inEdit = ${node.context.workspace.personalWorkspace}

problems of the 8.3 rendering mode attached to the nodes context

Accessing the actual rendering mode in fusion is done via node.context.currentRenderingMode.edit which is implemented here:

public function getCurrentRenderingMode(): UserInterfaceMode
{
return $this->interfaceRenderModeService->findModeByCurrentUser();
}

But this always requires the rendering mode to be set globally and it cannot be changed individually for one rendering.

A separate variable is preferred.

proposed solution with Neos 9

With 9.0 we have a chance to clean this up. Using node.context.inBackend won't work anymore as Neos 9 doesnt have a context, and the check has to be migrated either way.
(currently this is done via rector to Neos.Node.inBackend(node))

The concept of custom Fusion global variables would allow us to introduce node/request independent helpers like renderingMode.isEdit and renderingMode.isPreview.

The introduction of the renderingMode per se will not solve the above outlined problem fully, but we would also have set the renderingMode to not edit if we use the "Show Preview" button.
Followup issue: #4959

Discussions about implementation

Fusion global vs regular Fusion context variable

Introducing Fusion globals to allow a request like global is important so the variable is always available and will not be "lost" if not specified in @cache.mode = 'uncached' segments.

A "regular" Fusion context variable would have to be breaking and would need manual adjustments in every dynamic and uncached cache configuration:

@cache {
   mode = 'uncached'
   context {
     0 = 'node'
     1 = 'renderingMode'
   }
}

We also discussed to introduce a default cache context like Neos.Fusion:GlobalCacheIdentifiers so this is less breaking but discarded this idea at the dresden sprint 23.

Fusion global vs attached to request

Silently attaching the rendering mode to the request or worse having some singleton and eel helper to fetch it will make custom use cases of rendering a page super tricky. We would have to expect a similar workaround like this now reverted change: neos/neos-ui@ce395ca
Passing the rendering mode explicitly from outside will hopefully solve that two tabs can have a distinct rendering mode which is not determined by the session: #965

migration

We discussed to remove our "temporary" 9.0-dev method Neos.Node.inBackend(node) and replace it with renderingMode.isEdit

A simple migration for current 9.0 projects can be done by hand or via a regex replace from Neos.Node.inBackend\([a-zA-Z]*\) to renderingMode.isEdit.

The rector migration neos/rector#22 will take care of migrating node.context.inBackend to renderingMode for Neos 8.3

implementation

@mhsdesign mhsdesign changed the title Rendering semantically correct inBackend Rendering: Semantically correct inBackend Jul 10, 2023
@kitsunet
Copy link
Member

I see two lines of arguing here:

Option 1: Your line of thought that basically replaces it with "edit or preview mode", but in that case I really think we should offer the syntactic sugar of inBackend and do the or internally, given that probalby inBackend is the 90% case.

Option 2: we assume that inBackend mostly means "you are in edit mode" despite a preview mode also being inBackend, because usually you want to make something easier for editors (or more accessible eg. sliders) in that case we could even rector directly ot just a check for edit mode, and then we also need nothing else in terms of syntactic sugar. I feel this is actually the way things should be, but I have no numbers to back that up :D I guess we could ask what people use inBackend for atm.

@mhsdesign
Copy link
Member Author

mhsdesign commented Jul 10, 2023

Im for Option 2, as Neos.Backend.isPreviewMode would be true when you open the page externally:

image

And thus when we use inBackend == Neos.Backend.isEditMode(request) || Neos.Backend.isPreviewMode(request) it would still be true even if one left the Neos Ui.

@mficzel
Copy link
Member

mficzel commented Jul 10, 2023

I also would prefer the strictness of option 2 since we usually mean the editMode. An inBackend method does not add any meaning compared to Neos.Backend.isEditMode(request) || Neos.Backend.isPreviewMode(request).

If we decide to add a convenience method i would prefer to call it Neos.Backend.isEditOrPreviewMode(request)

@skurfuerst
Copy link
Member

skurfuerst commented Jul 11, 2023

I also prefer option 2 (and support the proposal in general) 👍 Thanks for taking care!

@kitsunet
Copy link
Member

kitsunet commented Jul 11, 2023

... as Neos.Backend.isPreviewMode #4067 (comment) when you open the page externally:

this is another implementaiton problem we have right now, IMHO it should not and the PR from martin should fix that.

@skurfuerst
Copy link
Member

@mhsdesign Can we get this to a done state? Seems we agree on a solution here :) (or did I overlook the solution already?) All the best!

@mhsdesign
Copy link
Member Author

#4505 will actually only solve half of it.
We will also remove Neos.Node.inBackend(node) for beta 2!

We have several places in php where we check against the live content stream (which as written above should NOT say anything about if the node is in backend or not!)

https://github.com/neos/neos-development-collection/blob/4cf807a991ab81a9678ef5b807d7049ccb08c7ee/Neos.Neos/Classes/FrontendRouting/NodeUriBuilder.php#L67C1-L67C1

@ahaeslich ahaeslich moved this from In Progress 🚧 to Todo in Neos 9.0 Release Board Nov 10, 2023
@mhsdesign mhsdesign moved this from Todo to Low Priority in Neos 9.0 Release Board Nov 10, 2023
@mhsdesign mhsdesign moved this from Low Priority to Todo in Neos 9.0 Release Board Nov 10, 2023
@mhsdesign
Copy link
Member Author

mhsdesign commented Nov 10, 2023

first goal would be to find out all paces to touch and determine breakiness and maybe migrate the breaking parts for 9.0 already ;)

Edit was quite doable see #4733 and #4874 ;)

mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this issue Nov 12, 2023
@mhsdesign mhsdesign changed the title Rendering: Semantically correct inBackend Rendering: Semantic correct inBackend Nov 12, 2023
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this issue Nov 17, 2023
mhsdesign added a commit that referenced this issue Nov 17, 2023
…IsEditChecksInsteadOfNodeIsLive

TASK: #4396 prefer renderingMode::isEdit over node::isLive
@mhsdesign mhsdesign moved this from Todo to In Progress 🚧 in Neos 9.0 Release Board Feb 8, 2024
@mhsdesign mhsdesign changed the title Rendering: Semantic correct inBackend FEATURE: Fusion renderingMode semantic correct inBackend Mar 23, 2024
@mhsdesign
Copy link
Member Author

I created a followup issue for all the leftover problems that are now fixable with the newly introduced concept of the renderingMode

#4959

I updated the issues description and consider this part closed.

@github-project-automation github-project-automation bot moved this from In Progress 🚧 to Done ✅ in Neos 9.0 Release Board Mar 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants