FEATURE: Node copying Version 3 (as a Neos service)

[mhsdesign]

Resolves: #5359 (because it will be obsolete)
Resolves: #5350
Resolves: #5351

Upgrade instructions

Instead of using CopyNodesRecursively::createFromSubgraphAndStartNode and handling that in the CR, please use:

#[Flow\Inject()]
protected NodeDuplicationService $nodeDuplicationService;

// ...

$this->nodeDuplicationService->copyNodesRecursively(
    $contentRepositoryId,
    $workspaceName,
    $sourceDimensionSpacePoint,
    $sourceNodeAggregateId,
    $targetDimensionSpacePoint,
    $targetParentNodeAggregateId,
    $targetSucceedingSiblingNodeAggregateId
);

In case a leaf node that is tethered is copied, we un-tether it.
A migration flow migrateevents:migratecopytetherednode fixes this for previous cases.

Usages of the legacy copy node must be published before migrating to the final Neos version, to detect those see: ./flow migrateevents:copynodesstatus

Review instructions

Neos UI part: neos/neos-ui#3887

The with dd6408e introduced CopyNodesRecursively content repository command is not stable, see its problems outline in #5351.

For one not all features were implemented (or only recently) like reference copying and subtree tag copying. Also there was little to no test coverage (2 tests)

Further by emitting events directly we would have to be super careful - which we were not. It was possible to copy tethered nodes and they kept their classification: #5350.

And lastly the event to command relation ship (needed for rebasing) is a problem. We cannot determine the copy command from the original event (which prevents us from removing the commandClass mapping).
And more importantly for rebasing the copy the WHOLE subtree snapshot is stored in the one events metadata field which can grow way to large (and is hard to maintain and write migrations for - everything is serialised in there). We argued which changes have to be done to keep copy nodes as a content repository command and found that there were almost no constraint checks for reapplying the copy (like the references of copied nodes could be deleted already), if we would check the constraints correctly, that would mean that the whole copy command could fail then which would not be desired. Instead, also to resolve the event to command relation ship we were thinking that a better low level copy implementation would generate the events with the command classes as if they were created by hand. Guessing the command payload (especially with the complexity of tethered nodes) would be utterly complex and errors would only be spottet during rebase. So we decided to actually issue REAL commands in a batch. As this is architecturally not possible from an actual cr command (would require full recursion) and that is okay, we discussed that we want a node copy service.
The disadvantages are that copying can fail now but thats also its positive side for rebasing. As a followup we need to find solutions to be graceful while copying and reapplying the copy. (Maybe by using NodeAggregateCommandHandler::withoutAncestorNodeTypeConstraintChecks)

And lastly, the copy command and its signature are not ideal as they dont allow multi dimension copying and also expose way to much internals: #5359 This is no problem anymore by using this service.

Checklist

Added Test-cases

• test that the tethered state of the initial node is ignored
• test that nested tethered nodes are copied WITH tethered state
  • Even when its tethered directly in another thethered
• test that tethered state of child nodes is redetermined as the parent node is now detached so a second level tethered node might not be tethered anymore, actually forbid that case
• copying disabled nodes
• root nodes cannot be copied
• recursive copying, with properties, with references

[mhsdesign]

to support usecases like Flowpack/Flowpack.NodeTemplates#60 were we need to return a list of commands and not handle them ourselves, it would be practical to expose a way to get the commands we would be handle but dont handle them.

Like:

public function calculateCopyNodesRecursively(
    ContentRepositoryId $contentRepositoryId,
    WorkspaceName $workspaceName,
    DimensionSpacePoint $sourceDimensionSpacePoint,
    NodeAggregateId $sourceNodeAggregateId,
    OriginDimensionSpacePoint $targetDimensionSpacePoint,
    NodeAggregateId $targetParentNodeAggregateId,
    ?NodeAggregateId $targetSucceedingSiblingNodeAggregateId,
    ?NodeAggregateIdMapping $nodeAggregateIdMapping = null
): Commands;

i would declare this as experimental api but it would be helpful to have that. As otherwise node copying would not be possible there without duplicating this.

[kitsunet]

Sounds reasonable to me

[mhsdesign]

The CopyNodesRecursively command will be removed with the release of neos 9, but is still present to allow the publication of workspaces. A warning is emitted in case workspaces contain changes.

migrateevents:copyNodesStatus

