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 references to CommandResult in return types #5061

Merged
merged 6 commits into from
Oct 20, 2024

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented May 17, 2024

Followup to #5059

Extracted from #4988

We will remove the result object for now since we removed the ->block() earlier and currently have no information we need to pass to the user after calling $cr->handle(). In the future we can still add an object if deemed necessary as its non breaking.

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

@bwaidelich
Copy link
Member

I'm not 100% sure whether it might be useful to have some return type that tells something about the published events (e.g. last sequence number) and maybe even about the updated/skipped projections

@mhsdesign
Copy link
Member Author

yes but id rather discuss this in #4988 or an issue?
This was just meant so these files dont have to be adjusted with your pr. I checked out these files and voila ^^
For more transparency i would actually prefer at first to set some correlation metata in the events itself for node migration and then we can improve cli output.
Your pr can very well introduce some Result object, but that doesnt have to be used yet here.

@kitsunet
Copy link
Member

I am fine either way, CommandResult sounds right, we might just replace the inner workings and keep it around?

@bwaidelich
Copy link
Member

I am fine either way, CommandResult sounds right

yes, my thinking. Does it make sense to remove the class if we re-add it a couple of PRs later?

@mhsdesign
Copy link
Member Author

Okay so then lets not remove it via your pr #4988:
and we can close this for now and reopen for the case we want to get rid of it.

@mhsdesign
Copy link
Member Author

As discussed with @bwaidelich we will remove the result object for now as the skipping of projections will be superseded soonish and we dont have a reason for a result. In the future we can still add an object if deemed necessary as its non breaking.

@mhsdesign mhsdesign force-pushed the task/removeCrCommandResultInReturnTypes branch from 9eebb1e to c9f8ce2 Compare October 14, 2024 20:48
@mhsdesign mhsdesign force-pushed the task/removeCrCommandResultInReturnTypes branch from c9f8ce2 to 95e797e Compare October 14, 2024 20:51
Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

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

I am really unclear what changed about the reasoning to maybe keeping it around for the future, but 🤷

@mhsdesign mhsdesign merged commit c5cbbe6 into neos:9.0 Oct 20, 2024
6 of 8 checks passed
@mhsdesign mhsdesign deleted the task/removeCrCommandResultInReturnTypes branch October 20, 2024 18:38
@mhsdesign
Copy link
Member Author

Bastian and me discussed it in detail that neither the highestCommittedVersion or highestCommittedSequenceNumber from the CommitResult of $eventStore::commit() are relevant to be returned from the content repository, nor the - currently still - skipped projections. Also if we have a clear usecase we can reintroduce it. On the road to creating a release candidate we should remove the code that we were not sure we would like to keep as releasing this unfinished is meh^^

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