You are here

Allow Entity Controllers to have their dependencies injected

Requirements

Entity Controllers need to be services so they can get things like database connection or cache backends/bins injected (e.g. storage controller).

The entity manager should be able to get instances of the controllers, without getting them injected, as we want them to be lazy loaded.

Note: This requirement differs from the one outlined in #1863816: Allow plugins to have services injected because we don’t really want to inject the controllers into our plugin instances but instead register them with the plugin manager. In return, the plugin manager would get forwarded to the plugin instances upon instantiation so that we can easily access it in places like $entity->save(), $entity->delete() or $entity->view().

Proposal

Make controllers services, so they define their dependencies. The database entity storage controller, for example would get a database connection and a cache backend injected, whilst the config entity storage controller would get the config storage injected.

In order to let the entity manager lazy load controllers on the fly it should get a controller factory, which is itself ContainerAware. This allows the controller factory to load the required services from the container on demand, and keeping the memory footprint as low as possible.

We would then register our entity controller services with that controller factory... Possibly through a compiler pass or other means.

Developer Experience

This proposal requires controllers to be defined as services. Unless we want to come up with a syntax to define services including dependencies, etc. in the annotations of the controller class we will have to register our controllers manually (e.g. NodeBundle).

In the long run, however, we might be able to use this to decouple the controller definitions from the entity class annotations and instead simply rely on unique service names to register controllers for entity types. Simply put, we could turn the coupling of entity class and entity controllers upside down.

Comments

Short addition: If we keep the entity controller definitions in the annotations whilst trying to register them with a possible controller factory that would mean we would be reading from $entity_manager->getDefinitions() in a compiler pass to register controller instances with the factory which in return should actually get injected into the $entity_manager itself. Umh?! How much of a wtf is that? :)

Instead of registering the controllers in the DIC we can use the approach that we have for route controllers, and hopefully soon for plugins as well: a static create() factory on the controller itself. The entity manager would then be controller-aware and simply do $class::create($this->controller, $foo, $bar) instead of new $class($foo, $bar). I think that is the cleanest approach and is (hopefully) emerging as sort of a standard.

The entity controllers should very much be "true" services, and either registered in the DIC or spawned from it, meaning they can take everything from straight up constructor injection. No statics needed. Entity Managers should NOT be container aware.

@Crell: Keep in mind that we're talking about a lot of services here. I'm counting 45 entity types right now in Core, including a lot of test entity types and ~5 different controller classes (config entities usually don't use that many). We have to register them explicitly for every entity type as they get the type and entity info as constructor argument.

Berdir: 45 is not a lot of services. The container can scale to about 2000 entries without performance degradation on a stock APC configuration. (Higher than that, up your APC file size limit and you can scale as far as you want.)

My underlying point is that I am *not* in favor of using the create() static for them. That makes sense for things that we're going to have hundreds or thousands of: Plugins, controllers, etc. Controllers should be glue-code anyway, not carrying for-reals logic that we'd want to unit test.

Entity storage and management is not such a use case. They're carrying real logic that we absolutely do want to unit test, with PHPUnit, with mock objects. (That's what I mean by "real services".) Making things container aware undermines that.

And that's just core (probably half of those test types, though). Add a site with commerce, rules, search api and a few additional modules like that to the mix and it starts to get interesting :)

Entity storage and management is not such a use case. They're carrying real logic that we absolutely do want to unit test, with PHPUnit, with mock objects. (That's what I mean by "real services".) Making things container aware undermines that.

Agreed, but why does a create() method prevent that? We still have a constructor with explicit arguments.

