Problem/Motivation

We need it because the name does not communicate at all what is actually going on in this class.

Proposed resolution

Rename RegisterKernelListenersPass to RegisterEventSubscribersPass

Remaining tasks

Create patch.

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because we just have to rename the class. This is just a DX improvement.
Issue priority Not critical because this is not any vulnerability/performance issue
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

joshi.rohit100’s picture

Assigned: Unassigned » joshi.rohit100
dawehner’s picture

While doing that I think we should document this class

joshi.rohit100’s picture

Status: Active » Needs review
FileSize
2.63 KB
dawehner’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterEventSubscribersPass.php
@@ -10,7 +10,10 @@
+/**
+ * Registers all event subscribers to the container.
+ */

Well it doesn't really register to the container but rather to the event dispatcher

joshi.rohit100’s picture

Status: Needs work » Needs review
FileSize
2.64 KB
705 bytes
joshi.rohit100’s picture

Assigned: joshi.rohit100 » Unassigned
subhojit777’s picture

Status: Needs review » Reviewed & tested by the community

#6 fixes the problem. Patch looks good. Tests all green. Good to go.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 2546782-6.patch, failed testing.

dawehner’s picture

Issue tags: +Needs reroll
dawehner’s picture

We need an issue summary and beta evaluation as well here.

Nikolay Shapovalov’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.26 KB

Rerolled.

subhojit777’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
subhojit777’s picture

subhojit777’s picture

Status: Needs review » Reviewed & tested by the community

#12 Looks good :)

joshi.rohit100’s picture

Status: Reviewed & tested by the community » Needs work

As per #12, we are deleting and then creating new one, which is making this patch heavy. Instead, it should just rename it.

cilefen’s picture

Re #16, git diff -M10% will make a smaller patch.

joshi.rohit100’s picture

@cilefen: You got me with that -M :)

subhojit777’s picture

Status: Needs work » Needs review

@joshi.rohit100 I guess it back to needs review then.

cilefen’s picture

@joshi.rohit100

You got me with that -M :)

In a good way?

joshi.rohit100’s picture

@cilefen:

In a good way?

Yes. The tricky part was '-M' for this patch :)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Well yeah the patch is still fine IMHO :) It is just not the right format

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

So if anything is depending on this well they shouldn't and this is an improvement in clarity that brings understanding so I'm going to commit it under the committer discretion proviso. Committed f99f051 and pushed to 8.0.x. Thanks!

  • alexpott committed f99f051 on 8.0.x
    Issue #2546782 by joshi.rohit100, zniki.ru, dawehner: Rename \Drupal\...

Status: Fixed » Closed (fixed)

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