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

CommentFileSizeAuthor
#13 check-is-propagation-stopped-3319791-13.patch2.61 KBcyberwolf
#13 check-is-propagation-stopped-3319791-13-TEST-ONLY-FAIL.patch1.21 KBcyberwolf
#10 check-is-propagation-stopped-3319791-10.patch2.18 KBcyberwolf
#10 check-is-propagation-stopped-3319791-10-TEST-ONLY-FAIL.patch1.14 KBcyberwolf
#8 check-is-propagation-stopped-3319791-8.patch2.53 KBcyberwolf
#8 check-is-propagation-stopped-3319791-8-TEST-ONLY-FAIL.patch1.49 KBcyberwolf
#3 check-is-propagation-stopped-3319791-3.patch749 bytescyberwolf

Comments

Chi created an issue. See original summary.

chi’s picture

chi commented
Issue summary: View changes
cyberwolf’s picture

cyberwolf commented
StatusFileSize
new check-is-propagation-stopped-3319791-3.patch749 bytes

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

chi’s picture

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.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Should we do #2909185: Replace ContainerAwareEventDispatcher with Symfony EventDispatcher instead?

chi’s picture

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.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs work
Issue tags: +Needs tests

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

cyberwolf’s picture

cyberwolf commented
StatusFileSize
new check-is-propagation-stopped-3319791-8-TEST-ONLY-FAIL.patch1.49 KB
new check-is-propagation-stopped-3319791-8.patch2.53 KB

Attaching a patch for Drupal 10 including a test.

cyberwolf’s picture

cyberwolf commented
Status: Needs work » Needs review
cyberwolf’s picture

cyberwolf commented
StatusFileSize
new check-is-propagation-stopped-3319791-10-TEST-ONLY-FAIL.patch1.14 KB
new check-is-propagation-stopped-3319791-10.patch2.18 KB
chi’s picture

chi commented

I think this line deserves a comment explaining what exactly is testing there.

$this->assertInstanceOf(\stdClass::class, $this->dispatcher->dispatch(new \stdClass(), self::PREFOO));

Also I propose we move checking for StoppableEventInterface out of double loop.

cyberwolf’s picture

cyberwolf commented
Assigned: Unassigned » cyberwolf
Status: Needs review » Needs work

Thanks for the feedback, I will try to process it today.

cyberwolf’s picture

cyberwolf commented
StatusFileSize
new check-is-propagation-stopped-3319791-13-TEST-ONLY-FAIL.patch1.21 KB
new check-is-propagation-stopped-3319791-13.patch2.61 KB
cyberwolf’s picture

cyberwolf commented
Status: Needs work » Needs review

The last submitted patch, 10: check-is-propagation-stopped-3319791-10-TEST-ONLY-FAIL.patch, failed testing. View results

chi’s picture

chi commented
Status: Needs review » Reviewed & tested by the community

Thank you.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 10.1.x-dev » 10.0.x-dev
Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed 17fa174 on 10.1.x 
    Issue #3319791 by Cyberwolf, Chi, longwave:...

  • alexpott committed e5fbc65 on 10.0.x 
    Issue #3319791 by Cyberwolf, Chi, longwave:...

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone commented
Issue tags: -Needs tests

Removing tag because a test was added.