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 that GenericEvent defines. It should be changed to subclass Event directly instead of GenericEvent.
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 that GenericEvent defines. It should be changed to subclass Event directly instead of GenericEvent.
core/tests/Drupal/Tests/Component/EventDispatcher/ContainerAwareEventDispatcherTest.php
This tests that GenericEvent() does not trigger the deprecation warning for \Symfony\Component\EventDispatcher\Event. Even though GenericEvent() subclasses Event(), the EventDispatcher makes an explicit exception for Symfony subclasses of Event and excludes them from the deprecation warning. The only change needed here for D10 is to correct the fully-qualified name of GenericEvent to be Symfony\Component\EventDispatcher\GenericEvent (Symfony 5) instead of Symfony\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.

Comments

TR created an issue. See original summary.

tr’s picture

Status: Active » Needs review
StatusFileSize
new2.05 KB
tr’s picture

Issue summary: View changes
tr’s picture

StatusFileSize
new1.24 KB

I 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.

joachim’s picture

Status: Needs review » Reviewed & tested by the community

Very clearly explained, thanks! LGTM.

  • catch committed 6958a65 on 9.4.x
    Issue #3260520 by TR: GenericEvent is used improperly
    
    (cherry picked...

  • catch committed 4fa26fd on 10.0.x
    Issue #3260520 by TR: GenericEvent is used improperly
    
catch’s picture

Version: 10.0.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed

the testbot says Symfony\Contracts\EventDispatcher\GenericEvent is not available.

It'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!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.