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: Edit preview mode support for Neos 9 #4067

Closed
wants to merge 8 commits into from

Conversation

mficzel
Copy link
Member

@mficzel mficzel commented Mar 6, 2023

The main change is that edit and preview action of the node controller are now separated with distinct constraints. This now allows for an mode to be edit and preview mode at the same time which is allowed by the configuration anyways.

  • NodeController: has separate edit and preview actions that take the previewMode as argument
  • NodeUriBuilder and LinkingService: will use preview / edit action once the main request is from the same action
  • Neos.BackendHelper: has additional methods isEditMode, isPreviewMode and renderingModeCacheIdentifier

!!! The Neos.UI needs a change to provide the editPreviewMode parameter and call the edit or the preview action !!!

!!! This pr changes some behaviors slightly:

  1. The distinction between edit and preview mode is now made based on the called action of the nodeController
  2. The eel method Neos.Node.inBackend(node) is removed and is replaced with Neos.Backend.isEditMode(request) || Neos.Backend.isPreviewMode(request) this is non breaky as this method was never in an official release

Resolves: #4086
Resolves partly: #4396

Upgrade instructions

If fusion code relies on the edit preview mode beeing available via node.context.currentRenderingMode it has to be adjusted. Usually this means adjusting from code like ${node.context.currentRenderingMode.edit} to ${Neos.Backend.isEditMode(request)}.

Review instructions

Until the ui is adjusted this will always the preview as this was the existing method of the node controller. By setting the default value of the editPreviewMode on the previewAction to rawContent you can verify the rendering via different fusion pathes.

For now you can use the console to call the edit preview modes in your browser. You will have to replace the node with a node-address for your setup. You can then verify that the edit / preview mode is persisted during navigation after the initial call.

  1. Open document in default preview mode document.querySelector('iframe').src = "https://neos-development-distribution-9-0.ddev.site/neos/preview?node=user-admin__eyJsYW5ndWFnZSI6ImVuX1VTIn0%3D__995c9174-ddd6-4d5c-cfc0-1ffc82184677"
  2. Open document default edit mode document.querySelector('iframe').src = "https://neos-development-distribution-9-0.ddev.site/neos/edit?node=user-admin__eyJsYW5ndWFnZSI6ImVuX1VTIn0%3D__995c9174-ddd6-4d5c-cfc0-1ffc82184677"
  3. Open document in rawContent mode (edit)
    document.querySelector('iframe').src = "https://neos-development-distribution-9-0.ddev.site/neos/edit?node=user-admin__eyJsYW5ndWFnZSI6ImVuX1VTIn0%3D__995c9174-ddd6-4d5c-cfc0-1ffc82184677&editPreviewMode=rawContent"

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

Todos

@ahaeslich
Copy link
Member

@mficzel This will probably create a merge conflict with #4074.

@mficzel mficzel force-pushed the 90/editPreviewModeSupport branch 2 times, most recently from 753351a to febb202 Compare March 7, 2023 14:42
@mficzel mficzel requested a review from skurfuerst March 7, 2023 16:34
@skurfuerst
Copy link
Member

@mficzel @mhsdesign you let me know when we can review / merge this? :) all the best :)

@skurfuerst skurfuerst added this to the 9.0 (ES CR) milestone Mar 17, 2023
@mficzel
Copy link
Member Author

mficzel commented Apr 4, 2023

@akurfuerst a little rebase aside this should be ready to go. Only problem is the ui change that is needed here neos/neos-ui#3411

@ahaeslich do you remember why we did not add a description

@mficzel
Copy link
Member Author

mficzel commented Apr 5, 2023

@ahaeslich I added a description from memory. Did i miss something important.

I also merged the 9.0 branch into the state here. Looking OK so far and it seems to work fine for now.

@mficzel
Copy link
Member Author

mficzel commented May 3, 2023

@mficzel mficzel force-pushed the 90/editPreviewModeSupport branch 2 times, most recently from f9c134e to 20a8bd0 Compare May 11, 2023 07:24
The main change is that edit and preview action are now separated with distinct constraints.

- NodeController: has separate edit and preview actions that take the previewMode as argument
- NodeUriBuilder and LinkingService: will use preview / edit action once the main request is from the same action
- Neos.BackendHelper: has additional methods isEditMode, isPreviewMode and editPreviewModeCacheIdentifier
@mficzel mficzel force-pushed the 90/editPreviewModeSupport branch from 20a8bd0 to 323e8ad Compare May 11, 2023 07:30
… the fusion path is set by the nodeController

The main difference is that this leads to edit preview modes now rendering shortcut nodes using the configured fusionPath
@bwaidelich
Copy link
Member

Any updates on this one?

…ered as before

