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

Simplify class interface surface, embrace callables #14

Open
chillu opened this issue Jul 8, 2021 · 0 comments
Open

Simplify class interface surface, embrace callables #14

chillu opened this issue Jul 8, 2021 · 0 comments

Comments

@chillu
Copy link
Member

chillu commented Jul 8, 2021

This module claims PSR-14 compat, yet it creates two layers of abstraction on top of that:

  1. Dispatcher (which doesn't implement Psr\EventDispatcher\EventDispatcherInterface)
  2. EventDispatcherInterface (which introduces the concept of strongly typed EventHandlerInterface listener classes, and EventContextInterface for events)

Only the underlying library (symfony/event-dispatcher) actually implements PSR-14. That's OK in general, Laravel Events is very powerful without PSR-14.

This module seems to have three objectives:

  1. Create a facade with our own API surface that we control, rather than exposing Symfony's underlying API.
  2. Enable declarative event handling through Silverstripe's YAML config, which in turn inherits the powerful extensibility properties of this layer (allowing Silverstripe modules to define priority, unregister listeners). This means defining event handlers as YAML (service/class references), which doesn't support anonymous function definitions.
  3. Add a more strictly typed event handling layer through EventHandlerInterface::fire() (rather than callables) and EventContextInterface rather than plain PHP objects.

I see a number of issues:

  • Listener simplicity: The module assumes all listeners are defined as classes, rather than a variety of callables (anonymous functions, static/instance method calls as arrays). This makes it harder to test logic (although anonymous classes help), and to add low levels events outside of the config layer. This has been the trigger for me here, I want to procedurally add an event listener to CoreKernel
  • Interop: We negate all the interop benefits from PSR, making it more of an obscure implementation detail than a benefit for this module. If we used some third party library (e.g. a payment integration library) that accepted a dispatcher to emit its own events, we couldn't pass in the module dispatcher, and would have to create a translation layer for this library (see rationale)
  • Transferrable knowledge: APIs diverging from PSR add more complexity to any PHP dev who is familiar with other PSR-14 event handlers, that knowledge will confuse more than it helps.
  • Language variations: The API uses slight language variations ("dispatch" vs. "trigger", "listener" vs. "handler", "action" vs "subject", "context" vs. "event")
  • Event class identity vs. event name. The PSR spec says this: "Listener Providers SHOULD use the class name of an Event to differentiate one event from another. They MAY also consider any other information on the event as appropriate.". Symfony uses dot notation for grouping event handlers. We kind of force usage of GenericEvent over specific event classes through SilverStripe\EventDispatcher\Symfony\Event.

I don't have recommendations on how to fix this (yet), but this should be enough to start a discussion.

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

1 participant