Updated: Comment #38

Problem/Motivation

Contrib (and even optional core modules like config_translation) have no uniform way to add and invoke custom controllers.

EntityType does not have bespoke getters for controllers, and is missing setters for some.
If an EntityType class is swapped out and wants to override a controller-specific method, it will be completely bypassed by EntityManager.
Furthermore, these entity controllers share a lot more in common than they did initially.
For example, every single type of controller (excluding forms, which use FormInterface/ContainerInjectionInterface) need the module handler, but half use setter injection and half pollute their constructor.

Proposed resolution

Add EntityManagerInterface::getControlller() to help contrib/optional modules
Add getters and setters to EntityType for core controllers
Add setModuleHandler to all, not just half, of the controller types that require it
Fix config_translation to be the example of best practices, and not a clever workaround
Write tests to cover it all

Remaining tasks

N/A

User interface changes

N/A

API changes

Changes

EntityManager::getControllerClass() is replaced by EntityManager::getController()

Drupal\config_translation\Exception\InvalidMapperDefinitionException is now Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException, and is to be used whenever a plugin definition does not meet expectations (instead of the previous usage of \InvalidArgumentException)

The 'list_controller' property of hook_config_translation_info() is gone, you should use hook_entity_info() or hook_entity_info_alter() to set the 'config_translation_list' controller:

<?php
function mymodule_entity_info($entity_info) {
  if (isset(
$entity_info['node'])) {
   
$entity_info['node']->setController('config_translation_list', 'Drupal\mymodule\MyModuleConfigTranslationListController');
  }
}
?>

Additions

List, Access, Storage, ViewBuilder controllers can implement the following two methods if needed.
They are required for EntityForm controllers:

<?php
 
/**
   * Sets the module handler for this controller.
   *
   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
   *   The module handler.
   *
   * @return $this
   */
 
public function setModuleHandler(ModuleHandlerInterface $module_handler);

 
/**
   * Sets the translation manager for this controller.
   *
   * @param \Drupal\Core\StringTranslation\TranslationInterface $translation_manager
   *   The translation manager.
   *
   * @return $this
   */
 
public function setTranslationManager(TranslationInterface $translation_manager);
?>

EntityType has getters and setters for the 5 core controller types, and has() methods for the optional ones (hasFormClasses() hasListClass() hasViewBuilderClass()).

Comments

tim.plunkett’s picture

Title:Move Controller Functions to Entity Type Class» Move *Controller() methods to EntityType class
Assigned:Unassigned» tim.plunkett
Issue summary:View changes
Status:Active» Needs review
Issue tags:+DX (Developer Experience), +API change
StatusFileSize
new336.06 KB
FAILED: [[SimpleTest]]: [MySQL] 57,951 pass(es), 242 fail(s), and 111 exception(s).
[ View ]

I like the idea! Here's a patch.

EclipseGc’s picture

StatusFileSize
new335.14 KB

rerolling the patch against head. This should not fix any tests.

Eclipse

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new2.77 KB
new335.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity-type-2168333-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixed a handful of tests. Setting needs review to get a complete list of still failing tests.

Eclipse

tim.plunkett’s picture

Title:Move *Controller() methods to EntityType class» Provide controller-specific getter methods on the EntityType class
Status:Needs work» Needs review
Issue tags:+Needs issue summary update
StatusFileSize
new43.67 KB
FAILED: [[SimpleTest]]: [MySQL] 59,498 pass(es), 91 fail(s), and 32 exception(s).
[ View ]

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.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new48.4 KB
PASSED: [[SimpleTest]]: [MySQL] 59,579 pass(es).
[ View ]
new8.92 KB
tim.plunkett’s picture

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

EclipseGc’s picture

Why not just do this in getDefinition?

Eclipse

tim.plunkett’s picture

Issue summary:View changes
Issue tags:-Needs issue summary update+D8MI
StatusFileSize
new70.14 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new39.82 KB

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.

tim.plunkett’s picture

StatusFileSize
new72.76 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new5.31 KB

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

