[pp-1] Explore adding an ImplementationList object for hook implmentations

Let's have one round of review, to see if people agree with the direction or have different ideas where this should go.

donquixote → changed the visibility of the branch 3519561-DeprecationCanaryTest to hidden.

donquixote → changed the visibility of the branch 3519561-hook-ImplementationList-remove-getHookListeners to hidden.

donquixote → changed the visibility of the branch 3519561-DeprecationCanaryTest to hidden.

I added the unit test for ImplementationList in this issue, to prepare for 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active .

Took a pass at it I think this is a pretty nice consolation!

Qrite a few comments and I definitely need to take a deeper look, but I like the direction.

Ok I think this is ready!

I've read through this several times, both changes in isolation and the files as they will be merged.

This is a nice consolidation separating the concerns so module handler doesn't need to parse so much.
I checked through all of the places that ModuleHandler gets the hooks and confirmed they are replaced with ImplementationLists.

Early on I was concerned how this would work with Alter, but they are already ordered and they are retrieved before runtime ordering happens.

Tests look good, I didn't see anything missing a test.

Edit, actually this might need a CR since we're adding a new object and we're changing some methods.

Actually on second thought this might need a CR since we're adding a new object and we're changing some methods.

Do we write change records for protected methods and protected properties?
And do we write change records for new classes marked as internal?

Ok I asked in slack:

@godotislate shared this link https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... →

And @quietone shared this link https://www.drupal.org/about/core/policies/core-change-policies/change-r... →

They basically said it depends on contrib usage and the docs say changes to internal code so not need a cr.

The thing is this isn't just a change to internal code it's renaming and changing the return type of a method.

I think it's low risk for a few reasons:
1 I maintain the only module I'm aware of that decorate module handler.
2 this is required to simplify the removal of event dispatcher from hooks and that will necessarily be a large bc break.
3 I don't see any way to cleanly deprecate getflathooklisteners
4 the method itself is brand new

I'm going to mark this as ready, I'm happy to write a cr if someone else thinks it's necessary.

nicxvan → credited godotislate → .

nicxvan → credited quietone → .

Took a pass at credit, added @godotislate and @quietone for their assistance in slack.