From c0c59065b021ed9d68f0a4ec5e81384a849aee4b Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Thu, 5 Dec 2024 11:27:09 +0100 Subject: [PATCH 1/9] BUGFIX: Asset that `SetNodeProperties` is not handled empty --- .../Feature/NodeModification/Command/SetNodeProperties.php | 3 +++ .../Feature/NodeModification/Dto/PropertyValuesToWrite.php | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/Neos.ContentRepository.Core/Classes/Feature/NodeModification/Command/SetNodeProperties.php b/Neos.ContentRepository.Core/Classes/Feature/NodeModification/Command/SetNodeProperties.php index bb978e7e948..0972da056b3 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/NodeModification/Command/SetNodeProperties.php +++ b/Neos.ContentRepository.Core/Classes/Feature/NodeModification/Command/SetNodeProperties.php @@ -48,6 +48,9 @@ private function __construct( public OriginDimensionSpacePoint $originDimensionSpacePoint, public PropertyValuesToWrite $propertyValues, ) { + if ($this->propertyValues->isEmpty()) { + throw new \InvalidArgumentException(sprintf('The command "SetNodeProperties" for node %s must contain property values', $this->nodeAggregateId->value), 1733394351); + } } /** diff --git a/Neos.ContentRepository.Core/Classes/Feature/NodeModification/Dto/PropertyValuesToWrite.php b/Neos.ContentRepository.Core/Classes/Feature/NodeModification/Dto/PropertyValuesToWrite.php index dcc0f414f3c..b27d956d5f5 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/NodeModification/Dto/PropertyValuesToWrite.php +++ b/Neos.ContentRepository.Core/Classes/Feature/NodeModification/Dto/PropertyValuesToWrite.php @@ -95,4 +95,9 @@ public function getPropertiesToUnset(): PropertyNames ) ); } + + public function isEmpty(): bool + { + return $this->values === []; + } } From c6d093b87ebaeab6b8bf06432371e0b7c73b1032 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Thu, 9 Jan 2025 11:03:31 +0100 Subject: [PATCH 2/9] TASK: Fix constraint test of failing too early > InvalidArgumentException (1733394351): The command "SetNodeProperties" for node lady-eleonode-rootford must contain property values We want a NodeAggregateIsRoot exception --- .../01-SetNodeProperties_ConstraintChecks.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/04-NodeModification/01-SetNodeProperties_ConstraintChecks.feature b/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/04-NodeModification/01-SetNodeProperties_ConstraintChecks.feature index 968798de225..24a22c54ebf 100644 --- a/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/04-NodeModification/01-SetNodeProperties_ConstraintChecks.feature +++ b/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/04-NodeModification/01-SetNodeProperties_ConstraintChecks.feature @@ -67,7 +67,7 @@ Feature: Set node properties: Constraint checks | Key | Value | | nodeAggregateId | "lady-eleonode-rootford" | | originDimensionSpacePoint | {"language":"de"} | - | propertyValues | {} | + | propertyValues | {"text":"New text"} | Then the last command should have thrown an exception of type "NodeAggregateIsRoot" Scenario: Try to set properties in an origin dimension space point that does not exist From f0fc051e0365200e0f005ffb4987115400eae2a8 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Mon, 13 Jan 2025 17:51:26 +0100 Subject: [PATCH 3/9] TASK: Ensure that `Events` are never instantiated empty by enforcing this via "non-empty-array" we can better catch runtime type errors beforehand via phpstan. As otherwise the event store will fail with > Writable events must contain at least one event Some internal utility methods previously also returned possibly empty "Events" instances which were then transformed via iterator_to_array back to arrays and merged. This was not simplified to just work with arrays directly to denote that they might be empty. In cases were we work with fully sure filled intermediate event arrays we use non-empty-array. With this now fine-grained type checking aside from the original found set properties error following cases will also break with 0 events: - handleSetSerializedNodeProperties (known) - handleSetSerializedNodeReferences - handlePublishWorkspace and handlePublishIndividualNodesFromWorkspace if there were no applied events that implement PublishableToWorkspaceInterface -> goes into direction of https://github.com/neos/neos-development-collection/pull/5337 - structure adjustment TETHERED_NODE_WRONGLY_ORDERED --- .../Classes/EventStore/Events.php | 21 +++---- .../Common/NodeTypeChangeInternals.php | 15 +++-- .../Feature/Common/NodeVariationInternals.php | 6 +- .../Feature/Common/TetheredNodeInternals.php | 56 +++++++++---------- .../Feature/NodeCreation/NodeCreation.php | 14 +++-- .../Feature/NodeTypeChange/NodeTypeChange.php | 18 +++--- .../Classes/Feature/RebaseableCommand.php | 2 +- .../RootNodeCreation/RootNodeHandling.php | 14 +++-- 8 files changed, 79 insertions(+), 67 deletions(-) diff --git a/Neos.ContentRepository.Core/Classes/EventStore/Events.php b/Neos.ContentRepository.Core/Classes/EventStore/Events.php index f9d9abc8191..2401685f992 100644 --- a/Neos.ContentRepository.Core/Classes/EventStore/Events.php +++ b/Neos.ContentRepository.Core/Classes/EventStore/Events.php @@ -10,16 +10,17 @@ * @implements \IteratorAggregate * @internal only used during event publishing (from within command handlers) - and their implementation is not API */ -final class Events implements \IteratorAggregate, \Countable +final readonly class Events implements \IteratorAggregate, \Countable { /** - * @var array + * @var non-empty-array */ - private readonly array $events; + public array $items; private function __construct(EventInterface|DecoratedEvent ...$events) { - $this->events = $events; + /** @var non-empty-array $events */ + $this->items = $events; } public static function with(EventInterface|DecoratedEvent $event): self @@ -29,11 +30,11 @@ public static function with(EventInterface|DecoratedEvent $event): self public function withAppendedEvents(Events $events): self { - return new self(...$this->events, ...$events->events); + return new self(...$this->items, ...$events->items); } /** - * @param array $events + * @param non-empty-array $events * @return static */ public static function fromArray(array $events): self @@ -43,21 +44,21 @@ public static function fromArray(array $events): self public function getIterator(): \Traversable { - yield from $this->events; + yield from $this->items; } /** * @template T * @param \Closure(EventInterface|DecoratedEvent $event): T $callback - * @return list + * @return non-empty-list */ public function map(\Closure $callback): array { - return array_map($callback, $this->events); + return array_map($callback, $this->items); } public function count(): int { - return count($this->events); + return count($this->items); } } diff --git a/Neos.ContentRepository.Core/Classes/Feature/Common/NodeTypeChangeInternals.php b/Neos.ContentRepository.Core/Classes/Feature/Common/NodeTypeChangeInternals.php index 31d944fe121..abcb07b5bff 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/Common/NodeTypeChangeInternals.php +++ b/Neos.ContentRepository.Core/Classes/Feature/Common/NodeTypeChangeInternals.php @@ -16,7 +16,7 @@ use Neos\ContentRepository\Core\DimensionSpace\DimensionSpacePointSet; use Neos\ContentRepository\Core\DimensionSpace\OriginDimensionSpacePointSet; -use Neos\ContentRepository\Core\EventStore\Events; +use Neos\ContentRepository\Core\EventStore\EventInterface; use Neos\ContentRepository\Core\Feature\NodeRemoval\Event\NodeAggregateWasRemoved; use Neos\ContentRepository\Core\NodeType\NodeType; use Neos\ContentRepository\Core\Projection\ContentGraph\ContentGraphInterface; @@ -34,13 +34,15 @@ trait NodeTypeChangeInternals /** * NOTE: when changing this method, {@see NodeTypeChange::requireConstraintsImposedByHappyPathStrategyAreMet} * needs to be modified as well (as they are structurally the same) + * + * @return array */ private function deleteDisallowedNodesWhenChangingNodeType( ContentGraphInterface $contentGraph, NodeAggregate $nodeAggregate, NodeType $newNodeType, NodeAggregateIds &$alreadyRemovedNodeAggregateIds, - ): Events { + ): array { $events = []; // if we have children, we need to check whether they are still allowed // after we changed the node type of the $nodeAggregate to $newNodeType. @@ -117,15 +119,18 @@ private function deleteDisallowedNodesWhenChangingNodeType( } } - return Events::fromArray($events); + return $events; } + /** + * @return array + */ private function deleteObsoleteTetheredNodesWhenChangingNodeType( ContentGraphInterface $contentGraph, NodeAggregate $nodeAggregate, NodeType $newNodeType, NodeAggregateIds &$alreadyRemovedNodeAggregateIds, - ): Events { + ): array { $events = []; // find disallowed tethered nodes $tetheredNodeAggregates = $contentGraph->findTetheredChildNodeAggregates($nodeAggregate->nodeAggregateId); @@ -156,7 +161,7 @@ private function deleteObsoleteTetheredNodesWhenChangingNodeType( } } - return Events::fromArray($events); + return $events; } /** diff --git a/Neos.ContentRepository.Core/Classes/Feature/Common/NodeVariationInternals.php b/Neos.ContentRepository.Core/Classes/Feature/Common/NodeVariationInternals.php index 12f3df80da1..349de0d4ced 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/Common/NodeVariationInternals.php +++ b/Neos.ContentRepository.Core/Classes/Feature/Common/NodeVariationInternals.php @@ -89,7 +89,7 @@ protected function handleCreateNodeSpecializationVariant( /** * @param array $events - * @return array + * @return non-empty-array */ protected function collectNodeSpecializationVariantsThatWillHaveBeenCreated( ContentGraphInterface $contentGraph, @@ -152,7 +152,7 @@ protected function handleCreateNodeGeneralizationVariant( /** * @param array $events - * @return array + * @return non-empty-array */ protected function collectNodeGeneralizationVariantsThatWillHaveBeenCreated( ContentGraphInterface $contentGraph, @@ -215,7 +215,7 @@ protected function handleCreateNodePeerVariant( /** * @param array $events - * @return array + * @return non-empty-array */ protected function collectNodePeerVariantsThatWillHaveBeenCreated( ContentGraphInterface $contentGraph, diff --git a/Neos.ContentRepository.Core/Classes/Feature/Common/TetheredNodeInternals.php b/Neos.ContentRepository.Core/Classes/Feature/Common/TetheredNodeInternals.php index 57b99797015..c53ca1f4313 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/Common/TetheredNodeInternals.php +++ b/Neos.ContentRepository.Core/Classes/Feature/Common/TetheredNodeInternals.php @@ -17,6 +17,7 @@ use Neos\ContentRepository\Core\DimensionSpace\OriginDimensionSpacePoint; use Neos\ContentRepository\Core\DimensionSpace\OriginDimensionSpacePointSet; use Neos\ContentRepository\Core\DimensionSpace\VariantType; +use Neos\ContentRepository\Core\EventStore\EventInterface; use Neos\ContentRepository\Core\EventStore\Events; use Neos\ContentRepository\Core\Feature\NodeCreation\Dto\NodeAggregateIdsByNodePaths; use Neos\ContentRepository\Core\Feature\NodeCreation\Event\NodeAggregateWithNodeWasCreated; @@ -164,6 +165,9 @@ protected function createEventsForMissingTetheredNode( ); } + /** + * @return array + */ protected function createEventsForMissingTetheredNodeAggregate( ContentGraphInterface $contentGraph, TetheredNodeTypeDefinition $tetheredNodeTypeDefinition, @@ -173,7 +177,7 @@ protected function createEventsForMissingTetheredNodeAggregate( ?NodeAggregateId $succeedingSiblingNodeAggregateId, NodeAggregateIdsByNodePaths $nodeAggregateIdsByNodePaths, NodePath $currentNodePath, - ): Events { + ): array { $events = []; $tetheredNodeType = $this->requireNodeType($tetheredNodeTypeDefinition->nodeTypeName); $nodeAggregateId = $nodeAggregateIdsByNodePaths->getNodeAggregateId($currentNodePath) ?? NodeAggregateId::create(); @@ -243,22 +247,20 @@ protected function createEventsForMissingTetheredNodeAggregate( foreach ($tetheredNodeType->tetheredNodeTypeDefinitions as $childTetheredNodeTypeDefinition) { $events = array_merge( $events, - iterator_to_array( - $this->createEventsForMissingTetheredNodeAggregate( - $contentGraph, - $childTetheredNodeTypeDefinition, - $affectedOriginDimensionSpacePoints, - $coverageByOrigin, - $nodeAggregateId, - null, - $nodeAggregateIdsByNodePaths, - $currentNodePath->appendPathSegment($childTetheredNodeTypeDefinition->name), - ) + $this->createEventsForMissingTetheredNodeAggregate( + $contentGraph, + $childTetheredNodeTypeDefinition, + $affectedOriginDimensionSpacePoints, + $coverageByOrigin, + $nodeAggregateId, + null, + $nodeAggregateIdsByNodePaths, + $currentNodePath->appendPathSegment($childTetheredNodeTypeDefinition->name), ) ); } - return Events::fromArray($events); + return $events; } protected function createEventsForWronglyTypedNodeAggregate( @@ -311,21 +313,17 @@ protected function createEventsForWronglyTypedNodeAggregate( // remove disallowed nodes if ($conflictResolutionStrategy === NodeAggregateTypeChangeChildConstraintConflictResolutionStrategy::STRATEGY_DELETE) { - array_push($events, ...iterator_to_array( - $this->deleteDisallowedNodesWhenChangingNodeType( - $contentGraph, - $nodeAggregate, - $tetheredNodeType, - $alreadyRemovedNodeAggregateIds - ) + array_push($events, ...$this->deleteDisallowedNodesWhenChangingNodeType( + $contentGraph, + $nodeAggregate, + $tetheredNodeType, + $alreadyRemovedNodeAggregateIds )); - array_push($events, ...iterator_to_array( - $this->deleteObsoleteTetheredNodesWhenChangingNodeType( - $contentGraph, - $nodeAggregate, - $tetheredNodeType, - $alreadyRemovedNodeAggregateIds - ) + array_push($events, ...$this->deleteObsoleteTetheredNodesWhenChangingNodeType( + $contentGraph, + $nodeAggregate, + $tetheredNodeType, + $alreadyRemovedNodeAggregateIds )); } @@ -338,7 +336,7 @@ protected function createEventsForWronglyTypedNodeAggregate( if ($tetheredChildNodeAggregate === null) { $events = array_merge( $events, - iterator_to_array($this->createEventsForMissingTetheredNodeAggregate( + $this->createEventsForMissingTetheredNodeAggregate( $contentGraph, $childTetheredNodeTypeDefinition, $nodeAggregate->occupiedDimensionSpacePoints, @@ -347,7 +345,7 @@ protected function createEventsForWronglyTypedNodeAggregate( null, $nodeAggregateIdsByNodePaths, $currentNodePath->appendPathSegment($childTetheredNodeTypeDefinition->name), - )) + ) ); } elseif (!$tetheredChildNodeAggregate->nodeTypeName->equals($childTetheredNodeTypeDefinition->nodeTypeName)) { $events = array_merge($events, iterator_to_array( diff --git a/Neos.ContentRepository.Core/Classes/Feature/NodeCreation/NodeCreation.php b/Neos.ContentRepository.Core/Classes/Feature/NodeCreation/NodeCreation.php index b73ba4750f6..df4415e6dfb 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/NodeCreation/NodeCreation.php +++ b/Neos.ContentRepository.Core/Classes/Feature/NodeCreation/NodeCreation.php @@ -17,6 +17,7 @@ use Neos\ContentRepository\Core\CommandHandler\CommandHandlingDependencies; use Neos\ContentRepository\Core\DimensionSpace; use Neos\ContentRepository\Core\DimensionSpace\DimensionSpacePointSet; +use Neos\ContentRepository\Core\EventStore\EventInterface; use Neos\ContentRepository\Core\EventStore\Events; use Neos\ContentRepository\Core\EventStore\EventsToPublish; use Neos\ContentRepository\Core\Feature\Common\InterdimensionalSiblings; @@ -212,7 +213,7 @@ private function handleCreateNodeAggregateWithNodeAndSerializedProperties( ) ]; - array_push($events, ...iterator_to_array($this->handleTetheredChildNodes( + array_push($events, ...$this->handleTetheredChildNodes( $command, $contentGraph, $nodeType, @@ -220,7 +221,7 @@ private function handleCreateNodeAggregateWithNodeAndSerializedProperties( $command->nodeAggregateId, $descendantNodeAggregateIds, null - ))); + )); return new EventsToPublish( ContentStreamEventStreamName::fromContentStreamId($contentGraph->getContentStreamId()) @@ -261,6 +262,7 @@ private function createRegularWithNode( /** * @throws ContentStreamDoesNotExistYet * @throws NodeTypeNotFound + * @return array */ private function handleTetheredChildNodes( CreateNodeAggregateWithNodeAndSerializedProperties $command, @@ -270,7 +272,7 @@ private function handleTetheredChildNodes( NodeAggregateId $parentNodeAggregateId, NodeAggregateIdsByNodePaths $nodeAggregateIds, ?NodePath $nodePath - ): Events { + ): array { $events = []; foreach ($nodeType->tetheredNodeTypeDefinitions as $tetheredNodeTypeDefinition) { $childNodeType = $this->requireNodeType($tetheredNodeTypeDefinition->nodeTypeName); @@ -298,7 +300,7 @@ private function handleTetheredChildNodes( SerializedNodeReferences::createEmpty(), ); - array_push($events, ...iterator_to_array($this->handleTetheredChildNodes( + array_push($events, ...$this->handleTetheredChildNodes( $command, $contentGraph, $childNodeType, @@ -306,9 +308,9 @@ private function handleTetheredChildNodes( $childNodeAggregateId, $nodeAggregateIds, $childNodePath - ))); + )); } - return Events::fromArray($events); + return $events; } } diff --git a/Neos.ContentRepository.Core/Classes/Feature/NodeTypeChange/NodeTypeChange.php b/Neos.ContentRepository.Core/Classes/Feature/NodeTypeChange/NodeTypeChange.php index 5436b048236..acb6e5a4c96 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/NodeTypeChange/NodeTypeChange.php +++ b/Neos.ContentRepository.Core/Classes/Feature/NodeTypeChange/NodeTypeChange.php @@ -17,6 +17,7 @@ use Neos\ContentRepository\Core\CommandHandler\CommandHandlingDependencies; use Neos\ContentRepository\Core\DimensionSpace\OriginDimensionSpacePoint; use Neos\ContentRepository\Core\DimensionSpace\OriginDimensionSpacePointSet; +use Neos\ContentRepository\Core\EventStore\EventInterface; use Neos\ContentRepository\Core\EventStore\Events; use Neos\ContentRepository\Core\EventStore\EventsToPublish; use Neos\ContentRepository\Core\Feature\RebaseableCommand; @@ -90,6 +91,9 @@ abstract protected function areNodeTypeConstraintsImposedByGrandparentValid( NodeType $nodeType ): bool; + /** + * @return array + */ abstract protected function createEventsForMissingTetheredNodeAggregate( ContentGraphInterface $contentGraph, TetheredNodeTypeDefinition $tetheredNodeTypeDefinition, @@ -99,7 +103,7 @@ abstract protected function createEventsForMissingTetheredNodeAggregate( ?NodeAggregateId $succeedingSiblingNodeAggregateId, NodeAggregateIdsByNodePaths $nodeAggregateIdsByNodePaths, NodePath $currentNodePath, - ): Events; + ): array; abstract protected function createEventsForWronglyTypedNodeAggregate( ContentGraphInterface $contentGraph, @@ -230,18 +234,18 @@ private function handleChangeNodeAggregateType( // remove disallowed nodes $alreadyRemovedNodeAggregateIds = NodeAggregateIds::createEmpty(); if ($command->strategy === NodeAggregateTypeChangeChildConstraintConflictResolutionStrategy::STRATEGY_DELETE) { - array_push($events, ...iterator_to_array($this->deleteDisallowedNodesWhenChangingNodeType( + array_push($events, ...$this->deleteDisallowedNodesWhenChangingNodeType( $contentGraph, $nodeAggregate, $newNodeType, $alreadyRemovedNodeAggregateIds, - ))); - array_push($events, ...iterator_to_array($this->deleteObsoleteTetheredNodesWhenChangingNodeType( + )); + array_push($events, ...$this->deleteObsoleteTetheredNodesWhenChangingNodeType( $contentGraph, $nodeAggregate, $newNodeType, $alreadyRemovedNodeAggregateIds - ))); + )); } // handle (missing) tethered node aggregates @@ -254,7 +258,7 @@ private function handleChangeNodeAggregateType( foreach ($newNodeType->tetheredNodeTypeDefinitions as $tetheredNodeTypeDefinition) { $tetheredNodeAggregate = $contentGraph->findChildNodeAggregateByName($nodeAggregate->nodeAggregateId, $tetheredNodeTypeDefinition->name); if ($tetheredNodeAggregate === null) { - $events = array_merge($events, iterator_to_array($this->createEventsForMissingTetheredNodeAggregate( + $events = array_merge($events, $this->createEventsForMissingTetheredNodeAggregate( $contentGraph, $tetheredNodeTypeDefinition, $nodeAggregate->occupiedDimensionSpacePoints, @@ -263,7 +267,7 @@ private function handleChangeNodeAggregateType( $succeedingSiblingIds[$tetheredNodeTypeDefinition->nodeTypeName->value] ?? null, $command->tetheredDescendantNodeAggregateIds, NodePath::fromNodeNames($tetheredNodeTypeDefinition->name) - ))); + )); } elseif (!$tetheredNodeAggregate->nodeTypeName->equals($tetheredNodeTypeDefinition->nodeTypeName)) { $events = array_merge($events, iterator_to_array($this->createEventsForWronglyTypedNodeAggregate( $contentGraph, diff --git a/Neos.ContentRepository.Core/Classes/Feature/RebaseableCommand.php b/Neos.ContentRepository.Core/Classes/Feature/RebaseableCommand.php index b62d85eddd8..2ea502274f3 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/RebaseableCommand.php +++ b/Neos.ContentRepository.Core/Classes/Feature/RebaseableCommand.php @@ -66,7 +66,7 @@ public static function enrichWithCommand( $processedEvents = []; $causationId = null; $i = 0; - foreach ($events as $event) { + foreach ($events->items as $event) { if ($event instanceof DecoratedEvent) { $undecoratedEvent = $event->innerEvent; if (!$undecoratedEvent instanceof PublishableToWorkspaceInterface) { diff --git a/Neos.ContentRepository.Core/Classes/Feature/RootNodeCreation/RootNodeHandling.php b/Neos.ContentRepository.Core/Classes/Feature/RootNodeCreation/RootNodeHandling.php index 1ed9b90f862..8c131e00a00 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/RootNodeCreation/RootNodeHandling.php +++ b/Neos.ContentRepository.Core/Classes/Feature/RootNodeCreation/RootNodeHandling.php @@ -17,6 +17,7 @@ use Neos\ContentRepository\Core\CommandHandler\CommandHandlingDependencies; use Neos\ContentRepository\Core\DimensionSpace\DimensionSpacePointSet; use Neos\ContentRepository\Core\DimensionSpace\OriginDimensionSpacePoint; +use Neos\ContentRepository\Core\EventStore\EventInterface; use Neos\ContentRepository\Core\EventStore\Events; use Neos\ContentRepository\Core\EventStore\EventsToPublish; use Neos\ContentRepository\Core\Feature\Common\InterdimensionalSiblings; @@ -104,7 +105,7 @@ private function handleCreateRootNodeAggregateWithNode( ]; foreach ($this->getInterDimensionalVariationGraph()->getRootGeneralizations() as $rootGeneralization) { - array_push($events, ...iterator_to_array($this->handleTetheredRootChildNodes( + array_push($events, ...$this->handleTetheredRootChildNodes( $contentGraph->getWorkspaceName(), $contentGraph->getContentStreamId(), $nodeType, @@ -113,7 +114,7 @@ private function handleCreateRootNodeAggregateWithNode( $command->nodeAggregateId, $command->tetheredDescendantNodeAggregateIds, null - ))); + )); } $contentStreamEventStream = ContentStreamEventStreamName::fromContentStreamId($contentGraph->getContentStreamId()); @@ -185,6 +186,7 @@ private function handleUpdateRootNodeAggregateDimensions( /** * @throws ContentStreamDoesNotExistYet * @throws NodeTypeNotFound + * @return array */ private function handleTetheredRootChildNodes( WorkspaceName $workspaceName, @@ -195,7 +197,7 @@ private function handleTetheredRootChildNodes( NodeAggregateId $parentNodeAggregateId, NodeAggregateIdsByNodePaths $nodeAggregateIdsByNodePath, ?NodePath $nodePath - ): Events { + ): array { $events = []; foreach ($nodeType->tetheredNodeTypeDefinitions as $tetheredNodeTypeDefinition) { $childNodeType = $this->requireNodeType($tetheredNodeTypeDefinition->nodeTypeName); @@ -218,7 +220,7 @@ private function handleTetheredRootChildNodes( $initialPropertyValues ); - array_push($events, ...iterator_to_array($this->handleTetheredRootChildNodes( + array_push($events, ...$this->handleTetheredRootChildNodes( $workspaceName, $contentStreamId, $childNodeType, @@ -227,10 +229,10 @@ private function handleTetheredRootChildNodes( $childNodeAggregateId, $nodeAggregateIdsByNodePath, $childNodePath - ))); + )); } - return Events::fromArray($events); + return $events; } private function createTetheredWithNodeForRoot( From 5e31760804d9df6f28613a1752ce35e8bbdf53ed Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Mon, 13 Jan 2025 20:45:58 +0100 Subject: [PATCH 4/9] BUGFIX: Prevent commit error for structure adjustment TETHERED_NODE_WRONGLY_ORDERED EventsToPublish must not contain events with no entries otherwise this error is thrown > Writable events must contain at least one event we prevent that by ensuring reorderNodes is called with at least two entries of nodes to be reordered as otherwise there is nothing to do. --- .../src/Adjustment/TetheredNodeAdjustments.php | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/Neos.ContentRepository.StructureAdjustment/src/Adjustment/TetheredNodeAdjustments.php b/Neos.ContentRepository.StructureAdjustment/src/Adjustment/TetheredNodeAdjustments.php index 9d985cc2399..24708ebe36b 100644 --- a/Neos.ContentRepository.StructureAdjustment/src/Adjustment/TetheredNodeAdjustments.php +++ b/Neos.ContentRepository.StructureAdjustment/src/Adjustment/TetheredNodeAdjustments.php @@ -141,7 +141,9 @@ function () use ($tetheredNodeAggregate) { } } - if (array_keys($actualTetheredChildNodes) !== array_keys($nodeType->tetheredNodeTypeDefinitions->toArray())) { + $expectedTetheredNodeOrder = array_keys($nodeType->tetheredNodeTypeDefinitions->toArray()); + if (count($expectedTetheredNodeOrder) > 1 && array_keys($actualTetheredChildNodes) !== $expectedTetheredNodeOrder) { + /** @var array,string> $expectedTetheredNodeOrder */ // we need to re-order: We go from the last to the first yield StructureAdjustment::createForNodeIdentity( $nodeAggregate->workspaceName, @@ -149,7 +151,7 @@ function () use ($tetheredNodeAggregate) { $nodeAggregate->nodeAggregateId, StructureAdjustment::TETHERED_NODE_WRONGLY_ORDERED, 'Tethered nodes wrongly ordered, expected: ' - . implode(', ', array_keys($nodeType->tetheredNodeTypeDefinitions->toArray())) + . implode(', ', $expectedTetheredNodeOrder) . ' - actual: ' . implode(', ', array_keys($actualTetheredChildNodes)), fn () => $this->reorderNodes( @@ -157,7 +159,7 @@ function () use ($tetheredNodeAggregate) { $this->contentGraph->getContentStreamId(), $nodeAggregate->getCoverageByOccupant($originDimensionSpacePoint), $actualTetheredChildNodes, - array_keys($nodeType->tetheredNodeTypeDefinitions->toArray()) + $expectedTetheredNodeOrder ) ); } @@ -216,7 +218,7 @@ protected function getPropertyConverter(): PropertyConverter * array key: name of tethered child node. Value: the Node itself. * @param array $actualTetheredChildNodes * an array depicting the expected tethered order, like ["node1", "node2"] - * @param array $expectedNodeOrdering + * @param array,string> $expectedNodeOrdering */ private function reorderNodes( WorkspaceName $workspaceName, @@ -254,6 +256,7 @@ private function reorderNodes( $succeedingSiblingNodeName = $nodeNameToMove; } + /** @var non-empty-array $events */ $streamName = ContentStreamEventStreamName::fromContentStreamId($contentStreamId); return new EventsToPublish( $streamName->getEventStreamName(), From 86a7d0495ab60990ed26b5ce3ad6b7cf9a783d73 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Mon, 13 Jan 2025 20:50:07 +0100 Subject: [PATCH 5/9] BUGFIX: BUGFIX: Asset that `SetNodeReferences` is not handled empty --- .../Feature/NodeReferencing/Command/SetNodeReferences.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Neos.ContentRepository.Core/Classes/Feature/NodeReferencing/Command/SetNodeReferences.php b/Neos.ContentRepository.Core/Classes/Feature/NodeReferencing/Command/SetNodeReferences.php index 30424548e8f..6ac9fe5f613 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/NodeReferencing/Command/SetNodeReferences.php +++ b/Neos.ContentRepository.Core/Classes/Feature/NodeReferencing/Command/SetNodeReferences.php @@ -36,6 +36,9 @@ private function __construct( public OriginDimensionSpacePoint $sourceOriginDimensionSpacePoint, public NodeReferencesToWrite $references, ) { + if ($this->references->isEmpty()) { + throw new \InvalidArgumentException(sprintf('The command "SetNodeReferences" for node %s must contain references to modify', $this->sourceNodeAggregateId->value), 1736797678); + } } /** From f5b3fd6fa7f0c6b3c9aa716a0e0cf0b3a670605e Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Mon, 13 Jan 2025 20:55:17 +0100 Subject: [PATCH 6/9] TASK: Add dummy typeguard to prevent phpstan from complaining it does not understand that the command fields are never empty ... but we ensure that --- .../Classes/Feature/NodeModification/NodeModification.php | 5 +++++ .../Classes/Feature/NodeReferencing/NodeReferencing.php | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/Neos.ContentRepository.Core/Classes/Feature/NodeModification/NodeModification.php b/Neos.ContentRepository.Core/Classes/Feature/NodeModification/NodeModification.php index 7608fef3c50..334491930e5 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/NodeModification/NodeModification.php +++ b/Neos.ContentRepository.Core/Classes/Feature/NodeModification/NodeModification.php @@ -129,6 +129,11 @@ private function handleSetSerializedNodeProperties( $events = $this->mergeSplitEvents($events); + if ($events === []) { + // cannot happen here as the command could not be instantiated without any intention see constructor validation + throw new \RuntimeException('Cannot handle "SetSerializedNodeProperties" with no properties to modify', 1736798016); + } + return new EventsToPublish( ContentStreamEventStreamName::fromContentStreamId($contentGraph->getContentStreamId()) ->getEventStreamName(), diff --git a/Neos.ContentRepository.Core/Classes/Feature/NodeReferencing/NodeReferencing.php b/Neos.ContentRepository.Core/Classes/Feature/NodeReferencing/NodeReferencing.php index cbfb71679d5..5bee0701006 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/NodeReferencing/NodeReferencing.php +++ b/Neos.ContentRepository.Core/Classes/Feature/NodeReferencing/NodeReferencing.php @@ -148,6 +148,11 @@ private function handleSetSerializedNodeReferences( ); } + if ($events === []) { + // cannot happen here as the command could not be instantiated without any intention see constructor validation + throw new \RuntimeException('Cannot handle "SetSerializedNodeReferences" with no references to modify', 1736797975); + } + $events = Events::fromArray($events); return new EventsToPublish( From 35f58a4bcfafcde534d12738bf45985ac95b69f8 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Mon, 13 Jan 2025 21:04:34 +0100 Subject: [PATCH 7/9] TASK: Document why Events does not exist empty --- .../Classes/EventStore/Events.php | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/Neos.ContentRepository.Core/Classes/EventStore/Events.php b/Neos.ContentRepository.Core/Classes/EventStore/Events.php index 2401685f992..622f625cac3 100644 --- a/Neos.ContentRepository.Core/Classes/EventStore/Events.php +++ b/Neos.ContentRepository.Core/Classes/EventStore/Events.php @@ -4,8 +4,18 @@ namespace Neos\ContentRepository\Core\EventStore; +use Neos\EventStore\EventStoreInterface; + /** - * A set of Content Repository "domain events" + * A set of Content Repository "domain events", part of {@see EventsToPublish} + * + * For better type checking we ensure that this collection is never empty. + * That is because {@see EventStoreInterface::commit()} will throw an exception if there are 0 events passed: + * + * > Writable events must contain at least one event + * + * We do not skip the case for 0 events to ensure each command always maps to a mutation. + * Forgiving noop behaviour is not intended for this low level code. * * @implements \IteratorAggregate * @internal only used during event publishing (from within command handlers) - and their implementation is not API From 6fa883cae80b33ece9d1373782274bd76cede75d Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Mon, 13 Jan 2025 21:17:57 +0100 Subject: [PATCH 8/9] TASK: Ensure that publishing does not attempt to publish 0 events The event store would reject that: > Writable events must contain at least one event --- .../Feature/WorkspaceCommandHandler.php | 75 ++++++++++--------- 1 file changed, 39 insertions(+), 36 deletions(-) diff --git a/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php b/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php index abb65c4c824..9b3c1467510 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php +++ b/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php @@ -232,24 +232,26 @@ static function ($handle) use ($rebaseableCommands): void { $commandSimulator->eventStream(), ); - try { - yield new EventsToPublish( - ContentStreamEventStreamName::fromContentStreamId($baseWorkspace->currentContentStreamId) - ->getEventStreamName(), - $eventsOfWorkspaceToPublish, - ExpectedVersion::fromVersion($baseWorkspaceContentStreamVersion) - ); - } catch (ConcurrencyException $concurrencyException) { - yield $this->reopenContentStreamWithoutConstraintChecks( - $workspace->currentContentStreamId - ); - throw $concurrencyException; + if ($eventsOfWorkspaceToPublish !== null) { + try { + yield new EventsToPublish( + ContentStreamEventStreamName::fromContentStreamId($baseWorkspace->currentContentStreamId) + ->getEventStreamName(), + $eventsOfWorkspaceToPublish, + ExpectedVersion::fromVersion($baseWorkspaceContentStreamVersion) + ); + } catch (ConcurrencyException $concurrencyException) { + yield $this->reopenContentStreamWithoutConstraintChecks( + $workspace->currentContentStreamId + ); + throw $concurrencyException; + } } yield $this->forkContentStream( $command->newContentStreamId, $baseWorkspace->currentContentStreamId, - Version::fromInteger($baseWorkspaceContentStreamVersion->value + $eventsOfWorkspaceToPublish->count()) + Version::fromInteger($baseWorkspaceContentStreamVersion->value + ($eventsOfWorkspaceToPublish?->count() ?? 0)) ); yield new EventsToPublish( @@ -303,7 +305,7 @@ private function getCopiedEventsOfEventStream( WorkspaceName $targetWorkspaceName, ContentStreamId $targetContentStreamId, EventStreamInterface $eventStream - ): Events { + ): Events|null { $events = []; foreach ($eventStream as $eventEnvelope) { $event = $this->eventNormalizer->denormalize($eventEnvelope->event); @@ -316,7 +318,8 @@ private function getCopiedEventsOfEventStream( } } - return Events::fromArray($events); + // this could technically empty, but we handle it as a no-op + return $events !== [] ? Events::fromArray($events) : null; } /** @@ -485,31 +488,32 @@ static function ($handle) use ($commandSimulator, $matchingCommands, $remainingC }; } - // this could empty and a no-op for the rare case when a command returns empty events e.g. the node was already tagged with this subtree tag $selectedEventsOfWorkspaceToPublish = $this->getCopiedEventsOfEventStream( $baseWorkspace->workspaceName, $baseWorkspace->currentContentStreamId, $commandSimulator->eventStream()->withMaximumSequenceNumber($highestSequenceNumberForMatching), ); - try { - yield new EventsToPublish( - ContentStreamEventStreamName::fromContentStreamId($baseWorkspace->currentContentStreamId) - ->getEventStreamName(), - $selectedEventsOfWorkspaceToPublish, - ExpectedVersion::fromVersion($baseWorkspaceContentStreamVersion) - ); - } catch (ConcurrencyException $concurrencyException) { - yield $this->reopenContentStreamWithoutConstraintChecks( - $workspace->currentContentStreamId - ); - throw $concurrencyException; + if ($selectedEventsOfWorkspaceToPublish !== null) { + try { + yield new EventsToPublish( + ContentStreamEventStreamName::fromContentStreamId($baseWorkspace->currentContentStreamId) + ->getEventStreamName(), + $selectedEventsOfWorkspaceToPublish, + ExpectedVersion::fromVersion($baseWorkspaceContentStreamVersion) + ); + } catch (ConcurrencyException $concurrencyException) { + yield $this->reopenContentStreamWithoutConstraintChecks( + $workspace->currentContentStreamId + ); + throw $concurrencyException; + } } yield from $this->forkNewContentStreamAndApplyEvents( $command->contentStreamIdForRemainingPart, $baseWorkspace->currentContentStreamId, - Version::fromInteger($baseWorkspaceContentStreamVersion->value + $selectedEventsOfWorkspaceToPublish->count()), + Version::fromInteger($baseWorkspaceContentStreamVersion->value + ($selectedEventsOfWorkspaceToPublish?->count() ?? 0)), new EventsToPublish( WorkspaceEventStreamName::fromWorkspaceName($command->workspaceName)->getEventStreamName(), Events::fromArray([ @@ -780,7 +784,7 @@ private function forkNewContentStreamAndApplyEvents( ContentStreamId $sourceContentStreamId, Version $sourceContentStreamVersion, EventsToPublish $pointWorkspaceToNewContentStream, - Events $eventsToApplyOnNewContentStream, + Events|null $eventsToApplyOnNewContentStream, ): \Generator { yield $this->forkContentStream( $newContentStreamId, @@ -797,13 +801,12 @@ private function forkNewContentStreamAndApplyEvents( yield new EventsToPublish( ContentStreamEventStreamName::fromContentStreamId($newContentStreamId) ->getEventStreamName(), - $eventsToApplyOnNewContentStream->withAppendedEvents( - Events::with( - new ContentStreamWasReopened( - $newContentStreamId - ) + Events::fromArray([ + ...($eventsToApplyOnNewContentStream ?? []), + new ContentStreamWasReopened( + $newContentStreamId ) - ), + ]), ExpectedVersion::fromVersion(Version::first()->next()) ); } From 31e374513fc4e830066ad40db2bc79a635185907 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Mon, 13 Jan 2025 21:25:23 +0100 Subject: [PATCH 9/9] TASK: Deobscure use of array_keys with TetheredNodeTypeDefinitions::toArray --- .../NodeType/TetheredNodeTypeDefinitions.php | 16 +++++++++------- .../src/Adjustment/TetheredNodeAdjustments.php | 3 ++- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/Neos.ContentRepository.Core/Classes/NodeType/TetheredNodeTypeDefinitions.php b/Neos.ContentRepository.Core/Classes/NodeType/TetheredNodeTypeDefinitions.php index 17ba4edfc28..c259b359022 100644 --- a/Neos.ContentRepository.Core/Classes/NodeType/TetheredNodeTypeDefinitions.php +++ b/Neos.ContentRepository.Core/Classes/NodeType/TetheredNodeTypeDefinitions.php @@ -63,16 +63,18 @@ public function get(NodeName|string $nodeName): ?TetheredNodeTypeDefinition return $this->tetheredNodeTypeDefinitions[$nodeName] ?? null; } - public function getIterator(): \Traversable + /** + * @template T + * @param \Closure(TetheredNodeTypeDefinition): T $callback + * @return list + */ + public function map(\Closure $callback): array { - yield from $this->tetheredNodeTypeDefinitions; + return array_map($callback, array_values($this->tetheredNodeTypeDefinitions)); } - /** - * @return array - */ - public function toArray(): array + public function getIterator(): \Traversable { - return $this->tetheredNodeTypeDefinitions; + yield from $this->tetheredNodeTypeDefinitions; } } diff --git a/Neos.ContentRepository.StructureAdjustment/src/Adjustment/TetheredNodeAdjustments.php b/Neos.ContentRepository.StructureAdjustment/src/Adjustment/TetheredNodeAdjustments.php index 24708ebe36b..896cfcdd349 100644 --- a/Neos.ContentRepository.StructureAdjustment/src/Adjustment/TetheredNodeAdjustments.php +++ b/Neos.ContentRepository.StructureAdjustment/src/Adjustment/TetheredNodeAdjustments.php @@ -19,6 +19,7 @@ use Neos\ContentRepository\Core\NodeType\NodeType; use Neos\ContentRepository\Core\NodeType\NodeTypeManager; use Neos\ContentRepository\Core\NodeType\NodeTypeName; +use Neos\ContentRepository\Core\NodeType\TetheredNodeTypeDefinition; use Neos\ContentRepository\Core\Projection\ContentGraph\ContentGraphInterface; use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\FindChildNodesFilter; use Neos\ContentRepository\Core\Projection\ContentGraph\Node; @@ -141,7 +142,7 @@ function () use ($tetheredNodeAggregate) { } } - $expectedTetheredNodeOrder = array_keys($nodeType->tetheredNodeTypeDefinitions->toArray()); + $expectedTetheredNodeOrder = $nodeType->tetheredNodeTypeDefinitions->map(fn (TetheredNodeTypeDefinition $definition) => $definition->name->value); if (count($expectedTetheredNodeOrder) > 1 && array_keys($actualTetheredChildNodes) !== $expectedTetheredNodeOrder) { /** @var array,string> $expectedTetheredNodeOrder */ // we need to re-order: We go from the last to the first