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.
| /** | ||
| * 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.
| { | ||
| 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.
| $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?
| $beforeEvent = AiClient::dispatchEvent( | ||
| new BeforePromptSentEvent($this->messages, $model, $capability) | ||
| ); | ||
| $messages = $beforeEvent->getMessages(); |
There was a problem hiding this comment.
Why do we want to allow modifying the messages here? I would prefer if we didn't allow that, at least not from the start. We can always add it later if there's a need for it.
I think for now I'd prefer that we kept these events "immutable", I.e. more like an action in WordPress, not a filter.
| // Video generation is not yet implemented | ||
| throw new RuntimeException('Output modality "video" is not yet supported.'); | ||
| } | ||
|
|
||
| // TODO: Add support for other capabilities when interfaces are available |
| * | ||
| * @since n.e.x.t | ||
| */ | ||
| class AfterPromptSentEvent |
There was a problem hiding this comment.
How about instead naming this AfterGenerateResultEvent? I think that fits better with the logic as part of which is run, and is less low-level than whether "prompts were sent".
| * | ||
| * @since n.e.x.t | ||
| */ | ||
| class BeforePromptSentEvent |
There was a problem hiding this comment.
See above, how about BeforeGenerateResultEvent?
| * @param list<Message> $messages The modified messages. | ||
| * @return void | ||
| */ | ||
| public function setMessages(array $messages): void |
There was a problem hiding this comment.
See above, I think we should remove this, at least for now.
Another reason this is better for now: If we allow altering the input here, we would be required to have the same hook run before the corresponding is_supported_* check, otherwise the outcome could go out of sync. We would need to ensure the data is altered in the same way in both places.
I think we can separately open an issue to discuss this. Maybe a separate event just to allow altering messages would be ideal, and that event could have a different name so that it could be fired in multiple places (both before generation and before support check).
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.