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

pKallert · 2024-10-23T07:41:38Z

As the Neos ui now uses options.moveNodeStrategy to determine whether to move all nodes or just on dimension: neos/neos-ui#3876
And we discussed that we DONT want to make that dependant on aggregate: true in the core like in 8.3

And the fact that aggregate is now fully unused and its description and docs are outdated we removed the unused aggregate flag on NodeTypes.
It was originally introduced via defa572
We decided against using the aggregate flag to determine scatter or gather node move behaviour like in Neos 8 because the naming aggregate might be confusing now.

Review instructions

Checklist

Code follows the PSR-2 coding style
Tests have been created, run and adjusted as needed
The PR is created against the lowest maintained branch
Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
Reviewer - The first section explains the change briefly for change-logs
Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

skurfuerst

nice, only two nitpicks :)

Neos.ContentRepository.Core/Classes/Feature/NodeMove/Command/MoveNodeAggregate.php

Neos.ContentRepository.Core/Classes/Feature/NodeMove/Dto/RelationDistributionStrategy.php

Co-authored-by: Sebastian Kurfürst <[email protected]>

…com/pKallert/neos-development-collection into feature/nodeMove-strategy-by-config

mhsdesign · 2024-10-27T18:21:13Z

Huuch i remember having created an issue for this once neos/neos-ui#3587 and funnily there was even a pr from Bernhard both for neos and the ui #4994 but that seems to be superseded then ...

bwaidelich · 2024-11-01T10:29:15Z

In order to mimic the behavior of 9.0 we decided to introduce a setting for move strategies on NodeType level.

is that correct? I'm confused as to what this PR is supposed to solve (by just reading the description)

pKallert · 2024-11-01T21:31:36Z

In order to mimic the behavior of 9.0 we decided to introduce a setting for move strategies on NodeType level.

is that correct? I'm confused as to what this PR is supposed to solve (by just reading the description)

Sorry, of course I meant 8.3 😄

This is for differentiating between document and content nodes when moving nodes.
For example: When moving a document node in the UI the node should be moved in all dimensions. When moving a content node (like in 8.3) the node should perhaps not be moved in all dimensions.
This is now a setting that can be set for each node type with the behavior from 8.3 set as the default.

We also talked about implementing a query window in the UI, but that could get annoying really fast especially if you move nodes very often. This can be further enhanced in future iterations but for now we landed on setting it in the node type.

There are still some edge cases that I have to have a look at and think about(when I get my local environment running again) to fix the failing test.
I updated the description accordingly and set the PR to draft since the tests are failing and my local env keeps giving errors

bwaidelich · 2024-11-01T23:05:58Z

Thanks a lot for the great explanation! 🤩

bwaidelich · 2024-11-12T08:46:19Z

Neos.ContentRepository.Core/Classes/Feature/NodeMove/Dto/RelationDistributionStrategy.php

+    {
+        if (str_starts_with($name, 'RelationDistributionStrategy::')) {
+            $name = substr($name, strpos($name, '::') + 2);
+            foreach (self::cases() as $status) {
+                if ($name === $status->name) {
+                    return $status;
+                }
+            }
+        }


Why this complex logic, every backed enum has a built-in from and tryFrom

In this case, we want to get the value from key.
Enum ->from gets the key from a value, as far as I understand. So I could pass gatherSpecializations and would get RelationDistributionStrategy::STRATEGY_GATHER_SPECIALIZATIONS.
Here, we have RelationDistributionStrategy::STRATEGY_SCATTER
Basically we need to cast the string RelationDistributionStrategy::STRATEGY_GATHER_ALL into an actual enum. Not sure if there is a way to do it that is not ugly?

I just realized that we use a different value for name and value.
btw that's IMO a limitation of PHP enums, it would make things much easier if enums without backed type would just serialize to string and have (try)from to avoid that confusion.
But, anyways, when using backed enums we should probably always refer to the value instead of the key when (de)serializing the enum

…gy scatter

bwaidelich · 2024-11-13T14:50:47Z

Neos.Neos/NodeTypes/Mixin/Content.yaml

@@ -3,6 +3,8 @@
    'Neos.Neos:Node': true
    'Neos.Neos:Hidable': true
  abstract: true
+  strategy:
+    moveNode: RelationDistributionStrategy::STRATEGY_SCATTER


I think that's the root cause. Why not

Suggested change

moveNode: RelationDistributionStrategy::STRATEGY_SCATTER

moveNode: scatter

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

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 :)

