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.

#1981516: Refactor ConfigStorageController to use entity query

CommentFileSizeAuthor
#98 1909418-98.patch47.09 KBdamiankloip
#96 1909418-95.patch47.08 KBdamiankloip
#94 1909418-94.patch767 bytesdamiankloip
#94 interdiff-1909418-94.txt767 bytesdamiankloip
#92 1909418-92.patch47.23 KBdamiankloip
#92 interdiff-1909418-92.txt794 bytesdamiankloip
#88 drupal-1909418-88.patch47.25 KBdawehner
#88 interdiff.txt519 bytesdawehner
#86 drupal-1909418-86.patch47.55 KBdawehner
#86 interdiff.txt5.46 KBdawehner
#83 1909418-83.patch45.41 KBdamiankloip
#83 interdiff-1909418-83.txt12.08 KBdamiankloip
#82 1909418-82.patch44.52 KBdamiankloip
#82 interdiff-1909418-82.txt5.35 KBdamiankloip
#78 1909418-78.patch44.64 KBdamiankloip
#78 interdiff-1909418-78.txt2 KBdamiankloip
#73 drupal-1909418-73.patch59.67 KBtim.plunkett
#73 interdiff.txt2.37 KBtim.plunkett
#71 entity-controllers-1909418-71.patch59.47 KBtim.plunkett
#71 interdiff.txt8.65 KBtim.plunkett
#69 entity-controllers-1909418-69.patch48.42 KBtim.plunkett
#65 drupal-1909418-65.patch48.81 KBdawehner
#65 interdiff.txt4.51 KBdawehner
#61 entity-controller-1909418-61.patch44.36 KBtim.plunkett
#61 interdiff.txt7.91 KBtim.plunkett
#57 interdiff.txt4.96 KBtim.plunkett
#57 entity-controller-injection.patch43.3 KBtim.plunkett
#51 entity-handler-1909418-51.patch43.22 KBtim.plunkett
#45 1909418-entity-controller-45.patch43.3 KBkim.pepper
#41 entity-controller-1909418-41.patch43.44 KBtim.plunkett
#37 entity-controllers-di-1909418.37.interdiff.txt1.44 KBlarowlan
#37 entity-controllers-di-1909418.37.patch43.45 KBlarowlan
#34 entity-controller-1909418-34.patch43.61 KBtim.plunkett
#34 interdiff.txt27.6 KBtim.plunkett
#31 entity-controllers-di-1909418.31.interdiff.txt8.78 KBlarowlan
#31 entity-controllers-di-1909418.31.patch20.85 KBlarowlan
#25 entity-controllers-di-1909418.24.interdiff.txt5.14 KBlarowlan
#25 entity-controllers-di-1909418.24.patch21.32 KBlarowlan
#19 drupal-1909418-19.patch20.48 KBdawehner
#19 interdiff.txt3.66 KBdawehner
#16 entity-controllers-di-1909418.16.interdiff.txt2.79 KBlarowlan
#16 entity-controllers-di-1909418.16.patch20.16 KBlarowlan
#13 entity-controllers-di-1909418.patch20.95 KBlarowlan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fubhy’s picture

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? :)

tstoeckler’s picture

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.

dawehner’s picture

I totally agree. Once we have figured out the plugin manager one, we would just be able to use the same kind of container object to create those instances or use Drupal::() that's not clear.

larowlan’s picture

Assigned: Unassigned » larowlan

going to see if this one isn't beyond me

Crell’s picture

Title: Controller Injection » Entity Controller/Handler Injection

Clarifying title.

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.

Berdir’s picture

@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.

larowlan’s picture

Assigned: larowlan » Unassigned

Yeah #5 is beyond me, #3 made sense

dawehner’s picture

The entity controllers should very much be "true" services,

Can you describe your thoughts a bit more? I think your point could be, that entity controllers are logic objects, which deal with data directly, like different kind of cache backends.

Crell’s picture

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.

Berdir’s picture

@Crell 45 entity types, not controllers :) Assuming we have ~3 controllers per entity type, that's 120. And not everything has been converted to (config) entities yet, e.g. node types and fields/instances. If we split storage controller (generic + type specific one), that means +1 for all.

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.

Crell’s picture

Where and how do you specify the class on which to call create()?

Berdir’s picture

larowlan’s picture

Status: Active » Needs review
FileSize
20.95 KB

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.

Status: Needs review » Needs work

The last submitted patch, entity-controllers-di-1909418.patch, failed testing.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -278,7 +297,7 @@ public function deleteRevision($revision_id) {
+      $this->databasedelete($this->revisionTable)

@@ -483,12 +502,12 @@ public function delete(array $entities) {
-      db_delete($this->entityInfo['base_table'])
+      $this->databasedelete($this->entityInfo['base_table'])
...
-        db_delete($this->revisionTable)
+        $this->databasedelete($this->revisionTable)

Missing ->

larowlan’s picture

Status: Needs work » Needs review
FileSize
20.16 KB
2.79 KB

Find and replace fail.

Status: Needs review » Needs work

The last submitted patch, entity-controllers-di-1909418.16.patch, failed testing.

larowlan’s picture

Any reason why menu link rebuild doesn't use the entity manager?

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.66 KB
20.48 KB

The problem is that menu_link module is not enabled, which means that the entity manager doesn't have the menu link entity annotation scanned, so the storage controller is not available.

Just some minor fixes.

Status: Needs review » Needs work

The last submitted patch, drupal-1909418-19.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review
+++ b/core/lib/Drupal/Core/Entity/EntityHandlerInterface.php
@@ -0,0 +1,39 @@
+  public static function createInstance(ContainerInterface $container, Array $arguments);

Minor nit: I think we're using lowercase "array" for type hinting.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -133,6 +135,13 @@
 class EntityManager extends PluginManagerBase {
 
   /**
+   * 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.

Are we really, really sure there's no other option?

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -265,7 +277,14 @@ public function getControllerClass($entity_type, $controller_type, $nested = NUL
-      $this->controllers['storage'][$entity_type] = new $class($entity_type);
+      if (in_array('Drupal\Core\Entity\EntityHandlerInterface', class_implements($class))) {
+        $this->controllers['storage'][$entity_type] = $class::createInstance($this->container, array(
+          'entity_type' => $entity_type
+        ));
+      }
+      else {
+        $this->controllers['storage'][$entity_type] = new $class($entity_type);
+      }
     }

This is fine for now, but if we're going to do this we should just damn well do it. No option here. Entity Handlers must be injected.

larowlan’s picture

Assigned: Unassigned » larowlan

I have an idea about how to inject specific services into the controllers without their create method requiring the container. Only the entity manager will need it then.

larowlan’s picture

This is fine for now, but if we're going to do this we should just damn well do it. No option here. Entity Handlers must be injected.

Agreed, this is just a proof of concept.

larowlan’s picture

Assigned: larowlan » Unassigned

After reading again, realised Crell is talking about the manager, so my idea is no better.
Not much of an argument, but I type hinted the interface.

larowlan’s picture

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.

Berdir’s picture

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.

tim.plunkett’s picture

ControllerResolver::createController() uses a similar pattern with ControllerInterface, but that's not temporary, those checks will remain.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -112,14 +115,33 @@ class DatabaseStorageController implements EntityStorageControllerInterface {
+  public static function createInstance(ContainerInterface $container, array $arguments) {
+    return new static(
+      $arguments['entity_type'],
+      $container->get('database')
+    );

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -301,7 +331,16 @@ public function getListController($entity_type) {
+      // @todo remove this once all controllers implement
+      // Drupal\Core\Entity\EntityHandlerInterface.
+      if (in_array('Drupal\Core\Entity\EntityHandlerInterface', class_implements($class))) {
+        $this->controllers['form'][$operation][$entity_type] = $class::createInstance($this->container, array(
+          'operation' => $operation
+        ));
+      }
+      else {
+        $this->controllers['form'][$operation][$entity_type] = new $class($operation);

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.

larowlan’s picture

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.

msonnabaum’s picture

If you'd always call setEntityType, why not just add type to the factory method signature? If a controller object isn't complete until that's set, it should probably be in the constructor.

msonnabaum’s picture

Oops, should have refreshed before I posted. Agree with #28 :)

larowlan’s picture

So this adds the three arguments received by each of the constructors as parameters to the createInstance method on the interface. Those that aren't always required have defaults of NULL.

Each controller's createInstance method can pass on the required arguments to the constructor as required.

Entity type is required but the form controller createInstance would just ignore that argument.

Also renamed to EntityControllerInterface, there is another issue tracking renaming all of the entity controllers to handlers (they're not controllers in the true sense of the word).

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityControllerInterface.phpundefined
@@ -30,10 +31,13 @@
+  public static function createInstance(ContainerInterface $container, $entity_type, EntityStorageControllerInterface $storage = NULL, $operation = NULL);

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.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Working on a patch with my suggestions, and some more conversions to get a feel for it.

tim.plunkett’s picture

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.

Status: Needs review » Needs work

The last submitted patch, entity-controller-1909418-34.patch, failed testing.

larowlan’s picture

Need to add the third arg to the new MenuLinkStorageController() call in _menu_navigation_links_rebuild() if someone beats me to it, if not - will be tomorrow.

larowlan’s picture

Status: Needs work » Needs review
FileSize
43.45 KB
1.44 KB

Fingers crossed.
Also re-roll for the service container yml changes.

Status: Needs review » Needs work

The last submitted patch, entity-controllers-di-1909418.37.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
msonnabaum’s picture

I'm happy with #37.

tim.plunkett’s picture

Was trying to do

-    $query = \Drupal::service('request')->query;
+    $query = $this->request->query;

Now that hunk is

-    $query = \Drupal::request()->query;
+    $query = $this->request->query;
larowlan’s picture

#41: entity-controller-1909418-41.patch queued for re-testing.

larowlan’s picture

Status: Needs review » Needs work

The last submitted patch, entity-controller-1909418-41.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
43.3 KB

Re-rolled on latest head, fixed a couple of merge conflicts.

tim.plunkett’s picture

#45: 1909418-entity-controller-45.patch queued for re-testing.

msonnabaum’s picture

Status: Needs review » Reviewed & tested by the community

Still looks good to me.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs architectural review, +WSSCI Conversion

The last submitted patch, 1909418-entity-controller-45.patch, failed testing.

Sylvain Lecoy’s picture

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.

Secondly, as stated in Avoid your code becoming dependent on the container, it is not a good practice to let objects configure themselves through a container instance.

Instead, we should configure the container to inject dependencies:

<?php

$container->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.

Then we can refactor some of the EntityManager functions, such as:

<?php
  public function hasController($entity_type, $controller_type) {
    return $this->container->has("$entity_type.controller.$controller_type");
  }

  // Remove the getControllerClass() if not used.

  public function getStorageController($entity_type) {
    return $this->container->get("$entity_type.controller.storage");
  }
?>

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.

The manager would be the top-level class webchick is talking about in #1875086-48: Improve DX of drupal_container()->get(), handling the injector crap, with nice @return typehint for improved DX.

Should I work on a patch ?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
43.22 KB

Nope, that sounds like a completely different approach to how we're done all other controller injection. I'd suggest opening a new issue.

This is just s/_controller_class//, thanks to #1807042: Reorganizie entity storage/list/form/render controller annotation keys

Sylvain Lecoy’s picture

I thought this issue was asking architectural review, that's why I proposed an alternative approach.

tim.plunkett’s picture

Ah, my apologies. That tag predates the usage of this pattern in other places, I think we're pretty happy with it.

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -9,11 +9,38 @@
+   * @param \Drupal\user\TempStoreFactory $temp_store_factory
+   *   The factory for the temp store object.
+   */
+  public function __construct($operation, AliasManagerCacheDecorator $path_alias_manager) {

This comment seems wrong.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -34,10 +37,17 @@ class MenuLinkStorageController extends DatabaseStorageController {
   /**
+   * @var \Symfony\Cmf\Component\Routing\RouteProviderInterface
+   */
+  protected $routeProvider;

Can we describe the full variable?

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -34,10 +37,17 @@ class MenuLinkStorageController extends DatabaseStorageController {
+  public function __construct($entityType, Connection $database, RouteProviderInterface $route_provider) {

Needs documentation. We ad new parameters, so we need new @param

+++ b/core/modules/user/lib/Drupal/user/UserStorageController.phpundefined
@@ -20,6 +24,42 @@
+  public function __construct($entity_type, Connection $database, PasswordInterface $password, UserDataInterface $user_data) {

need docs.

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewAddFormController.phpundefined
@@ -8,14 +8,45 @@
+  /**
+   * @var \Drupal\views\Plugin\ViewsPluginManager
+   */
+  protected $wizardManager;

Docs

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewEditFormController.phpundefined
@@ -13,11 +13,57 @@
+  /**
+   * Implements \Drupal\Core\Entity\EntityControllerInterface::createInstance().
+   */
+  public static function createInstance(ContainerInterface $container, $entity_type, $operation = NULL) {
+    return new static(

Can be {@inheritdoc}

Sylvain Lecoy’s picture

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 ?

Sylvain Lecoy’s picture

For more information on how I see this - I may be wrong, please tell me - check the branch 7.x of the Doctrine ORM project.

Here you'll have a doctrine.inject.inc file which leverage the ContainerBuilder to configures its dependencies, and a service locator function called doctrine().

<?php
/**
 * Gets the doctrine Entity Manager configured for Drupal.
 *
 * @return Doctrine\ORM\EntityManager
 */
function doctrine() {
  // Hard dependency on Dependency Injection component.
  return drupal_container()->get('doctrine.orm.manager');
}
?>

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 schema driver implementation.
  $container->setDefinition('doctrine.orm.driver', new Definition('Drupal\doctrine\Schema\SchemaDriver'));

  // 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')));

  // Registers doctrine entity manager.
  $container->setDefinition('doctrine.orm.manager', new Definition(
    'Doctrine\ORM\EntityManager',
    array(new Reference('doctrine.dbal.connection'), new Reference('doctrine.orm.configuration'))
  ))
  ->setFactoryClass('Doctrine\ORM\EntityManager')
  ->setFactoryMethod('create');
}
?>

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
43.3 KB
4.96 KB

Addresses #54.

Sylvain Lecoy’s picture

Status: Needs review » Needs work

Very first requirement is still not met (Entity Controllers need to be services).

Either change the issue summary or implement differently the component instantiation.

Sylvain Lecoy’s picture

Status: Needs work » Needs review

Never mind I've created a task, 50% of the requirement in this issue, the other 50% in the issue bellow: #1980310: Entity Controller/Handler as a service. That's how we like drupal I guess :-)

dawehner’s picture

I'm sorry :( Some of that has been part of my previous review

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -77,11 +81,21 @@ class ConfigStorageController implements EntityStorageControllerInterface {
   /**
+   * @var \Drupal\Core\Config\ConfigFactory
+   */
+  protected $configFactory;
+
+  /**
+   * @var \Drupal\Core\Config\StorageInterface
+   */
+  protected $configStorage;
...
-  public function __construct($entityType) {
+  public function __construct($entityType, ConfigFactory $config_factory, StorageInterface $config_storage) {

Needs some documentation :(

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -112,14 +114,33 @@ class DatabaseStorageController implements EntityStorageControllerInterface {
+  /**
+   * Implements \Drupal\Core\Entity\EntityControllerInterface::createInstance().
+   */

@inheritdoc

+++ b/core/lib/Drupal/Core/Entity/EntityControllerInterface.phpundefined
@@ -0,0 +1,42 @@
+ * form|render|storage|access|translation|list controllers that require

Do we really want to list all kind of entity controllers here? Wouldn't that be possible extended by contrib, so that's never 100% accurate

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -9,11 +9,38 @@
+   * @param \Drupal\user\TempStoreFactory $temp_store_factory
+   *   The factory for the temp store object.
...
+  public function __construct($operation, AliasManagerCacheDecorator $path_alias_manager) {

Documentation and code does not match.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -34,10 +37,17 @@ class MenuLinkStorageController extends DatabaseStorageController {
+   * @var \Symfony\Cmf\Component\Routing\RouteProviderInterface
...
+  protected $routeProvider;

Needs docs.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -34,10 +37,17 @@ class MenuLinkStorageController extends DatabaseStorageController {
    * Overrides DatabaseStorageController::__construct().
...
+  public function __construct($entityType, Connection $database, RouteProviderInterface $route_provider) {

Relative to DatabaseStorageController, this adds new paramaters, so we maybe should add documentation as well.

+++ b/core/modules/user/lib/Drupal/user/UserStorageController.phpundefined
@@ -20,6 +24,42 @@
+  public function __construct($entity_type, Connection $database, PasswordInterface $password, UserDataInterface $user_data) {

docs.

+++ b/core/modules/user/lib/Drupal/user/UserStorageController.phpundefined
@@ -62,7 +102,7 @@ public function create(array $values) {
+      $entity->uid = $this->database->nextId(db_query('SELECT MAX(uid) FROM {users}')->fetchField());

Shouldn't we also use $this->database->query instead of db_query?

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewAddFormController.phpundefined
@@ -8,14 +8,45 @@
+   * @var \Drupal\views\Plugin\ViewsPluginManager

docs

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewAddFormController.phpundefined
@@ -8,14 +8,45 @@
+  /**
+   * Implements \Drupal\Core\Entity\EntityControllerInterface::createInstance().
+   */

@inheritdoc

tim.plunkett’s picture

Title: Entity Controller/Handler Injection » Allow Entity Controllers to have their dependencies injected
FileSize
7.91 KB
44.36 KB

Whoops, my interdiff in #57 wasn't actually applied to the patch in #57.

So this interdiff is against the patch in #57, and includes the last interdiff. Sorry for the confusion.

Status: Needs review » Needs work

The last submitted patch, entity-controller-1909418-61.patch, failed testing.

tim.plunkett’s picture

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).

Um, what?

dawehner’s picture

So again, that's another issue where something is in the form state which stores some kind of plugin manager (in this case the entity manager)
which then fails completely.

This time it's the DisplayOverview object, which always had the Entity Manager injected, which now fails. Maybe we have to go with the \Drupal:: approach as well?

dawehner’s picture

FileSize
4.51 KB
48.81 KB

What about this piece of code, to decouple this UI code from the storage controllers? It doesn't work yet, but it's just a general idea.

fago’s picture

The approach taken looks good to me, however the issue summary seems to say something else. Does it need an update?

dawehner’s picture

Status: Needs work » Needs review

Just see the errors. In general we would have to wait until the db connection is serialization.

Status: Needs review » Needs work

The last submitted patch, drupal-1909418-65.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
48.42 KB

I like the idea of only storing the objects when needed, but in general it might be more useful to contrib to have the service available, especially for subclasses.
Straight reroll of #65 after #1913618: Convert EntityFormControllerInterface to extend FormInterface

Status: Needs review » Needs work

The last submitted patch, entity-controllers-1909418-69.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
8.65 KB
59.47 KB

Needed changes for #1946404: Convert forms in field_ui.admin.inc to the new form interface.
Fixing the damn entity_type thing since that has broken my patches enough times.

Status: Needs review » Needs work

The last submitted patch, entity-controllers-1909418-71.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.37 KB
59.67 KB

Sloppy reroll on my part.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -32,6 +33,13 @@
+   * The injection container that should be injected into all controllers.

It feels wrong to name it "injected into all controllers", but it's rather passed to the controller factory.

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewEditFormController.phpundefined
@@ -12,11 +12,57 @@
    * Overrides Drupal\Core\Entity\EntityFormController::form().

Can we pull the request in the form method and set it to the form state? Paris suggested that to be the proper way instead of storing on the controller.

tim.plunkett’s picture

Issue tags: -WSSCI Conversion

#73: drupal-1909418-73.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSSCI Conversion

The last submitted patch, drupal-1909418-73.patch, failed testing.

yched’s picture

#73 includes field_ui snippets that look like a leak from #1982138: Clean out field_ui.admin.inc ?

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2 KB
44.64 KB

Rerolled and made those changes, I'm still nto sure about the request, so I've moved it into the form method instead.

Trying to move this along so #1981516: Refactor ConfigStorageController to use entity query can get in. Sorry Tim, I know this is assigned to you but it's been a few days.

Status: Needs review » Needs work
Issue tags: -WSSCI Conversion

The last submitted patch, 1909418-78.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review

#78: 1909418-78.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSSCI Conversion

The last submitted patch, 1909418-78.patch, failed testing.

damiankloip’s picture

FileSize
5.35 KB
44.52 KB
damiankloip’s picture

FileSize
12.08 KB
45.41 KB

Just talking to dawehner, we should/could also inject the entity info, then we remove the call to entity_get_info which is what we want anwyay.

damiankloip’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1909418-83.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
5.46 KB
47.55 KB

There has been a couple of issues with failing adapations and a problem with serialization on the drupal kernel.

yched’s picture

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -17,6 +17,7 @@
+use Symfony\Component\Serializer\SerializerInterface;

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)

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

dawehner’s picture

FileSize
519 bytes
47.25 KB

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).

Status: Needs review » Needs work

The last submitted patch, drupal-1909418-88.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

So it's: $form_state -> form controller -> entity manager -> container ( -> kernel).
And $form_state gets serialized.

That sounds exactly like the "reference chain turns to serialization hell" problem at its maximum ?
Serializing the container seems like a *really* bad idea :-/

Serializing the container sounds *really* bad :-/

dawehner’s picture

So it's: $form_state -> form controller -> entity manager -> container ( -> kernel).
And $form_state gets serialized.

That sounds exactly like the "reference chain turns to serialization hell" problem at its maximum ?
Serializing the container seems like a *really* bad idea :-/

Well, I personally think that we should not store the entity manager itself but just the controllers we need, like for example the storage controller.
This would allow us to get rid of that problem.

damiankloip’s picture

Assigned: tim.plunkett » Unassigned
FileSize
794 bytes
47.23 KB

Just fixing the tests.

Berdir’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/DrupalKernel/DrupalKernelTest.phpundefined
@@ -113,4 +113,21 @@ function testCompileDIC() {
+    // @todo: write a memory based storage backend for testing.
+    $module_enabled = array(
+      'system' => 'system',
+      'user' => 'user',

That @todo seems unecessary, no need to store something somewhere?

$module_enable isn't used.

+++ b/core/modules/system/lib/Drupal/system/Tests/DrupalKernel/DrupalKernelTest.phpundefined
@@ -113,4 +113,21 @@ function testCompileDIC() {
+    $this->assertTrue($unserialized_kernel instanceof DrupalKernel);

I guess there's not much more that can be asserted here?

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.

damiankloip’s picture

FileSize
767 bytes
767 bytes

So I think we can just remove this stuff?

Status: Needs review » Needs work

The last submitted patch, 1909418-94.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
47.08 KB

With the actual new patch and not just interdiffs

Status: Needs review » Needs work

The last submitted patch, 1909418-95.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
47.09 KB

Rerolled after the language patch just got in.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

We'll need a follow-up for __sleep()/__wakeup() but this is ready to go and blocks a huge chunk of issues.

Berdir’s picture

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.

tim.plunkett’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 7961e03 and pushed to 8.x. Thanks!

yched’s picture

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -134,6 +134,21 @@ public function __construct($environment, $debug, ClassLoader $class_loader, $al
   /**
+   * {@inheritdoc}
+   */
+  public function serialize() {
+    return serialize(array($this->environment, $this->debug, $this->classLoader, $this->allowDumping));
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function unserialize($data) {
+    list($environment, $debug, $class_loader, $allow_dumping) = unserialize($data);
+    $this->__construct($environment, $debug, $class_loader, $allow_dumping);
+  }
+
+  /**

Could we just add a @todo to remove those once #2004282: Injected dependencies and serialization hell is resolved ?
Trying to serialize the kernel is the sign of something terribly wrong, and IMO *should* break rather than silently succeed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

jibran’s picture

Title: Allow Entity Controllers to have their dependencies injected » Change notice: Allow Entity Controllers to have their dependencies injected
Priority: Normal » Critical
Status: Closed (fixed) » Active
Issue tags: +Needs change record

Sorry for the noise but i think this needs change notification.

dawehner’s picture

I can't even one for entity controllers itself.

dawehner’s picture

Status: Active » Needs review
andypost’s picture

Title: Change notice: Allow Entity Controllers to have their dependencies injected » Allow Entity Controllers to have their dependencies injected
Priority: Critical » Normal
Status: Needs review » Fixed

@dawehner thanx!

smiletrl’s picture

createInstance method

probably is not a consistent method of 'create' to inject dependency across core code:(

See alternative proposal at #2015535: Improve instantiation of entity classes and entity controllers.

dawehner’s picture

Yeah let's fix the changenotice once your other issue is in. I totally agree.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

xjm’s picture

Issue tags: -Needs change record

Untagging. Please remove the tag when the change notification task is completed.

xjm’s picture

Issue summary: View changes

Updated issue summary.