#1909418: Allow Entity Controllers to have their dependencies injected Brings good ideas in term of object encapsulation but instantation of components is still done the old way (through "isset" and "if" mechanisms). The controllers are ContainerAware but does not seems to use the provided ContainerAwareInterface, thus it seems it was not invented here and is bad practice in general: controllers configures themselves in a static createInstance method. Moreover the complexity of discovering controllers can be cached at container compile-time, avoiding expensives operations at runtime.
The proposed service name for these controllers would be:
'ENTITY_TYPE.controller.CONTROLLER_TYPE' which is a sort of namespace by component.
There is also the option of using: 'entity.ENTITY_TYPE.CONTROLLER_TYPE'
For the user storage controller:
<?php
$container->setDefinition('user.controller.storage', new Definition('Drupal\user\UserStorageController',
array('user', new Reference('database'), new Reference('password'), new Reference('user.data')));
?>Then the refactored Manager, or "service locator".
<?php
public function hasController($entity_type, $controller_type) {
return $this->container->has("$entity_type.controller.$controller_type");
}
// TODO: Remove the getControllerClass().
/**
* @return Drupal\Core\Entity\EntityStorageControllerInterface
*/
public function getStorageController($entity_type) {
return $this->container->get("$entity_type.controller.storage");
}
?>We can then get rid of EntityControllerInterface, which mimics the provided ContainerAwareInterface in term of functionalities. Remove the createInstance methods and de-couple controllers from container. Tree inheritance and dependency management becomes less complexe and easier to maintain as we all know principles of good programming: loose coupling and high cohesion.
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | entity-1980310-14.patch | 3.32 KB | tim.plunkett |
Comments
Comment #0.0
Sylvain Lecoy commentedtypo
Comment #0.1
Sylvain Lecoy commentedtypo
Comment #1
Sylvain Lecoy commentedIn the code, we'll also avoid the need for:
But instead use:
A bit like the Drupal super-class service locator. Of course experimented programmers who need menu storage as dependency of their object can still configure the container to build their object and inject dependencies for them. The EntityControllerLocator would be then a short-cut method useful to know the name of services and to help procedural code leveraging lazy-loading of components.
Comment #2
fubhy commentedWe had a similiar issue a while ago: #1821662: Register entity controllers with the DIC
Comment #3
Sylvain Lecoy commentedSo let's use the
'entity.ENTITY_TYPE.CONTROLLER_TYPE'pattern :-)Comment #4
Sylvain Lecoy commentedThe basic idea is then to register all controllers through *.services.yml.
Here we'll be able to override controllers thanks to Symfony overriding service capability, hence we'll eliminate the need for entity_info_alter when redefining a specific controller.
However, I am blocked with form controller as they need an $operation argument.
Having a look at EntityFormController, the $operation class member does not have any cohesion with the class, other than an accessor getOperation() defined through EntityFormControllerInterface. Low cohesive classes are not only a bad design habit, but it also prevent me configuring the service within the container, as this operation argument is a runtime parameter.
To fully implement this patch, I'll have to remove the operation from constructor and instead pass it as an argument in a setOperation method for now.
Comment #5
andypost@Sylvain Lecoy +1 to idea!!!
I really dislike the way to store container in entity manager as approached in #1909418: Allow Entity Controllers to have their dependencies injected
It brings us to situation when EM and container have circular references
'entity.ENTITY_TYPE.CONTROLLER_TYPE'should be added to summaryWaiting a patch to review!
Comment #6
tim.plunkettBy my count, there are 115 entity controllers in core right now, and we're not done.
What is your plan to scale that to contrib? Crell has estimated that the scaling of the DIC breaks down around 2000 services.
Comment #6.0
tim.plunketttypo
Comment #7
msonnabaum commentedThey are not container aware. The instances have no knowledge of it, only the factory method. It's no different than having a separate factory class.
Comment #8
Sylvain Lecoy commentedIt is a difference as it violate SRP rule. Factory are a separate class for a reason: to loosen the coupling.
Comment #9
msonnabaum commentedIt really doesn't violate SRP at all. The downsides to violating SRP are nonexistent here. It's simply Factory Method.
Also, SRP is in no way a rule that we need to adhere to 100% (not that this is an issue of violating it…)
Comment #10
Sylvain Lecoy commentedFactories are a well-known creational pattern defined and used since 1994 and maybe before. The abstract factory provide an interface for creating families of related or depedent object without specifying their concrete classes while the factory method defines an interface for creating an object, but let subclasses decide which class to instantiate. Thus, Factory Method lets a class defer instantiation to subclasses.
The controller object, by implementing this behaviour, breaks SRP. This object set of feature has been augmented, to include a factory. Plus, we also have augmented the coupling between the Controller and the Container. By configuring the controller through an external factory, we loosen the coupling. Best; if we use correctly DI mechanisms, we don't even need factories and we elminates the need for an external factory.
Comment #10.0
Sylvain Lecoy commentedtypo
Comment #11
Sylvain Lecoy commentedRelated problem in plug-in definitions where we lock the __construct() signature as highlighted in #1821662-11: Register entity controllers with the DIC.
Comment #12
effulgentsia commentedThe routing system allows routing.yml files to specify (route) controllers either as classes or as service ids. Is there any reason for EntityManager not to allow @EntityType annotations to specify (entity) controllers the same way? If not, would it make sense to use this issue to implement the needed changes to support that choice, and then have a follow up for discussing which option should be used by core when?
If this was already discussed and shot down in other issue comments, please provide links to them.
Comment #13
tim.plunkett#2168333: Ensure custom EntityType controllers can be provided by modules would pave the way for that.
Though in *.routing.yml, you must specify a method as well, which is not applicable here.
Comment #14
tim.plunkettActually nevermind my worry in #13, I was thinking of #2165475: Provide a generic class resolver, and that doesn't even handle the service part.
This will not apply now, but will work fine once #2168333: Ensure custom EntityType controllers can be provided by modules is in.
Comment #15
eclipsegc commentedFWIW, for those of you who've seen my attempt at utilizing events for entity controller instantiation, it was trying to find a middle ground between what tim is doing in #14 and what core does currently. This would basically remove my existing arguments against what core does (though I don't see a need for the static createInstance() method if we support this, but w/e seems like the sort of thing we could discuss later).
Eclipse
Comment #16
tim.plunkettThe reason to keep that is because we're not *forcing* anything. Both a vanilla class, and one implementing ContainerInjectionInterface would continue to work. That's how route controllers work.
Comment #17
tim.plunkettAlso, since this is an established pattern already, I don't see how using events for declaring something like this would be a middleground. That was a completely new approach we hadn't seen before, which is why I think there was pushback.
(The approach #16 mentions is https://gist.github.com/EclipseGc/8385561)
Comment #18
eclipsegc commentedYeah, the one big difference is that you can't iteratively create a bunch of services, though the flips side to that is that we can process the definition to add those, again that leaves contrib out in the cold to a certain degree, but it's much less of an issue with this case I think. After a night of sleep, still ++
Eclipse
Comment #19
dawehnerWell, I think we should just add that.
Comment #33
smustgrave commentedComment #34
tim.plunkettConfirmed.