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: 4942 - refactor global transformation commands… #4976

Merged
merged 1 commit into from Apr 5, 2024

Conversation

nezaniel
Copy link
Member

@nezaniel nezaniel commented Apr 4, 2024

… to also work with workspaces

Resolves #4942

Upgrade instructions

I think the command structure migration has to be adjusted and re-run, @mhsdesign could you please take care of that?

Review instructions

the global transformations interface has been stripped of the content stream id, because both were implemented as commands that didn't need them. maybe we should keep it for non-command implementations?

Checklist

  • Code follows the PSR-12 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the 9.0 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

@nezaniel nezaniel marked this pull request as draft April 4, 2024 04:44
@nezaniel nezaniel requested a review from mhsdesign April 4, 2024 04:44
@mhsdesign
Copy link
Member

I think the command structure migration has to be adjusted and re-run

im nearly sure (because of our last conversation) that this is not the case, as they are currently always ever run on the live workspace which we dont rebase. So no need for a migration ;)

Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

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

Nice cleanup ❤️ looks good by reading, but maybe sebastian or bastian want to have a look at this as well ... idk if there is a rare case that a command MUST operate on a cs id directly ... but i cant think of a reason so thanks :)

@nezaniel nezaniel marked this pull request as ready for review April 4, 2024 10:03
@mhsdesign
Copy link
Member

@bwaidelich @kitsunet we concluded that we are okay with this change so merge it?

Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

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

Yes, +1 by (superfluous) reading

@@ -114,7 +114,7 @@ public function moveDimensionSpacePointCommand(string $source, string $target, s

$contentRepositoryInstance->handle(
MoveDimensionSpacePoint::create(
$workspaceInstance->currentContentStreamId,
Copy link
Member

Choose a reason for hiding this comment

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

we wouldnt event actually need to resolve the workspace instance here but i guess it leads to better errors if we do so.

@nezaniel nezaniel merged commit 3e38b36 into 9.0 Apr 5, 2024
12 checks passed
@nezaniel nezaniel deleted the 4942-refactorStructureCommands branch April 5, 2024 17:12
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.

9.0 Odd RebasableToOtherWorkspaceInterface commands AddDimensionShineThrough and MoveDimensionSpacePoint
3 participants