So here's the ::create (factory) approach.
We have to use createInstance in this case because DatabaseStorageController::create already exists.
This patch adds Drupal\Core\Entity\EntityHandlerInterface as I thought EntityControllerInterface was misleading (they're not controllers in true sense of the word) but the name is just semantics.
Any entity controller (list, form, storage, translation, render) can implement that interface and then add a static createInstance method.
This receives the container as the first argument and then the other constructor arguments as a keyed array.
This patch changes the DatabaseStorageController and sub-classes to implement this interface and uses $this->database instead of the various db_select|update|insert|transaction functions as a POC.

/**+ * The injection container that should be injected into all controllers.+ *+ * @var Symfony\Component\DependencyInjection\ContainerInterface+ */+ protected $container;+

This makes the Entity system tightly coupled to the Symfony DIC. Are we really sure we want that? Really? We want our storage system to be non-functional without the DIC?

That half-defeats the purpose of using DI. We can still inject everything else, sure, but this makes EntityManager dependent on having the DIC, even for testing purposes. That is, unit testing it just got 10x harder.

Should fix test fails.
Discussed issue with injecting container interface into the entity manager with @dawehner on irc.
Quoting dawehner 'thought if we do unit tests we should always access/contstruct the entity controllers directly, so this shouldn't be a major problem?'
I tend to agree but let's see what others think.

Why make the create method required? It doesn't prevent controllers from not injecting their dependencies and some might actually not need any dependencies (test implementations if no others).

I think @EclipseGc and also others voted pretty strongly for not requiring such a method for plugins, so why do it here?

Patch looks otherwise quite fine to me, just wondering how far we want to go. Do we want to, as an example, convert the database storage controller to be fully injected (as much as possible) as an example or do that in follow-ups as it might get quite big, there are a lot of dependencies in there I think.

This is pretty crappy DX, since we end up with a magical array. What about instead of using a constructor for the pre-existing values, each controller type provides setters? It would help codify what each one needs. So EntityStorageControllerInterface would have a setEntityType() method, that EntityManager::getStorageController() would call.

Discussed this on irc, consensus is to add entity_type argument to form controller constructor.
Then we can add entity type argument to the createInstance method in the new interface.
Then we need setters on the list controller to set the storage controller and one on the form controller to set the operation.

AFAIK, if they are = NULL, putting them in the interface doesn't matter, and they can be left off all of the ones that don't need it.

So it could just be public static function createInstance(ContainerInterface $container, $entity_type); in the interface, and everyone else does public static function createInstance(ContainerInterface $container, $entity_type, $my_thing = NULL) { where needed.

So, unlike the ControllerInterface classes, where they're generally one-off and pretty slim, there is a good deal of extending here, which makes the __construct() a little brittle. But I still think this is a reasonable approach.

I am not sure we need those createInstance static functions in controllers classes, since anyway the container will manage instances for us. The EntityControllerInterface is for me not needed at all, and it adds complexity to controllers.

->setDefinition('user.controller.storage', new Definition('Drupal\user\UserStorageController', array('user', new Reference('database'), new Reference('password'), new Reference('user.data')));

?>

This will produces a semantical identical code in the dumped container than the static createInstance function, without the need of a new Interface which the only contract is to define a factory method, thus looks not correct for me.

For five methods, we are copy/pasting blatant logic which should be handled by the container itself. It is bad as it not fun to write nor to maintain, it is pure wiring glue code that someone has already written and tested before us, and it is passing a ContainerInterface implementation to our objects. While it is OK for the manager to have a container for DX shorthand, it is not for managed objects. A part from this disagrement, the other parts of the patch are for me really good, objects starts being awesome in Drupal :-)

Oviously we should first agree on whether we want to add these controllers definitions in container. But I already saw a topic on making these controllers services, so the idea is there. Secondly, the registration mechanism would be then delegated to pre-dump phase of the container, removing some discovery overhead at run-time, but assumes refactoring.

Is this a better approach to resolve at runtime the controller to instanciate or to configure the container so it is resolved at "compile-time" and comes for no cost at runtime ? I don't think...

Also, from my limited experience, successful software do not add a lot of interfaces, base class to extends, and so one... People like simplicity. That's why I don't think its a good idea to add another interface, plus there is already ContainerAwareInterface which is existing so why not reusing it ? Not invented here ? :(-

Eventually, I don't see the point creating a new issue when the first requirement is:

Entity Controllers need to be services so they can get things like database connection or cache backends/bins injected (e.g. storage controller).

If by service you mean contained and managed by the Symfony container then it is likely what I believe Crell thought it was initially and the patch proposed does not respect this requirement. If by service you mean a completely another story then I'll create a new issue/follow-up as suggested.

I just need to know where to register these services prior to "compile-time": in an EntityBundle ? in the CoreBundle ?

This service locator have obviously a hard dependency on the DI component but it is good as a shortcut for programmers. Advanced programmers who needs the EntityManager as dependency of their object might not use this locator function but instead wire their object to the 'doctrine.orm.manager' reference directly.

<?php/** * Implements hook_inject_init(). */function doctrine_inject_init(ContainerBuilder $container) { global $databases;// Configures an instance of Doctrine DBAL Connection which is a wrapper // around the underlying driver connection (which is in this case a PDO instance). // This connection details identify the database to connect to as well as the // credentials to use. The connection details can differ depending on the used driver.$container->setDefinition('doctrine.dbal.connection', new Definition('Doctrine\DBAL\Connection', array(doctrine_connection_params($databases['default']['default'])) )) ->setFactoryClass('Doctrine\DBAL\DriverManager') ->setFactoryMethod('getConnection');

// Registers doctrine configuration.$container->setDefinition('doctrine.orm.configuration', new Definition('Doctrine\ORM\Configuration')) ->setFactoryClass('Doctrine\ORM\Tools\Setup') ->setFactoryMethod('createConfiguration')// Sets the directory where Doctrine generates any proxy classes. // A proxy object is an object that is put in place or used instead of the // "real" object. A proxy object can add behavior to the object being proxied // without that object being aware of it. In Doctrine 2, proxy object are used // to realize several features but mainly for transparent lazy-loading.->addMethodCall('setProxyDir', array(variable_get('file_temporary_path', file_directory_temp())))// Sets the meta-data driver implementation that is used by Doctrine to // acquire the object-relational meta-data for entities. The meta-data driver // used here is the Drupal Schema API defined which is responsible to map // schema definitions, data type map, to Doctrine understandable language.->addMethodCall('setMetadataDriverImpl', array(new Reference('doctrine.orm.driver')));

By analogy, we can see the EntityManager as an EntityControllerLocator, which will provide typehinted controller through the container (see my comment in #50). I think caching these services in the container makes a lot of sense, like Crell already stated, container has been tested and can support more than 2000 services without any problem.

Exception: Drupal\field\Plugin\Core\Entity\FieldInstance::serialize() must return a string or NULL in serialize() (line 99 of /Users/tim/www/d8/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php).

Doesn't seem needed ? (Symfony's SerializerInterface is serialize() + *de*serialize() so it doesn't seem to be what is implemented here, but rather \Serializable)

Oh you are totally right I tried to go with the interface first but then I realized that overriding the other methods seems just better.

Also - why do we suddenly have a need to serialize the kernel ?

The form objects which build the form, are part of the form_state, which is serialized due to caching of forms. Previously we hadn't the container stored on the entity manger but now any kind of form controller, that stores the entity manager now has at some point the kernel. (as it's part of the container).

This looks nice otherwise, as discussed, we will have to inject the entity manager, no way around that. But what I think we should do, not necessarly in this issue, is implement __sleep()/__wakeup() where we throw out stuff we don't want to serialize (controllers, module handler, container, kernel, ....) and on wakeup get it back from the global container. I really think testability/mocking and stuff doesn't matter anymore at that point. In fact, we should probably actually make sure that we do have the current global services after wakeup as we might otherwise use stale data (e.g. an outdated field definition cache, in case someone added a field while we filled out the node form..., or invoke a module hook that was disabled in the meantime)

Speaking of the module handler, what about all the hooks in there? No need to bother about field attach callbacks though, that's being worked on in a different issue.

Note that this will at least logically conflict with #1893820: Manage entity field definitions in the entity manager, which accesses the entity manager from the storage controller and we'd have to inject that too, which in turn would make __sleep() stuff from #99 necessary here. I guess this is blocking more things, so probably has a higher priority.