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
🌱
Use final to define classes that are NOT extension points
Active
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