Whenever I have done an audit for a Drupal codebase, one of the first things I manually review and profile is dependency injection anti-patterns in custom code and any contributed modules used. The anti-pattern isn't for accessing services statically through \Drupal: service. These anti-patterns live within the class's __construct method.

It's generally recommended to keep constructors as simple as possible when using dependency injection. The __construct method is used to assign the dependencies as properties. Putting additional logic into constructors can lead to unexpected behavior. This is an anti-pattern as it creates unintended side effects when the service is constructed. While I am not a die-hard follower of SOLID design patterns, adding extra logic in your object construction goes against the single responsibility principle because the constructor does more than assign dependencies.

Nearly all developers have seen or written one of the following lines of code in a service's __construct method.

• The code assigns the current request object to a property instead of the request stack.
• The code assigns entity storage to a property instead of the entity manager.
• The code assigns a configuration object to a property instead of the configuration factory.

The pattern here is taking the injected service and retrieving an object from that service to assign to a property. This causes additional code execution in your constructor. There are chances it can also lead to invalid objects. What if something in the system changes from service construction to your code's actual execution? What if your service became serialized and therefore you have stale objects?

If your dependent service is a stack or factory: do not invoke it in your constructor, only retrieve data at the time of need.

These can be seen as micro-optimizations. And that is true. But, in my time, I have encountered some quirky bugs caused by these anti-patterns. And if you're aiming for performance, every millisecond counts.

Assigning the current request object to a property instead of the request stack.

The request stack is a stack data structure. Requests are pushed in and may be popped. It's used to determine the main request (first pushed request) and the current request (the request at the bottom of the stack.) You may have the wrong request object in your logic by assigning the current request object as a property instead of the request stack.

$this->request = $request_stack->getCurrentRequest();

The request is now a snapshot of the current request when your service is constructed. A new request may have been pushed before your service interacts with the request. It's best to always retrieve the request from the stack at the time of need.

// __construct
$this->requestStack = $request_stack;

// method
$request = $this->requestStack->getCurrentRequest();

Assigning entity storage to a property instead of the entity manager.

The entity type manager is a plugin manager and handler factory. It is used to return entity type definitions and construct handler objects that interact with that entity type. The most common example is setting an entity type's storage handler as a property.

$this->nodeStorage = $entity_type_manager->getStorage('node');

By calling getStorage, the entity type manager may perform plugin discovery if there is a cache miss on plugin definitions. It also wastes time if that entity storage isn't used. For example, an event subscriber manipulates entities based on conditions. It doesn't load or query entities if the conditions are unmet. In this case, the entity storage is never used and never had to be created by the factory.

// __construct
$this->entityTypeManager = $entity_type_manager;

// method
$node_storage = $this->entityTypeManager->getStorage('node');

Assigning a configuration object to a property instead of the configuration factory

This anti-pattern is the most common, as everyone works with configuration objects. The configuration factory gets injected into the service, retrieving the configuration object during service construction. This involves creating objects from a factory during construction instead of at time of need. Loading configuration objects should be delegated until the configuration object is needed.

$this->config = $config_factory->get('mymodule.settings');

I have also seen developers interact with the configuration in the object to set up other properties as flags inside a service. If you need these properties to act as flags from configuration values, consider how or why those properties are used. It has a code smell for a refactoring target.

// __construct
$this->configFactory = $config_factory;

// method
$config = $this->configFactory->get('mymodule.settings');