Problem/Motivation
Core's use of the Symfony GenericEvent is wrong and breaks the contract defined by GenericEvent. \Drupal\Core\Entity\EntityTypeEvent and \Drupal\Core\Field\FieldStorageDefinitionEvent both subclass GenericEvent, but then override the parent constructor with a different signature and fail to call the parent constructor to initialize the parent properties. The result is that the parent class properties are not set and not usable. And the parent methods, which rely on the parent properties, are similarly unusable. Any code that tries to interact with these core events as if they were GenericEvent subclasses will not work - you have to ignore that and treat them as Event subclasses instead. Both these core Drupal classes ignore the capabilities/interface/intent of GenericEvent and are written as if they are subclassing Event instead.
See https://symfony.com/doc/current/components/event_dispatcher/generic_even... for details of how GenericEvent is designed and how it is intended to work. Here is a excerpt:
The base Event class provided by the EventDispatcher component is deliberately sparse to allow the creation of API specific event objects by inheritance using OOP. This allows for elegant and readable code in complex applications.
The GenericEvent is available for convenience for those who wish to use just one event object throughout their application. It is suitable for most purposes straight out of the box, because it follows the standard observer pattern where the event object encapsulates an event 'subject', but has the addition of optional extra arguments.
Symfony provides an Event class for use when an application needs or wants a hierarchy of Event subtypes. Symfony also provides a GenericEvent, which is intended to be used instead of or in addition to an Event hierarchy. Specifically, GenericEvent is meant to be used for a wide variety of different events - therefore GenericEvent::__construct() is designed to accept an associative array of property names => property values which are stored in an internal $arguments array and accessed through GenericEvent::getArgument($property_name). As its name implies, GenericEvent is thus designed to be used for a wide variety of events with different numbers and types of properties, obviating the need for concrete Event subclasses each with its own set of properties and each with its own set of accessor/mutator methods.
In addition, GenericEvent provides a number of other methods, which are dependent upon the correct arguments being passed to its constructor. If the GenericEvent object is not initialized in the constructor, then these methods are useless.
Proposed resolution
\Drupal\Core\Entity\EntityTypeEvent and \Drupal\Core\Field\FieldStorageDefinitionEvent should subclass \Drupal\Component\EventDispatcher\Event instead of \Symfony\Component\EventDispatcher\GenericEvent. These two events were introduced into core by #2332935: Allow code to respond to entity/field schema changes.
There are actually three places in core where GenericEvent is used:
- core/lib/Drupal/Core/Field/FieldStorageDefinitionEvent.php
- This is defined and used as a regular
Event- it overrides and ignores all of the methods and properties thatGenericEventdefines. It should be changed to subclassEventdirectly instead ofGenericEvent. - core/lib/Drupal/Core/Entity/EntityTypeEvent.php
- This is defined and used as a regular
Event- it overrides and ignores all of the methods and properties thatGenericEventdefines. It should be changed to subclassEventdirectly instead ofGenericEvent. - core/tests/Drupal/Tests/Component/EventDispatcher/ContainerAwareEventDispatcherTest.php
- This tests that
GenericEvent()does not trigger the deprecation warning for\Symfony\Component\EventDispatcher\Event. Even thoughGenericEvent()subclassesEvent(), theEventDispatchermakes an explicit exception for Symfony subclasses ofEventand excludes them from the deprecation warning. The only change needed here for D10 is to correct the fully-qualified name ofGenericEventto beSymfony\Component\EventDispatcher\GenericEvent(Symfony 5) instead ofSymfony\Component\EventDispatcher\GenericEvent(Symfony 4). See the Change Record.
User interface changes
None.
API changes
This does represent an API change, because changing the parent class will change the defined methods/properties. However, the usable API will not change. Currently, FieldStorageDefinitionEvent and EntityTypeEvent shadow the parent GenericEvent properties and make the parent methods useless. After the change, those parent properties and parent methods won't exist, but they never worked in the first place. So effectively, this can't affect any contributed modules.
Data model changes
None.
Release notes snippet
D10:
\Drupal\Core\Entity\EntityTypeEvent and \Drupal\Core\Field\FieldStorageDefinitionEvent now subclass
\Drupal\Component\EventDispatcher\Event instead of \Symfony\Contracts\EventDispatcher\GenericEvent.
D9:
\Drupal\Core\Entity\EntityTypeEvent and \Drupal\Core\Field\FieldStorageDefinitionEvent now subclass
\Drupal\Component\EventDispatcher\Event instead of \Symfony\Component\EventDispatcher\GenericEvent.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 3260520-4-generic-event.patch | 1.24 KB | tr |
Comments
Comment #2
tr commentedComment #3
tr commentedComment #4
tr commentedI don't really understand why the testbot says Symfony\Contracts\EventDispatcher\GenericEvent is not available.
Regardless, here's another patch that should avoid that error.
Comment #5
joachim commentedVery clearly explained, thanks! LGTM.
Comment #8
catchIt's because only the Symfony\Component\EventDispatcher defines that, not Symfony\Contracts.
Patch and release note both look great. Agreed this isn't an API change as such, so release note by itself seems plenty.
Committed/pushed to 10.0.x and cherry-picked to 9.4.x, thanks!