Introduce event dispatching #144
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
| /** | ||
| * Sets the event dispatcher for prompt lifecycle events. | ||
| * | ||
| * The event dispatcher will be used to dispatch BeforePromptSentEvent and | ||
| * AfterPromptSentEvent during prompt generation. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @param EventDispatcherInterface|null $dispatcher The event dispatcher, or null to disable. | ||
| * @return void | ||
| */ | ||
| public static function setEventDispatcher(?EventDispatcherInterface $dispatcher): void | ||
| { | ||
| self::$eventDispatcher = $dispatcher; | ||
| } | ||
|
|
||
| /** | ||
| * Gets the event dispatcher for prompt lifecycle events. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @return EventDispatcherInterface|null The event dispatcher, or null if not set. | ||
| */ | ||
| public static function getEventDispatcher(): ?EventDispatcherInterface | ||
| { | ||
| return self::$eventDispatcher; | ||
| } | ||
|
|
||
| /** | ||
| * Dispatches an event if an event dispatcher is registered. | ||
| * | ||
| * This is a convenience method that handles the null check internally, | ||
| * only dispatching if a dispatcher has been set via setEventDispatcher(). | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @template T of object | ||
| * @param T $event The event to dispatch. | ||
| * @return T The event (potentially modified by listeners). | ||
| */ | ||
| public static function dispatchEvent(object $event): object | ||
| { | ||
| if (self::$eventDispatcher !== null) { | ||
| /** @var T */ | ||
| return self::$eventDispatcher->dispatch($event); | ||
| } | ||
|
|
||
| return $event; | ||
| } |
There was a problem hiding this comment.
Instead of having this in AiClient, I think we should come up with a dedicated class like this, mirroring the approach of HttpTransporter, which receives PSR interfaces for request handling.
That new class could receive an EventDispatcherInterface in the constructor.
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.
src/Builders/PromptBuilder.php
Outdated
| $model = $this->getConfiguredModel($capability); | ||
|
|
||
| // Dispatch BeforePromptSentEvent (allows message modification) | ||
| $beforeEvent = AiClient::dispatchEvent( |
There was a problem hiding this comment.
Following up to my above comment: Can we instead access an instance of our new EventDispatcherInterface wrapper class 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. |
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.