bwaidelich · 2024-11-13T14:51:02Z

Neos.Neos/NodeTypes/Mixin/Document.yaml

@@ -7,6 +7,8 @@
    'Neos.Neos:Hidable': true
  abstract: true
  aggregate: true
+  strategy:
+    moveNode: RelationDistributionStrategy::STRATEGY_GATHER_ALL


..and

Suggested change

moveNode: RelationDistributionStrategy::STRATEGY_GATHER_ALL

moveNode: gatherAll

bwaidelich · 2024-11-13T14:53:42Z

Neos.ContentRepository.Core/Classes/Feature/NodeMove/Dto/RelationDistributionStrategy.php

+    {
+        if (str_starts_with($name, 'RelationDistributionStrategy::')) {
+            $name = substr($name, strpos($name, '::') + 2);
+            foreach (self::cases() as $status) {
+                if ($name === $status->name) {
+                    return $status;
+                }
+            }
+        }


I just realized that we use a different value for name and value.
btw that's IMO a limitation of PHP enums, it would make things much easier if enums without backed type would just serialize to string and have (try)from to avoid that confusion.
But, anyways, when using backed enums we should probably always refer to the value instead of the key when (de)serializing the enum

bwaidelich · 2024-11-13T15:25:30Z

Neos.ContentRepository.Core/Classes/Feature/NodeMove/Dto/RelationDistributionStrategy.php

+                }
+            }
+        }
+        return self::STRATEGY_GATHER_ALL;


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

Neos.ContentRepositoryRegistry/Resources/Private/Schema/NodeTypes.schema.yaml

mhsdesign · 2024-11-14T15:44:02Z

Neos.Neos/NodeTypes/Mixin/Document.yaml

@@ -7,6 +7,8 @@
    'Neos.Neos:Hidable': true
  abstract: true
  aggregate: true


aggregate is dead and can be removed :)

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

mhsdesign · 2024-11-14T15:45:01Z

Neos.Neos/NodeTypes/Mixin/Document.yaml

@@ -7,6 +7,8 @@
    'Neos.Neos:Hidable': true
  abstract: true
  aggregate: true
+  strategy:
+    moveNode: RelationDistributionStrategy::STRATEGY_GATHER_ALL


Suggested change

moveNode: RelationDistributionStrategy::STRATEGY_GATHER_ALL

moveNode

could be relationDistribution

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

pKallert · 2024-11-15T07:45:41Z

To summarize our discussion from yesterday:

The Strategy will be set in the Neos UI depeding on NodeType when a node is moved.
The setting will be moved to options, not in "strategy"
Since the options are not as important, I can just chose the naming however I want to.
"aggregate" will be removed

mhsdesign · 2024-11-19T10:11:15Z

Thanks for your adjustments, i also removed the aggregate=true part from the documentation and the NodeType class.
And as the Neos UI now evaluates alone options.moveNodeStrategy i moved the configuration to the Neos Ui package. (and we now use gatherAll instead of the ENUM name) :)

mhsdesign · 2024-11-27T08:10:27Z

@kitsunet you said you want to test this? It would be really nice, otherwise ill experiment with it.

Feature: Add nodeMove strategy configuration to nodetypes

b4306d2

github-actions bot added Feature 9.0 labels Oct 23, 2024

Feature: linting

182808a

skurfuerst reviewed Oct 23, 2024

View reviewed changes

Neos.ContentRepository.Core/Classes/Feature/NodeMove/Command/MoveNodeAggregate.php Outdated Show resolved Hide resolved

Neos.ContentRepository.Core/Classes/Feature/NodeMove/Dto/RelationDistributionStrategy.php Outdated Show resolved Hide resolved

pKallert and others added 3 commits October 23, 2024 11:16

Fjx

26da299

Co-authored-by: Sebastian Kurfürst <[email protected]>

Feature: check if setting has right class

b63f93b

Merge branch 'feature/nodeMove-strategy-by-config' of https://github.…

ad80902

…com/pKallert/neos-development-collection into feature/nodeMove-strategy-by-config

pKallert mentioned this pull request Oct 23, 2024

FEATURE: Add options.moveNodeStrategy configuration to NodeTypes neos/neos-ui#3876

Merged

pKallert added 7 commits October 23, 2024 11:27

Fix: make relationDistributionStrategy nullable

3529326

Feature: Allow nullable nodetype

dd62862

Fix: make relationDistributionStrategy nullable

43d4d85

Feature: linting

4096cf9

