Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TASK: Remove obsolete aggregate as Neos Ui handles nodeMove strategy now via special configuration #5314

Merged
merged 19 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ private function __construct(
public WorkspaceName $workspaceName,
public DimensionSpacePoint $dimensionSpacePoint,
public NodeAggregateId $nodeAggregateId,
public RelationDistributionStrategy $relationDistributionStrategy,
public ?RelationDistributionStrategy $relationDistributionStrategy,
public ?NodeAggregateId $newParentNodeAggregateId,
public ?NodeAggregateId $newPrecedingSiblingNodeAggregateId,
public ?NodeAggregateId $newSucceedingSiblingNodeAggregateId,
Expand All @@ -72,12 +72,12 @@ private function __construct(
* @param WorkspaceName $workspaceName The workspace in which the move operation is to be performed
* @param DimensionSpacePoint $dimensionSpacePoint This is one of the *covered* dimension space points of the node aggregate and not necessarily one of the occupied ones. This allows us to move virtual specializations only when using the scatter strategy
* @param NodeAggregateId $nodeAggregateId The id of the node aggregate to move
* @param RelationDistributionStrategy $relationDistributionStrategy The relation distribution strategy to be used ({@see RelationDistributionStrategy})
* @param RelationDistributionStrategy|null $relationDistributionStrategy The relation distribution strategy to be used ({@see RelationDistributionStrategy})
pKallert marked this conversation as resolved.
Show resolved Hide resolved
* @param NodeAggregateId|null $newParentNodeAggregateId The id of the new parent node aggregate. If given, it enforces that all nodes in the given aggregate are moved into nodes of the parent aggregate, even if the given siblings belong to other parents. In latter case, those siblings are ignored
* @param NodeAggregateId|null $newPrecedingSiblingNodeAggregateId The id of the new preceding sibling node aggregate. If given and no successor found, it is attempted to insert the moved nodes right after nodes of this aggregate. In dimension space points this aggregate does not cover, other siblings, in order of proximity, are tried to be used instead
* @param NodeAggregateId|null $newSucceedingSiblingNodeAggregateId The id of the new succeeding sibling node aggregate. If given, it is attempted to insert the moved nodes right before nodes of this aggregate. In dimension space points this aggregate does not cover, the preceding sibling is tried to be used instead
*/
public static function create(WorkspaceName $workspaceName, DimensionSpacePoint $dimensionSpacePoint, NodeAggregateId $nodeAggregateId, RelationDistributionStrategy $relationDistributionStrategy, ?NodeAggregateId $newParentNodeAggregateId = null, ?NodeAggregateId $newPrecedingSiblingNodeAggregateId = null, ?NodeAggregateId $newSucceedingSiblingNodeAggregateId = null): self
public static function create(WorkspaceName $workspaceName, DimensionSpacePoint $dimensionSpacePoint, NodeAggregateId $nodeAggregateId, ?RelationDistributionStrategy $relationDistributionStrategy = null, ?NodeAggregateId $newParentNodeAggregateId = null, ?NodeAggregateId $newPrecedingSiblingNodeAggregateId = null, ?NodeAggregateId $newSucceedingSiblingNodeAggregateId = null): self
{
return new self($workspaceName, $dimensionSpacePoint, $nodeAggregateId, $relationDistributionStrategy, $newParentNodeAggregateId, $newPrecedingSiblingNodeAggregateId, $newSucceedingSiblingNodeAggregateId);
}
Expand All @@ -91,7 +91,9 @@ public static function fromArray(array $array): self
WorkspaceName::fromString($array['workspaceName']),
DimensionSpacePoint::fromArray($array['dimensionSpacePoint']),
NodeAggregateId::fromString($array['nodeAggregateId']),
RelationDistributionStrategy::fromString($array['relationDistributionStrategy']),
isset($array['relationDistributionStrategy'])
? RelationDistributionStrategy::fromString($array['relationDistributionStrategy'])
: null,
isset($array['newParentNodeAggregateId'])
? NodeAggregateId::fromString($array['newParentNodeAggregateId'])
: null,
Expand All @@ -118,6 +120,19 @@ public function matchesNodeId(NodeIdToPublishOrDiscard $nodeIdToPublish): bool
&& $this->dimensionSpacePoint === $nodeIdToPublish->dimensionSpacePoint;
}

public function withRelationDistributionStrategy(RelationDistributionStrategy $relationDistributionStrategy): self
{
return new self(
$this->workspaceName,
$this->dimensionSpacePoint,
$this->nodeAggregateId,
$relationDistributionStrategy,
$this->newParentNodeAggregateId,
$this->newPrecedingSiblingNodeAggregateId,
$this->newSucceedingSiblingNodeAggregateId
);
}

public function createCopyForWorkspace(
WorkspaceName $targetWorkspaceName,
): self {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,17 @@ public static function fromString(?string $serialization): self
: self::STRATEGY_GATHER_ALL;
}

public static function fromName(string $name): self
{
$name = substr($name, strpos($name, '::') + 2);
pKallert marked this conversation as resolved.
Show resolved Hide resolved
foreach (self::cases() as $status) {
if ($name === $status->name) {
return $status;
}
}
return self::STRATEGY_GATHER_ALL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, one more addition:
If we decide to keep the value based serialization we should at least fail if referring to an invalid value, rather than falling back to some strategy

}

public function jsonSerialize(): string
{
return $this->value;
Expand Down
17 changes: 12 additions & 5 deletions Neos.ContentRepository.Core/Classes/Feature/NodeMove/NodeMove.php
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,12 @@ private function handleMoveNodeAggregate(
$contentGraph,
$command->nodeAggregateId,
);

if ($command->relationDistributionStrategy === null) {
$nodeType = $this->nodeTypeManager->getNodeType($nodeAggregate->nodeTypeName);
$command = $command->withRelationDistributionStrategy($nodeType->getMoveNodeRelationDistributionStrategy());
}

$this->requireNodeAggregateToNotBeRoot($nodeAggregate);
$this->requireNodeAggregateToBeUntethered($nodeAggregate);
$this->requireNodeAggregateToCoverDimensionSpacePoint($nodeAggregate, $command->dimensionSpacePoint);
Expand Down Expand Up @@ -125,11 +131,12 @@ private function handleMoveNodeAggregate(
if ($nodeAggregate->nodeName) {
$this->requireNodeTypeNotToDeclareTetheredChildNodeName($newParentNodeAggregate->nodeTypeName, $nodeAggregate->nodeName);
}

$this->requireNodeAggregateToCoverDimensionSpacePoints(
$newParentNodeAggregate,
$affectedDimensionSpacePoints
);
if ($command->relationDistributionStrategy !== RelationDistributionStrategy::STRATEGY_SCATTER) {
$this->requireNodeAggregateToCoverDimensionSpacePoints(
$newParentNodeAggregate,
$affectedDimensionSpacePoints
);
}

$this->requireNodeAggregateToNotBeDescendant(
$contentGraph,
Expand Down
10 changes: 10 additions & 0 deletions Neos.ContentRepository.Core/Classes/NodeType/NodeType.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
* source code.
*/

use Neos\ContentRepository\Core\Feature\NodeMove\Dto\RelationDistributionStrategy;
use Neos\ContentRepository\Core\SharedModel\Exception\InvalidNodeTypePostprocessorException;
use Neos\ContentRepository\Core\SharedModel\Exception\NodeConfigurationException;
use Neos\ContentRepository\Core\SharedModel\Node\NodeName;
Expand Down Expand Up @@ -543,4 +544,13 @@ protected function setFullConfiguration(array $fullConfiguration): void
{
$this->fullConfiguration = $fullConfiguration;
}

/**
* Get the MoveNode strategy used when moving a node
*/
public function getMoveNodeRelationDistributionStrategy(): RelationDistributionStrategy
{
$strategy = $this->getConfiguration('strategy.moveNode') ?: 'RelationDistributionStrategy::STRATEGY_GATHER_ALL';
return RelationDistributionStrategy::fromName($strategy);
}
}
2 changes: 2 additions & 0 deletions Neos.Neos/NodeTypes/Mixin/Content.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
'Neos.Neos:Node': true
'Neos.Neos:Hidable': true
abstract: true
strategy:
moveNode: RelationDistributionStrategy::STRATEGY_SCATTER
Copy link
Member

@bwaidelich bwaidelich Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's the root cause. Why not

Suggested change
moveNode: RelationDistributionStrategy::STRATEGY_SCATTER
moveNode: scatter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This format is what we decided during the sprint when discussing the new setting. It is a lot clearer than only using the key

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is: If it is clearer, we should change the values of those enums to reflect this, i.e. change

case STRATEGY_SCATTER = 'scatter';

to

case STRATEGY_SCATTER = 'STRATEGY_SCATTER';

(like we did for many other enums exactly to avoid that confusion).

But without that change, having a different serialization format wether it's in the database (or a JSON representation) from the YAML representation is just error prone in my eyes.

Anyways, I don't want to block this PR of course – feel free to ignore me, just my 2ct :)

constraints:
nodeTypes:
'*': false
Expand Down
2 changes: 2 additions & 0 deletions Neos.Neos/NodeTypes/Mixin/Document.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
'Neos.Neos:Hidable': true
abstract: true
aggregate: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aggregate is dead and can be removed :)

this is no longer a supported concept. The relationDistributionStrategy must be used instead for default move behavior

strategy:
moveNode: RelationDistributionStrategy::STRATEGY_GATHER_ALL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

..and

Suggested change
moveNode: RelationDistributionStrategy::STRATEGY_GATHER_ALL
moveNode: gatherAll

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
moveNode: RelationDistributionStrategy::STRATEGY_GATHER_ALL
moveNode

could be relationDistribution

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to make this a little easier, if we put it to options

options:
  relationDistributionStrategy: gatherAll

and access it via Neos\ContentRepository\Core\NodeType\NodeType::getOptions

constraints:
nodeTypes:
'*': false
Expand Down
Loading