Why is dependency injection implemented this way?

aaronbauman → changed the visibility of the branch 3530865-why-is-dependency to hidden.

I started working on a MR, but this issue is pretty extensive.

Will wait to hear from maintainers as to whether this change would be considered before I sink more hours into it.

Hi Aaron,

This was an intentional change that isn't going to be reverted especially since contrib already adjusted to it.
This article summarizes my thoughts as well.

Basically, when setting dependencies from the constructor, any changes made to the parent constructor breaks your implementation right away.

Extending a plugin with this method only requires you to extend the create method, as long as you're calling the parent you're safe and protected from changes made to the parent class (while at the same time reducing drastically the amount of code you need to duplicate / copy).
With the "old" method, any time I needed a new dependency in one of the Commerce plugin implementations, I had to think twice and make sure to add backwards compatibility code and deprecations that would be removed in the next major branch only so that the child class doesn't break due to the missing service.

I see.
Do you have a thread open about this in Drupal core somewhere?
I'm not an expert on dependency injection, but my understanding is that this approach breaks testability and autowiring. Possibly other consequences as well. Seeing as how Commerce is, like, top 10 modules for Drupal, it's weird to implement an anti pattern.

I won't hold my breath about Commerce specifically, but I'll urge you to keep an open mind about it.

Do you have a thread open about this in Drupal core somewhere?

No I don't, simply because I'm not forcing Drupal core to adopt this pattern?

I'm not an expert on dependency injection, but my understanding is that this approach breaks testability and autowiring. Possibly other consequences as well.

Could you elaborate on this further? Afaik, there is no autowiring for plugins which is where this pattern is used? As for breaking testability, not exactly sure what you're referring to.

I won't hold my breath about Commerce specifically, but I'll urge you to keep an open mind about it.

I'm not planning to come back to this decision TBH, as this would be a step back, since these changes were implemented in Commerce 3 specifically. This makes it way easier to extend plugins without the risk of breakage due to changes to the parent constructors once again.

And additionally, this reduces the maintenance burden as I can safely introduce new dependencies I need in plugins that can potentially be extended without worrying about breaking existing implementations.

There was one or 2 issues opened about this in the past but in practice, most contrib modules from the Commerce ecosystem adopted the change without any push back.

I also see this pattern used in other contrib modules.

For reference, Webform (with 350K reported installs) also does the same: https://git.drupalcode.org/project/webform/-/blob/6.3.x/src/Plugin/Webfo....