tim.plunkett’s picture

StatusFileSize
new106.7 KB
FAILED: [[SimpleTest]]: [MySQL] 59,838 pass(es), 12 fail(s), and 12 exception(s).
[ View ]
new36.04 KB

Fixed the bug, and wrote a full EntityManagerTest.

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

tim.plunkett’s picture

Title:Provide controller-specific getter methods on the EntityType class» Ensure custom EntityType controllers can be provided by modules
Priority:Normal» Major
Issue summary:View changes

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.

tim.plunkett’s picture

StatusFileSize
new110.09 KB
PASSED: [[SimpleTest]]: [MySQL] 59,845 pass(es).
[ View ]
new4.64 KB

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

yched’s picture

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

tim.plunkett’s picture

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.

tim.plunkett’s picture

StatusFileSize
new120.73 KB
PASSED: [[SimpleTest]]: [MySQL] 59,901 pass(es).
[ View ]

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

1071 lines of tests added.

tim.plunkett’s picture

StatusFileSize
new121.02 KB
PASSED: [[SimpleTest]]: [MySQL] 59,869 pass(es).
[ View ]
new1.46 KB

Whoops, forgot a couple assertions.

dawehner’s picture

  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

yched’s picture

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

tim.plunkett’s picture

StatusFileSize
new8.67 KB
new120.16 KB
PASSED: [[SimpleTest]]: [MySQL] 59,798 pass(es).
[ View ]

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.

tim.plunkett’s picture

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

yched’s picture

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.

tim.plunkett’s picture

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.

tim.plunkett’s picture

StatusFileSize
new1.09 KB
new120.51 KB
PASSED: [[SimpleTest]]: [MySQL] 59,890 pass(es).
[ View ]

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

fago’s picture

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

tim.plunkett’s picture

StatusFileSize
new135.55 KB
FAILED: [[SimpleTest]]: [MySQL] 59,934 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new38 KB

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.

yched’s picture

  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 ?

Berdir’s picture

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

tim.plunkett’s picture

StatusFileSize
new136.17 KB
PASSED: [[SimpleTest]]: [MySQL] 59,941 pass(es).
[ View ]
new5.04 KB

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.

yched’s picture

Status:Needs review» Reviewed & tested by the community

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

Temptatively setting to RTBC then...

tim.plunkett’s picture

Issue summary:View changes

Updated the API additions and changes section.

tim.plunkett’s picture

Issue tags:+beta target

Tagging

tim.plunkett’s picture

Issue summary:View changes
msonnabaum’s picture

Status:Reviewed & tested by the community» Needs work
   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.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new141.15 KB
PASSED: [[SimpleTest]]: [MySQL] 59,913 pass(es).
[ View ]
new14.58 KB

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

msonnabaum’s picture

Status:Needs review» Reviewed & tested by the community

Much better. Moving back assuming tests pass.

damiankloip’s picture

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.

fago’s picture

Status:Reviewed & tested by the community» Needs work

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?

damiankloip’s picture

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

tim.plunkett’s picture

Status:Needs work» Reviewed & tested by the community

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

fago’s picture

Status:Reviewed & tested by the community» Needs work

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!

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new411.36 KB
PASSED: [[SimpleTest]]: [MySQL] 59,801 pass(es).
[ View ]
new2.05 KB

Sure...

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

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

aspilicious’s picture

Status:Reviewed & tested by the community» Needs work

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.

tim.plunkett’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new141.29 KB
PASSED: [[SimpleTest]]: [MySQL] 59,901 pass(es).
[ View ]

Forgot to rebase.

tim.plunkett’s picture

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

webchick’s picture

Status:Reviewed & tested by the community» Fixed
  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.

tim.plunkett’s picture

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.

Status:Fixed» Closed (fixed)

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

xjm’s picture

The followup issue never got filed for replacing EntityControllerBase (now EntityHandlerBase). I filed #2471663: Deprecate EntityHandlerBase and move module handler dependency to the implementations that need it for that.

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