Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
@felixarntz Do you think it's worth adding our own event interface so subsequent dispatchers can identify any events coming from our system? I think so, yeah? 🤔 |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@JasonTheAdams I think it's a solid start, but I think we can refine a couple things.
src/AiClient.php
Outdated
| { | ||
| if (self::$eventDispatcher !== null) { | ||
| /** @var T */ | ||
| return self::$eventDispatcher->dispatch($event); |
There was a problem hiding this comment.
I wonder whether we want to be a bit safer here.
I don't love that an event listener can arbitrarily replace the entire instance with another instance. I think we'll want to remain in control of what can and what cannot be modified.
While PSR-14 doesn't mandate that, it doesn't mean we cannot be stricter. I think we should include here a check to ensure that the return value is $modified_event === $event, and otherwise throw.
Yes, that technically means we wouldn't even need to return it, since objects are modified by reference. But I think this is preferable since that leaves us in control.
For example, right now you have a setMessages method on the event implementations, but having that doesn't even matter, since a listener could just create a new instance of the class and replace everything. I think that's not great.
There was a problem hiding this comment.
After some thought, I agree with your reasoning but not the placement. Hahah!
I think this should be a decision of the actual event dispatcher. Seeing as we're trying to be framework agnostic, we don't want to assume that our WP use case reflects how everyone wants event dispatching to work. This is something we can put in our own dispatcher in WP AI Client, but I don't think should be done here.
I like that idea, yeah. But it probably should simply be an empty interface. It'll allow to do exactly what you're saying, but otherwise we can adhere to the regular constraint of just In the WP AI Client for example, we can check for the class name of the event and based on that fire specific action hooks with names that make sense according to the event class name. |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@JasonTheAdams Looks great! Two minor points.
Co-authored-by: Felix Arntz <flixos90@gmail.com>
2 participants
Resolves #142
This introduces an abstract system for event dispatching using PSR-14 interfaces. An event dispatcher is set on the
AiClientwhich can then be used throughout the system. There are currently two events:AfterPromptSentEventBeforePromptSentEventWe could add more, of course, but these seem like the two most sensible.
Being abstract, it's up to the implementing system to connect this to whatever concrete event system they have. It's also their responsibility to set up the
PSR-14event listeners, which would be fired when the dispatching is fulfilled. This makes the actual event system within the PHP AI Client pretty lightweight.An intention here is to have a WP AI Client counterpart which hooks this up to the WordPress actions system.