FEATURE: Soft removal subtree tag `removed` #5463

kitsunet · 2025-02-09T14:14:39Z

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.

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

mhsdesign · 2025-02-10T07:23:38Z

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 · 2025-02-10T07:57:29Z

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.

Neos.ContentRepository.Core/Classes/Feature/SubtreeTagging/Dto/SubtreeTag.php

We dont make it a policy because no one should have "Neos.Neos:ContentRepository.ReadDeletedNodes" granted

with the soft deletion - by tagging a node `deleted` - actual node deletions via (NodeAggregateWasRemoved) inside a non-live workspace are not desired in Neos if we publish a site we will include real removals like that in the publish or discard operation to prevent the change from being orphaned. Note that the site might not necessarily be the site where the change was made in multisite environments, but we cannot determine the hierarchy for removed nodes and edges.

because nodes are just tagged deleted we can still find them and their ancestors via the subgraph

mhsdesign · 2025-02-14T09:15:46Z

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 · 2025-02-14T09:27:15Z

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 · 2025-02-14T09:29:16Z

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

this aligns more to our wording in `NodeAggregateWasRemoved` and we used generally the term 'remove' in neos 9

mhsdesign · 2025-02-26T22:10:37Z

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

via the neos auth provider we will see disabled nodes in the backed (because its enabled for editor roles) but not the `removed` nodes which is correct. The restricted subtree tags are: array(1) { [0]=> string(7) "removed" }

…l be!

mhsdesign · 2025-03-03T07:52:53Z

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

The removed tagging is NOT a core concern, and thus we create a dedicated factory similar to our `ContentStreamEventStreamName` factory.

and move logic out of `ContentRepositoryAuthorizationService` agian

…reateEmpty()` We had the discussion again how to name an empty object and for normal collections we used `createEmpty()` in the past and should continue to do so. The use of `none()` snuck in lately via the subscription engine. neos#5463 (comment)

After the discussion https://neos-project.slack.com/archives/C04PYL8H3/p1741197705659749?thread_ts=1741193962.566189&cid=C04PYL8H3 We concluded that `without` is misleading and normally used either way for immutable object member methods.

The method is relatively new from beta 16 and seldom used: neos@14c9f84 to be au pair to `NeosVisibilityConstraints::excludeDisabled` Also we had the discussion that the naming neos#5463 (comment) "TagConstraints" is confusing because there is no such value object. Funnily this reverts neos@f1c66a3 which was just a wip.

And deprecate `SubtreeTag::disabled` see discussion in neos#5463 (comment) The only behavioural change is that with that the content repository does not know any subtree tag by default and thus the new default of the `StaticAuthProvider::getVisibilityConstraints` is to have nothing excluded!

as per neos#5463

…egate` as per neos/neos-development-collection#5463

The method is relatively new from beta 16 and seldom used: neos/neos-development-collection@14c9f84 to be au pair to `NeosVisibilityConstraints::excludeDisabled` Also we had the discussion that the naming neos/neos-development-collection#5463 (comment) "TagConstraints" is confusing because there is no such value object. Funnily this reverts neos/neos-development-collection@9372b79 which was just a wip.

The method is relatively new from beta 16 and seldom used: neos/neos-development-collection@f3bc82e to be au pair to `NeosVisibilityConstraints::excludeDisabled` Also we had the discussion that the naming neos/neos-development-collection#5463 (comment) "TagConstraints" is confusing because there is no such value object. Funnily this reverts neos/neos-development-collection@702839d which was just a wip.

And deprecate `SubtreeTag::disabled` see discussion in neos/neos-development-collection#5463 (comment) The only behavioural change is that with that the content repository does not know any subtree tag by default and thus the new default of the `StaticAuthProvider::getVisibilityConstraints` is to have nothing excluded!

The method is relatively new from beta 16 and seldom used: neos/neos-development-collection@14c9f84 to be au pair to `NeosVisibilityConstraints::excludeDisabled` Also we had the discussion that the naming neos/neos-development-collection#5463 (comment) "TagConstraints" is confusing because there is no such value object. Funnily this reverts neos/neos-development-collection@f1c66a3 which was just a wip.

