While discussing #2266817: Deprecate empty AccessInterface and remove usages/#2222719: Use parameter matching via reflection for access checks instead of pulling variables from request attributes, it is clear that it is difficult to wire dependencies in Drupal, while adhering to good OOP practices. A dependency on symfony/expression-language was previously removed (#2747083: drupal/core-dependency-injection wrongly requires symfony/expression-language) due to being unused, and not required. In this issue, I am suggesting how to make use of it.
Problem/Motivation
The structure of many components in Drupal mean that services have dependencies on objects that aren't constructed by the dependency injection container, and therefore do not have direct access to them. As a result of this, services are required to depend on the parent service, and attempt to extract the actual dependency from that service.
An example would be that you were designing a service that needed to load nodes from the database, your service now has a dependency on Drupal\node\NodeStorage
. This dependency is constructed by Drupal\Core\Entity\EntityTypeManager
as a result of Drupal\Core\Entity\Annotation\EntityType
expecting a list of classes via handlers = { ... }
. This means that the only way to access this service is by using a dependency on entity_type.manager
, abstracted by the Drupal\Core\Entity\EntityTypeManagerInterface
, and call EntityTypeManagerInterface::getStorage(): EntityStorageInterface
to get your real dependency. Also, since we are strictly depending on the node storage, we don't really want it to look like any other entity storage can be injected, and so we need to instead depend on Drupal\node\NodeStorageInterface
.
This implementation breaks Law of Demeter and SOLID principals, as we are injecting a service, only to retrieve another other object - this requires nested stubbing/mocking in order to unit test. As well as this, Lack of interface segregation means that your service is aware of all of the methods on both EntityTypeManagerInterface
and EntityStorageInterface
, when you only actually want to call the rather obscure and nested NodeStorageInterface::loadMultiple(): EntityInterface[]
.
So your class must look like this:
# services.yml
mymodule.service:
class: Drupal\mymodule\Service
arguments:
- '@entity_type.manager'
class Service {
protected $nodeStorage;
public function __construct(EntityTypeManagerInterface $entityTypeManager) {
$this->nodeStorage = $entityTypeManager->getStorage('node');
if (!$this->nodeStorage instanceof NodeStorageInterface) {
throw new \InvalidArgumentException(...);
}
}
And really, you still only know that you are getting EntityInterface[]
when you do call $this->nodeStorage->loadMultiple($nodeIds);
, rather than actual nodes (Drupal\node\Entity\Node[])
.
Another example is #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API, addressed via Drupal\Core\Routing\RouteMatchInterface
(#2238217: Introduce a RouteMatch class), which probably could be better segregated also. The solution being pursued here is using reflection to decide which dependencies to pass into a service method, which does solve the issue with Law of Demeter, however causes more SOLID issues.
It can sometimes be difficult to wire dependencies in a massive application, however the more components fail to stick to the basics will mean that the they aren't truly decoupled, and it will become much harder to refactor later.
Proposed resolution
Symfony's dependency injection component takes advantage of the expression language component, to allow you to depend on things like nested services or properties. This means that your code can be written cleanly, and you can just let the container care about wiring:
# services.yml
mymodule.service:
class: Drupal\mymodule\Service
arguments:
- "@=service('entity_type.manager').getStorage('node')"
class Service {
protected $nodeLoader;
public function __construct(NodeMultipleLoaderInterface $nodeLoader) {
$this->nodeLoader = $nodeLoader;
}
This means that the code is completely future-proof to being re-architected (at the container level), which may be necessary where expressions are used repeatedly. For instance, if this is happening a lot, then perhaps node module is pushed to expose node.storage
:
# node.services.yml
node.storage:
factory: ['@entity_type.manager', getStorage]
arguments: ['node']
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedGroovyCarrot created an issue. See original summary.
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedJust to add another good use-case for this:
You could instead just inject the config you need:
Comment #3
alexpottI really like this idea. Does it actually work with the Drupal container?
Comment #4
pounardIf you are using the standard symfony dic, it should work with Drupal container, I guess.
Comment #5
alexpottOur container is based on the Symfony DIC but not quite the same - hence #2747083: drupal/core-dependency-injection wrongly requires symfony/expression-language
Comment #6
dawehnerI really like this for things like entity storages. One thing we should though keep in mind: Calling out to other services on service constructor time can deal to really hard to debug problems.
Comment #7
pounardI guess so, but under the hood you should have a real container builder ? Anyway if this doesn't work it would seem that you seriously mess up the original Symfony's container, but I guess this would be easily fixable.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedI did have a rough go a prototyping it a while back, but found that either some effort had been taken to remove the compatibility in the first place, or when overriding functionality in Symfony, the decision had been made to leave it out.
This is kind of related to a ticket I raised recently #2828099: Service container aliases do not work regarding functionality that has been lost from Symfony. I think that dawehner's (#4) comment is a good idea, it is probably worth either trying to apply any unit tests in Symfony for that method, or writing new unit tests to ensure that the functionality in Symfony, and new functionality for Drupal all works as expected.
Comment #9
dawehnerTo be clear we didn't rewrote the container for fun. We had to because the concept of compiling the running container to disk is highly problematic in a system like Drupal, where the webserver is able to alter this behaviour. Note: You can install modules via the UI. This causes a container rebuild, which requires a newly written container in the default symfony implementation. Now imagine having multiple webserver somehow propagating this information. Putting the container information into a cache, and by that sharing it automatically via the database solved a lot of problems.
Comment #10
pounard@dawehner I agree with you about the why, but expressions parser and factories should not be altered behaviours? If so it's probably a but that can be fixed, no matter if the container was overriden or not. I trust core developers for being able to untangle this and restore the common Symfony DIC behaviour.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedThere's certainly a lot more problems that Drupal has to solve than the average Symfony application. I just would say that this is probably a key step in pushing a unit testing initiative in Drupal; so that services can be properly isolated from one-another in the system, and you can properly stub direct/actual dependencies.
Comment #12
dawehnerOh yeah I think we just from a compability point of view, we should add support for it. The question of how often we should use it itself in core then is an additional question. Let's do it?
Comment #23
geek-merlinSome now and then i think that'd be fine.
Comment #26
joachim CreditAttribution: joachim as a volunteer commentedThe main use case for this, of injecting entity storage handlers, no longer applies, as there was a change in storage handlers that means they should no longer be injected but only instantiated when required.
Comment #27
sayco CreditAttribution: sayco at Droptica commentedAny updates on this?
Opposite to the comment above, I think entity storage is not the only case, there are multiple cases such as taking objects from factories - the config, queue, plugins, etc.
If there is a chance that it is easy to integrate (compatible with Drupal core container), then it is worth giving a try, especially if good practice/SOLID is at stake.