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

Files: 
CommentFileSizeAuthor
#98 1909418-98.patch47.09 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,737 pass(es).
[ View ]
#96 1909418-95.patch47.08 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1909418-95.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#94 1909418-94.patch767 bytesdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1909418-94.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#94 interdiff-1909418-94.txt767 bytesdamiankloip
#92 1909418-92.patch47.23 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,890 pass(es).
[ View ]
#92 interdiff-1909418-92.txt794 bytesdamiankloip
#88 drupal-1909418-88.patch47.25 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 55,716 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#88 interdiff.txt519 bytesdawehner
#86 drupal-1909418-86.patch47.55 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,382 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#86 interdiff.txt5.46 KBdawehner
#83 1909418-83.patch45.41 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 53,565 pass(es), 412 fail(s), and 316 exception(s).
[ View ]
#83 interdiff-1909418-83.txt12.08 KBdamiankloip
#82 1909418-82.patch44.52 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 56,361 pass(es), 1 fail(s), and 97 exception(s).
[ View ]
#82 interdiff-1909418-82.txt5.35 KBdamiankloip
#78 1909418-78.patch44.64 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 55,862 pass(es), 2 fail(s), and 96 exception(s).
[ View ]
#78 interdiff-1909418-78.txt2 KBdamiankloip
#73 drupal-1909418-73.patch59.67 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1909418-73.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#73 interdiff.txt2.37 KBtim.plunkett
#71 entity-controllers-1909418-71.patch59.47 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,491 pass(es), 509 fail(s), and 493 exception(s).
[ View ]
#71 interdiff.txt8.65 KBtim.plunkett
#69 entity-controllers-1909418-69.patch48.42 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,141 pass(es), 565 fail(s), and 425 exception(s).
[ View ]
#65 drupal-1909418-65.patch48.81 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1909418-65.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#65 interdiff.txt4.51 KBdawehner
#61 entity-controller-1909418-61.patch44.36 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,504 pass(es), 95 fail(s), and 9 exception(s).
[ View ]
#61 interdiff.txt7.91 KBtim.plunkett
#57 interdiff.txt4.96 KBtim.plunkett
#57 entity-controller-injection.patch43.3 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,484 pass(es).
[ View ]
#51 entity-handler-1909418-51.patch43.22 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,287 pass(es).
[ View ]
#45 1909418-entity-controller-45.patch43.3 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1909418-entity-controller-45.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#41 entity-controller-1909418-41.patch43.44 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity-controller-1909418-41.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#37 entity-controllers-di-1909418.37.interdiff.txt1.44 KBlarowlan
#37 entity-controllers-di-1909418.37.patch43.45 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 54,150 pass(es).
[ View ]
#34 entity-controller-1909418-34.patch43.61 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 54,099 pass(es), 0 fail(s), and 42 exception(s).
[ View ]
#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
PASSED: [[SimpleTest]]: [MySQL] 54,109 pass(es).
[ View ]
#25 entity-controllers-di-1909418.24.interdiff.txt5.14 KBlarowlan
#25 entity-controllers-di-1909418.24.patch21.32 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 54,227 pass(es).
[ View ]
#19 drupal-1909418-19.patch20.48 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 53,991 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#19 interdiff.txt3.66 KBdawehner
#16 entity-controllers-di-1909418.16.interdiff.txt2.79 KBlarowlan
#16 entity-controllers-di-1909418.16.patch20.16 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 54,040 pass(es), 0 fail(s), and 63 exception(s).
[ View ]
#13 entity-controllers-di-1909418.patch20.95 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 51,157 pass(es), 698 fail(s), and 267 exception(s).
[ View ]

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.

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.

Assigned:Unassigned» larowlan

going to see if this one isn't beyond me

Title:Controller InjectionEntity 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.

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

Assigned:larowlan» Unassigned

Yeah #5 is beyond me, #3 made sense

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.

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.

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

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

Status:Active» Needs review
StatusFileSize
new20.95 KB
FAILED: [[SimpleTest]]: [MySQL] 51,157 pass(es), 698 fail(s), and 267 exception(s).
[ View ]

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.

+++ 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 ->

Status:Needs work» Needs review
StatusFileSize
new20.16 KB
FAILED: [[SimpleTest]]: [MySQL] 54,040 pass(es), 0 fail(s), and 63 exception(s).
[ View ]
new2.79 KB

Find and replace fail.

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review
StatusFileSize
new3.66 KB
new20.48 KB
FAILED: [[SimpleTest]]: [MySQL] 53,991 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

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.

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.

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.

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.

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.

StatusFileSize
new21.32 KB
PASSED: [[SimpleTest]]: [MySQL] 54,227 pass(es).
[ View ]
new5.14 KB

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.

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.

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.

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.

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

StatusFileSize
new20.85 KB
PASSED: [[SimpleTest]]: [MySQL] 54,109 pass(es).
[ View ]
new8.78 KB

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

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

Assigned:Unassigned» tim.plunkett

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

StatusFileSize
new27.6 KB
new43.61 KB
FAILED: [[SimpleTest]]: [MySQL] 54,099 pass(es), 0 fail(s), and 42 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new43.45 KB
PASSED: [[SimpleTest]]: [MySQL] 54,150 pass(es).
[ View ]
new1.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.

Status:Needs work» Needs review

I'm happy with #37.

StatusFileSize
new43.44 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity-controller-1909418-41.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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;

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new43.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1909418-entity-controller-45.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

Status:Needs review» Reviewed & tested by the community

Still looks good to me.

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.

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 ?

Status:Needs work» Needs review
StatusFileSize
new43.22 KB
PASSED: [[SimpleTest]]: [MySQL] 55,287 pass(es).
[ View ]

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

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

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

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}

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 ?

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.

Status:Needs work» Needs review
StatusFileSize
new43.3 KB
PASSED: [[SimpleTest]]: [MySQL] 55,484 pass(es).
[ View ]
new4.96 KB

Addresses #54.

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.

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

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

Title:Entity Controller/Handler InjectionAllow Entity Controllers to have their dependencies injected
StatusFileSize
new7.91 KB
new44.36 KB
FAILED: [[SimpleTest]]: [MySQL] 55,504 pass(es), 95 fail(s), and 9 exception(s).
[ View ]

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.

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?

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?

StatusFileSize
new4.51 KB
new48.81 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1909418-65.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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

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.

Status:Needs work» Needs review
StatusFileSize
new48.42 KB
FAILED: [[SimpleTest]]: [MySQL] 55,141 pass(es), 565 fail(s), and 425 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new8.65 KB
new59.47 KB
FAILED: [[SimpleTest]]: [MySQL] 55,491 pass(es), 509 fail(s), and 493 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new2.37 KB
new59.67 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1909418-73.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Sloppy reroll on my part.

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

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.

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

Status:Needs work» Needs review
StatusFileSize
new2 KB
new44.64 KB
FAILED: [[SimpleTest]]: [MySQL] 55,862 pass(es), 2 fail(s), and 96 exception(s).
[ View ]

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.

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.

StatusFileSize
new5.35 KB
new44.52 KB
FAILED: [[SimpleTest]]: [MySQL] 56,361 pass(es), 1 fail(s), and 97 exception(s).
[ View ]

StatusFileSize
new12.08 KB
new45.41 KB
FAILED: [[SimpleTest]]: [MySQL] 53,565 pass(es), 412 fail(s), and 316 exception(s).
[ View ]

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.

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new5.46 KB
new47.55 KB
FAILED: [[SimpleTest]]: [MySQL] 56,382 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

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

+++ 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 ?

StatusFileSize
new519 bytes
new47.25 KB
FAILED: [[SimpleTest]]: [MySQL] 55,716 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

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.

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 :-/

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.

Assigned:tim.plunkett» Unassigned
StatusFileSize
new794 bytes
new47.23 KB
PASSED: [[SimpleTest]]: [MySQL] 55,890 pass(es).
[ View ]

Just fixing the tests.

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

StatusFileSize
new767 bytes
new767 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1909418-94.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

So I think we can just remove this stuff?

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new47.08 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1909418-95.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

With the actual new patch and not just interdiffs

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new47.09 KB
PASSED: [[SimpleTest]]: [MySQL] 55,737 pass(es).
[ View ]

Rerolled after the language patch just got in.

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.

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.

Status:Reviewed & tested by the community» Fixed

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

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

Title:Allow Entity Controllers to have their dependencies injectedChange 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.

I can't even one for entity controllers itself.

Status:Active» Needs review

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

@dawehner thanx!

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.

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.

Issue tags:-Needs change record

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

Issue summary:View changes

Updated issue summary.