Problem/Motivation

There are a slew of issues popping up in contrib that try to move _all_ constructor logic into the ContainerAware create factory method. Apparently this is in part based on the documentation in FormBase.

This is a misunderstanding of the intent of both the interface and the documentation. The documentation in FormBase is using a setter on a trait because the setter does not have a constructor. constructor logic should be in the constructor. If you don't a ton of problems arise.

1) Your class can't be effectively extended. Inheritance got us in this problem but site developers commonly have to override contrib and core and you're making our life harder.
2) It makes testing harder. You can't just inject dependencies you have to mock a populated container and fill the dependencies indirectly.
3) Its the wrong pattern. create _looks_ like a factory but its glue to populate dependencies of the container. That's it and breaking that contract messes up 1 & 2.
4) The pattern is a lie. It doesn't save you from interface changes because you implicit rely on the same constructor if you anything like testing or any interactions with the code because your code implicitly relies on the parent classes dependencies. This is the cost of inheritance.

Proposed resolution

Fix documentation

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Related issues

#3121750: Fix subclassing and stop overriding constructors in xmlsitemap\Form\XmlSitemapSettingsForm
#3111730: Fix subclassing and stop overriding constructors
#3067546: [Webform 6.x] Fix subclassing and stop overriding constructors

Comments

neclimdul created an issue. See original summary.

jrockowitz’s picture

jrockowitz commented

@neclimdul Thank you for starting this discussion.

I understand most of the concerns and I want to weigh in on my experience of gradually working to implement this new pattern for the Webform module for Drupal 9.

1) Your class can't be effectively extended. Inheritance got us in this problem but site developers commonly have to override contrib and core and you're making our life harder.

If a parent class' constructor relies on a deprecated dependency, it became challenging to remove/fix this dependency. This problem became so challenging, I had to give up on fixing deprecated dependencies in the Webform module for Drupal 8.

2) It makes testing harder. You can't just inject dependencies you have to mock a populated container and fill the dependencies indirectly.

I found it not very difficult to "mock a populated container and fill the dependencies."

3) Its the wrong pattern. create _looks_ like a factory but its glue to populate dependencies of the container. That's it and breaking that contract messes up 1 & 2.

I agree this new pattern is breaking the expected contract of OO inheritance.

4) The pattern is a lie. It doesn't save you from interface changes because you implicit rely on the same constructor if you anything like testing or any interactions with the code because your code implicitly relies on the parent classes dependencies. This is the cost of inheritance.

This pattern saves me from not being able to add new dependencies to a plugin because I worried that am going to break every contrib and custom module that is extending a plugin.

neclimdul’s picture

neclimdul
He/Him
commented

I've been struggling coming back to this. I hate that I'm coming into this after its been adopted into module like webform and that I wasn't around at the time to notice or participate in any discussions around this. I also want to acknowledge I hear that this is making things easier for developers. I still feel strongly that this is the wrong approach though. Easier is neither less complicated or better and I do believe this is more complicated and architecturally worse.

1) Your class can't be effectively extended. Inheritance got us in this problem but site developers commonly have to override contrib and core and you're making our life harder.

If a parent class' constructor relies on a deprecated dependency, it became challenging to remove/fix this dependency. This problem became so challenging, I had to give up on fixing deprecated dependencies in the Webform module for Drupal 8.

I feel like I'm not understanding what you're saying and that's probably on me. I don't think hiding it in layering of factories really solves your problem. If I was extending your class I was probably depending on the same dependencies or logic and you fixing the deprecation is probably still going to break me. If we're just talking a plugin, that responsibility is pretty obviously on the child class for extending a non-base class.

2) It makes testing harder. You can't just inject dependencies you have to mock a populated container and fill the dependencies indirectly.

I found it not very difficult to "mock a populated container and fill the dependencies."

All extending classes still need to implement all that logic to test and failures become less explicit. Also, if we're talking real prophecy/mockery mocks, they _really_ slow and quite a bit more complicated than just just passing a class to a constructor.

4) The pattern is a lie. It doesn't save you from interface changes because you implicit rely on the same constructor if you anything like testing or any interactions with the code because your code implicitly relies on the parent classes dependencies. This is the cost of inheritance.

This pattern saves me from not being able to add new dependencies to a plugin because I worried that am going to break every contrib and custom module that is extending a plugin.

I kinda regret that wording. Again, I think you're still going to break them. Maybe slightly less often but the coupling through inheritance is still there.

I think there's consensus that inheritance is the root of this. We should be avoiding it where ever possible. My point is that we shouldn't be hiding technical debt from that inheritance on the container through a pseudo factory.

neclimdul’s picture

neclimdul
He/Him
commented

Having taken a step back and thinking about this I think its probably worth re-titling this and putting it in the technical steering queue if that's still a thing. The suggestion in these issues and from Drupalcon was to set properties in plugin create methods. The code in FormBase implies its a good idea to use setters for injecting dependencies. That's a bit of a different beast and experience with plugins tells me we can we can resolve an issue around clarifying FormBase's setter example a lot quicker than a larger architectural discussion around how to create plugins.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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.