Do not override constructors for service objects, plugins, and controllers

[GueGuerreiro]

Problem/Motivation

According to the Drupal 8 Backward Compatibility Policy, constructors for service objects are considered internal, which means they are subject to change on a minor release:

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.

The implication is that if we override their constructors, and they happen to change their signature on a minor release, it would require us to update our current code for all instances where we're overriding their constructors, to conform to the new signature. This shouldn't have to happen to conform to a minor release.

There is a good blog post from jrockwitz (maintainer of Webform for D8), that goes into more detail: https://www.jrockowitz.com/blog/webform-road-to-drupal-9

Solution

We can implement a new design pattern that would fix the problem.

As an example, from the controller command, injecting 3 services as an example.

Before:

...
  public function __construct(
    RequestStack $request_stack,
    LoggerInterface $logger,
    Client $httpClient) {
    $this->request = $request_stack->getCurrentRequest();
    $this->logger = $logger;
    $this->httpClient = $httpClient;
  }

  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('request_stack'),
      $container->get('logger.factory')->get('foo'),
      $container->get('http_client')
    );
  }
...

After:

...
  public static function create(ContainerInterface $container) {
    $instance = parent::create($container);
    $instance->request = $container->get('request_stack')->getCurrentRequest();
    $instance->logger = $container->get('logger.factory')->get('foo');
    $instance->httpClient = $container->get('http_client');
    return $instance;
  }
...

Some of the most used Drupal contrib modules are already using this design pattern, like the aforementioned Webform, Migrate Plus, Search API, etc.

Some extra benefits from this approach also include less code bloat:

• We no longer need a __construct() method, since the __construct() and create() methods are effectively merged into one.
• We no longer need a use statement per service injected. Their only purpose was so we could typehint on the constructor.

That reduces the final code lines by quite a bit. Also due to the above, adding a new service dependency injection after already running the generate command is no longer a major PITA.

I think this solution would include:

• A change in di.twig's container(services) macro that would use this new design pattern.
• A change to the generator twig template files, to use this new function, and also remove the now unnecessary use statements as well as the constructor.

I have a PR ready for these changes.

This solution could be applied to the following generators:

• [x] controller
• [x] block
• [x] views-argument-default
• [x] views-field

[Chi-teck]

I am aware about that approach. I think it was initially published in PreviousNext blog. https://www.previousnext.com.au/blog/safely-extending-drupal-8-plugin-classes-without-fear-of-constructor-changes

There are a few things that stoped me to apply that technique in my projects including DCG.

1. I've never experienced such a problem myself. Probably because Drupal core actually never changes contructor signutares?

2. If you are setting class properties in factory ::create method. You will not be able to instantiate the object properly via constructor ($plugin = new MyPlugin()) because it won't recieve all required dependencies. That means in Unit tests you will have to mock the whole container instead of particular dependencies.

3. This approach is not adopted in Drupal core itself.

By the way, the BC policy is not quite clear on this matter.

The constructor for a service object (one in the container), plugin, or controller is considered internal

But we are extening only abstract classes not real plugins or controllers. If constuctors are not open for extending why are they not final?

Also the approach with ::create() method is not applicable to services because they do not implement ContainerInjectionInterface or ContainerFactoryPluginInterface. I suppose we will have to set their dependencies through Setter Injection.

[andypost]

Re

1. I faced it few times, both custom code and contrib, core just started to use this approach because only in late 8.x we started to change constructors and this problem appear
2. wrong statement, if plugin has container injection it is not supposed to be created directly, so tests (unit ones specifically) must respect interface
3. core already adopted it for few forms, maybe few plugins too

[Chi-teck]

if plugin has container injection it is not supposed to be created directly

Factory method is just a helper to facilitate instantiation. Otherwise the object contructor would be protected or private.

[andypost]

Yes, that works but only in theory, in reality core still not adopted BC for constructors so setters is more clean approach but amount of boilerplate code for constructors convince to use the suggested approach

[Chi-teck]

Here is the quote from the original article.

From time to time you may find you need to extend another module's plugins to add new functionality.

But, we don't actually extend other module plugins. We only extend abstract classes that are supposed to be extended.

[GueGuerreiro]

There's precedent set with LinkBase: https://www.drupal.org/node/3023427 A constructor change happened, adding a new container injection to the constructor, and everyone extending it without the above design pattern would have had to change all their constructors to match.

[ghost]

One question I didn't see answered here yet is how does your IDE know the class of your service? All I've noticed is that this approach does not allow me to autocomplete functions on the properties. Which is annoying to say the least.

[GueGuerreiro]

The service instance should still be a property of your class, so that doesn't change. It works the same way with either approach.

For, say, an entity type manager service instance, you have:

  /**
   * An entity type manager service instance.
   *
   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
   */
  protected $entityTypeManager;

And a bit further down, the dependency injection itself:

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    $instance = parent::create($container);
    $instance->entityTypeManager = $container->get('entity_type.manager');
    return $instance;
  }

And everytime you do $this->entityTypeManager, your IDE will know what type that property is, because you've already informed it that it is an object of type \Drupal\Core\Entity\EntityTypeManagerInterface.

Perhaps you're missing the property definition in your class?

[ghost]

@GueGuerreiro you're right. I was missing them. Maybe I generate a bit too much code these days expecting everything to be there.

[Chi-teck]

Closing this. We can reopen the issue once Drupal core adopts this approach.