-
-
Notifications
You must be signed in to change notification settings - Fork 223
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
Conversation
a044ee5
to
f27296d
Compare
753351a
to
febb202
Compare
@mficzel @mhsdesign you let me know when we can review / merge this? :) all the best :) |
@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 |
@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. |
Noticed the related PRs: |
f9c134e
to
20a8bd0
Compare
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
20a8bd0
to
323e8ad
Compare
… 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
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.
71b63f3
to
5098a1e
Compare
@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:
If we agree on those changes in general we should adjust the UI as follows: neos/neos-ui#3411
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.
|
5098a1e
to
c4fa795
Compare
isEditingMode: true | ||
isPreviewMode: false | ||
isPreviewMode: true |
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.
Your thesis:
defaultEditPreviewMode should have both to true
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.
One could rephrase this as the ui has to switch to edit / preview depending on what the defaultMode is.
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 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.
…s done in preview
c7b22ec
to
fbd61dc
Compare
I applied the suggested changes. |
Co-authored-by: Bastian Waidelich <[email protected]> Co-authored-by: Marc Henry Schultz <[email protected]>
fbd61dc
to
ff71109
Compare
we should deprecate
|
This PR should cover the content collection, right?: neos-development-collection/Neos.Neos/Resources/Private/Fusion/Prototypes/ContentCollection.fusion Line 30 in f4ed836
(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.
@grebaldi @mhsdesign good catch. Removed |
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 :) |
@grebaldi zu needs this to get along with removed |
@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 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. |
Im just wondering about caching. When retrieving the pseudo But when taking the
edit: it seems you have done that in the global cache ids already:
|
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.
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 bothedit
- ANDpreview
-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
- Within this PR this is currently the default for
- 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
- The alternative would be to put the current
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.
) { | ||
} |
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.
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).
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.
dont say enum ;)
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 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); |
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.
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); |
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'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...
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.
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 ;)
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.
@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. ;)
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.
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)) { |
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'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
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.
$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); |
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 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;
}
$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); |
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.
Likewise
Am AFK for the next three weeks. Feel free to add all the changes that are necessary to this pr. |
Close in favor of #4505 please let the branch live for a bit in case it is needed as comparison. |
The main change is that
edit
andpreview
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.!!! 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:
Neos.Node.inBackend(node)
is removed and is replaced withNeos.Backend.isEditMode(request) || Neos.Backend.isPreviewMode(request)
this is non breaky as this method was never in an official releaseResolves: #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 thepreviewAction
torawContent
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.document.querySelector('iframe').src = "https://neos-development-distribution-9-0.ddev.site/neos/preview?node=user-admin__eyJsYW5ndWFnZSI6ImVuX1VTIn0%3D__995c9174-ddd6-4d5c-cfc0-1ffc82184677"
document.querySelector('iframe').src = "https://neos-development-distribution-9-0.ddev.site/neos/edit?node=user-admin__eyJsYW5ndWFnZSI6ImVuX1VTIn0%3D__995c9174-ddd6-4d5c-cfc0-1ffc82184677"
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
FEATURE|TASK|BUGFIX
!!!
and have upgrade-instructionsTodos
Neos.Backend.isEditMode
automated migration Migratenode.context.inBackend
toNeos.Backend.isEditMode(request)
rector#21Neos.Backend.isEditMode
migration in the UI TASK: Replace usage ofNeos.Node.inBackend(node)
neos-ui#3571