WARNING: 1 content streams contain unpublished legacy copy node events. They MUST be published before migrating to Neos 9 and will not be publishable afterwards.
Content stream e76a0472-0c6f-4b98-bfac-12c7a2515dfb (admin-admington-1)
  - @1093 copy node 55861d58-c4e5-6121-b7cf-ef9320ebfbb0 to 55861d58-c4e5-6121-b7cf-ef9320ebfbb0
NOTE: To reduce the number of "false positives" above and to cleanup the event store run `./flow contentStream:removeDangling` and `./flow contentStream:pruneRemovedFromEventStream`

[kitsunet]

took me a moment here, I think I might prefer a Subtrees::empty() and then array_key_exists($nodeAggregateId, $subtreesByParentNodeId) ? Subtrees::fromArray(array_reverse($subtreesByParentNodeId[$nodeAggregateId])) : Subtrees:empty()

[kitsunet]

Provisional +1, looks already pretty good, no major complaints atm.

[mhsdesign]

In a followup we could improve things and address the optional todos

For example copying a node with tethered children that is not tethered according to the current node type schema, or copying properties that are not defined
in the current node type schema anymore. In those cases the structure adjustments have to be executed. (todo only copy what is applicable and be graceful)

In Flowpack.NodeTemplates i already build a graceful mechanism by carefully checking all constraints beforehand. This is of-course tedious but might be worth in in a later point:

https://github.com/Flowpack/Flowpack.NodeTemplates/releases/tag/2.0.0

And also add tests like this:

Scenario: Coping fails if a node with tethered children is copied that are not tethered according to the current node type schema
  When I change the node types in content repository "default" to:
  """yaml
  'Neos.ContentRepository.Testing:TetheredDocument':
    childNodes: {} # no tethered node anymore!
  'Neos.ContentRepository.Testing:DocumentWithoutTetheredChildren': []
  """

  And I expect the node aggregate "davids-tether" to exist
  And I expect this node aggregate to be classified as "tethered"

  When copy nodes recursively is executed with payload:
    | Key                         | Value                 |
    | sourceDimensionSpacePoint   | {}                    |
    | sourceNodeAggregateId       | "sir-david-nodenburg" |
    | targetDimensionSpacePoint   | {}                    |
    | targetParentNodeAggregateId | "node-mc-nodeface"    |

[mhsdesign]

Hmm i just noticed that copying behaved differently in Neos 8.3 :O

As per

neos-development-collection/Neos.ContentRepository/Classes/Domain/Model/NodeType.php

Line 283 in b03cca1

* - Recursive copying only happens *inside* this aggregate, and stops at nested aggregates.

Recursive copying only happens inside this aggregate, and stops at nested aggregates.

Meaning that when copying a document only its content children are copied but not nested documents? That could be implemented now with a Neos.Neos:Content filter on the subtree... And this would be even fully okay as this is a Neos Service now.

But it makes me wonder if we really should remove the isAggregate as that would mean we would never be able to have this 1-to-1 copy behaviour in the core as the core doesnt know anything about content vs document.

[kitsunet]

Recursive copying only happens inside this aggregate, and stops at nested aggregates.

This is a lie though, I see no such check in Node::createRecursiveCopy and am not aware of one... 😜

[mhsdesign]

Jup tested it in 8.3 it doesnt do that :D

But we actually used it as $detachedCopy flag:

neos-development-collection/Neos.ContentRepository/Classes/Domain/Model/Node.php

Line 812 in 6936b7b

$copiedNode = $this->copyIntoInternal($referenceNode, $nodeName, $this->getNodeType()->isAggregate());

$detachedCopy only has an influence if we are copying from one dimension to the other, possibly creating a new node variant

see

neos-development-collection/Neos.ContentRepository/Classes/Domain/Model/Node.php

Lines 1670 to 1681 in 6936b7b

	* Create a recursive copy of this node below $referenceNode with $nodeName.
	*
	* $detachedCopy only has an influence if we are copying from one dimension to the other, possibly creating a new
	* node variant:
	*
	* - If $detachedCopy is true, the whole (recursive) copy is done without connecting original and copied node,
	* so NOT CREATING a new node variant.
	* - If $detachedCopy is false, and the node does not yet have a variant in the target dimension, we are CREATING
	* a new node variant.
	*
	* As a caller of this method, $detachedCopy should be true if $this->getNodeType()->isAggregate() is true, and false
	* otherwise.

so not interestring for our case as copying for dimensions is kinda broken still :D -> #5054