Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Currently it calls isPropagationStopped()
unconditionally.
Steps to reproduce
Dispatch an event object that does not implement StoppableEventInterface
.
This will product the following error.
Error: Call to undefined method Some\NonStoppable\Event::isPropagationStopped()
Proposed resolution
Check if the event object implements StoppableEventInterface
before invoking isPropagationStopped()
method.
Example:
https://github.com/symfony/symfony/blob/v6.2.0-BETA2/src/Symfony/Compone...
Comment | File | Size | Author |
---|---|---|---|
#13 | check-is-propagation-stopped-3319791-13.patch | 2.61 KB | Cyberwolf |
| |||
#13 | check-is-propagation-stopped-3319791-13-TEST-ONLY-FAIL.patch | 1.21 KB | Cyberwolf |
Comments
Comment #2
Chi CreditAttribution: Chi commentedComment #3
Cyberwolf CreditAttribution: Cyberwolf at 2DotsTwice bvba for Dropsolid commentedI added a check if the method is callable in the attached patch. Currently in D9 the included Symfony/EventDispatcher module's Event class (extended by Drupal\Component\EventDispatcher\Event) does not implement this newer interface yet, so using the interface is not an option (yet).
Comment #4
Chi CreditAttribution: Chi commented@Cyberwolf The issue is for Drupal 10. So we can safely check
StoppableEventInterface
.I think, it's Ok to have a different patch for Drupal 9.
Comment #5
longwaveShould we do #2909185: Replace ContainerAwareEventDispatcher with Symfony EventDispatcher instead?
Comment #6
Chi CreditAttribution: Chi commented@longway, I think we should not.
This one is a bug that is trivial to fix while #2909185 is a task that might not be ever resolved.
Comment #7
longwaveThe patch needs work for #4 first (we can consider backporting to 9.x later) and also needs test coverage to ensure we don't break it again.
Comment #8
Cyberwolf CreditAttribution: Cyberwolf as a volunteer commentedAttaching a patch for Drupal 10 including a test.
Comment #9
Cyberwolf CreditAttribution: Cyberwolf as a volunteer commentedComment #10
Cyberwolf CreditAttribution: Cyberwolf as a volunteer commentedComment #11
Chi CreditAttribution: Chi commentedI think this line deserves a comment explaining what exactly is testing there.
Also I propose we move checking for
StoppableEventInterface
out of double loop.Comment #12
Cyberwolf CreditAttribution: Cyberwolf as a volunteer commentedThanks for the feedback, I will try to process it today.
Comment #13
Cyberwolf CreditAttribution: Cyberwolf as a volunteer commentedComment #14
Cyberwolf CreditAttribution: Cyberwolf as a volunteer commentedComment #16
Chi CreditAttribution: Chi commentedThank you.
Comment #17
alexpottCommitted and pushed 17fa174546 to 10.1.x and e5fbc65128 to 10.0.x.
This bugfix is not relevant for 9.5.x as there is an event class does not use StoppableEventInterface in quite the same way - and there Event classes that don't use it at all.
Comment #21
quietone CreditAttribution: quietone at PreviousNext commentedRemoving tag because a test was added.