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

use custom aggregate type mapping #65

Merged
merged 5 commits into from
Jun 21, 2017
Merged

use custom aggregate type mapping #65

merged 5 commits into from
Jun 21, 2017

Conversation

prolic
Copy link
Member

@prolic prolic commented Jun 20, 2017

resolves #64

@prolic prolic requested a review from codeliner June 20, 2017 09:30
@coveralls
Copy link

Coverage Status

Coverage decreased (-3.7%) to 85.821% when pulling 00b8bbf on aggregate_type_mapping into 43741b0 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.7%) to 85.821% when pulling 00b8bbf on aggregate_type_mapping into 43741b0 on master.

'aggregate_repository' => [
'user_repository' => [ //<-- here the container id is referenced
'repository_class' => MyUserRepository::class,
'aggregate_type' => 'user', //<-- The custom aggregate type
Copy link
Member

Choose a reason for hiding this comment

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

this would be nice:

'aggregate_type' => ['user' => MyUserClass],

With AggregateType::fromArray(['user' => MyUserClass]) and $aggregateType->mappedClass()`
But I'm not sure if this is a BC break.

Or maybe AggregateType::fromString|fromAggregate|fromAggregateRootClass('user', ['user' => MyUserClass, 'todo' => MyTodoClass])

You have a global aggregateTypeMap and you can pass it (new optional arg) to the AggregateType value object so that mapping is done insight the VO.

Just some thoughts for improvements because repo construct takes already a lot of optional args

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, I will check what's possible.

Copy link
Member

Choose a reason for hiding this comment

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

cool, thx

@prolic
Copy link
Member Author

prolic commented Jun 20, 2017

updated

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.04%) to 88.476% when pulling 8d0172d on aggregate_type_mapping into 43741b0 on master.

## Aggregate Type Mapping

It's possible to map an aggregate type `user` to an aggregate root class like `My\Model\User`. To do that, add the
aggregate type mapping to your repository and use the provided aggregate type. The aggregate type mapping is the last
Copy link
Member

Choose a reason for hiding this comment

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

text needs update ;)

@codeliner
Copy link
Member

Minor issue regarding docs but LGTM
@basz can you do a second review?

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.04%) to 88.476% when pulling cba43c2 on aggregate_type_mapping into 43741b0 on master.

Copy link
Member

@basz basz left a comment

Choose a reason for hiding this comment

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

beside fromString still being here LGTM

}

/**
* Use this factory when the aggregate type is not equal to the aggregate root class
*
* @throws Exception\InvalidArgumentException
*/
public static function fromString(string $aggregateTypeString): AggregateType
Copy link
Member

Choose a reason for hiding this comment

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

do we still need this method now we go for fromMapping

@prolic
Copy link
Member Author

prolic commented Jun 20, 2017 via email

@basz
Copy link
Member

basz commented Jun 20, 2017

kk

@prolic prolic merged commit b27c3bb into master Jun 21, 2017
@prolic prolic deleted the aggregate_type_mapping branch June 21, 2017 10:23
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.

AggregateType in RepositoryFactory
4 participants