Add $EntityType::load() and loadMultiple() to simplify loading entities

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -363,6 +363,44 @@ public function referencedEntities() {
+    $called_class = get_called_class();
+    foreach (\Drupal::entityManager()->getDefinitions() as $entity_type => $entity_info) {
+      if ($entity_info->getClass() == $called_class || $entity_info->isSubclassOf($called_class)) {
+        return \Drupal::entityManager()->getStorageController($entity_type)->loadMultiple($ids);
+      }
+    }

As mentioned in IRC, let's move this logic to a protected static method, so that the create() issue can use it too.

Ok, based on a discussion with @timplunkett, this won't work if someone alters the entity class, to for example MyOwnNode implements NodeInterface.

Because the Node will no longer be the or a sub/parent class of the defined klass.

I can see an option or two (EDIT: ok, 4):

a) Add the interface to the annotation, check that.
b) Require that entity class changes *must* subclass the default
c) Keep the original class around somewhere in EntityType, check against thta
d) Go back to msonnabaum's class name => ID magic.

d) can be fixed by requiring to implement it if your class doesn't match the pattern but I didn't say it's not ugly :)

e) Yeah, was thinking about that, but how do we know which interface is the right now? Could be pretty hard to figure out automatically?

Out of the given options in #7, b) is the most natural, realistic, and conceptually valid:

b) Require that entity class changes *must* subclass the default

An entity class represents a domain object. An entity class only exists, because it consists of custom business logic. The (additional) business logic is what differentiates an entity from another entity. The business logic is custom to the use-case of the originating module.

The time of universally relying on Node entities is a relict of the past. If you want the concept of node entities but not Nodes, then what you do is to copy/fork the entire module directory. There's no point in hacking the Node entity class.

Consequently, the only valid use-case for replacing the entity class of a module is to override its methods for very specific (weird) reasons on your custom site.

→ If you need to hack an entity class for whichever weird reason, then it is only sensible that your hack can only be an override of the original class.

→ If that's not your intention, then stop trying to hack Node.

Works for me I think. It's a separate method, and if you really do something crazy, then you can always overwrite that method and return "node" in your class.

What's the next step here then?

I think some specific test coverage of this would be good, + converting a bunch of exemplary load function calls (entity_load(), but also node_load(), taxonomy_term_load(), .. and the multiple version of them), and deprecate them in favor of this for procedural code/$storage->load()/loadMultiple() for injected code.

We also need explicit test coverage for altering an entity class I think, so making sure that alter Node to TestNode extends Node in the entity type definition and then Node::load() works as expected.

Maybe we can do that test as a unit test, I'm not completely sure.

From #2177031: Remove menu_get_item() and every use of it. :

- $menu_item = menu_get_item('menu-test/context');
+ $menu_items = \Drupal::entityManager()->getStorageController('menu_link')->loadByProperties(array('route_name' => 'menu_test.context'));

We desperately need this, IMO.

But yet, this issue has only discussed load() and loadMultiple. That last snippet is loadByProperties().
How do we decide what public methods of the storage controller to duplicate on the entity?
And why just the storage controller? We already have access(). Why not view()? Etc.

I'm going to work on the tests for this. I'm going to build on #2134857: PHPUnit test the entity base classes, because it is laying the whole groundwork for unit testing Entities, and it is currently in RTBC so it'll probably get in before this one.

Added a test. My first PHPUnit test, took some figuring out :)

Thanks, this looks quite good. What would be great is if we could find a way to test the edge cases, like calling load() on a subclass, but I'm not sure if that's possible, maybe we need a real entity class for that? And calling it on an entity class that has no corresponding entity type definition, that should be pretty easy by simply creating a new mock instance with an entity type ID that doesn't exist.

Also wondering if and how much we already want to deprecate on this, I think it would be nice if node_load() and so on would be @deprecated in favor of this. There qre quite a few such functions though and we can't deprecated entity_load() itself on this as it's not possible to pass an entity type ID.

Maybe a handful example conversions, pick one simple module as an example? Then we can have novice issues as a follow-up for each module where it applies?

