-
Notifications
You must be signed in to change notification settings - Fork 74
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
Update fromStream for Projection, Query and ReadModelProjection #350
Conversation
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.
One use case I see for that is when projection only relies on a subset of the events (by type for instance).
Is that what you had in mind as well?
I'm wondering if there is a reason why this wasn't in the interface already?
src/Projection/Query.php
Outdated
@@ -29,6 +30,8 @@ public function fromStream(string $streamName): Query; | |||
|
|||
public function fromStreams(string ...$streamNames): Query; | |||
|
|||
public function fromStreamWithMetadataMatcher(string $streamName, MetadataMatcher $metadataMatcher): Query; |
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 is a BC break and should be released in a new major version, shouldn't it?
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.
Exactly, my bad. I think this should work:
public function fromStream(string $streamName, MetadataMatcher $metadataMatcher = null): Query;
My main reason for this is to have the possibility to filter the stream by the aggregateID to calculate additional data bei a given time range. But its also possible to filter by type, version and so on. You can also add custom meta data to your events and filter by it. |
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.
Additionally I'd like to have this functionality in Projection and ReadModelProjection as well.
src/Projection/Query.php
Outdated
@@ -29,6 +30,8 @@ public function fromStream(string $streamName): Query; | |||
|
|||
public function fromStreams(string ...$streamNames): Query; | |||
|
|||
public function fromStreamWithMetadataMatcher(string $streamName, MetadataMatcher $metadataMatcher): Query; |
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.
Exactly, my bad. I think this should work:
public function fromStream(string $streamName, MetadataMatcher $metadataMatcher = null): Query;
Looks good so far. Before merging this, I'd like to see two things.
|
A PR for the PDO Event Store is already open: prooph/pdo-event-store#177 Docs will be updated asap |
this Method has now a second optional MetaMatcher as parameter. This change make it possible to query for a single aggregate with a SingleStream- and SimpleStreamStrategy behind.
The change is only in implementation, not in the interface itself.
…On Sat, Nov 3, 2018, 23:33 Gildas Quéméner ***@***.*** wrote:
***@***.**** commented on this pull request.
------------------------------
In src/Projection/ReadModelProjector.php
<#350 (comment)>:
> @@ -35,7 +36,7 @@
*/
public function init(Closure $callback): ReadModelProjector;
- public function fromStream(string $streamName): ReadModelProjector;
+ public function fromStream(string $streamName, MetadataMatcher $metadataMatcher = null): ReadModelProjector;
Changing a method signature (even with optional parameters) is a BC break
and all custom implementations will break after this change.
php > interface X
php > {
php { public function foo(string $a, int $b = null): void
php { ;
php { }
php > class A implements X
php > {
php { public function foo(string $a): void
php { {
php { }
php { }
Fatal error: Declaration of A::foo(string $a): void must be compatible with X::foo(string $a, ?int $b = NULL): void in php shell code on line 1
A BC solution would have been to create a separate interface, which would
have provided the fromStreamWithMetadataMatcher method OR to release this
in a new major version.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#350 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAYEvBT3x0N6jiL2AFVk38NgGhh2SxKnks5urbdYgaJpZM4YII9M>
.
|
Oh right, I see.
Sorry, I got confused with the github discussion feed.
|
No problem
…On Sat, Nov 3, 2018, 23:40 Gildas Quéméner ***@***.*** wrote:
Oh right, I see.
Sorry, I got confused with the github discussion feed.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#350 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAYEvIndheI0TUOcC5Ttf4Nn-75WXUWGks5urbjbgaJpZM4YII9M>
.
|
This change make it possible to query for a single aggregate with a SingleStream- and SimpleStreamStrategy behind.