And deprecate `SubtreeTag::disabled` see discussion in neos/neos-development-collection#5463 (comment) The only behavioural change is that with that the content repository does not know any subtree tag by default and thus the new default of the `StaticAuthProvider::getVisibilityConstraints` is to have nothing excluded!

The method is relatively new from beta 16 and seldom used: neos/neos-development-collection@4a230cc to be au pair to `NeosVisibilityConstraints::excludeDisabled` Also we had the discussion that the naming neos/neos-development-collection#5463 (comment) "TagConstraints" is confusing because there is no such value object. Funnily this reverts neos/neos-development-collection@f1c66a3 which was just a wip.

And deprecate `SubtreeTag::disabled` see discussion in neos/neos-development-collection#5463 (comment) The only behavioural change is that with that the content repository does not know any subtree tag by default and thus the new default of the `StaticAuthProvider::getVisibilityConstraints` is to have nothing excluded!

as per neos/neos-development-collection#5463

neos#5463 deprecated parts from the `VisibilityConstraints`

as per neos/neos-development-collection#5463

…reateEmpty()` We had the discussion again how to name an empty object and for normal collections we used `createEmpty()` in the past and should continue to do so. The use of `none()` snuck in lately via the subscription engine. neos/neos-development-collection#5463 (comment)

TASK: Delete as tag

f24d8b5

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

github-actions bot added Task 9.0 labels Feb 9, 2025

kitsunet mentioned this pull request Feb 9, 2025

TASK: Delete as tag neos/neos-ui#3919

Merged

mhsdesign reviewed Feb 14, 2025

View reviewed changes

Neos.ContentRepository.Core/Classes/Feature/SubtreeTagging/Dto/SubtreeTag.php Outdated Show resolved Hide resolved

mhsdesign added 3 commits February 14, 2025 09:09

TASK: Hardcode deleted tag to be never visible

31caee0

We dont make it a policy because no one should have "Neos.Neos:ContentRepository.ReadDeletedNodes" granted

TASK: Remove hack to calculate root line for removed nodes

6dda7f1

because nodes are just tagged deleted we can still find them and their ancestors via the subgraph

mhsdesign added 2 commits February 26, 2025 22:43

TASK: Rename SubtreeTag::deleted to removed

c1d53c7

this aligns more to our wording in `NodeAggregateWasRemoved` and we used generally the term 'remove' in neos 9

TASK: Make VisibilityConstraints::default include new removed tag

9676659

mhsdesign added 2 commits February 27, 2025 08:44

Merge remote-tracking branch 'origin/9.0' into task/soft-deleted-tag

3a70a78

TASK: Use getContentSubgraph in frontend controller

5bc69e3

via the neos auth provider we will see disabled nodes in the backed (because its enabled for editor roles) but not the `removed` nodes which is correct. The restricted subtree tags are: array(1) { [0]=> string(7) "removed" }

mhsdesign changed the title ~~TASK: Delete as tag~~ FEATURE: Soft removal subtree tag removed Feb 28, 2025

github-actions bot added the Feature label Feb 28, 2025

mhsdesign marked this pull request as ready for review February 28, 2025 08:17

mhsdesign mentioned this pull request Mar 2, 2025

FEATURE: Soft removal garbage collection #5479

Merged

6 tasks

mhsdesign added 3 commits March 2, 2025 16:46

Merge remote-tracking branch 'origin/9.0' into task/soft-deleted-tag

2ed0770

TASK: Add test for soft removed node handling for change projection

754c0f9

TASK: Denote that this change implementation is not api and never wil…

50a3953

…l be!

mhsdesign added 4 commits March 3, 2025 12:22

TASK: Remove SubtreeTag::removed and introduce SoftRemovedTag

48d0ac6

The removed tagging is NOT a core concern, and thus we create a dedicated factory similar to our `ContentStreamEventStreamName` factory.

TASK: Deprecate VisibilityConstraints::withoutRestrictions

9cd7d52

TASK: Add neos specific static factory for VisibilityConstraints

3a85f75

TASK: Introduce NeosVisibilityConstraints

73ffc88

and move logic out of `ContentRepositoryAuthorizationService` agian

mhsdesign requested a review from bwaidelich March 5, 2025 14:08

mhsdesign mentioned this pull request Mar 5, 2025

TASK: Code style adjust 5 internal usages of Object::none() to ::createEmpty() #5497

Merged

6 tasks

mhsdesign added 3 commits March 5, 2025 19:07

TASK: Rename factories to exclude*

8077337

After the discussion https://neos-project.slack.com/archives/C04PYL8H3/p1741197705659749?thread_ts=1741193962.566189&cid=C04PYL8H3 We concluded that `without` is misleading and normally used either way for immutable object member methods.

TASK: Deprecate fromTagConstraints instead of removing directly ;)

7e37a51

kitsunet merged commit 79671f9 into neos:9.0 Mar 5, 2025
12 checks passed

mhsdesign added a commit to nezaniel/neos-development-collection that referenced this pull request Mar 5, 2025

TASK: Use new NeosSubtreeTag::removed() instead of temporary stub

ab9c585

as per neos#5463

mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Mar 5, 2025

TASK: Use new NeosSubtreeTag::removed() instead of temporary stub

27dfbe0

as per neos#5463

mhsdesign added a commit to mhsdesign/neos-ui that referenced this pull request Mar 5, 2025

TASK: Use new NeosSubtreeTag::disabled() and avoid `DisableNodeAggr…