This is necessary as the fusionPath is now set on the view directly if the edit preview mode defined one.
@mficzel mficzel force-pushed the 90/editPreviewModeSupport branch from 71b63f3 to 5098a1e Compare May 24, 2023 16:21
@mficzel
Copy link
Member Author

mficzel commented May 24, 2023

@bwaidelich i consider this ready backend-wise. However the ui will not work until it is adjusted to this change as it will default to preview mode if unadjusted.

The really important changes are for now:

  1. The edit and preview are now distinct actions of the NodeController now -> this idea taken from the pr WIP: Rename previewAction to editAction #4217 from @daniellienert
  2. The current fusion path (if specified) is set on the view directly instead of switching in the rootCase.
  3. The backend helper has new methods isEditMode, isPreviewMode and renderingModeCacheIdentifier that each take the request as argument.
  4. The LinkingService and NodeUriBuilder will also check the current request to create links with in the current editPreviewMode.

If we agree on those changes in general we should adjust the UI as follows: neos/neos-ui#3411

  1. The ui has to links to current document via the edit or the preview action (since before there was only the preview action one currently ends up in preview mode)
  2. The ui edit preview mode selector has to adjust to the possibility of modes beeing edit- and previewMode at the same time. (the Settings allowed that anyways)
  3. Investigate why new content in rawContent mode leads to reloading the document in inPlace mode

For now you can use the console to call the edit preview modes in your browser. You will have to replace the node with a node-address for your setup. You can then verify that the edit / preview mode is persisted during navigation after the initial call.

  1. Open document in default preview mode document.querySelector('iframe').src = "https://neos-development-distribution-9-0.ddev.site/neos/preview?node=user-admin__eyJsYW5ndWFnZSI6ImVuX1VTIn0%3D__995c9174-ddd6-4d5c-cfc0-1ffc82184677"
  2. Open document default edit mode document.querySelector('iframe').src = "https://neos-development-distribution-9-0.ddev.site/neos/edit?node=user-admin__eyJsYW5ndWFnZSI6ImVuX1VTIn0%3D__995c9174-ddd6-4d5c-cfc0-1ffc82184677"
  3. Open document in rawContent mode (edit)
    document.querySelector('iframe').src = "https://neos-development-distribution-9-0.ddev.site/neos/edit?node=user-admin__eyJsYW5ndWFnZSI6ImVuX1VTIn0%3D__995c9174-ddd6-4d5c-cfc0-1ffc82184677&editPreviewMode=rawContent"

@mficzel mficzel force-pushed the 90/editPreviewModeSupport branch from 5098a1e to c4fa795 Compare May 24, 2023 16:34
@mficzel mficzel requested review from bwaidelich and ahaeslich May 24, 2023 16:44
Comment on lines 280 to +281
isEditingMode: true
isPreviewMode: false
isPreviewMode: true
Copy link
Member

Choose a reason for hiding this comment

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

Your thesis:

defaultEditPreviewMode should have both to true

Copy link
Member Author

Choose a reason for hiding this comment

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

One could rephrase this as the ui has to switch to edit / preview depending on what the defaultMode is.

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 agree this is not required anymore once the ui knows about the distinction of edit vs preview action. Currently this ensures that at least the preview mode is shown instead of an error.

Neos.Neos/Classes/Fusion/Helper/BackendHelper.php Outdated Show resolved Hide resolved
Neos.Neos/Classes/Fusion/Helper/BackendHelper.php Outdated Show resolved Hide resolved
Neos.Neos/Classes/Fusion/Helper/BackendHelper.php Outdated Show resolved Hide resolved
Neos.Neos/Classes/Fusion/Helper/BackendHelper.php Outdated Show resolved Hide resolved
Neos.Neos/Classes/Fusion/Helper/BackendHelper.php Outdated Show resolved Hide resolved
Neos.Neos/Classes/Fusion/Helper/BackendHelper.php Outdated Show resolved Hide resolved
Neos.Neos/Classes/Fusion/Helper/BackendHelper.php Outdated Show resolved Hide resolved
Neos.Neos/Classes/Service/LinkingService.php Outdated Show resolved Hide resolved
@mficzel mficzel force-pushed the 90/editPreviewModeSupport branch from c7b22ec to fbd61dc Compare June 19, 2023 12:33
@mficzel
Copy link
Member Author

mficzel commented Jun 19, 2023

I applied the suggested changes.

Co-authored-by: Bastian Waidelich <[email protected]>
Co-authored-by: Marc Henry Schultz <[email protected]>
@mficzel mficzel force-pushed the 90/editPreviewModeSupport branch from fbd61dc to ff71109 Compare June 19, 2023 12:49
@mhsdesign
Copy link
Member

