RemoveNodeAggregate command should not specify removalAttachmentPoint

[mhsdesign]

I just stumbled upon the removalAttachmentPoint field of the RemoveNodeAggregate command (

neos-development-collection/Neos.ContentRepository.Core/Classes/Feature/NodeRemoval/Command/RemoveNodeAggregate.php

Line 54 in 3e48b42

public readonly ?NodeAggregateId $removalAttachmentPoint = null

).
But IMO it is really flaky to leave this up to the calling side (and make it optional).
Can't we just add this information in the command handler?
...or can we even get rid of it in the meantime? IIRC we already use the CatchUpHook in the UI

[mhsdesign]

Originally a draft from @bwaidelich ... i must say that as this is api its super confusing to end users what this field means 😂 and i also like for it to be gone ^^

[mhsdesign]

we want to remove it from the main create static factory #4489

[bwaidelich]

We can't remove the field just yet because the Neos UI still depends on it, but it is now explicitly internal with #4489

[mhsdesign]

Ill reopen this - not because we need it for the release - but as this is still a thing we can work on in the future.

More discussion regarding this happened in: https://neos-project.slack.com/archives/C3MCBK6S2/p1682513945947879

The main thesis keeps being: One needs to explicitly set this field correctly so the Neos Ui can remove this deletion via a page publish (and not via publish all only), and its not particularly easy to set the field correctly as the Neos UI might have set it do the Node itself which does not work:

neos-development-collection/Neos.Neos/Classes/Domain/Service/WorkspacePublishingService.php

Line 447 in ac2c359

private function isChangeWithSelfReferencingRemovalAttachmentPoint(Change $change): bool

In #4943 (review) i argued that Neos should have a higher level service - like the WorkspacePublishingService - for node removal and maybe other things.
Neos - knowing the difference between content and document - could then expose a RemoveNodeAggregateChangeAware or RemoveContent or RemoveDocument command or method. Taking care of setting $removalAttachmentPoint correctly.
This is currently spread over multiple repositories (Neos.ContentRepositoryCore (stores meta data), Neos.Neos (uses metadata via change projection), Neos.Neos.Ui (sets metadata on removal)):

https://github.com/neos/neos-ui/blob/4e3824a1693de5c2bc19d3bf7c8284478d2afc09/Classes/Domain/Model/Changes/Remove.php#L94

But establishing yet another service to do things instead of just using the pretty core api does not help either. The main problem is after a hard removal (not soft like in 8.3) we cannot find out where the node was previously in the tree.

But this is actually solveable even right now: By using catchup hooks and the onBeforeEvent!. That would allow us to fetch a node before it was removed, and then store the metadata. Similarly to the old asset usage projection it would mean that the change projection will also be a catchup hook, because it is currently just more powerful.
We discussed also the possibilities of making it easier to write projections (see dilemma wich nodes were recursively deleted ... making a projection without implementing the full graph impossible) and instead of specifying all these metadata in the core events - which is just not strictly necessary - we could just introduce higher level events containing more information, making projections actually viable.

So for example the core NodeAggregateWasRemoved could omit the $removalAttachmentPoint and so the command, while some high level event could just contain all parent node ids:
(of course we could also just add this information directly to NodeAggregateWasRemoved but the question is were does it stop? Do we want also all children NodeAggregateIds in a `NodeRemoval

final readonly class HighLevelNodeAggregateWasRemoved implements EventInterface
{
    public function __construct(
        public WorkspaceName $workspaceName,
        public ContentStreamId $contentStreamId,
        public NodeAggregateId $nodeAggregateId,
        public OriginDimensionSpacePointSet $affectedOccupiedDimensionSpacePoints,
        public DimensionSpacePointSet $affectedCoveredDimensionSpacePoints,
        public NodeAggregateIds $ancestorNodeAggregateIds,
        // to solve other problems we should extend this too:
        // public NodeAggregateIds $descendantNodeAggregateIds,
    ) {
    }

This is all under the premise that we need this metadata for the Ui also for the long run for example also to show which nodes are deleted already still in the tree:

neos/neos-ui#2974

Although that would rise even more questions regarding 'previous sibling' and their properties and meta data.

unless ...

A simple fix? Change the Ui to introduce a trash can!?

A simple fix out of this would be just to show a trash can in the Neos ui with all nodes in a flat list ^^ where you can either publish their removal or discard them (like in your trash can on your os)

[kitsunet]

A simple fix out of this would be just to show a trash can in the Neos ui with all nodes in a flat list ^^ where you can either publish their removal or discard them (like in your trash can on your os)

Bit problematic because with the OS trash can people assume things are not visible anymore when they are in the trash can. Having to go in there and publish the deletion seems weird (yes emptying the bin is a thing, but that feels different, especially as the deletion CAN very well be related to other things, e.g. the creation of an alternative element).

it would mean that the change projection will also be a catchup hook, because it is currently just more powerful.

Then it wouldn't be replayed anymore, would it? Which would be broken AF.

[mhsdesign]

Another point which speaks for a catchup is that the information we get from the change projection are actually partially useless without the content graph.

For EACH change we will call findClosestNode to check if it is inside the document node to publish. Meaning for 1000 changes 1000 individual queries only to publish two changes on a document.

-> this is especially important as a Neos Ui PublishAll doesnt do a real publish all but will STILL check if the node is in this site for there could be multiple sites.

neos-development-collection/Neos.Neos/Classes/Domain/Service/WorkspacePublishingService.php

Line 404 in 8ad0a3e

$actualAncestorNode = $subgraph->findClosestNode(

a catchup hook could understand the hierarchy better by just asking finding the closest parent once and saving it do the database.

Or higher level events, one or the other.

A third solution could be to diff the workspace nodes vs the base but for that the base has to be up to date. Removals could be detectable with their original position as well as knowing which is document node for a change