Skip to content
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

[RFC] Simplify Event Store plugin registration #232

Closed
sandrokeil opened this issue Dec 14, 2016 · 9 comments
Closed

[RFC] Simplify Event Store plugin registration #232

sandrokeil opened this issue Dec 14, 2016 · 9 comments
Assignees

Comments

@sandrokeil
Copy link
Member

From a user POV I think it's a little bit confusing how to register plugins. The \Prooph\Snapshotter\SnapshotPlugin plugin is a good example. To register a plugin in the Event Store, each plugin must implement the setUp method.

public function setUp(EventStore $eventStore)
{
    $eventStore->getActionEventEmitter()->attachListener('commit.post', [$this, 'onEventStoreCommitPost'], -1000);
}

Why don't we use an attach / detach method like Zend\EventManager has. I guess it's more intuitive to register the plugin in an Event Store instead of registering the Event Store in the plugin, which then registers the plugin in the Event Store with $eventStore->->getActionEventEmitter()->attachListener(...). The Event Store should implement.

public function attach($eventName, callable $listener, $priority = 1)
{
    $this->actionEventEmitter->attachListener($eventNamer, $listener, $priority);
}

If we use this, I guess we also don't need to expose the action event emitter with getActionEventEmitter.

The same behavior is used for Service Bus plugins. Maybe we should switch it too? Here is an example

public function attach(ActionEventEmitter $emitter)
{
    $this->trackHandler($emitter->attachListener(MessageBus::EVENT_INITIALIZE, [$this, 'onDispatchInitialize']));
}
@prolic
Copy link
Member

prolic commented Dec 15, 2016

So if you want to register a plugin, you have to do it outside in the factory? What I really liked is EventStore->setup($plugin); and the rest is hidden.
Having only attach from outside is a worse solution imho.

@prolic
Copy link
Member

prolic commented Dec 15, 2016

@codeliner your thoughts?

@codeliner
Copy link
Member

Let's use the same mechanism as we have for service-bus

  • Plugin has a attach method which takes the AtionEventEmitter of the event store as argument
  • EventStore has a utilize method which takes the plugin as argument

This way attaching plugins work always the same no matter the component. Thoughts?

@prolic
Copy link
Member

prolic commented Dec 17, 2016

sounds good. will provide a pr if none of you guys is faster :)

@prolic
Copy link
Member

prolic commented Dec 18, 2016

I have to take back my last comment - the issue is not solvable like this. Let me explain with the problem of dual-plugins (a plugin for command bus and event store).

In message-bus we have this:

public function utilize(ActionEventListenerAggregate $plugin)
{
    $plugin->attach($this->getActionEventEmitter());
}

if we add now the same for event-store, the plugin attach() method needs to know, which action event listener is passed (from command-bus or from event-store) in order to attach correctly to the given action event emitter.

Method overloading would be great, like:
$plugin->setup(ActionEventEmitterEventStore $eventStore) and $plugin->setup(MessageBus $messageBus).

Unfortunately PHP is lacking support of it. So we have two options I think:

  1. Remove typehint in setup-method of the plugin. So we have only $plugin->setup($something);

In this case, we can do type checks within the plugin itself and check if we have an event store, a message bus or something else. This is kind of bad, as I really like typed parameters.

  1. Have distinct named methods, like:
    $plugin->setupEventStore(ActionEventEmitterEventStore $eventStore) and $plugin->setupMessageBus(MessageBus $messageBus).

In this case I just don't like the name of the methods, but that's perhaps something we have to live with.

Any better ideas?

@codeliner
Copy link
Member

Another option:

We make these new availableEventNames an explicit concept: https://github.com/prooph/common/blob/develop/src/Event/ProophActionEventEmitter.php#L34

and add a new ActionEventEmitter::availableEventNames(): array method to the interface so that a plugin can check if events are provided by the emitter

@prolic
Copy link
Member

prolic commented Dec 18, 2016

It's about the event target, not the event names. While it would work with additional if checks, it's not really a good solution

@codeliner
Copy link
Member

interface EventStorePlugin
{
    public function attachToEventStoreEmitter(ActionEventEmitter $actionEventEmitter): void;
}

?

@prolic prolic self-assigned this Dec 22, 2016
@prolic
Copy link
Member

prolic commented Dec 22, 2016

I am working on this, will submit a PR maybe tomorrow.

prolic added a commit to prooph/service-bus that referenced this issue Dec 23, 2016
prolic added a commit that referenced this issue Dec 25, 2016
@prolic prolic closed this as completed Dec 26, 2016
prolic added a commit to prooph/event-store-bus-bridge that referenced this issue Jan 3, 2017
ChrisSanderser added a commit to ChrisSanderser/event-store that referenced this issue Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants