FEATURE: Soft removal subtree tag removed

[kitsunet]

Allows for soft deletion and querying the node correctly within the projection while marked deleted.

This change implements soft-removal as discussed #5459 (comment) under "soft removal of nodes in the graph projection". With that we can solve the #4997 of not being able to interpret changes inside deleted hierarchies. Further it should also fixe the problem of having nodes moved out of a deleted tree part which is published: #5364.

[mhsdesign]

okay just for reference: This change implements soft-removal as discussed here under "soft removal of nodes in the graph projection". With that we can solve the bug of not being able to interpret changes inside deleted hierarchies. Further it should also fixe the problem of having nodes moved out of a deleted tree part which is published: #5364.

regarding the implementation: i would say that the change projection should no longer be able to handle the real NodeAggregateWasRemoved due to the sheer complexity (removalAttachmentPoint) and it still being broken after all. So if we introduce the complexity with the deleted tag id say we make that part simpler, to be as stupid as:

Currently 'real' deletions on a workspace are discouraged and not publishable via the neos ui, only a full workspace publish works. Please see to soft delete nodes instead in the neos context.

we could even think about (deprecating) and translating RemoveNodeAggregate in the core to be a TagSubtree(deleted) (like we did for the disable commands) and simultaneously introduce a new kind of removal command like PruneNodeAggregate that reintroduces the previous functionality but with a new invariant that this command is only allowed in the base workspace? That would ensure user workspaces never contain real deleted nodes for now.

regarding the new policy: shouldn't the deleted condition will be hardcoded in the CR Auth Service so in Neos.Neos a subgraph is never fetched with deleted nodes?. For the workspace publishing service we could use the manual way of ->getContentGraph()->getSubgraph(...) and specify no restrictions as we need to see all nodes, disabled and deleted.

[kitsunet]

no longer be able to handle NodeAggregateWasRemoved

I guess we could do that, but wouldn't we need to offer some kind of migration for beta users? Or do we add a huge warning that people should publish/discard workspaces before updating to this?

regarding the new policy: shouldn't the deleted condition will be hardcoded

Mmmm, I am happy to do it either way, deleted could indeed be hardcoded if we wanted. I guess any special use, say a trash can or so, can easily use the low level apis to access it.... Either way would be fine by me.

[mhsdesign]

okay i got rid of the removal attachment point flag now and also tested it in combination with the neos ui change and the ui e2e tests are fine as well

left open points:

• using VisibilityConstraints::withoutRestrictions() will now show also removed nodes which is probably not desired in all places where we use it
• make soft deletion a core concept? FEATURE: Soft removal subtree tag removed #5463 (comment) (maybe even with high level commands that are translated?)
• we seem to display the changed tags as raw information in the workspace ui review: TagContentChange(addedTags,removedTags) having here deleted show up will be confusing in this place?
• how do we want to cleanup all the soft deleted nodes at one point? how can we fetch them and issue deletion commands? ALL workspaces need to be rebased too for this to go in effect

[kitsunet]

https://github.com/neos/neos-development-collection/pull/5463/files#diff-7afd4d9a8fcb7259c9136da8cba0e3b5b84fab7f01043a9a3c5ab54c881efc54R233 Should've taken care of the workspace ui problem? I saw that as well and after adjusting this here so that changes are marked as deleted it showed up as deleted in the workspace UI.

[kitsunet]

ok the link doesn't work, but it's in the ChangeProjectino where a if $event->tag == deleted results in a deleted change

[mhsdesign]

Regarding

using VisibilityConstraints::withoutRestrictions() will now show also removed nodes which is probably not desired in all places where we use it

Aside from many constraint checks and write site parts in the core (where the use of withoutRestrictions -> eg. showing the tagged nodes is legitimate)

These are the other cases i found:

• NodesController -> we are in a controller and thus might use getContentSubgraph instead, which is more correct either way as its checks the permission
• AssetUsageIndexingProcessor / AssetUsageCatchUpHook -> keeps asset usage for soft removed nodes which is definitely warranted! -> no we want to introduce soft deletion too
• NodeDuplicationService -> duplicates also nodes tagged removed now, but i think we can better restricted that instead to withoutRemoved
• TimeableNodeVisibilityService -> fetches now also removed nodes, which means a timed disabled in here would work? -> but that is not needed. This can be better restricted instead to withoutRemoved

[mhsdesign]

Todos as we discussed in the weekly last Friday:

• deprecate VisibilityConstraints::withoutRestrictions() and make it not show removed nodes
• introduce new VisibilityConstraints::createEmpty() or none() (but createEmpty() is more stream lined) and have this be used in the core
• introduce similar to NodeTypeNameFactory a Neos' VisibilityConstraintsFactory with constructors for frontend() and editPreviewMode() ?
  • using backend() or editPreviewMode() might be short sighted as removed nodes will probably show up as a trash symbol at some point.
• figure out where to place SubtreeTag disabled and removed.
  • Disabled, because of DisabledNodeAggregate is still a core concern?
  • But removed not?! So we need a place where to add a factory for removed
    maybe just into Neos\Neos\Domain\SoftRemoval we add a SoftRemovedTag::get() factory? Or similar to our ContentStreamEventStreamName pattern a SoftRemovedTag::create()->getSubtreeTag() and SoftRemovedTag::matches(SubtreeTag::fromString("removed"))

