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

Move 'aggregateId' up #74

Closed
oqq opened this issue Dec 19, 2017 · 3 comments
Closed

Move 'aggregateId' up #74

oqq opened this issue Dec 19, 2017 · 3 comments
Labels

Comments

@oqq
Copy link
Member

oqq commented Dec 19, 2017

The aggregateId method from EventProducerTrait and EventSourcedTrait are not related to that traits, but to the abstract AggregateRoot class which uses this traits.

So we should move this method up.

Already done in this PR: https://github.com/prooph/event-sourcing/pull/73/files
But suspended until we release the next major version

cc @prolic

@codeliner
Copy link
Member

not sure about this change. pinging @Xerkus (author of the traits)

The method is needed by the ClousureAggregateTranslator

And the primary goal of the traits is that you can use them without the need to extend from AggregateRoot, hence the method must be present in the AR class using the traits ...

@Xerkus
Copy link
Contributor

Xerkus commented Dec 19, 2017

Here is why it is included in traits: #62 (comment)

By adding it to both traits we ensure presence of aggregateId if AggregateRoot or one of the traits is used.

@Xerkus
Copy link
Contributor

Xerkus commented Dec 19, 2017

You can safely remove them and that won't be a BC break btw. Abstract methods are implemented in users code.

@prolic prolic closed this as completed Feb 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants