FEATURE: Highlevel Workspace API

[nezaniel]

EDIT: Most parts of the api have been overhauled again with #5146

This provices a high level workspace model to be used e.g. by the Neos UI.

(see neos/neos-ui#3752)

Its purpose is to explicitly cover Neos' intentions of publishing, being publishing / discarding whole documents or sites. This is implemented as explicitly named methods that can be used by the UI. Internally it relies on the change finder projection, which is properly filled with above UI change.

[bwaidelich]

Really good idea to coordinate these tasks in a central place!
Just did a first pass and added some nitpicky comments. Apart from those I have a couple of questions/suggestions:

• I struggle a bit with the naming of the commands, e.g. DiscardDocument does not really discard any document. Should we suffix those with "*Changes", e.g. DiscardDocumentChanges?
• Out of scope but looking at the code I was thinking: Shouldn't most projections work with workspace names instead of cs ids eventually? E.g. $changeFinder->findByContentStreamId($contentStreamId); should become $changeFinder->findByWorkspaceName($workspaceName); at some point. This would mean some more workspace<->csid tables but the logic is pretty simple and we would not have to rely on the WorkspaceFinder everywhere (i.e. the workspace projection would be used only to manage workspaces really)

[grebaldi]

I noticed a thing:

The way we are distinguishing documents and sites leads to unintended beavior when our document happens to be the site. Basically, when I do publishDocument on the site node, the behavior is indistinguishable from publishSite. We will need some guard around publishDocument to ensure that only content nodes will be published.
see: #4943 (comment)

EDIT:

Regarding the naming of the commands (as raised by @bwaidelich), I'd opt for:

• PublishChangesInSite
• DiscardChangesInSite
• PublishChangesInDocument
• DiscardChangesInDocument

...to clarify that it's not only about changes to the respective node itself.

[kitsunet]

@grebaldi shouldn't the check in isChangePublishableWithinAncestorScope ultimately limit it to anything not of the defined type and that would make the difference between document and site?

[grebaldi]

@kitsunet:

@grebaldi shouldn't the check in isChangePublishableWithinAncestorScope ultimately limit it to anything not of the defined type and that would make the difference between document and site?

You are correct, of course. My assessment was wrong.

I could swear I've seen this behavior during the UI implementation, so I was a bit too quick on deducing this to be the cause 😅. I just tested this again, and it works correctly. Don't mind my observation from above.

[mhsdesign]

Tested successfully locally against the Neos Ui e2e and found some additional adjustments / anti footgun adjustments: #4974

I didnt want to push these commits directly into your branch, but feel free to merge my review pr into this one @nezaniel ;)

[nezaniel]

get* implies a no-op, which potential creation is not. Either we switch to getOrCreate or something neutral like provide*

[mhsdesign]

True that. Was inspired by the ContentRepositoryRegistry where its get but actually also rather a getOrBuild... maybe retrieveForWorkspaceName is the best naming?

[nezaniel]

Mmh, not sure, retrieve sounds like it is already there and just has to be brought.

[nezaniel]

I think provideFor* makes the most sense, especially later for provideForCurrentUser. Also, that's what providers do, just like Finders find* stuff and Factories build* stuff.

[bwaidelich]

+1 by reading, loving it!