-
Notifications
You must be signed in to change notification settings - Fork 7
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
allow aggregate state to be an object #45
Conversation
@@ -45,8 +45,12 @@ function changeUserName(callable $stateResolver, ChangeUserName $command): array | |||
|
|||
const apply = '\Prooph\MicroExample\Model\User\apply'; | |||
|
|||
function apply(array $state, Message ...$events): array | |||
function apply($state, Message ...$events): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function requires $state
to be an array
or null
and would raise an exception otherwise.
So I think ?array $state
is the better type hint here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
array_merge would not work with an object. Also if $events is an empty array, $state would be returned untouched. The return type hint is array
and could therefore not be an object
without raising an exception.
The change here is an object would also be okay.
…On Oct 2, 2017 15:25, "Eric Braun" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In examples/Model/User.php
<#45 (comment)>:
> @@ -45,8 +45,12 @@ function changeUserName(callable $stateResolver, ChangeUserName $command): array
const apply = '\Prooph\MicroExample\Model\User\apply';
-function apply(array $state, Message ...$events): array
+function apply($state, Message ...$events): array
This function requires $state to be an array or null and would raise an
exception otherwise.
So I think ?array $state is the better type hint here.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#45 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAYEvH4vsVB8fvDIKJ2nJdGD_u9b1RSXks5soI_ggaJpZM4Pp0zP>
.
|
This is the user implementation where an array is used. So this is ok here.
…On Oct 2, 2017 15:30, "Eric Braun" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In examples/Model/User.php
<#45 (comment)>:
> @@ -45,8 +45,12 @@ function changeUserName(callable $stateResolver, ChangeUserName $command): array
const apply = '\Prooph\MicroExample\Model\User\apply';
-function apply(array $state, Message ...$events): array
+function apply($state, Message ...$events): array
array_merge would not work with an object. Also if $events is an empty
array, $state would be returned untouched. The return type hint is array
and could therefore not be an object without raising an exception.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#45 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAYEvB51TY-rJLqnNC91kZvPjWV1Awirks5soJEcgaJpZM4Pp0zP>
.
|
@codeliner can you please check this PR? |
LGTM A state object is mutable unless implemented as an immutable object, whereby PHP arrays are semi-immutable because they are not passed by reference.And we lost the type hints (or turned them into type checks performed by prooph micro) for state. BUT passing objects around is faster than passing arrays and state objects are more user and IDE friendly, so +1 for the change. @prolic Did you get the idea from aggregates in Haskell article? ;) |
Hm, probably we could just count the events maybe?
On Oct 3, 2017 02:04, "Alexander Miertsch" <notifications@github.com> wrote:
LGTM
A state object is mutable unless implemented as an immutable object,
whereby PHP arrays are semi-immutable because they are not passed by
reference.And we lost the type hints (or turned them into type checks
performed by prooph micro) for state. BUT passing objects around is faster
than passing arrays and state objects are more user and IDE friendly, so +1
for the change.
@prolic <https://github.com/prolic> Did you get the idea from aggregates in
Haskell article? ;)
Should we also determine new aggregate state version based on new recorded
events?
ATM we require the developer to set new version: https://github.com/prooph/
micro/blob/master/examples/Model/User.php#L32
I'd like to get rid of it if possible.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#45 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAYEvIodgDd76sIj0d7iXlxZjc0-v1Uxks5soSWsgaJpZM4Pp0zP>
.
|
yeah, count events should work. I'll create a separate issue. |
No description provided.