Feature: Adjust NodeTypesSchema

6f11943

Feature: Adjust Node Schema

b4b0192

Merge branch '9.0' into feature/nodeMove-strategy-by-config

d994cde

pKallert changed the title ~~FEATURE: Add nodeMove strategy configuration to nodetypes~~ Draft: FEATURE: Add nodeMove strategy configuration to nodetypes Nov 1, 2024

bwaidelich reviewed Nov 12, 2024

View reviewed changes

mhsdesign mentioned this pull request Nov 12, 2024

BUG: Cut & paste in different dimension leads to unpublishable change #4614

Closed

1 task

pKallert added 2 commits November 13, 2024 15:28

Merge branch '9.0' into feature/nodeMove-strategy-by-config

2d8f48e

Fix: require NodeAggregateToCoverDimensionSpacePoints also for strate…

b3c708e

…gy scatter

bwaidelich reviewed Nov 13, 2024

View reviewed changes

bwaidelich reviewed Nov 14, 2024

View reviewed changes

Neos.ContentRepositoryRegistry/Resources/Private/Schema/NodeTypes.schema.yaml Outdated Show resolved Hide resolved

mhsdesign reviewed Nov 14, 2024

View reviewed changes

pKallert added 3 commits November 15, 2024 07:26

Feature: Remove NodeMove logic from Neos.Neos

335bc33

Fix: Undo more changes in Core

74dab5b

Merge branch '9.0' into feature/nodeMove-strategy-by-config

98aac52

mhsdesign mentioned this pull request Nov 15, 2024

BUGFIX: #4614 NodeAggregateWasMoved change not marked as changed in correct dimension #5369

Closed

6 tasks

mhsdesign added 2 commits November 19, 2024 10:46

TASK: Move moveNodeStrategy to Neos UI

acc07c8

TASK: Fully remove aggregate=true

56953bd

mhsdesign changed the title ~~Draft: FEATURE: Add nodeMove strategy configuration to nodetypes~~ FEATURE: Add nodeMove strategy configuration to nodetypes Nov 19, 2024

mhsdesign approved these changes Nov 22, 2024

View reviewed changes

mhsdesign requested a review from kitsunet November 27, 2024 08:09

kitsunet approved these changes Nov 27, 2024

View reviewed changes

kitsunet merged commit 8ebf99c into neos:9.0 Nov 27, 2024
12 checks passed

mhsdesign changed the title ~~FEATURE: Add nodeMove strategy configuration to nodetypes~~ TASK: Remove obsolete aggregate as Neos Ui handles nodeMove strategy now via special configuration Dec 1, 2024

github-actions bot added the Task label Dec 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pKallert commented Oct 23, 2024 •

edited by mhsdesign

Loading

skurfuerst left a comment

mhsdesign commented Oct 27, 2024

bwaidelich commented Nov 1, 2024

pKallert commented Nov 1, 2024

bwaidelich commented Nov 1, 2024

bwaidelich Nov 12, 2024

pKallert Nov 13, 2024

bwaidelich Nov 13, 2024

bwaidelich Nov 13, 2024 •

edited

Loading

pKallert Nov 13, 2024

bwaidelich Nov 13, 2024

bwaidelich Nov 13, 2024

bwaidelich Nov 13, 2024

bwaidelich Nov 13, 2024

mhsdesign Nov 14, 2024

mhsdesign Nov 14, 2024

mhsdesign Nov 14, 2024

pKallert commented Nov 15, 2024

mhsdesign commented Nov 19, 2024

mhsdesign commented Nov 27, 2024

	moveNode: RelationDistributionStrategy::STRATEGY_SCATTER
	moveNode: scatter

	moveNode: RelationDistributionStrategy::STRATEGY_GATHER_ALL
	moveNode: gatherAll

	moveNode: RelationDistributionStrategy::STRATEGY_GATHER_ALL
	moveNode

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

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

Conversation

pKallert commented Oct 23, 2024 • edited by mhsdesign Loading

skurfuerst left a comment

Choose a reason for hiding this comment

mhsdesign commented Oct 27, 2024

bwaidelich commented Nov 1, 2024

pKallert commented Nov 1, 2024

bwaidelich commented Nov 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwaidelich Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pKallert commented Nov 15, 2024

mhsdesign commented Nov 19, 2024

mhsdesign commented Nov 27, 2024

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

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

pKallert commented Oct 23, 2024 •

edited by mhsdesign

Loading

bwaidelich Nov 13, 2024 •

edited

Loading