Problem/Motivation

https://www.drupal.org/core/d8-bc-policy currently states this:

The constructor for a service object (one in the container), plugin, or controller is considered internal unless otherwise marked, and may change in a minor release. These are objects where developers are not expected to call the constructor directly in the first place. Constructors for value objects where a developer will be calling the constructor directly are excluded from this statement.

I think when that was written, it wasn't quite clear to us just how common it will be that constructors are overriden due to dependency injection. Especially on base classes (e.g. ContentEntityForm, entity handlers, plugin base classes) but also specific implementations.

Recently, we started being more careful with those kind of changes, because we did break quite a few contrib and custom modules and that doesn't help when we're trying to keep everyone on the most recent minor version. However, there are no clear rules, which resulted in very different approaches, some cases being extremely strict, while others have been committed with possibly breaking changes.

See also #3019332: Use final to define classes that are NOT extension points on a discussion as to when and how we should specifically disallow subclasses.

Proposed resolution

As part of the dozens of constructor changes due to entity.manager replacements, the following rules have been applied:

1. If the replacement also implements the same interface as the original service (e.g. EntityManager implements all its 10 or so replacement interfaces and can be injected as EntityTypeManagerInterface $entity_type_manager for example), then we can just replace it. In case of EntityManager, we will then do @trigger_error() on all method calls and catch it through that.
2. Other cases are added as new optional arguments, with a @trigger_error() + and fallback to \Drupal::service() if not provided.

In some rare cases, arguments also need to be removed, this hasn't been clearly defined yet. See #3025152: Remove entityManager constructor argument from SystemManager, there is a way to remove type hints and then dynamically check what we received, we could just remove it or we could wait until the 9.x branch to just remove it. The first approach likely only makes sense in case we would need to remove an argument to a very common base class.

Yet another challenge are injecting dependencies into a class that previous did not have a constructor at all.

I (@Berdir) would suggest we define it as a best-effort/best-practive thing, the default is that we do it, but we also can make an exception if there' s a reason, for example when we have to add a completely new constructor, or remove an argument from a class that is very unlikely (or rarely) subclassed. That also includes that we will not add @legacy tests for such constructor changes and usually no dedicated CR is necessary. We might want to do one to announce the change in BC policy, there is now also already a snippet in the EntityManager CR: https://www.drupal.org/node/2549139.

There have also been some practices on how services as well as anything that is based on one of the ContainerFactory interfaces can be subclassed and extra dependency injected, typically through setter methods that do not require an overridden constructor. We should decide if we want to adopt that in core to set an example, so that changes like this will in the future be easier to make.

Services, using setter methods in the service definition: https://www.md-systems.ch/en/blog/techblog/2016/12/17/how-to-safely-inje...

Plugins, by calling setter methods from the create() method, after calling the parent: https://www.previousnext.com.au/blog/safely-extending-drupal-8-plugin-cl....

Remaining tasks

Define a new BC policy for constructors, update documentation for that and best practices, if we agree on that.

User interface changes

API changes

Data model changes

Release notes snippet

Comments

Berdir created an issue. See original summary.

dww’s picture

dww
we/he/they
commented

Thanks for opening this. I agree with (most of) your proposal.

There have also been some practices on how services as well as anything that is based on one of the ContainerFactory interfaces can be subclassed and extra dependency injected, typically through setter methods that do not require an overridden constructor. We should decide if we want to adopt that in core to set an example, so that changes like this will in the future be easier to make.

I'm not totally sure I like core doing lots of setters-inside-create(), but especially for likely-to-have-been-extended classes, this probably makes sense, too. Just don't mess with the constructor at all (until D10, I suppose).

I'd love a formal policy on all this, both as someone that's been burned by BC breaks in "minor" releases, and as someone actively breaking constructors in core right now. ;) #2492171: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc)

Thanks,
-Derek

gapple’s picture

gapple
he/they
Primary language English
commented

Using Service Decoration is another method to avoid issues from constructor changes in core services. The IE9 Compatibility module's CssCollectionRenderer is an example use.

If an interface adds a new method a service decorator implementing it will break until it's updated to proxy the new method, but the decorator will remain compatible with the older version of the interface.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

> I'm not totally sure I like core doing lots of setters-inside-create(), but especially for likely-to-have-been-extended classes, this probably makes sense, too

It's not so much about the base classes, it is more about their subclasses that are extending others that do dependency injection. So regular services, forms, controllers and so on that do not have to call a parent with 5 services already as the "base line" can stay just like they are now.

