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

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

Conversation

mhsdesign
Copy link
Member

see #4614 (comment) for explanation

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

Comment on lines +217 to +220
// WORKAROUND: we simply use the event's DSP here as the origin dimension space point.
// But this DSP is not necessarily occupied.
// @todo properly handle this by storing the necessary information in the projection
// todo so we need to create a change if there is none????
Copy link
Member Author

Choose a reason for hiding this comment

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

this will start to bite us with #5314 in place as scatter will be started to be used:

But i wonder if that is not already handled via modifyChange? I guess a test could answer that ...
\Neos\Neos\PendingChangesProjection\ChangeProjection::modifyChange

Comment on lines +66 to +69
/**
* @var 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
*/
public DimensionSpacePoint $dimensionSpacePoint,
Copy link
Member Author

Choose a reason for hiding this comment

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

so that we need the originally attempted dimensionSpacePoint to be moved seems correct. But the question is, see #5360 if also the relationDistributionStrategy should be part of the event?

Whas is your opinion? @nezaniel

Copy link
Member

@kitsunet kitsunet Nov 30, 2024

Choose a reason for hiding this comment

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

I think it wouldn't hurt to have the strategy in here if only for potential debugging help....

@mhsdesign
Copy link
Member Author

With our new solution how to get publishing release ready: #5387
We went another way.

so instead of putting the info from the original command into the event well just mark this change as changed in all affected dimensions and then allow it to be published from all dimensions that are affected. No selective dimension publishing.

But the real issue is that the original $dimensionSpacePoint is NOT part of the event and must be handled by the change projection to make the correct node as changed: This is also further described here: #5360

@mhsdesign mhsdesign closed this Nov 30, 2024
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.

2 participants