Problem/Motivation

Currently we have:

  • A unit test ContainerAwareEventDispatcherTest which tests the ContainerAwareEventDispatcher class itself.
    This does not cover the ways in which events can be registered by modules.
  • Various kernel or functional tests that implicitly cover the event dispatcher system.

We do not have a kernel test to directly cover the event dispatching system.
This means we currently only cover the ways in which core modules register events.

Problems:

  • If all the core event subscribers use a new way of registering their events, then we lose that coverage for legacy methods to register events.
  • Coverage is only indirect, so it will be hard to debug failures.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#13 3379488-nr-bot.txt90 bytesneeds-review-queue-bot

Issue fork drupal-3379488

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

donquixote created an issue. See original summary.

donquixote’s picture

donquixote commented
Parent issue: » #2909185: Replace ContainerAwareEventDispatcher with Symfony EventDispatcher

donquixote opened merge request !4548

donquixote’s picture

donquixote commented
Status: Active » Needs review

What else should we cover?

smustgrave’s picture

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

Believe this added test coverage looks good. Marking for committers.

Good catch on seeing this.

donquixote’s picture

donquixote commented

Thanks @smustgrave for rtbc!
But, I really want these to be somewhat "complete". I would like to add anything that makes sense to add before we merge this.

Are you sure it is enough, or can you think of anything we should add?

donquixote’s picture

donquixote commented

Background: We want to catch regressions from replacing the event subscriber in #2909185: Replace ContainerAwareEventDispatcher with Symfony EventDispatcher.

smustgrave’s picture

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

To me it looks good. But maybe should get a second +1

donquixote’s picture

donquixote commented

Perhaps we should cover different return value array shapes from the event subscriber.
Given that we would replace the compiler pass, we want to be sure all of that is still supported.

donquixote’s picture

donquixote commented

I think now it covers all of RegisterEventSubscribersPass.

There is some logic in ContainerAwareEventDispatcher that is not covered by this new kernel test, but it is already covered sufficiently by the unit test.
We just need to watch out when we replace the ContainerAwareEventDispatcher with the symfony event dispatcher, that the behavior covered in the unit test equally applies to the symfony event dispatcher.

smustgrave’s picture

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

Moving to committers queue.

quietone’s picture

quietone commented

I'm triaging RTBC issues. I read the IS, the comments and skimmed the MR. I didn't find any unanswered questions.

Leaving at RTBC.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3379488-nr-bot.txt90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.