A good example is DefaultSelection and ( in #3023981: Add @trigger_error() to deprecated EntityManager->EntityRepository methods, DefaultSelection needs a whole bunch of extra services now and UserSelection has to duplicate all that and also copy the whole docblock and adjust it every time the parent changes. If we'd change that to just doing this in create(), it would be much simpler:

$instance = parent::create($container, configuration, $plugin_id, $plugin_definition);
$instance->setConnection($container->get('database'));
return $instance;

Technically, you can even do $instance->connection = $container->get('database'); because you are inside the class, but then you can't actually do DI in e.g. a unit test. But in contrib, I've started doing just that, I can add a setter anytime when I actually have to write a unit test.

And yes, service decorators is a useful patterns, but it is mostly about allowing multiple decorators on services, either you don't extend from the existing class and then you risk errors when an interface is extended, or you do and you're back to square one. Also service decorators only work on, well, services, and it is not possible to override protected methods, only stuff that's part of the public API.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
  1. When subclassing, always use setFooService() instead of appending to the constructor.
  2. Try to avoid subclassing: decorating is less brittle!

WRT point 2: not many examples of this in core, but there are some, search for decorates: in *.services.yml.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

#5

1. I agree, but I don't think we officially document or even do that somewhere in core? And even I only saw approaches to do the same with *ContainerFactory-based

2. This is not just about services, mostly it is actually about plugins, entity handlers and so on. See overrides of \Drupal\Core\Entity\EntityStorageBase::__construct, \Drupal\Core\Entity\EntityViewBuilder::__construct, \Drupal\views\Plugin\views\PluginBase::__construct and so on. Also some controller/form shenanigans around \Drupal\Core\Entity\Controller\EntityViewController::__construct and various node controllers (which sometimes even creates new instances of other controllers), and also \Drupal\taxonomy\Form\OverviewTerms::__construct(). In fact, service subclasses are often the easiest ones. And as I wrote in #4, decorators have clear limits and you either subclass decorators too or you have to implement the interface from scratch, which means your implementation will break if new methods are added. The main problem they solve is that multiple implementations can extend a service, it doesn't solve the DI problem.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
  1. I agree, but I don't think we officially document or even do that somewhere in core?

    I just spent 10 mins searching, failed. I think you're right, nothing is doing that in core. In contrib, I know that https://www.drupal.org/project/jsonapi_extras and https://www.drupal.org/project/big_pipe_sessionless are doing it.

  2. Sure. But too few people know that decorating is even an option. You can't always use this pattern, but when you can, it's a huge win. \Drupal\big_pipe_sessionless\Render\BigPipeSessionless extends BigPipe but adds an additional service dependency via setPageCacheMiddleware(). Maybe I got lucky there. Maybe we in Drupal don't tend to design our services well. Maybe a bit of both.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

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

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

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.

akalam’s picture

akalam commented

I came to here while checking if there was a standard way of doing dependency injection in core. Even if it's a bit more difficult to maintain to call the parent constructor I thought there was a reason for that: make the code testable. If the dependencies are passed to the constructor, a phpunit test can create the object passing mocked classes to the constructor so test is not affected by failures in other classes, among being more performative. Am I missing something?

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.

gaëlg’s picture

gaëlg
he/him
Primary language French
Location Lille, France
commented

@akalam I ended here searching for the same. After a bit of reading, here's what I found out:

  • AFAIK, there is no official way to subclass dependency-injecting classes (services, plugins,...).


  • For services, it's usually better to decorate than subclass when possible (for plugins, I guess it's too complex to decorate).


  • It's not official, but if you have to subclass and do not control the parent class (you're in custom or contrib and parent class is in core or other contrib), you should not override the constructor but rather inject additional needed services directly in create(), as explained in various blog posts such as https://www.hashbangcode.com/article/drupal-9-extending-drupal-base-clas... and https://www.previousnext.com.au/blog/safely-extending-drupal-8-plugin-cl...


  • Why? Because your code won't break if the parent constructor changes its signature.


  • Drupal core still overrides constructors because it can: it controls the parent classes, which are in core too. This may explain why the bad practice of overriding constructors is widespread, because core is taken as an example.


  • Solely injecting additional needed services in create() and not in constructor can be a problem in one case: unit tests. So, if/when you need to unit-test your class, you can declare a setter that allows you to set the service just after construction.


  • Some notorious contrib modules have got rid of the constructor-overriding bad practice, such as Webform: https://www.drupal.org/node/3076421

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.

prudloff’s picture

prudloff commented

This change encourages extending plugin constructors: https://www.drupal.org/node/3542837
If the constructor changes, this will break.