While discussing
π
Deprecate empty AccessInterface and remove usages
Needs work
/
#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']
Remaining tasks
User interface changes
API changes
Data model changes