further things, bernhard checked all handlers for NodeAggregateWasRemoved which need checking:

• the graph projections -> no adjustments needed
• the documentUriPath projection -> needs to decide how to deal with soft removal: Is it to be handled like hard removal or like disabling? -> Adjustment PR: TASK: Adjust document uri projection to soft removals #5485
• the router cache hook; should work the same way as the documentUriPath projection does, right?
• the pendingChanges projection -> is adjusted by this change FEATURE: Soft removal subtree tag removed #5463 and works properly without the removalAttachmentPoint
• the AssetUsageCatchupHook -> needs to decide how to deal with soft removal: Is it to be handled like hard removal or like disabling?
• the GraphProjectorCatchUpHookForCacheFlushing -> needs to decide how to deal with soft removal: Is it to be handled like hard removal or like disabling / general tagging?
• the Neos UI ConflictsFactory -> needs to decide whether soft removal is something special
• the RedirectHandler's DocumentUriPathProjectionHook -> needs to decide how to deal with soft removal

Also we still have these cases to adjust where we now want to avoid having removed nodes shown up

• NodeDuplicationService -> duplicates also nodes tagged removed now, but i think we can better restricted that instead to withoutRemoved
• TimeableNodeVisibilityService -> fetches now also removed nodes, which means a timed disabled in here would work? -> but that is not needed. This can be better restricted instead to withoutRemoved

[mhsdesign]

Okay the change is now finished (and its followup prs too)

I hoped we could fully get rid of the removalAttachmentPoint: #4487
instead this pr merely prepares the full removal of this hack.

main point why i think we still need it in the core for now is to allow a fluid migration from existing workspaces when updating to beta 19.

That means legacy removes of contents on a document are still publishable and shown as changed when saying publish document.

I think we can remove the removalAttachmentPoint at any point later we like - probably already Neos 9.1.

Behaviour of the soft removal publishing e.g. which events are included will be document by test via #5494. That pr also shows how hard removals (without removalAttachmentPoint) will not be publishable in any way via the workspace publishing service -> as currently too.

[bwaidelich]

I really like the change in general!
Just a few, partly nitpicky, comments regarding method names and an argument against the new classes NeosVisibilityConstraints and SoftRemovedTag

[bwaidelich]

suggestion: wouldn't VisibilityConstraints::none() be more descriptive?

[mhsdesign]

thought so too but none() as factory for collection objects is not present for any api in Neos. A few usages to internal objects have been sneakily introduced via your subscription change but they are all internal and ill rename them probably to createEmpty too ^^

We had a similar naming conflict when introducing createWithoutDimensions in #4632

so maybe we can go also the road of createWithoutConstraints?

[mhsdesign]

Just out of principle i of course DID open that can of worms of none vs createEmpty: #5497

Just to avoid running into that wormhole @kitsunet and me also discussed

• VisibilityConstraints::excludeNone()
• VisibilityConstraints::withoutExclude()

but after all we have done enough optimisation for now and will keep it createEmpty.

Objection overruled.

[bwaidelich]

suggestion: Not changed with this PR, but since it changes the API of this class: this constructor should be deprecated in favor of a new one VisibilityConstraints::fromSubtreeTags() IMO. What are "tag constraints"?

[mhsdesign]

jup youre reading my mind! I had this change already in my local working directory an decided not to do that because it was just a recent addition of yours.
But as you feel the same - and seeing this is a brand new fresh api with little to no usages, we can definitely rename it without risking any likely breaking things!

[mhsdesign]

As we just decided for NeosVisibilityConstraints::excludeDisabled how about the idea to carry the Exclude wording also down the line meaning:

final readonly class VisibilityConstraints
{
    private function __construct(
        public SubtreeTags $excludedSubtreeTags,
    ) {
    }

    public static function excludeSubtreeTags(SubtreeTags $tagConstraints): self;
}

[mhsdesign]

adjusted via abd571e

[bwaidelich]

I haven't understood why we do

SubtreeTag::disabled()

but

SoftRemovedTag::getSubtreeTag()

Yet another abstraction is not really helpful IMO

What about simply doing sth like:

final readonly class NeosSubtreeTags {
    public static function disabled(): SubtreeTag {
        return SubtreeTag::fromString('disabled');
    }

    public static function removed(): SubtreeTag {
        return SubtreeTag::fromString('removed');
    }
}

?

This way we can make this a pure Neos concept and still avoid the magic strings to spread.

Instead of SoftRemovedTag::isRemovedSubtreeTag($tag) you'd write $tag->equals(NeosSubtreeTags::removed())

[mhsdesign]

the suggestion to fully move out the disabled concern from the core is what i do really like. It also is what convinced me to implement: #5496

I introduced a NeosSubtreeTag as you said and let extend the pr #5496 to specify also a disabled constructor and have it deprecated in the core.

see commit 632b216

[mhsdesign]

The idea of a SoftRemovedTag came from the wrapper objects ContentStreamEventStreamName and WorkspaceEventStreamName where we could have had also a ContentRepositoryEventStream object or something...

I guess a SoftRemovedTag and aDeletedTag objects are not too bad and can actually be nicely encapsulated into their own contexts/ folders but having a NeosSubtreeTags seems indeed the better choice to have it all in one place