1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -393,6 +393,52 @@ public function referencedEntities() {
   +   */
   +  public static function load($id) {
   +    $entities = static::loadMultiple(array($id));
   +    return isset($entities[$id]) ? $entities[$id] : NULL;
   +  }
   

   Wondering if we should use load() of the storage controller directly here?

2. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
   @@ -230,6 +230,68 @@ public function testLanguage() {
   +    $test_id = rand(1, 100000);
   

   Not sure if we really want to use random ID's. Probably ok.

3. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
   @@ -230,6 +230,68 @@ public function testLanguage() {
   +    $this->assertSame($test_entity, $this->entity->load($test_id));
   

   I think we should call the method statically here, should be possible with get_class($this->entity) or so?

For the subclass use case, can we maybe mock the mock or something like that to get a subclass and then set the entity type class to that and call it on the first mock class? That's what we want to test.

Going to work on this.

We're missing something here which we had in entity_load_multiple(): the ability to reset the entity cache. I suggest we put this in.

Discussed the above with @Berdir, but he suggested that the clearing of the entity cache should be decoupled from loading entities. It is not a common use case.

In this patch I have deprecated all instances of ${entity_type}_load() and ${entity_type}_load_multiple() and have converted the entity loading functions from the Breakpoint module as an example:

• breakpoint_load()
• entity_load('breakpoint', ...)
• entity_load_multiple('breakpoint', ...)
• entity_load('breakpoint_group', ...)
• entity_load_multiple('breakpoint_group', ...)

Created a meta issue to do the rest of the conversions: #2225347: [META] Replace calls to ENTITY_TYPE_load() and ENTITY_TYPE_load_multiple() with static method calls

I'm going to continue working on this, but wanted to get some test feedback on the work so far, so setting this to needs review for the bot.

Setting to needs review for the bot.

+++ b/core/modules/aggregator/aggregator.module
@@ -265,9 +266,12 @@ function aggregator_refresh(FeedInterface $feed) {
+ * @deprecated in Drupal 8.x-dev, will be removed before Drupal 9.0.
+ *   Use \Drupal\aggregator\Entity\Feed::load().

I'm unsure of the rationale for keeping these in Drupal 8.0, given the other wrappers we have removed/are removing, but if we do keep them the comment should be "@deprecated in Drupal 8.0, will be removed in Drupal 9.0".

8.x-dev makes sense when your audience is core developers and bleeding edge module developers, but not for your average API user.

Posting work in progress. Addressed all remarks. Still probably need to deduplicate some duplicated code in the tests, and am not sure if we should check that isSubClassOf() is not called at all, since this is testing the actual implementation of the method.

I introduced the load() and loadMultiple() methods on the interface as I figured we would want to ensure that these are always available from now on. These were not implemented in ViewUI which caused the fatal error.

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -386,6 +386,39 @@ public function referencedEntities() {
+    foreach (\Drupal::entityManager()->getDefinitions() as $entity_type) {
+      if ($entity_type->getClass() == $called_class || $entity_type->isSubclassOf($called_class)) {
+        return $entity_type->id();
+      }
+    }
+

As discussed, I'm wondering if this needs to be a bit more complicated.

Should we prefer if it's an exact match?

Use case:

Someone does Whatever extends Node and exposes it as a new entity. We actually do that for EntityTest/EntityTestRev/EntityTestMul/EntityTestMulRev. So that would result in Node::load(1) suddenly stop working because of that module.

So calling EntityTest::load(1) then you're actually not sure which one you will get.

Second, should we abort if we find more than one match and throw an exception?

So you would basically loop over all, find exact and subclass matches, first look if you have an exact match and then use that, second check subclasses. And check if you have multiple before each and throw an exception.

1. +++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
   @@ -889,6 +890,20 @@ public function createDuplicate() {
   +    return Entity::load($id);
   ...
   +    return Entity::loadMultiple($ids);
   

   Not that anyone would ever do this, but should this call View:: instead?

2. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
   @@ -230,6 +231,197 @@ public function testLanguage() {
   +    // Entity::load() is a wrapper for EntityStorageController::loadMultiple().
   

   wrapper for ... load()

3. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
   @@ -230,6 +231,197 @@ public function testLanguage() {
   +    $test_entity->type = $this->randomName();
   

   do we need this? can't we just have a separate $entity_type_id variable?

1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -386,6 +386,39 @@ public function referencedEntities() {
   +    throw new \RuntimeException(sprintf('The %s class does not correspond to an entity type.', $called_class));
   

   This sounds for me like an BadMethodCallException

2. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
   @@ -230,6 +231,197 @@ public function testLanguage() {
   +    $methods = get_class_methods('Drupal\Core\Entity\Entity');
   +    unset($methods[array_search('load', $methods)]);
   +    $test_entity = $this->getMockBuilder('Drupal\Core\Entity\Entity')
   +      ->disableOriginalConstructor()
   +      ->setMethods($methods)
   +      ->getMock();
   

   Ha, this reminds me a bit of ViewUIObjectTest

3. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
   @@ -230,6 +231,197 @@ public function testLanguage() {
   +    $this->entityType->expects($this->once())
   +      ->method('isSubClassOf')
   +      ->with('Drupal\Core\Entity\Entity')
   +      ->will($this->returnValue(TRUE));
   

   Can't we exclude this method above and rely on being the mock an actual entity? It would be cool to not rely too much of the internal code, but test the actual behavior

4. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
   @@ -230,6 +231,197 @@ public function testLanguage() {
   +  /**
   +   * @covers ::load
   +   *
   +   * Tests Entity::load() when called statically on a subclass of Entity.
   +   */
   

   I really like this new comment way for tests!

5. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
   @@ -230,6 +231,197 @@ public function testLanguage() {
   +    // Entity::load() is a wrapper for EntityStorageController::loadMultiple().
   +    // Test that the same mock entity object that is returned by loadMultiple()
   +    // on the storage controller will be returned by Entity::load().
   +    $methods = get_class_methods('Drupal\Core\Entity\Entity');
   +    unset($methods[array_search('load', $methods)]);
   +    $test_entity = $this->getMockBuilder('Drupal\Core\Entity\Entity')
   +      ->disableOriginalConstructor()
   +      ->setMethods($methods)
   +      ->getMock();
   +    $test_entity->id = 1;
   +    $test_entity->type = $this->randomName();
   +
   +    // Mock the methods called by Entity::load() and
   +    // Entity::getEntityTypeFromStaticClass().
   +    $storage = $this->getMock('\Drupal\Core\Entity\EntityStorageControllerInterface');
   +    $storage->expects($this->once())
   +      ->method('load')
   +      ->with(1)
   +      ->will($this->returnValue($test_entity));
   +    $this->entityManager->expects($this->once())
   +      ->method('getStorageController')
   +      ->with($test_entity->type)
   +      ->will($this->returnValue($storage));
   +    $this->entityManager->expects($this->once())
   +      ->method('getDefinitions')
   +      ->will($this->returnValue(array($test_entity->type => $this->entityType)));
   +    $this->entityType->expects($this->once())
   +      ->method('getClass')
   +      ->will($this->returnValue(get_class($test_entity)));
   +    $this->entityType->expects($this->exactly(0))
   +      ->method('isSubClassOf');
   +    $this->entityType->expects($this->once())
   +      ->method('id')
   +      ->will($this->returnValue($test_entity->type));
   

   Can/should we just share this code between the two test methods and even the next one?

Picking this back up.

Implemented @berdir's suggestion from #33. Going to work on the remarks from #34 and #35 now. Will also extend test coverage so we can assure that the exceptions are thrown.

Addressed remarks from #34 and #35. Rerolled against latest HEAD.

I rewrote testLoad() to actually test the behaviour rather than the implementation, using actual entities as suggested by @dawehner. Also provided custom exceptions and wrote tests that check if the exceptions are correctly thrown.

Will now rework the remaining 3 tests to test behaviour instead of implementation.

Updated issue summary.

Rewrote the remaining tests. Because the implementation is no longer tested I could reuse large parts of the test setup. Nice stuff. Am really starting to appreciate PHPUnit :)

Addressed the failures.

42: 2190313-42.patch queued for re-testing.

Mostly a simple reroll due to context changes around use statements, but two things to note:
- I have removed the use Drupal\Core\Entity\Entity; from core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php as this is no longer used (see #38)
- menu_load() has been renamed to menu_ui_load() since the last patch, and we are now deprecating this function, to be removed before Drupal 9. That seems silly - either the function should be renamed back to menu_load() (and therefore moved out of menu_ui.module) or it should just be removed. See CR https://drupal.org/node/2216585 and #2085243: Rename Menu module into Menu UI module

Would be nice to get @fago's OK to move forward here, he had some reservations when we talked about it in Szeged.

I do like it a lot and I think the patch is pretty ready, but I haven't done an in-depth review of the test changes in the last few patches.

I agree that menu_ui_load() is weird, but it's already weird in HEAD, I'm fine with removing that, it's pointless to have an entity load function in a UI module.

The concern I raised is that having Class::load($type) functions available that are so nice to feature IDE auto-completion will lead to people preferring them over the "right" variant of going with dependency injected services. So the core of the problem is that dependency injected services offer no good DX and no auto-completion.

Still, the status quo is having loader functions what is even worse as you have to create and maintain them, so the patch is an improvement here. However, I wonder why we want to keep the functions - even if deprecated? I can see us keeping a deprecated entity_load(), but is there a point in keeping a deprecated node_load()?

Regardless of the concern I think the patch is good DX improvements for situations where you cannot use dependency injected services anyway, i.e. hook implementations. So I guess it's the right thing to do anyway.

For the concern to be addressed we'd have to do #2144677: Add per entity type managers to improve DX I guess.

Speaking of auto-completion, it's actually not really correct as it should go with the entity type interface instead of the class. So, auto-completion might lead developers to go with public non-interface methods then - not sure that's an issue?

We discussed the class vs. interface return before and I think it's ok, public methods on the class that are not on the interface is going to be a very untypical pattern I think.

About the load methods, dropping stuff like node_load() would be ugly as that is called a lot. However, many of the load functions, especially config entities and new entities are hardly ever called and contrib isn't going to care if we drop something like image_style_load() I think? So maybe just get rid of them? I've highlighted the ones that I think we can drop below.

1. +++ b/core/modules/aggregator/aggregator.module
   @@ -157,9 +158,12 @@ function aggregator_queue_info() {
     *
     * @return \Drupal\aggregator\FeedInterface
     *   An object describing the feed.
   + *
   + * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.
   + *   Use \Drupal\aggregator\Entity\Feed::load().
     */
    function aggregator_feed_load($fid) {
   -  return entity_load('aggregator_feed', $fid);
   +  return Feed::load($fid);
    }
   

   This is used in 3 places, 2 of those are tests.. drop?

2. +++ b/core/modules/block/block.module
   @@ -309,9 +310,12 @@ function block_list($region) {
     * @return \Drupal\block\Entity\Block
     *   The loaded block object.
   + *
   + * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.
   + *   Use \Drupal\block\Entity\Block::load().
     */
    function block_load($entity_id) {
   -  return entity_load('block', $entity_id);
   +  return Block::load($entity_id);
    }
   

   This is only used for a #exists callback.. drop?

3. +++ b/core/modules/block/custom_block/custom_block.module
   @@ -58,9 +58,12 @@ function custom_block_theme($existing, $type, $theme, $path) {
     *   A CustomBlockType object or NULL if the requested $id does not exist.
   + *
   + * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.
   + *   Use \Drupal\custom_block\Entity\CustomBlockType::load().
     */
    function custom_block_type_load($id) {
   -  return entity_load('custom_block_type', $id);
   +  return CustomBlockType::load($id);
    }
   

   Same, only use case is an #exists callback.

4. +++ b/core/modules/block/custom_block/custom_block.module
   @@ -71,9 +74,12 @@ function custom_block_type_load($id) {
     * @return \Drupal\custom_block\Entity\CustomBlock|null
     *   A CustomBlock object or NULL if the requested $id does not exist.
   + *
   + * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.
   + *   Use \Drupal\custom_block\Entity\CustomBlock::load().
     */
    function custom_block_load($id) {
   -  return entity_load('custom_block', $id);
   +  return CustomBlock::load($id);
    }
   

   Used 3 times, only in tests...

5. +++ b/core/modules/breakpoint/breakpoint.module
   @@ -49,9 +49,12 @@ function breakpoint_help($path, $arg) {
     * @see http://drupal.org/node/1798214
   + *
   + * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.
   + *   Use \Drupal\breakpoint\Entity\Breakpoint::load().
     */
    function breakpoint_load($id) {
   -  return entity_load('breakpoint', $id);
   +  return Breakpoint::load($id);
    }
   

   Also almost only used in tests and already converted, drop?

6. +++ b/core/modules/contact/contact.module
   @@ -133,9 +135,12 @@ function contact_entity_extra_field_info() {
   + *
   + * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.
   + *   Use \Drupal\contact\Entity\Category::load().
     */
    function contact_category_load($id) {
   -  return entity_load('contact_category', $id);
   +  return Category::load($id);
    }
   

   Same story, an #exists callback.

7. +++ b/core/modules/image/image.module
   @@ -251,9 +252,12 @@ function image_path_flush($path) {
   + * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.
   + *   Use \Drupal\image\Entity\ImageStyle::load().
     */
    function image_style_load($name) {
   -  return entity_load('image_style', $name);
   +  return ImageStyle::load($name);
    }
   

   Same.

8. +++ b/core/modules/menu_ui/menu_ui.module
   @@ -119,9 +119,12 @@ function menu_ui_theme() {
   + *
   + * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.
   + *   Use \Drupal\system\Entity\Menu::load().
     */
    function menu_ui_load($menu_name) {
   -  return entity_load('menu', $menu_name);
   +  return Menu::load($menu_name);
    }
   

   As discussed, let's remove this is as it was already recently renamed and it's weird to have a load function in a UI module.

9. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -77,13 +78,15 @@ function responsive_image_menu() {
     *
   + * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.
   + *   Use \Drupal\responsive_image\Entity\ResponsiveImageMapping::load().
   + *
     * @todo Needed for menu_callback
     *
     * @see http://drupal.org/node/1798214
   - *
     */
    function responsive_image_mapping_load($id) {
   -  return entity_load('responsive_image_mapping', $id);
   +  return ResponsiveImageMapping::load($id);
   

   See @todo, this is unused apart from an #exists callback and new in 8.x, let's just drop it.

10. +++ b/core/modules/shortcut/shortcut.module
    @@ -131,9 +132,12 @@ function shortcut_set_switch_access($account = NULL) {
      *   - 'label': The title of the shortcut set.
      *   If the shortcut set does not exist, the function returns NULL.
    + *
    + * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.
    + *   Use \Drupal\shortcut\Entity\ShortcutSet::load().
      */
     function shortcut_set_load($id) {
    -  return entity_load('shortcut_set', $id);
    +  return ShortcutSet::load($id);
     }
    

    Used a few times but probably mostly internal too?

11. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -240,6 +243,201 @@ public function testLanguage() {
    +    $this->entity->id = 1;
    

    What are we using this for exactly?

12. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -240,6 +243,201 @@ public function testLanguage() {
    +    $this->assertSame($this->entity, Entity::load(1));
    

    Can we call it directly on the mock class?

13. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -240,6 +243,201 @@ public function testLanguage() {
    +      Entity::load(1);
    

    Can we call EntityTestMulRev::load() here and below?

Ok, discussed the load function removal with alexpott and he was OK with my suggestion, working on that, let's see if I got everything right..

@amateescu: LinkAccessCheck seems to be unused and looks weird, I guess we could drop the whole thing?

->id was not used, removed that, but we can't really change the load calls, calling it directly on Entity is a bit weird but ok...

Also moved the exception classes into the new Exception subnamespace and using @expectedException in the tests.

Forgot to rebase before I uploaded...

Missed a few calls.

Thanks, seeing all the accessors removed is great. I'm fine with keeping a deprecated node_load(), that should ease transition while it's still clear that you should use Node::load().

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -416,6 +418,57 @@ public function getListCacheTags() {
+      // If an entity type exactly matches the called class, return it.
+      if ($entity_type->getClass() == $called_class) {

This is going to fail silently if an entity type implements a deriver to make use of the same entity class for multiple entity types. We should better count the classes as we do for the subclasses to make sure we get the right entity type. -> Setting to needs work.

Besides that I wonder whether this type-search affects performance if quite some entity load calls happen on a page. We could statically cache the "class -> entity type" map?

Hm, *derivers* in the plugin way will not be possible for entity types, because they currently enforce : as the base plugin type delimiter and it is not possible to have : or . in the entity_type_id (routing, combined ID's like fields, and so on..). But it will be possible to re-use the same class with hook_entity_type_build(), so yes, that would be possible. Nobody should attempt to call that directly then, though?

We can check it, makes it even more complicated :(

I doubt that performance is such a big problem, most calls are in tests and so on, I would suggest to not worry about it until we have numbers that can prove it? Most entity loads that happen on real sites will go through routing, entity reviews and views I think, none of those is even capable of using this API as they don't have hardcoded entity types...

I doubt that performance is such a big problem, most calls are in tests and so on, I would suggest to not worry about it until we have numbers that can prove it?

Sounds good to me - yeah, I'm unsure how much this is going to be used in practice, let's see.

@derivers: Hm, if not possible with the default deriver, the pattern can be easily customized and I could see modules like ECK to pursue that? So as it's conceivable that this situation will happen I think we should check for it. Given we already have a suiting exception that's certainly what one would except to get then.

Yeah, it should be possible to do something like that, but as written in IRC, why would someone call SomeGenericECKEntityClass::load(1), it's kind of obvious that it won't work, as you don't know what entity this is in the first place, unlike Node::load() or User::load(). It's as senseless as calling Entity::load(1) :)

Anyway, if you insist on it, I'll extend it, but probably not before tomorrow.. so if someone else has time before that.. will assign to me when I start to work on it.

I still think this is unnecessary overhead because I can't think of a reason why you'd want to call the static load on such a generic class, but here's a patch that does it...

Well, it's quite ambiguous if we do the check for SomeEntityConstructionKitType and not for Entity. I'd agree that people probably won't call it for either class, but once we've that exception documented it should work in either case imo.

Changes look good and come with test coverage, thanks! If you are ok with the check, then I'd say this is ready. Should we add a change notice for that?

Created https://drupal.org/node/2266845.

I will also go through all change records that mention entity_load() (there are a lot of them) and update them to use this approach where it makes sense once this is in.

I'm OK with this going in as well, let's do it :)

Thanks, the change notice looks good.

I will also go through all change records that mention entity_load() (there are a lot of them) and update them to use this approach where it makes sense once this is in.

berdir++

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -416,6 +418,67 @@ public function getListCacheTags() {
+   * Tries to guess an entity type ID based on the class that is called.

This is possibly the least optimistic method description I've ever seen. It's appropriate for the code it describes, but I think it could do with a bit higher level description about what is and isn't supported. i.e. finds the class or subclass used for an entity type, throws an exception if there is a duplicate or no match?

Re-rolled (use conflict) and updated the description a bit.

This would help us with #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form. Assigning to a committer for visibility.

Not sure this needed to be assigned to webchick, so went ahead and committed/pushed to 8.x, thanks!

Yay, published the change record and updated entity_loads() where useful:

https://drupal.org/node/2187853/revisions/view/7161925/7261839
https://drupal.org/node/2083591/revisions/view/2832035/7261851
https://drupal.org/node/2079767/revisions/view/7233657/7261857
https://drupal.org/node/2050669/revisions/view/2778645/7261875
https://drupal.org/node/2032185/revisions/view/2764149/7261877
https://drupal.org/node/2012896/revisions/view/6981155/7261891
https://drupal.org/node/1972410/revisions/view/2784587/7261893
https://drupal.org/node/1910176/revisions/view/2552696/7261955
https://drupal.org/node/1907430/revisions/view/7161951/7261961
https://drupal.org/node/1905910/revisions/view/6921509/7261963
https://drupal.org/node/1895936/revisions/view/2778059/7261971
https://drupal.org/node/1888504/revisions/view/2740903/7261973
https://drupal.org/node/1876852/revisions/view/7161947/7261981
https://drupal.org/node/1873210/revisions/view/2499456/7261983
https://drupal.org/node/1862420/revisions/view/6745963/7261987
https://drupal.org/node/1553180/revisions/view/2651988/7262001

Unrelated fix:
https://drupal.org/node/2054619/revisions/view/6961249/7261865
https://drupal.org/node/1721572/revisions/view/2329728/7261997

Uff.

I got this issue mixed up with the ::create() one.
No one ever responded to my question in #2096899-1: Add $EntityType::create() to simplify creating new entities.

Now our entity types are not swappable at all, I don't even know why we're pretending anymore. Might as well remove all of the interfaces, they're useless now.

The only limitation this introduced is that if you swap out the entity class then it must be a subclass of the one you're replacing. What use cases do you have to use a completely different implementation that must extend from a different class?

I don't think it would be valid to change a config entity to be something that is not a config entity, and it would be one hell of a task to provide a config entity that doesn't extend from ConfigEntityBase and if you need that, there's usually not much in the specific entity class that could get in the way? Most of the code there is in pre/post methods and accessor methods and that's an important part for that entity to work correctly?

And as mentioned in #2028097: Map data types to interfaces to make typed data discoverable, swapping the class is even less useful for content entities, as they're just a container for their fields which can be extended and altered, and interfaces are limited for them anyway due to configurable fields.

As far as I understood with this implementation we can still swap out entity classes, it's just the type-hint which would not 100% correct as it still points to e.g. Node instead NodeInterface. That's acceptable though imho.