Add $EntityType::create() to simplify creating new entities

Entity types are theoretically swappable. You could swap the 'node' entity type out for a new class. If we start using Node::create(), that will fall through.

Indeed sometimes you want to denormalize something right into the Node table and do something with it; but one would need to review the node class whether you want to replace the Node for that or just NodeStorageController.

If we start using Node::create(), that will fall through.

During the discussion we figured this should not be a problem, as Node::create() then would just create an instance of the overridden class as a node_create() helper would do. As it's documented that it would return NodeInterface that should be fine as long as people fulfill that. (and if they don't, it's there fault)

Fair enough. new static I guess.

Here's a quick stab at how this could work. It shows an example of when entities would need to explicitly supply their name, but that should be the exception.

One problem I ran into on this however, is that the static entityManager property ends up in the config entities yaml file. No idea what's going on there, but it looks like we're doing some kind of introspection of properties to come up with that? Seems super fragile…

?

This should fix the bug I described in #5.

I think the document @return is quite a win and worth defining it on each of the entity classes. Shouldn't be a big deal, is it?

I thought the idea was that we'd either use the @method phpdoc and not override, or override and just call parent? I dont see a reason to implement any logic on a per entity basis there.

1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -23,6 +23,8 @@
   +  static $entityManager;
   

   Is this public so that we can reset it?

2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -59,6 +61,37 @@ class Entity implements \IteratorAggregate, EntityInterface {
   +  static function entityName() {
   

   I find this method a bit confusing: would it be so bad if we renamed it to Entity::entityType() and dropped the non-static one?

I thought the idea was that we'd either use the @method phpdoc and not override, or override and just call parent? I dont see a reason to implement any logic on a per entity basis there.

Yeah, as long as the entity-type specific return interface is documented I'm happy. But I do not see that in the patch?

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -59,6 +61,37 @@ class Entity implements \IteratorAggregate, EntityInterface {
+  static function manager() {
+    if (!static::$entityManager) {
+      static::$entityManager = \Drupal::entityManager();
+    }
+    return static::$entityManager;

I'm not sure this is really worth another static?

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -59,6 +61,37 @@ class Entity implements \IteratorAggregate, EntityInterface {
+  static function entityName() {
+    $parts = explode('\\', get_called_class());
+    return \Drupal::underscore(end($parts));

This should by entityType(), but that would clash with the non-static variant I guess?

Updated issue summary.

How would this play with the fact that runtime code isn't suppose to hardcode any knowledge of the actual class for an entity type, which could be swapped ?
That's the whole point of the "entity type string name" indirection (talk about 'node' rather than Node)

Also, how would this play with #1867228: Make EntityTypeManager provide an entity factory ?

Postponed on #2190313: Add $EntityType::load() and loadMultiple() to simplify loading entities per @Berdir.

Can we get some clarification on why this is postponed on that one? Seems like the two could easily happen in parallel and whichever's ready first wins.

The reason I ask is there are starting to be a bunch of patches that speed up tests (yay!), but as a result they make the tests 100x more verbose and complicated (as an example, see #2254183: Fix test performance of Drupal\filter\Tests\FilterAdminTest, which Dries rightly pushes back on for this reason). If instead this logic lived in FilterFormat::create(), for example, this would not only help test authors but also distribution authors and everyone else.

#2190313: Add $EntityType::load() and loadMultiple() to simplify loading entities is in, so this is all we need to do here now.

Sure, would this be enough?

That's a good addition and goes inline with #2190313: Add $EntityType::load() and loadMultiple() to simplify loading entities. Patch looks fine as well, RTBC+1.

Awesome, this patch makes me very happy from a DX perspective! :) And is a good complement to ::load().

Unfortunately, Tim's concerns are still unresolved. Though he did mention in IRC that #2028097: Map data types to interfaces to make typed data discoverable might help, and also acknowledged that this concern affects a very small minority of contrib. I think if this turns out to be a real limitation in a contrib module we can revisit at that time.

Committed and pushed to 8.x. Thanks!