we should deprecate Node.inBackend as the concept from using the request is the correct one.

node.workspace.isLive !== (!Node.inBackend)

@grebaldi
Copy link
Contributor

grebaldi commented Jul 8, 2023

This PR should cover the content collection, right?:

[email protected] = ${Neos.Node.inBackend(documentNode) && node.context.currentRenderingMode.edit}

(see: neos/rector#23)

…de()` && `Neos.Backend.isPreviewMode()`

`Neos.Node.inBackend` was intended to be a replacement for `node.context.inBackend` but the detection wether a node is rendered in the backend
is now done in the Backend helper by checking wich action has actually been called.
@mficzel
Copy link
Member Author

mficzel commented Jul 9, 2023

@grebaldi @mhsdesign good catch. Removed Neos.Node.inBackend(node) and replaced the usage with the Neos.Backend.isEditMode(request) || Neos.Backend.isPreviewMode(request)

@grebaldi
Copy link
Contributor

grebaldi commented Jul 9, 2023

Nice one @mficzel, thx!

I'll patch this into the test distribution over at neos/neos-ui#3569 and get back to you with a full review :)

@mficzel
Copy link
Member Author

mficzel commented Jul 9, 2023

@grebaldi zu needs this to get along with removed Neos.Node.inBackend(node) neos/neos-ui#3571

@mficzel
Copy link
Member Author

mficzel commented Jul 9, 2023

@grebaldi any support with adjusting the ui-side to the changes in the backend node entry points would be very much apreciated. Pretty sure the fact that this can be hardly tested is pert of the reason why there are no reviews yet.

@mhsdesign
Copy link
Member

mhsdesign commented Jul 9, 2023

should Neos.Backend.isPreviewMode be true when you open the page externally?

image

(havent checked the behavior of this pr - just a reminder for me)

@mficzel
Copy link
Member Author

mficzel commented Jul 9, 2023

should Neos.Backend.isPreviewMode be true when you open the page externally?

@mhsdesign Pretty sure yes ... that was basically what the pr #4217 was about that first tried to introduce separate edit and preview actions. Since previously the user session decided wether an edit or a preview mode was active it was hard to be in edit mode but render a preview using the show preview link.

@mhsdesign
Copy link
Member

Im just wondering about caching.

When retrieving the pseudo inBackend information from the node (which actually only says if the node is not in the live workspace) caching works out of the box right?, because its a different node when inBackend changes.

But when taking the request we need to adjust the caching configuration so that this works?

renderer = ${Neos.Backend.isEditMode(request) ? 'edit' : (Neos.Backend.isPreviewMode(request) ? 'preview' : 'live')}

edit:

it seems you have done that in the global cache ids already:

renderingMode = ${Neos.Backend.renderingModeCacheIdentifier(request)}

Copy link
Contributor

@grebaldi grebaldi left a comment

Choose a reason for hiding this comment

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

Hi @mficzel,

thanks for taking care of this :) I was able to successfully test this change in the scope of neos/neos-ui#3569.

Therein I stumbled upon some conceptual questions that we've already talked about briefly. The major questions are imho:

  • Should it be possible for an EditPreviewMode to be both edit- AND preview-mode
    • Within this PR this is currently the default for inPlace
    • As we've spoken about this, my impression was that we generally agree that this shouldn't be possible, but that we need a solution for the UI to behave correctly, because of the change of endpoints
  • Should the current EditPreviewMode be really dependent on the current action request?
    • The alternative would be to put the current EditPreviewMode object into the global fusion context
    • There are pros and cons for both solutions

All other comments I left are just some opinionated suggestions :)

So, in conclusion, we need to clear up those questions and then this should be good to go imho. But we need to coordinate closely, because once this is merged, the UI will cease to work, unless neos/neos-ui#3569 is merged as well.

Comment on lines +25 to +26
) {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We were talking about this model. It should contain a check, so that $isEditMode and $isPreviewMode cannot both be true at once (and that one of them must be true).

Copy link
Member

Choose a reason for hiding this comment

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

dont say enum ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't :D

But yeah, that would probably be better.

if (array_key_exists($name, $this->editPreviewModeConfigurations)) {
return EditPreviewMode::fromNameAndConfiguration($name, $this->editPreviewModeConfigurations[$name]);
}
throw new InvalidEditPreviewModeException(sprintf('"%s" is not a valid editPreviewMode', $name), 1683790077);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
throw new InvalidEditPreviewModeException(sprintf('"%s" is not a valid editPreviewMode', $name), 1683790077);
throw new InvalidEditPreviewModeException(sprintf('An editPreviewMode with the name "%s" does not exist', $name), 1683790077);

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm very unsure whether it's wise to check for the current edit/preview mode via this EEL-Helper and by analyzing the request. As a consequence of this strategy, this is what needs to be done for out-of-band rendering in the UI:

https://github.com/neos/neos-ui/blob/0a858bb1cd5a31e4143f9b06144b0704c3acaad7/Classes/Domain/Model/Feedback/Operations/ReloadContentOutOfBand.php#L136-L146
(These lines show the creation of a fake controller context, so this EEL-Helper is convinced to be executed in a different request than it actually is)

Intuitively, I'd say it'd be better to just throw the current EditPreviewMode object into the global fusion context, so it can just be used like

[email protected] = ${editPreviewMode.isEditMode}

On the other hand, that fake controller context may not be the worst idea after all, because usually integrators wouldn't expect (and shouldn't notice) their code to go through such different execution paths.

However, semantically it seems very arbitrary to me to bind the current edit preview mode to a specific controller action...

Copy link
Member

Choose a reason for hiding this comment

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

Yes i rather have this additional context too. I remember that we discussed this at the Dresden sprint, but the discussion came up that in fusion we can't currently add new context variables because users would have to account for them in caching.

There is a way though: We need to pass the context variable behind fusions back like request so its always available in eel.

I will prepare a draft pr to show you what i mean ;)

Copy link
Member

Choose a reason for hiding this comment

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

@grebaldi i prepared a concept over here: #4425

this way we open the api of the runtime to allow other variables like request who are always present (in the instance of the runtime) and thus not affected by caching.

This would allow us to add a global editPreviewMode object with no dependency on the request. ;)

Copy link
Member

Choose a reason for hiding this comment

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

new Runtime(
  defaultContextVariables: [
      'request' => $request,
      'Neos' => [
        'UiMode' => new UiModeHelper($isBackend)
      ]
  ]
)

@@ -367,7 +370,15 @@ public function createNodeUri(
$request = $controllerContext->getRequest()->getMainRequest();
$uriBuilder = clone $controllerContext->getUriBuilder();
$uriBuilder->setRequest($request);
$action = $workspace && $workspace->isPublicWorkspace() && !$hiddenState->isHidden ? 'show' : 'preview';

if (BackendHelper::isEditMode($request) || BackendHelper::isPreviewMode($request)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to move this closer to the domain. Looks like a job for the EditPreviewModeRepository:

final class EditPreviewModeRepository
{
    public function findOneByActionRequest(ActionRequest $request): ?EditPreviewMode;
}

So that this check translates to

Suggested change
if (BackendHelper::isEditMode($request) || BackendHelper::isPreviewMode($request)) {
$editPreviewMode = $this->editPreviewModeRepository->findOneByActionRequest($request);
if ($editPreviewMode) {

Depending on how we move forward with the BackendHelper itself, it should of course use the same method to retrieve the proper EditPreviewMode from the request.

Comment on lines +134 to +136
$editPreviewModeObject = $editPreviewMode ? $this->editPreviewModeRepository->findByName($editPreviewMode) : $this->editPreviewModeRepository->findDefault();
if ($editPreviewModeObject->isPreviewMode === false) {
throw new InvalidEditPreviewModeException(sprintf('"%s" is not a preview mode', $editPreviewMode), 1683127314);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also prefer to move this logic closer to the domain, by letting the EditPreviewModeRepository provide more specific retrieval methods:

final class EditPreviewModeRepository
{
    /** @throws NotAnEditModeException */
    public function findOneEditModeByName(string $name): ?EditPreviewMode;

    /** @throws DefaultEditModeNotFoundException */
    public function findDefaultEditMode(): EditPreviewMode;

    /** @throws NotAPreviewModeException */
    public function findOnePreviewModeByName(string $name): ?EditPreviewMode;

    /** @throws DefaultPreviewModeNotFoundException */
    public function findDefaultPreviewMode(): EditPreviewMode;
}

Comment on lines +156 to +158
$editPreviewModeObject = $editPreviewMode ? $this->editPreviewModeRepository->findByName($editPreviewMode) : $this->editPreviewModeRepository->findDefault();
if ($editPreviewModeObject->isEditMode === false) {
throw new InvalidEditPreviewModeException(sprintf('"%s" is not an edit mode', $editPreviewModeObject->name), 1683127295);
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise

@mficzel
Copy link
Member Author

mficzel commented Jul 27, 2023

Am AFK for the next three weeks. Feel free to add all the changes that are necessary to this pr.

@mficzel
Copy link
Member Author

mficzel commented Sep 12, 2023

Close in favor of #4505 please let the branch live for a bit in case it is needed as comparison.

@mficzel mficzel closed this Sep 12, 2023
@mficzel mficzel deleted the 90/editPreviewModeSupport branch November 3, 2023 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants