Ensure custom EntityType controllers can be provided by modules

I like the idea! Here's a patch.

Okay, scaled this back drastically after talking with msonnabaum, dawehner, and EclipseGC.
Basically, \Drupal\Core\Entity\EntityType is a domain object, and should not have instantiation logic.

EntityType will still get specific controller getters, but they will return the class name. EntityManager still handles the instantiation.

One other improvement from the patch is maintained, and that is bringing list controllers inline with form and access controllers, and using setter injection for the module handler.

Retitling, but the issue summary still needs an update.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -263,6 +263,25 @@ public function getAccessController($entity_type) {
+   * Returns an entity type, or throws an exception if it doesn't exist.
+   *
+   * @param string $entity_type_id
+   *   The entity type ID.
+   *
+   * @return \Drupal\Core\Entity\EntityTypeInterface
+   *   The entity type object.
+   *
+   * @throws \InvalidArgumentException
+   */
+  protected function getEntityTypeForReal($entity_type_id) {
+    if ($definition = $this->getDefinition($entity_type_id)) {
+      return $definition;
+    }
+
+    throw new \InvalidArgumentException(sprintf('The %s entity type does not exist.', $entity_type_id));
+  }

So we need a better name for this. I'm open to suggestions.

This compromises and adds a param to getDefinition to throw an exception.
It turns out that ever view builder and storage class also needs the module handler, making everything but forms the same. So I streamlined that.
config_translation, once again, was doing weird stuff, so I tried to clean that up. It helps highlight how contrib should define and invoke custom controllers.

After some discussion on IRC, decided to repurpose InvalidMapperDefinitionException as a generic exception.

Fixed the bug, and wrote a full EntityManagerTest.

The diffstat looks bad, but that's 878 lines of new test coverage.

The old title is one thing this does on the path towards this goal: Contrib should be able to sanely provide custom controllers.
config_translation is an example of how it was not easy, so workarounds and hacks were added.

This cleans them up, provides the ability for contrib to follow suit, and adds a good amount of test coverage.

The class_exists() check @dawehner asked for broke my entity form mode code. Fixed here.

- getList() / getAccess() - makes no real sense, doesn't return a list, doesn't return an "access".
I don't think we want to be rehashing the whole controller/handler/(null) debacle here. For now all of those are still called controllers, why don't we stick to that here, and rename in the issue that's supposed to rename ?

- Great thing that we unify how those various objects are instanciated, but:
Some receive a moduleHandler, some don't ? Yet they all have relatively equal chances to have a need for invoking a hook or an alter ?

Similarly, it's common for one controller to need functionnality provided by another controller, thus needing to get back to the EntityMangar. Could we inject the entity manager in all of those ?
(hm - or is that covered by the fact that they receive the EntityType, which now gives you shorthands to any controller ?)

Those are getters for the similarly named properties. There are already setters with those names.

AFAIK, all of the non-form controllers receive the module handler. All can easily opt into it by implementing a setModuleHandler method...

With the exception of the list controller needing the storage controller, I don't think any controllers ever need the other controllers.
They all can use injection to get whatever they want, as in HEAD, this is not changed here.

Rerolled after #1862202: Objectify the language system.
The EntityManagerTest now has 100% coverage.

1071 lines of tests added.

Whoops, forgot a couple assertions.

1. +++ b/core/lib/Drupal/Component/Plugin/Exception/InvalidPluginDefinitionException.php
   @@ -2,17 +2,15 @@
     * Defines a class for invalid configuration mapper definition exceptions.
   

   There seems to be documentation left from the original code.

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -159,6 +160,20 @@ public function clearCachedDefinitions() {
   +    if ($entity_type = parent::getDefinition($entity_type_id)) {
   +      return $entity_type;
   +    }
   +    elseif (!$exception_on_invalid) {
   +      return NULL;
   +    }
   +
   +    throw new UnknownPluginException($entity_type_id, sprintf('The "%s" entity type does not exist.', $entity_type_id));
   

   I kind of think that we should still check whether the $definition['class'] exists.

3. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -402,8 +383,8 @@ public function getFieldDefinitionsByConstraints($entity_type, array $constraint
   -    unset($this->entityFieldInfo);
   -    unset($this->fieldDefinitions);
   +    $this->entityFieldInfo = NULL;
   +    $this->fieldDefinitions = NULL;
   

   Oh since when don't we use unset() anymore?

4. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -333,9 +337,37 @@ public function setController($controller_type, $value) {
   +  public function hasController($controller_type, $nested = FALSE) {
   

   Any reason why this was not added onto the interface?

5. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationBlockListController.php
   @@ -28,9 +28,20 @@ class ConfigTranslationBlockListController extends ConfigTranslationEntityListCo
   +  public function __construct(EntityTypeInterface $entity_info, EntityStorageControllerInterface $storage, array $themes) {
   

   If we need to do that, we should better inject the theme handler and call listInfo in case it is needed.

6. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
   @@ -0,0 +1,954 @@
   +  }
   ...
   +  /**
   +   * Tests the __construct() method.
   +   */
   +  public function testConstructor() {
   +    new EntityManager(new \ArrayObject(), $this->container, $this->moduleHandler, $this->cache, $this->languageManager, $this->translationManager);
   

   What does this test?

7. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
   @@ -0,0 +1,954 @@
   +    $this->assertAttributeEquals('bananas', 'bundleInfo', $this->entityManager);
   

   <3

re: getList() / getAccess():

Those are getters for the similarly named properties. There are already setters with those names.

It's sad that those went in, then... Be it for getters or setters, those name are really unfortunate.

The method names should at least hint that this is about getting and setting classes.
getController() / setController() / getForm() / setForm()
...is pretty misleading.

EntityManager::getController() gives you an object
EntityType::getController() gives you a class ?
EntityManager::getListController() gives you an object
EntityType::getList() gives you a class ?

DX / consistency --

1) Fixed
2) Added
3) Reverted
4) Fixed
5) Fixed
6) Removed
7) Actually, I removed this as well, since it required overriding the property to make it public, and it didn't really test anything. That was per @msonnabaum's request.

getListController vs getList is very clear that they return different things.

The EntityType::getController() vs EntityManager::getController() is unfortunate, but *neither are added in this patch*. That problem is in HEAD, and is really more the territory of #1976158: Rename entity storage/list/form/render "controllers" to handlers

Sure, it's very clear that getListController() & getList() return different things, but very unclear what getList() actually returns.

EntityTypeInterface is a messy collection of methods IMO, but well, it's partly in core already, so we go further on that road and leave the cleanup for the already disastrous & hypothetical #1976158: Rename entity storage/list/form/render "controllers" to handlers ?
OK, why not.

EntityTypeInterface is pretty much just "let's put get/set in front of the old annotation key names". That, plus a few helpers, and that's it.

Fixing a couple docblocks, hopefully the last patch...? :)

+++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
@@ -187,6 +171,23 @@ public function clearCachedFieldDefinitions();
+   * @param string $controller_type
...
+   * @param string $controller_getter
+   *   (optional) The method to call on the entity type object to get the controller class.
...
+   *   The controller type to create an instance for.

That's quite confusing. Why is the getter necessary, couldn't we go with the controller type only?

EntityType will still get specific controller getters, but they will return the class name. EntityManager still handles the instantiation.

+1

DX / consistency --

Agreed, everything returning a class should have a Class suffix, .e.g. getControllerClass(). Not necessarily part of this patch though?

Why is the getter necessary, couldn't we go with the controller type only?

This is internal-only; it just ensures that core uses the custom getters the same as anyone else, in case they are overridden in some way.

Agreed, everything returning a class should have a Class suffix, .e.g. getControllerClass().

@msonnabaum and @dawehner agreed with you in IRC, and @yched said this would address his concerns above. So I went ahead and did that as part of this patch, as we were touching 90% of those lines anyway.

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -350,6 +329,7 @@ public function getFieldDefinitions($entity_type, $bundle = NULL) {
   +            // @todo This should check FieldDefinitionInterface.
                if ($definition instanceof FieldDefinition) {
                  $definition->setName($field_name);
   

   No, actually - this needs to run on FieldDefinition (base feilds), and not on Field / FieldInstance config entities.
   So not checking FiedDefinitionInterface is intentional.

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -246,40 +223,44 @@ public function getFormController($entity_type, $operation) {
   +      if ($controller_getter) {
   +        $class = $definition->{$controller_getter}();
          }
          else {
   -        $this->controllers[$controller_type][$entity_type] = new $class($this->getDefinition($entity_type));
   +        $class = $definition->getControllerClass($controller_type);
          }
   

   I might be missing something, but I'm not sure I see the interest of the $controller_getter, since the $controller_type param is required anyway ?

   Passing a "getter method" name feels a bit weird + wouldn't the generic getControllerClass($controller_type) work anyway ?

   At any rate, if that param stays, it should be renamed $controller_class_getter ?

3. +++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.php
   @@ -164,12 +172,12 @@ protected function postLoad(array &$queried_entities) {
   -    foreach (\Drupal::moduleHandler()->getImplementations('entity_load') as $module) {
   +    foreach ($this->moduleHandler->getImplementations('entity_load') as $module) {
          $function = $module . '_entity_load';
          $function($queried_entities, $this->entityType);
        }
        // Call hook_TYPE_load().
   -    foreach (\Drupal::moduleHandler()->getImplementations($this->entityType . '_load') as $module) {
   +    foreach ($this->moduleHandler->getImplementations($this->entityType . '_load') as $module) {
          $function = $module . '_' . $this->entityType . '_load';
          $function($queried_entities);
        }
   

   Not really related, but can't those be plain ->invokeAll() now ?

4. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -317,15 +317,19 @@ public function getControllers() {
   +
   +    return '';
   

   Why not just NULL ?

5. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -357,6 +403,50 @@ public function setList($class) {
   +  public function getViewBuilderClass() {
   +    return $this->getControllerClass('view_builder');
   +  }
   +
   (...)
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function hasViewBuilderClass() {
   +    return isset($this->controllers['view_builder']);
   +  }
   

   Inconsistency:
   - the generic hasControllerClass() includes a class_exists() check.
   - the generic getControllerClass() calls the above and thus includes the class_exists() check
   - the specific getXClass() methods call getControllerClass() and thus include the class_exists() check
   - but the specific hasXClass() methods are stricly isset() based, thus without the class_exists() check

6. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityApiInfoTest.php
   @@ -54,6 +54,6 @@ function testEntityInfoCacheWatchdog() {
   +    $this->assertEqual($info->getStorage(), 'Drupal\Core\Entity\DatabaseStorageController', 'Entity controller class info is correct.');
   ...
   -    $this->assertEqual($info->getController('storage'), 'Drupal\Core\Entity\DatabaseStorageController', 'Entity controller class info is correct.');
   

   Should be getStorageClass() ?

7. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
   @@ -0,0 +1,914 @@
   +interface TestContentEntityInterface extends \Iterator, ContentEntityInterface {
   +}
   +
   +/**
   + * Provides a testing version of EntityManager with an empty constructor.
   + */
   +class TestEntityManager extends EntityManager {
   

   Still not familiar with UnitTests, but is it standard to include several classes / interfaces in a test class ?

3. invokeAll() is not strictly the same as that, the current code theoretically allows to receive the arguments by reference if a hook wants to. Not sure if we do, but it's there right now...

1) We try really hard to not use concrete classes for things like this, it's a shame setName() isn't on any interface at all :(
I removed the @todo for now though.

2) As I said in #31, "This is internal-only; it just ensures that core uses the custom getters the same as anyone else, in case they are overridden in some way."
But good point on the variable name, changed it.

3) As @Berdir said in #33, that was originally done on purpose, leaving it here.

4) Changed, and updated the docs.

5) Thanks! Those were hurriedly added, I didn't think that through.

6) Thanks, that was the one failure of the last patch.

7) Yes it is, we do this very often.

Answers & interdiff in #36 work for me.
Many thanks for the setters renames :-)

Temptatively setting to RTBC then...

Updated the API additions and changes section.

Tagging

   protected function t($string, array $args = array(), array $options = array()) {
-    return $this->translationManager()->translate($string, $args, $options);
+    return $this->translationManager->translate($string, $args, $options);
   }
  

This is a regression. It's always a good idea to wrap access to properties like this in methods, especially in this case where the class has no dedicated factory and provides no default when the setter isn't used.

The old version of that method should be restored on all of these.

Fair point. Adding an abstract class EntityControllerBase for now, this should become a trait as soon as we can.

Much better. Moving back assuming tests pass.

Oh since when don't we use unset() anymore?

I actually think your changes to set the properties to NULL is correct. Using unset on a property removes it entirely - not really the same as resetting.

Kind of related: This is why I changed stuff in the ViewExecutable::destroy() method a while back; to get a more accurate reset.

So I went ahead and did that as part of this patch, as we were touching 90% of those lines anyway.

Awesome!

This is internal-only; it just ensures that core uses the custom getters the same as anyone else, in case they are overridden in some way.

I see. It's quite ugly, but when it's internal only it shouldn't be a big deal.
Looking up the patch it appears to be on the interface also though, where it shouldn't be then. -> setting back to needs work.

1. +++ b/core/lib/Drupal/Core/Entity/EntityControllerBase.php
   @@ -0,0 +1,86 @@
   +   * Gets the translation manager.
   +   *
   +   * @return \Drupal\Core\StringTranslation\TranslationInterface
   +   *   The translation manager.
   ...
   +  protected function translationManager() {
   

   Why do those methods not follow the get-pattern? I've seen that's not something new, but when we introduce a new base class it should follow current best practices. So, any reasons for not going with the pattern here?

2. +++ b/core/lib/Drupal/Core/Entity/EntityControllerBase.php
   @@ -0,0 +1,86 @@
   +   * @return $this
   

   In other patches those have been

   @return static
     The object itself for chaining.

   Does $this work with IDEs as well?

See #2158497: [policy, no patch] Use "$this" and "static" in documentation for @return types for more details and discussion on the whole $this thing in docblocks

I think you misunderstood me. EntityType::getStorageClass is a public method and should stay on the interface. I meant that the $controller_class_getter parameter of EntityManager::getController is internal.

We use getters for properties of the object itself, not for wrappers of services. See ControllerBase for example.

damiankloip pointed you at the issue for @return $this

I think you misunderstood me. EntityType::getStorageClass is a public method and should stay on the interface. I meant that the $controller_class_getter parameter of EntityManager::getController is internal.

yes, I was talking about that as well. So if it is internal, it shouldn't be on the interface, right? But the patch adds it to EntityManagerInterface:

+ public function getController($entity_type,
$controller_type, $controller_class_getter = NULL);

So this needs work then.

We use getters for properties of the object itself, not for wrappers of services. See ControllerBase for example.

Not sure I see why that makes a difference, but anyway that's not related to this issue then.

damiankloip pointed you at the issue for @return $this

yeah, thanks!

Sure...

This was just a documentation change and was exactly what fago asked for.

Something went wrong, look at the patch size. I also see some twig changes in the beginning of the file shouldn't be in there.

Forgot to rebase.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -159,65 +160,39 @@ public function clearCachedDefinitions() {
+  public function getDefinition($entity_type_id, $exception_on_invalid = FALSE) {
+    if (($entity_type = parent::getDefinition($entity_type_id)) && class_exists($entity_type->getClass())) {
+      return $entity_type;
     }
-    return FALSE;
+    elseif (!$exception_on_invalid) {
+      return NULL;
+    }
+
+    throw new UnknownPluginException($entity_type_id, sprintf('The "%s" entity type does not exist.', $entity_type_id));

Opened #2178795: Allow DiscoveryInterface::getDefinition() to throw an exception for an invalid plugin to discuss moving this upstream.

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -159,65 +160,39 @@ public function clearCachedDefinitions() {
   +    throw new UnknownPluginException($entity_type_id, sprintf('The "%s" entity type does not exist.', $entity_type_id));
   ...
   +  public function getDefinition($entity_type_id, $exception_on_invalid = FALSE) {
   ...
   +    elseif (!$exception_on_invalid) {
   +      return NULL;
   +    }
   +
   

   This is really helpful debugging information and I'm hard-pressed to think of a reason why you would NOT want it. (In fact, I have some DX issue open about the utter inability to debug a plugin-related error for exactly this reason.) It's weird to a) have a parameter for this, b) have it default to FALSE, and c) not do this on all plugins, not just this one. Tim's opening up a follow-up issue to discuss this pattern and its applicability across all plugin types (he did originally try to do this everywhere, but ran into issues like if ($this->getDefinition('invalid')...)

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -246,40 +223,56 @@ public function getFormController($entity_type, $operation) {
   +  public function getController($entity_type, $controller_type, $controller_class_getter = NULL) {
   ...
   +   * @param string $controller_class_getter
   +   *   (optional) The method to call on the entity type object to get the controller class.
   

   Uh, WAT? :)

   Here again we're doing weird things here that we don't in other plugins. I asked Tim more about this and he said that in #2005716: Promote EntityType to a domain object we added getters for these class names and so it makes sense to use them, vs. calling $entity_type->getControllerClass('storage') like everyone else does. Not sure I get that. Coming back to it in a bit.

   Ok, talked through with Tim. Basically, the crux of the issue is that config translation introduced its own custom info hook and started monkeying with entity_info properties there, like list_controller. It did so because it couldn't call getController since that wasn't public (now fixed), and added setter methods to mirror the getter methods added in the previous issue, so that it can use those instead. Fixing this accessor issue for config translation hopefully helps fix it for anyone else in contrib who has to pull similar shenanigans.

3. +++ b/core/modules/config_translation/config_translation.module
   @@ -70,6 +70,25 @@ function config_translation_theme() {
   +  foreach ($entity_info as $entity_type_id => $entity_type) {
   +    if ($entity_type_id == 'block') {
   +      $class = 'Drupal\config_translation\Controller\ConfigTranslationBlockListController';
   +    }
   +    elseif ($entity_type_id == 'field_instance') {
   +      $class = 'Drupal\config_translation\Controller\ConfigTranslationFieldInstanceListController';
   +    }
   

   This is a pre-existing condition, but it'd be great to understand *why* we are special-casing those particular things. A small follow-up for that would be nice.

4. +++ /dev/null
   @@ -1,42 +0,0 @@
   -  public function testMethods() {
   

   I love that we replaced like 7 assertions with 1200 lines of full test coverage. :) NICE! There's even a few 'banana' references in there, awww. ;)

OK, I can't find anything commit-blocking here. I'm not 100% sure I grok every line of this patch, but definitely get the gist and it seems solid.

Committed and pushed to 8.x. Thanks!

I don't *think* we need a change notice for this one, as it's mostly internal refactoring, but it wouldn't hurt to look over the various entity documentation to ensure something doesn't need to get updated.

Awesome! Thanks.

Well #2005716: Promote EntityType to a domain object hasn't had a change notice written yet, and that's the main one that would have been updated. I'll cross-post there.

The followup issue never got filed for replacing EntityControllerBase (now EntityHandlerBase). I filed #2471663: Undeprecate EntityHandlerBase for that.

https://twitter.com/xjmdrupal/status/497852222600790016 :)