Skip to content

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Mar 15, 2025

Remove possibly incorrect or undefined NodeAggregateWasRemoved::$affectedOccupiedDimensionSpacePoints

Originally 8bccb95 introduced $affectedOccupiedDimensionSpacePoints for the change projection

Now with soft removals we dont need to focus on NodeAggregateWasRemoved and could even ignore them. There is no need to difference between occupied and covered as soft removals cant do this either (#5507).
This change thus marks removal events in all affectedCoveredDimensionSpacePoints instead. They will not be shown in the workspace module either way and its more of a debug tool.

Previously removeNodeInDimensionSpacePointSet issued not necessarily occupied points and also when removing a root node aggregate it might be surprising to find out that $affectedOccupiedDimensionSpacePoints is empty because the root node doesnt occupy a special point.

To simplify the discussion for now we remove this now unused feature

Well have to revise the question for affected vs source dimension space points either way in a professional manner for other events, see #4265 (comment) but that is not for now.

Upgrade instructions

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

…ll affectedCoveredDimensionSpacePoints

With soft removals, actual removals are not desired and will not be shown in the workspace module.

Now it does not matter if we were to use the `affectedOccupiedDimensionSpacePoints` or `affectedCoveredDimensionSpacePoints`

Soft removals cannot make the distinction as well: neos#5507
…::$affectedOccupiedDimensionSpacePoints`

 Originally 8bccb95 introduced `$affectedOccupiedDimensionSpacePoints` for the change projection

 Now with soft removals we dont need to focus on `NodeAggregateWasRemoved` and could even ignore them. There is no need to difference between occupied and covered as soft removals cant do this either.

 Previously `removeNodeInDimensionSpacePointSet` issued not necessarily occupied points and also when removing a root node aggregate it might be surprising to find out that `$affectedOccupiedDimensionSpacePoints` is empty because the root node doesnt occupy a special point.

 To simplify the discussion for now we remove this now unused feature

 Well have to revise the question for affected vs source dimension space points either way in a professional manner for other events, see neos#4265 (comment) but that is not for now.
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Mar 15, 2025
Also mark empty dimensions as affected source, but that will be removed wither way with: neos#5516

because its wrong.
@mhsdesign mhsdesign changed the title TASK: Remove obolete affected source dsp from node aggregate was removed TASK: Remove obsolete affected source dsp from node aggregate was removed Mar 15, 2025
@mhsdesign
Copy link
Member Author

This is required for the fix of UpdateRootNodeAggregateDimensions in #5514 which removes the root node aggregate in dedicated dimensions.

At the current state of implementation i have to lie about the affected source:

// TODO root nodes never occupy, but this field is odd, unused and to be removed either way: https://github.com/neos/neos-development-collection/pull/5516
affectedOccupiedDimensionSpacePoints: OriginDimensionSpacePointSet::fromArray([]),

# todo remove this field, its wrong: https://github.com/neos/neos-development-collection/pull/5516
| affectedOccupiedDimensionSpacePoints | [] |
| affectedCoveredDimensionSpacePoints | [{"language":"de"},{"language":"gsw"}] |

We previously never ran into that problem because RemoveNodeAggregate did not allow to remove a root node in only a certain dimensions.

if ($nodeAggregate->classification->isRoot() && $command->nodeVariantSelectionStrategy !== NodeVariantSelectionStrategy::STRATEGY_ALL_VARIANTS) {
throw new \RuntimeException(sprintf('Root node aggregates can only be removed by using node variant selection strategy "%s"', NodeVariantSelectionStrategy::STRATEGY_ALL_VARIANTS->value), 1740753598);
}

But as we write removal events in UpdateRootNodeAggregateDimensions we do now care.

Copy link
Member

@nezaniel nezaniel left a comment

Choose a reason for hiding this comment

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

Since the affected occupied DSPs are always a subset of the affected covered ones and projections et al must keep track of origins themselves anyway (if they care for them at all), we can (and should) safely remove this.

neos-bot pushed a commit to neos/contentrepository-core that referenced this pull request Mar 25, 2025
Also mark empty dimensions as affected source, but that will be removed wither way with: neos/neos-development-collection#5516

because its wrong.
@kitsunet kitsunet merged commit a3d104b into neos:9.0 Mar 26, 2025
14 checks passed
mhsdesign added a commit that referenced this pull request Mar 26, 2025
`affectedOccupiedDimensionSpacePoints` is no longer a parameter
mhsdesign added a commit that referenced this pull request Mar 26, 2025
`affectedOccupiedDimensionSpacePoints` is no longer a parameter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants