Hooks are invoked and events dispatched using an arbitrary string. Hook implementations and event listeners then get picked up based on that arbitrary string.
However, with events, we've adopted the pattern of putting the string in a constant on an Events class. See for example https://api.drupal.org/api/drupal/core!modules!migrate!src!Event!Migrate...
This has caused problems on module enable, see #2712647: Update Symfony components to ~3.2 and #2776235: Cached autoloader misses cause failures when missed class becomes available.
Event listeners reference the event class in getSubscribedEvents() - but they either have to rely on the event class always being loaded, or use class_exists() to check if it's available. This introduces a very fragile dependency on the classloader being updated when the module list is.
Stop using event classes with constants for event names, just use strings everywhere.
Comments
Comment #2
dawehnerUsing constants had kind of an advantage of making it easier to search for usages. On the other hand, having issues with autoloadable code is a much worse problem to be fair.
+1 for doing so. When we do so we should
Comment #3
Crell commentedWould it make sense to keep them at the dispatch point, even if we remove them from the registration point? Or would that have the same potential autoloader fail?
Comment #4
dawehnerIMHO this should totally work. On the dispatcher level you need to have the event classes available as well, so code would fail, when the module namespaces cannot be loaded.
On the other hand this would be a bit non ideal, as it makes it less easy to find implementors and dispatchers of a certain event.
Comment #7
fgmOne thing I did when I had to implement a custom event dispatcher / emitter for D7 (for Heisencache) was the ability, just like with the NodeJS EventEmitter, to emit just about any string as an event. To keep discoverability, I added an EventEmitterInterface which enabled declaring emitted events.
In D8, a similar mechanism would allow event discovery: it could be a service tag (but of course that would mandate that emitters be services, which is not necessarily the case), or use a discovery compiler pass, like we do to discover serviceproviders: we could have eventproviders in the same vein.
Comment #10
andypostUsed to try that approach #3009377-93: Replace drupal_get_user_timezone with a date_default_timezone_get() and an event listener
Comment #12
andypostSF 4.3 changed dispatcher https://symfony.com/blog/new-in-symfony-4-3-simpler-event-dispatching
Comment #13
andypostComment #15
andypostComment #16
andypostit depends on used symfony version
Comment #17
xjm9.0.x should only contain major-only changes, or bugs that only affect 9.0.x+. Feature requests should be filed against 8.9.x and will be shortly updated to 9.1.x.
Since this apparently requires Symfony 4 (if I understand #16), moving to 9.1.x.
Comment #18
catch#3055194: [Symfony 5] The signature of the "Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch()" method should be updated to "dispatch($event, string $eventName = null)", not doing so is deprecated since Symfony 4.3. moves to not requiring an event name at all, and using the FQDN of the event class instead.
This shows that Symfony is moving away from event names altogether, but it does not necessarily change the decision we have to make here (especially since we have 1-N relationships with some events).
Comment #20
catch#3200062: EventSubscribers lead to fatal errors on module installation was duplicate, and critical.
Comment #21
catchThere is a workaround for contrib that's documented in the issue summary here - modules need to check class_exists() before subscribing to an event via one of these constants.
However it's definitely a bad pattern, splitting the difference at major.
Comment #22
mkalkbrennerThe issue not just happens when installing from existing config. it also happens when just installing a module in the standard way when the constant is defined in a dependency that is not yet installed. That's why I choose "critical".
Comment #23
catch#22 there's still a possible fix in the contrib module for that case too in that they can use class_exists(), which is why I'd suggest this is major - either way it's a real problem that we should fix. And just moving away from the class constants shouldn't be that difficult really.
Comment #24
berdirThe thing is that Symfony is moving in the opposite direction, to use the event class name instead of a separate constant, so that a bit more awkward to hardcode as a string. But this issue feels to a certain point by-design, at least for optional integrations.
Comment #25
catchThat's true but our event class constants aren't even on the event class, they're in weird classes all to themselves that just hold constants for event names (which I never understood the point of). https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Config%21...
It should be fine to use an event class and use the class name without a class_exists, since the class doesn't need to be loaded to use the event name as a string. So going in Symfony's direction ought to help here too.
Comment #28
fabianx commented#25 They are in extra classes, because that’s a Symfonyism.
I agree that it makes much more sense to use:
::class if there is only one event and maybe have an optimized class to subclass with a listen method that is automatically registered to reduce boilerplate?
And $classEvent::EVENT_1, $classEvent::EVENT_2 when there is more than one thing using the event (though the original pattern might have been there for discoverability) OR just create a base class and use ::class anyway.
eg workspace\PrePublishEvent extends WorkspaceEvent.
Which is probably cleaner anyway.
Comment #31
longwaveI run into this time and time again on customer projects where we add a new contrib module and an event subscriber for that contrib to an existing custom module, and then get fatal errors during deployment when the module isn't enabled yet.
Big +1 for moving to event class names directly, and subclassing as in #28 for the case where we have similar events.
Comment #32
longwaveSymfony supports event name aliases so we should be able to make this change in a backward compatible way.
https://symfony.com/doc/current/event_dispatcher.html#event-aliases
We would have to add
AddEventAliasesPassfor core events and also provide a mechanism to allow contrib/custom to add their own event aliases.Comment #33
claudiu.cristeaI think we should avoid using an event class for many events. Each event should have its own class, even that would need a base class or a trait to handle code reusability
Comment #35
andypost