-
Notifications
You must be signed in to change notification settings - Fork 35
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
Custom dependency injection implementation #155
Conversation
@@ -52,7 +51,7 @@ public function validate(): static | |||
} | |||
|
|||
if (method_exists($this->event, 'failedValidation')) { | |||
$this->event->failedValidation($this->state); |
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 technically a breaking change but I don't think anyone uses this functionality yet so it should be fine.
$resolver = DependencyResolver::for($this->event->authorize(...), event: $this->event); | ||
$result = call_user_func_array([$this->event, 'authorize'], $resolver()); |
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.
Right now authorize
, fired
, and handle
do not support prefixes (ie. authorizeFoo
will not be called by Verbs—only authorize
). There are reasons for this, but I wonder if we want to make it use the same prefix logic everywhere to be consistent.
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.
My first thought was that it could make the events hard to reason about if all the functions can have prefixed instances. validate
and apply
inherently make sense to me since the logic within is so specific to the states being passed - this may be a symptom of knowing the Verbs lifecycle already though.
Consistency is nice too though 🤷
$parameters = [ | ||
'e' => $event, | ||
'event' => $event, | ||
$event::class => $event, | ||
(string) Str::of($event::class)->classBasename()->snake() => $event, | ||
(string) Str::of($event::class)->classBasename()->studly() => $event, | ||
'meta' => $metadata, | ||
'metadata' => $metadata, | ||
'metaData' => $metadata, | ||
'meta_data' => $metadata, | ||
Metadata::class => $metadata, | ||
]; | ||
|
||
if ($state) { | ||
$parameters = [ | ||
...$parameters, | ||
's' => $state, | ||
'state' => $state, | ||
$state::class => $state, | ||
(string) Str::of($state::class)->classBasename()->snake() => $state, | ||
(string) Str::of($state::class)->classBasename()->studly() => $state, | ||
]; | ||
} |
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.
It's worth noting that Verbs will no longer inject parameters based on name. You need to provide a type hint. We could consider adding that functionality back in, but I think using types is significantly more common and then we don't have to worry about the multitude of naming conventions…
}); | ||
|
||
it('can inject states of same type into event handle methods by alias', function () { | ||
Verbs::fake(); | ||
DependencyInjectionTestMultiHandleEvent::commit(); | ||
}); | ||
|
||
// TODO: How do we handle nullable state_ids |
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.
I probably need to add some new tests :D
Great PR 👍 |
Re: state reconstitution bug — one solution would be to just recursively fetch all states and events related to the state that we're rebuilding and then re-apply them all to get new snapshots. For example, if we're dealing with select `event_id`
from `verb_state_events`
where `state_id` = 10;
-- take event IDs from above, and use in following:
select `state_id`
from `verb_state_events`
where `event_id` in(/* known event IDs */)
and `state_id` not in (/* known state IDs, which is just 10 right now */);
-- if we get any new state IDs from the above, use those in the following:
select `event_id`
from `verb_state_events`
where `state_id` in(/* state IDs from previous query */)
and `event_id` not in(/* known event IDs */); We would just repeat a version of this until we get no new data for one loop. The vast majority of the time, this will only result in one loop. In the future, we can probably put together a neat CTE that would improve performance significantly in MySQL, and maybe someone can PR a similar solution for Postgres and SQLite. |
Right now, we rely on the Laravel container to handle DI, which means that there is some Verbs-specific logic that is hard to implement. This PR introduces a new
DependencyResolver
class that handles resolving all dependencies in your event hooks (validate
,apply
, etc). Owning the dependency resolution lets us handle events attached to multiple states considerably easier.Breaking Change
The major consequence of this change is now all hooks can be called just once, and Verbs will figure out which dependency you want based on the following criteria:
State
orEvent
), we will resolve thru the Container normallyUserState
and you typehintapply(UserState $foo)
we will inject theUserState
associated with the event as the$foo
parameter)UsersState
s, say$actor_id
and$target_id
, then you must useUserState $actor
andUserState $target
to tell Verbs which you mean)Previously, if you had an event that fired on multiple states, hooks might be called multiple times:
Before
After
I think this change in API is significantly better and worth the BC break, but it's worth noting.