Introduce interfaces for the EventManager#100
Conversation
|
making it actually final should probably wait until we have an interface that can be used as parameter/property type. Using a final class as type prevents any kind of extension. |
|
And the need for an extension point is not theoretical. Symfony implements a lazy-loading event manager. |
8711955 to
25c97cd
Compare
|
@stof I've added an interface that could be implemented by |
|
@greg0ire if would first require all Doctrine projects to migrate to using the interface as type, before Symfony could implement the interface without extending the class. So I would say it is indeed too early (we need to release the interface before being able to update downstream projects) |
|
@stof OK, I've removed the soft final from this PR. |
src/EventManagerInterface.php
Outdated
| * The subscriber is asked for all the events it is interested in and added | ||
| * as a listener for these events. | ||
| */ | ||
| public function addEventSubscriber(EventSubscriber $subscriber): void; |
There was a problem hiding this comment.
I'm wondering whether the methods that are configuring the instance should be part of the interface or no.
There was a problem hiding this comment.
By comparison, the PSR-14 EventDispatcherInterface defines only a dispatchEvent method, and its ListenerProviderInterface defines a getListenersForEvent method, but the PSR-14 interfaces don't define any interface about how the event dispatcher is actually configured.
There was a problem hiding this comment.
If you're on the fence, maybe the solution is to have several interfaces?
Looking at the ORM code, it looks like the methods that are the most frequently used together are hasListeners and dispatchEvent (by far)
Then, you have addEventListener and removeEventListener, for temporary listeners that needs to be cleaned up during hydration.
And finally, you have hasListeners in combination with getListeners and getAllListeners, for debugging and introspection, inside the very recent DebugEventManagerDoctrineCommand
There was a problem hiding this comment.
To be frank, hasListeners seems like a micro-optimization, because dispatchEvent is a no-op when there are no listeners for the event passed in argument:
event-manager/src/EventManager.php
Lines 34 to 36 in cdc1197
I'm not saying we should deprecate it, but maybe it does not need to be in the same interface as dispatchEvent, and maybe we should rework the ORM code like so:
- if ($this->evm->hasListeners(Events::loadClassMetadata)) {
- $eventArgs = new LoadClassMetadataEventArgs($class, $this->em);
+ $eventArgs = new LoadClassMetadataEventArgs($class, $this->em);
- $this->evm->dispatchEvent(Events::loadClassMetadata, $eventArgs);
+ $this->evm->dispatchEvent(Events::loadClassMetadata, $eventArgs);
- }There was a problem hiding this comment.
This looks related: doctrine/mongodb-odm#2480
There was a problem hiding this comment.
This comment shows why we might want to keep it: doctrine/mongodb-odm#2480 (comment)
There was a problem hiding this comment.
there wouldn't be much overhead by always dispatching the event and letting $eventArgs->getFoundMetadata() return null when no listener was registered (and so no new metadata could be found).
There was a problem hiding this comment.
Definitely not, apparently this was more about readability.
12dae11 to
6683195
Compare
|
@stof I went with 4 interfaces. 3 of them extend the |
src/EventListenerIntrospector.php
Outdated
| /** | ||
| * Gets all listeners keyed by event name. | ||
| * | ||
| * @return array<string, object[]> The event listeners for the specified event, or all event listeners. |
There was a problem hiding this comment.
| * @return array<string, object[]> The event listeners for the specified event, or all event listeners. | |
| * @return array<string, object[]> The event listeners for all events. |
Getting listeners for a specified event requires using getListeners instead.
There was a problem hiding this comment.
Alternatively, remove the description entirely, as done in #101, as this is already described in the previous sentence anyway.
There was a problem hiding this comment.
I merged #101 up and went consistent with it.
|
The PR title needs to be updated, as this does not introduce an |
This should allow using other extension strategies than inheritance in the future, once other Doctrine project start accepting instances of this interface.
This should allow using other extension strategies than inheritance in
the future, once other Doctrine project start accepting instances of
this interface.