ea41bfe

…egate` as per neos/neos-development-collection#5463

neos-bot pushed a commit to neos/contentrepository-core that referenced this pull request Mar 6, 2025

TASK: Use new NeosSubtreeTag::removed() instead of temporary stub

e08ab36

as per neos/neos-development-collection#5463

neos-bot pushed a commit to neos/neos that referenced this pull request Mar 6, 2025

TASK: Use new NeosSubtreeTag::removed() instead of temporary stub

78e6627

as per neos/neos-development-collection#5463

mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Mar 7, 2025

TASK: Move deprecated api into dedicated trait to avoid confusion

b0aec96

neos#5463 deprecated parts from the `VisibilityConstraints`

mhsdesign mentioned this pull request Mar 7, 2025

TASK: Elaborate deprecations in VisibilityConstraints #5501

Open

6 tasks

dlubitz mentioned this pull request Mar 7, 2025

FEATURE: Neos 9.0 compatibility webexcess/openstreetmap#33

Draft

neos-bot pushed a commit to neos/contentrepository-core that referenced this pull request Mar 7, 2025

TASK: Use new NeosSubtreeTag::removed() instead of temporary stub

a795f91

as per neos/neos-development-collection#5463

neos-bot pushed a commit to neos/neos that referenced this pull request Mar 7, 2025

TASK: Use new NeosSubtreeTag::removed() instead of temporary stub

7cdcbeb

as per neos/neos-development-collection#5463

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEATURE: Soft removal subtree tag `removed` #5463

FEATURE: Soft removal subtree tag `removed` #5463

kitsunet commented Feb 9, 2025 •

edited

Loading

mhsdesign commented Feb 10, 2025

kitsunet commented Feb 10, 2025

mhsdesign commented Feb 14, 2025

kitsunet commented Feb 14, 2025 •

edited

Loading

kitsunet commented Feb 14, 2025

mhsdesign commented Feb 26, 2025 •

edited

Loading

mhsdesign commented Mar 3, 2025 •

edited

Loading

FEATURE: Soft removal subtree tag removed #5463

FEATURE: Soft removal subtree tag removed #5463

Conversation

kitsunet commented Feb 9, 2025 • edited Loading

mhsdesign commented Feb 10, 2025

kitsunet commented Feb 10, 2025

mhsdesign commented Feb 14, 2025

kitsunet commented Feb 14, 2025 • edited Loading

kitsunet commented Feb 14, 2025

mhsdesign commented Feb 26, 2025 • edited Loading

mhsdesign commented Mar 3, 2025 • edited Loading

FEATURE: Soft removal subtree tag `removed` #5463

FEATURE: Soft removal subtree tag `removed` #5463

kitsunet commented Feb 9, 2025 •

edited

Loading

kitsunet commented Feb 14, 2025 •

edited

Loading

mhsdesign commented Feb 26, 2025 •

edited

Loading

mhsdesign commented Mar 3, 2025 •

edited

Loading