Make EntityTypeManager provide an entity factory

So :
- Manager::getInstance() = load from storage using the passed in entity id, NULL if not exists (= entity_load())
- Manager::createInstance() = create a fresh structure from scratch (=entity_create())

That's very different from the usual pattern where getInstance() is a wrapper around createInstance(). The contracts in the current phpdoc for the respective FactoryInterface::createInstance() & Mapperinterface::getInstance() are currently a bit blurry, but I guess getInstance() is intentionally loose for any plugin type to give it its own meaning & behavior.

This is actually quite similar from the ConfigMapper @EclipseGc adds in #1535868: Convert all blocks into plugins (gets a plugin based on configuration loaded from a config file, if found, nothing of not). That patch does take care of adjusting MapperInterface though:

(from the blocks patch)
+++ b/core/lib/Drupal/Component/Plugin/Mapper/MapperInterface.php
@@ -24,9 +24,10 @@
-   * @return object
+   * @return object|false
    *   A fully configured plugin instance. The interface of the plugin instance
-   *   will depends on the plugin type.
+   *   will depends on the plugin type. If no instance can be retrieved, FALSE
+   *   will be returned.
    */
   public function getInstance(array $options);

(over there, FALSE is returned rather than NULL if not found in storage, though)

Other than that, I was a bit skeptical about making "node 1 is a plugin instance" turn into a reality. Mental bug.
OTOH, doing so means that if we solve #1863816: Allow plugins to have services injected, we solve "How to make entities receive injected objects from the container" as well.

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestFactory.php
@@ -0,0 +1,28 @@
+/**
+ * Defines a mock factory for creating entity_test entities in unit tests.
+ */
+class EntityTestFactory implements FactoryInterface {

Hm - can you explain why entity_test needs this special treatment? (perhaps even in code)

Why isn't it using the regular EntityManager?

Also, how does this relate to #1847002: Move entity type classes from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider\Entity which I thought we kinda agreed on doing?

At some point we want to deprecate most of (if not all of) entity.inc and replace the entire entity eco-system with fully injectable OO. I am 100++ doing this rather early than late and really think we should start discussing this as a whole. There is no META issue for that yet. Instead of scattering single conversion issues all over the place we should maybe look at the big picture first. We need a thoroughly thought-through battleplan for this as a whole.

Making them functional will allow classes that need to create and load entities to do so via an injected manager rather than by calling global functions. This is another step towards making things unit testable and better managed via the dependency injection container.

Totally! So, +1 on having that EntityManager. But the question is: Should the EntityManager be a PluginManager also?

As we need to solve #1863816: Allow plugins to have services injected for entities also, the plugin system actually seems to be a good fit. However, entities have the extra requirement of serialization, which we'd have to manage for injected services..

At least for creating, we don't need a factory. If we wait on #1831264: Use the Entity manager to create new controller instances, this is all we need.

(Not 100% sure about loading yet, since getInstance() takes an array)

+100000000000 to this.

Eclipse

No need for the factory now.

If that passes tests RTBC

Eclipse

This means that the storage controller de facto is the factory ? Would it make sense to embrace that and make storage controller implement FactoryInterface ?

#9: entity-1867228-9.patch queued for re-testing.

#12, you could do that, but it doesn't really matter and would just complicate EntityStorageControllerInterface. The EntityManager implements FactoryInterface, and just because it decides to proxy its createInstance() call to another class doesn't mean that class needs to implement FactoryInterface itself.

Whoops!

I agree with @yched.

Even though the override/interface situation is a bit weird here, since:

- EntityManager extends PluginManagerBase
- PluginManagerBase implements FactoryInterface already (but forwards all calls through $this->factory) [which is weird, since that essentially means that a plugin manager can fulfill the interfaces, even when not providing an actual factory]

...I still think it is technically correct that EntityManager should implement FactoryInterface, since it is explicitly taking over the interface method(s).

As such, the change mainly serves for long-term documentation and clarity purposes, but I think it's more correct.

Well, my point was about having the storage controller implememt FactoryInterface, and in fact I think @tim's answer in #14 works for me.

Less sure about #17. PluginManagers *are* factories, that's the very design of the plugin API, I don't think we need to make that extra explicit in the specific case of EntityManager.

I have a feeling that this boils down to "does it make sense that the storage controller is the thing that ultimately is in charge of entity_create(), or does create() belong to a real, storage-agnostic factory" - which IIRC was discussed at some point, and I'm not sure where we stand on this.
At any rate, changing that design is not a topic for this patch.

Yes, it was discussed in the Entity UUID issue where Dries thought that it's wrong that the storage controller is the factory.

We also discussed this: #1893772: Move entity-type specific storage logic into entity classes

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -311,6 +310,13 @@ public function processDefinition(&$definition, $plugin_id) {
+  public function createInstance($plugin_id, array $configuration = array()) {
+    return $this->getStorageController($plugin_id)->create($configuration);

I think this would at least deserve a comment that the "plugin configuration" are the entity values in the case of an "entity plugin".

Still, I must say I find that wording very confusing. But moreover, it doesn't work well with #1868004: Improve the TypedData API usage of EntityNG - what I still think is a good idea to do.

Making each entity type a proper data type of the typed data API would maked it a "type" plugin also, thus it would have to be create-able via the typed data manager, which uses "typed data definitions" as $configuration and provides another helper (TypedDataManager::create()) for instantiating objects + setting a value.

Likewise, I think the entity-factory should be used by the storage controller during load *and* create, while entity-create should be a separate helper using the factory *and* taking care of setting the values. So $configuration does not need to be $values - instead I think having an array including the $bundle would make sense here.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -170,7 +170,7 @@
-class EntityManager extends PluginManagerBase {
+class EntityManager extends PluginManagerBase implements FactoryInterface {

This is just redundant. What does it give us?

RTBC #16 in my opinion. The rest of the conversation seems to be mostly documentational in nature, and can be done as a follow up. Unless fago feels like the entire entity system can be rewritten as TypedData Plugins very quickly, I'd suggest we not prevent this issue from getting committed. In fact, even with fago's issue, I'd suggest this go in now. it's minor and doesn't increase the work that would have to be done to move to what fago was hoping for.

Eclipse

Note, #16 is RTBC not #17. Retitling to reflect the scope of the patch.

Just reuploading the exact file, to make sure the right patch goes in :)

Unless fago feels like the entire entity system can be rewritten as TypedData Plugins very quickly, I'd suggest we not prevent this issue from getting committed. In fact, even with fago's issue, I'd suggest this go in now.

That's fine with me, however I still think entity values == $configuration is a major WTF we should not do.

+ public function createInstance($plugin_id, array $configuration = array()) {
+ return $this->getStorageController($plugin_id)->create($configuration);

Citing from above:

Likewise, I think the entity-factory should be used by the storage controller during load *and* create, while entity-create should be a separate helper using the factory *and* taking care of setting the values. So $configuration does not need to be $values - instead I think having an array including the $bundle would make sense here.

So what about load also? If we have a factory we should use it.

I still think load() should be implemented as an implementation of MapperInterface::getInstance() per #1. But since given the other comments in this issue, I think it needs more discussion, so can we do that as a followup?

I don't think getInstance() == load and createInstance() == create works out well. Both are supposed to get you a new instance, while get instance can determine the instance based upon various options. But loading is not about returning a new instance of an object, it's more. I still think entity_load() should stay separately and use the factory when it's about to initialize the entity object during load - that is what a factory is for - not?

If FactoryInterface named the variable more generically, like $values or $data, then I don't think there's an architectural WTF in entity values equaling this argument.

True - but that's not the case. Likewise as for loading, it would make sense to me if we have StorageController's create to use the factory to initialize the entity object - just as load.

That would actually make $entity_manager->createInstance() a real factory which you can use to instantiate any kind of entity object. I've seen/fixed testing code in core that used entity_create() to instantiate an object although it is not new - so with that separation we could use $entity_manager->createInstance() in situations like that instead.

Thus, what about that?

EntityManager::createInstance() <--> factory, initiate entity objects for whatever use
entity_create() -> StorageController::create() -> EntityManager::createInstance()
entity_load() -> StorageController::load() -> EntityManager::createInstance()

Then, the question what $configuration isremains independently from that:

$configuration['values'] sounds better to me, but maybe it would be even better to re-name it to $data? Howsoever, at least it needs documentation.

I'm basically ++ to effulgentsia's comments here. createInstance() is intended to be public api for all plugins, it's part of what makes them plugins. I'd prefer it be useful to application code regardless of how the rest of core may or may not be using that method, effulgentsia's interpretation is the intent of the plugin system. I think the entity manager has expanded on the typical manager paradigm in a lot of really good ways that support entities very nicely and locate their api in one place. This is nice, and it's why I've been reviewing and rtbcing these patches as they come through. I think we're very close to being able to point to entities as a good example of how plugins should be implemented.

Eclipse

If we want to go with my interpretation, then another way we can achieve fago's desire for there to be a factory that both create() and load() use is we can make a separate EntityFactory that just does the new and pass this factory to StorageController (EntityManager can be responsible for that injection), but meanwhile keep #27's behavior of EntityManager::createInstance() calling StorageController::create().

The EntityFactory sounds like a good idea to me, but according to the interface createInstance() is already supposed to be a factory, see http://api.drupal.org/api/drupal/core!lib!Drupal!Component!Plugin!Factor.... I must say that an otherwise interpretation really confuses me?

createInstance() is intended to be public api for all plugins, it's part of what makes them plugins. I'd prefer it be useful to application code regardless of how the rest of core may or may not be using that method, effulgentsia's interpretation is the intent of the plugin system.

Surely, it's public API - it's the factory. That does not mean you cannot use a higher-level factory in practice though. That's the pattern you suggested me doing for the TypedData API and which we have implemented there, e.g. it has TypedDataManager::getPropertyInstance() and create() what is higher-level API that gets used in practice.

Also, I think we'll need to to #1868004: Improve the TypedData API usage of EntityNG for cleanly integrating entities with the TypedData API and properly work with stuff like its validation. Thus, we'd get the "plain factory" at the typed data level there - what would free us to use the EntityManager as higher-level factory.

But still, having a factory doing entity_load() or entity_create() is confusing to me and not what I would expect a factory to do. To me a factory has to care about object instantiation and nothing more. But maybe it's just me who gets that wrong? - so I'd love to here more opinions on that.

But still, having a factory doing entity_load() or entity_create() is confusing to me and not what I would expect a factory to do. To me a factory has to care about object instantiation and nothing more

This is why I now think this issue is won't fix. Entities aren't valid unless they've run through either one of those, just instantiating a new object of the right class isn't enough. You skip some important hooks and methods. At this point we don't need an Entity factory.

Entities aren't valid unless they've run through either one of those, just instantiating a new object of the right class isn't enough.

Oh, imo there are valid use cases for the factory. E.g. entity serialization API may want to create an entity without running create() or load(). Similarly, the use might be in test-cases where you create an entity with pre-defined values (maybe even including an ID) just for testing purposes - this manually created entity it's not necessarily new and thus should not be marked new or trigger creation hooks.

Re #33: Actually you can specify an ID in entity_create() and then (I think) isNew() will automatically realize that and return FALSE. Either way, the use-case you describe was specifically considered when designing the whole isNew() business. I don't know why we would ever want to work/code around entity_create().

We're not coding around entity_create() we're putting it in the centralize api on the same class as the rest of the relevant entity methods.

Right, that's not my problem. My problem is this:

this manually created entity it's not necessarily new and thus should not be marked new or trigger creation hooks

#27: entity-1867228-27.patch queued for re-testing.

Tagging.

Here's a new approach to this.
This is related to #1988492: Avoid having empty field items by default, since this will allow EntityNormalizer to identify non-modified fields, while we can provide sane empty values e.g array() instead NULL for lists.
I've some strange test-fails locally which I can't trace - let's see what the bot says.

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -76,6 +76,25 @@ public function __construct(array $values, $entity_type) {
+  public static function createInstance(array $values, $entity_type, $bundle = FALSE, $translations = array()) {

This seems very confusing to have the same method name as Plugins, and having this whole effort based on making them true plugins, but then not actually calling the plugin code

#40: d8-entity-manager-entity-factory-1867228-40.patch queued for re-testing.

1. +++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
   @@ -245,8 +234,8 @@ protected function mapFromStorageRecords(array $records, $load_revision = FALSE)
   +        $entities[$id] = \Drupal::entityManager()->instantiateEntity($this->entityType, $bundle, $entities[$id]);
   
   +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -196,6 +196,81 @@ public function hasController($entity_type, $controller_type) {
   +  public function instantiateEntity($entity_type, $bundle = FALSE, $values = array(), $translations = array()) {
   

   This seems weird, why not use createInstance()?

2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -76,6 +76,25 @@ public function __construct(array $values, $entity_type) {
   +    $definition = \Drupal::entityManager()->getDefinition($entity_type);
   +    if (!$definition) {
   +      throw new \InvalidArgumentException(sprintf('The %s entity type does not exist.', $entity_type));
   +    }
   +    // Check if this entity type has bundles and if so if a bundle is defined.
   +    $bundle_key = !empty($definition['entity_keys']['bundle']) ? $definition['entity_keys']['bundle'] : FALSE;
   +    if ($bundle_key) {
   +      if (empty($bundle)) {
   +        throw new EntityStorageException(format_string('Missing bundle for entity type @type', array('@type' => $entity_type)));
   +      }
   +    }
   

   All of this should be in the entity manager.

   This method should ONLY be called by EntityManager::createInstance()

3. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -76,6 +76,25 @@ public function __construct(array $values, $entity_type) {
   +    $entity = new $definition['class']($values, $entity_type, $bundle, $translations);
   

   This should be return new static($values, $entity_type, $bundle, $translations);

@tim.plunkett Thanks four your feedback, I tried to incorporate your feedback. I hope this is closer to what we're looking for.

#46: d8-entity-manager-entity-factory-1867228-46.patch queued for re-testing.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -199,63 +197,51 @@ public function hasController($entity_type, $controller_type) {
    * @param string $plugin_id
    *   The type of the entity.

Why not call this $entity_type here. $plugin_id makes no sense here.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -199,63 +197,51 @@ public function hasController($entity_type, $controller_type) {
    * @param array $configuration
-   *   The configuration to create an entity instance. Required keys are
-   *   "bundle" and "values".
+   *   The configuration to create an entity instance. By default available keys

Having that as the only API function as entity factory it's quite a WTF to call it $configuration.

Could we just rename $configuration to $values and pass it on as values + add additional parameters for $bundle and $translations?

I don't care about entities being plugins or not, but we should provide an entity factory with decent DX and that $configuration parameter is just confusing. Not sure what's the point of plugins forcing us to that method signature but that's probably another issue....

I could have lived with having the ugly createInstance() in addition to a proper API function like the previous instantiateEntity(), but just having the ugly method does not work out I think.

+++ b/core/lib/Drupal/Core/TypedData/ItemList.php
@@ -133,6 +133,10 @@ public function offsetGet($offset) {
   protected function createItem($offset = 0, $value = NULL) {
+    // If no value is set but it's a list, store an empty array instead NULL.
+    if (!isset($value) && !empty($this->definition['list'])) {
+      $value = array();

Unrelated?

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
@@ -250,9 +250,8 @@ public function getPropertyInstance(TypedDataInterface $object, $property_name,
-    if (isset($value)) {
-      $property->setValue($value, FALSE);
-    }
+    // Set even NULL values to make sure the value is set accordingly.
+    $property->setValue($value, FALSE);

Unrelated?

As of lack of knowledge of plugins and so on I don't feel entitled to have an opinion about the method names / signatures. Please let us meet, discuss that and find a conclusion. Then I'll happily convert that into code.

Unrelated?

Ooops, that's stuff of #1988492: Avoid having empty field items by default - removed.

Fixed ViewsUI.
Set to needs review to let the test bot do its work.

Discussed with tim.plunkett and daspeter that we should just fix the method signature of createInstance() to what makes sense - luckily we can do that as our second parameter is an array anyway. Also, we need to pass on the container to static createInstance() methods.

However opened #2100549: Plugin factories needlessly restrict configuration to arrays anyway.

Hang on, a very similar issue to this got closed (won't fix) a while ago. What's the difference now?

Which one? There are too many of those...

I hope this reflects what we've discussed earlier.
I tried hard to follow berdirs suggestion and put the values passed into the constructor of EntityNG into the raw values property of the class instead relying on the magic setter.
However, I miserably failed doing so - I wasn't able to get the tests working (e.g. ContentTranslationWorkflowsTest). However, the code of my attempts is still in the class but commented-out for the moment.
If someone has an idea how to get this running please feel free to give it a try :)

@fago, I think it was https://drupal.org/node/1931894. Seems like almost the same thing that was rejected for making DI worse, not better.

EDIT: Tim this question is for you too :-) you were against creating entities from the manager originally, which is why we closed it. What's changed?

Silly me, I forgot the actual fix that should avoid the fails :|

Blargh, set issue status for bot.

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -76,6 +77,13 @@ public function __construct(array $values, $entity_type) {
+   * Implements \Drupal\Core\Entity\EntityInterface::createInstance().
+   */
+  public static function createInstance(ContainerInterface $container, array $values, $entity_type, $bundle = FALSE, $translations = array()) {
+    return new static($values, $entity_type, $bundle, $translations);
+  }
+

This is just called create() everywhere else in core isn't it?

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -196,6 +194,64 @@ public function hasController($entity_type, $controller_type) {
+   * Returns an empty entity object. If you need an entity object with default
+   * values applied use \Drupal\Core\Entity\EntityManager::create() instead.

Docs seem wrong here, should probably be createEntity(). That being said, this is confusing. Why would we accept values here if we don't intend on using them? Following the flow of code, this ultimately runs through the Entity.php __construct() method which does indeed use the values, which for me calls into question the createEntity() method.

Looking at that method, this is all further confused by the fact that we use the storage controller in this case, but not in createInstance, nor within Entity::__construct(). That being said, this patch removes the __construct handling of $values, which again has me asking about createEntity vs createInstance. Seriously, W.T.F.? Please enlighten me.

In addition to all of this, I find the $bundle handling quite weird (and always have). Why don't we just add these parameters to $values since that's ultimately going to be what they represent on the entity anyway? If it's just a convenience thing, then that seems a perfect use case for createEntity() as a wrapper of createInstance() but none of that is happening here and I really don't understand why. I'm not trying to be antagonistic at all, but an explanation here would make this a whole lot easier.

Thanks!

Eclipse

I'll try to explain why I started to work on this this might shed some light on it.
A while ago I started working on #1988492: Avoid having empty field items by default, which showed that we've some trouble to distinguish between empty / and not set values / fields in the EntityNormalizer.
After some attempts to solve that - and breaking data type contracts while doing so (lists started to return NULL instead array()) we identified the fact that entity_create() applies default values to fields as a problem. Because of this we don't have a truly "unavailable" state of fields as required by the EntityNormalizer to figure out what was "unset" and what was untouched when merging data.
Now the idea came up to provide a low-level function to get an proper instance of an entity without default values applied: createInstance() was born.
And since we thought it would be handy to have those functionality provided on central place while respecting the fact that every entity could have it's own special way to build itself we started working in this issue.
While working on this I realized that DatabaseStorageControllerNG::create() doesn't pass the provided pre-set values to entity class - what doesn't make sense. So I started to move the handling of values to pre-set into EntityNG and only left the part that applies default values in DatabaseStorageControllerNG::create().

As far as I can see we've mainly a problem with naming methods and defining responsibilities. Unfortunately the only thing I can provide to support the decision making are my own goals:

Can anyone please bring up other goals / requirements / limitations?

That being said, this patch removes the __construct handling of $values, which again has me asking about createEntity vs createInstance. Seriously, W.T.F.? Please enlighten me.

I think you miss-read the patch. Actually it brings back the values handling into the constructor of EntityNG, it's just different code and moved around within the constructor. The unpatched version of the create method in the storage controller simply never passes values into the constructor and does everything on its own.

Docs seem wrong here, should probably be createEntity(). That being said, this is confusing. ...

No, they are not. They document current behaviour: entity_create() != a factory. That's not touched by this patch and changing entity_create() is not the scope of it either, it's about introducing the long-missing factory *only*.

That being said, this patch removes the __construct handling of $values, which again has me asking about createEntity vs createInstance. Seriously, W.T.F.? Please enlighten me

This part of the code was actually not used at all, but values sneaked in differently. Entity object instantiation is bit messy right now, this is why this patch has to clean it up. But yes, it should receive $values and apply them during construct.

In regard to the naming, there is no createEntity() (yet?). So far entity create() means the creation of new entities, not only new entity object instances.
We already use the static createInstance() for all entity controllers, so this patch just follows this pattern for now to avoid confusing with create(). I'm happy to discuss naming those things differently, but that discussion happens already over at #2015535: Improve instantiation of entity classes and entity controllers - so let's focus on it there?

Consequently, could we concentrate on adding a proper entity factory here, and settle on the names in #2015535: Improve instantiation of entity classes and entity controllers ? As das-peter points out we really need an entity factory - so let's do it with the names we have in place *now* and avoid derailing it on possible renames of entity_create().

Did people see #56? :)

Here's my next attempt.
I think I managed to get entityNG working with raw values instead the full fledged properties. Some really odd (magic) stuff related to languages / translations was happening here.
If this works out it likely improves the performance of entity_create() EntityNG entities.

By the way, does anybody have a feedback for Damian regarding #56?

damiankloip, the issue linked was just about creating short-cuts. This one isn't, it introduces something we are missing right now: an entity factory. $storage_controller->create() is no factory, e.g. you would not use it to create entity objects during load...

Thanks daspeter for figuring that out, here a review:

+++ b/core/includes/entity.inc
@@ -375,21 +375,25 @@ function entity_delete_multiple($entity_type, array $ids) {
 function entity_create($entity_type, array $values) {
-  return \Drupal::entityManager()
-    ->getStorageController($entity_type)
-    ->create($values);
+  return \Drupal::entityManager()->createEntity($entity_type, $values);

Imo this patch should not change how entity-create operations are performed - it's out of scope for introducing a factory. As said, let's leave the re-naming discussion to #2015535: Improve instantiation of entity classes and entity controllers.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -101,27 +101,16 @@ public function create(array $values) {
+    foreach ($entity->getPropertyDefinitions() as $name => $property_definition) {
+      if (!array_key_exists($name, $values)) {
         $entity->get($name)->applyDefaultValue();
       }
-      unset($values[$name]);

Looks good, but does this work for the bundle property if the $bundle is passed but not in $values?

+++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
@@ -30,6 +31,23 @@
+   * Instantiates a new empty entity object.

I'd suggest
"Creates an entity object instance based on its values."

Better do not use "new" here as it is not necessarily $entity->isNew().

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -196,6 +194,64 @@ public function hasController($entity_type, $controller_type) {
+   *   are
+   *   bundle: the bundle of the entity,
+   *   values: An array of values to set, keyed by property name,
+   *   translations: An array of available translations of this entity.

Should make use the usual dashes for the enumeration.

Misses a @throws for the exception thrown. The exception is not necessarily related to storage, maybe us "InvalidArgumentException" ?

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -196,6 +194,64 @@ public function hasController($entity_type, $controller_type) {
+  public function createEntity($entity_type, array $values) {
+    return $this->getStorageController($entity_type)->create($values);

out of scope?

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -156,7 +157,22 @@ public function __construct(array $values, $entity_type, $bundle = FALSE, $trans
+    // the raw values array of the entity.
+    // The raw values array will be used whenever properties accessed.

Elsewhere we've been calling them "plain values", maybe go with that here also?

Lastly, should we add a unit test case for it?

A bit lost between this and #2015535: Improve instantiation of entity classes and entity controllers - can we post an overall plan in a issue summary somewhere ?

Also, in Prague, it was mentioned that:
- we want to try to move away from direct $entity->property access on config entities in favor of getters / setters
- yet the current way we create entities (entity_create()) doesn't let IDEs get a proper type hint on the actual class/interface that is being created and is thus ruins method autocomplete.
Do we have a proposal to address that ?

@yched: #2096899: Add $EntityType::create() to simplify creating new entities

@Berdir: cool, thanks !
That's now three issues about entity factories though, so ++ on an overall plan somewhere :-)

Here's another update.
#67:

1. It doesn't change "how" the entity create operations are performed but where the "real" code is located. I thought it would be better to be consistent and have \Drupal::entityManager()->createEntity() and \Drupal::entityManager()->createInstance($entity_type, $values)
2. I've made sure that bundle is set in the values now (EntityManager::createInstance())
3. Adjusted
4. Adjusted
5. See 1.
6. Adjusted

While working on this if found some strange things:

• $user->homepage didn't have a property definition - changed
• EntityCrudHookTest used "language" instead "langcode" what lead to overwriting the protected language property of EntityNG aka ContentEntityBase! Since I've changed the values handling now that shouldn't happen anymore.

I'm afk until next Wednesday, so feel free to continue without me ;)
This patch wont pass all tests but it should be better than the last one.

PS: I've no clue why but I can't upload the patch as patch file (I get a 500 internal server error...). Thus I've attached it as zip archive.

Re-roll.

Fixing tests.

Some interesting fails in there.

- Most test fails are related to isDefaultRevision not being set correct, added a hack, seems that should be handled in a better way, e.g. by an explicit call from the storage controller, but that is currently hard before we refactor that more
- Then I changed the $values wrapping in language code to only happen for defined fields, that fixed the menu breadcrumb test because otherwise $node->menu was suddenly an array with key LANGCODE_DEFAULT and the menu link entity within. No why nothing else exploded.
- Then there's the weird autocomplete code that I already noticed to be broken/weird in #2078387: Add an EntityOwnerInterface, which will properly fix this as a side effect, this is just bandaid.
- The TranslationTest is weird. I wasn't able to reproduce those php notices, but it blew up completely for me, and the code there is imho old and bogus. Changing the actual field name there resulted in an error because it got too long, so I shortened the suffix.

I corrected spelling errors, all of them in documentation, with the exception of one in a translatable string.

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -164,6 +162,73 @@ public function hasController($entity_type, $controller_type) {
   +   * @param array $values
   +   *   The values to create an entity instance with. By default available keys
   +   *   are
   +   *   - bundle: the bundle of the entity,
   +   *   - values: An array of values to set, keyed by property name,
   +   *   - translations: An array of available translations of this entity.
   +   * @param string|false $bundle
   +   *   The bundle of the entity.
   +   * @param array $translations
   +   *   The langcodes of the available translations of the entity.
   

   This is confusing and probably due to moving back and forth, but what now, bundle as part of $values or argument? :)

2. +++ b/core/modules/user/lib/Drupal/user/Entity/User.php
   @@ -550,6 +550,12 @@ public static function baseFieldDefinitions($entity_type) {
          'type' => 'string_field',
        );
   +    $properties['homepage'] = array(
   +      'label' => t('Homepage'),
   +      'description' => t("The user's home page address."),
   +      'type' => 'string_field',
   +      'computed' => TRUE,
   +    );
        return $properties;
   

   user's don't have a homepage. Only anonymous users on comments have.. yes, weird.

   So, I think this is wrong. Let's remove it and see what happens, would like to see where this fails.

3. +++ b/core/modules/user/user.module
   @@ -659,8 +659,8 @@ function template_preprocess_username(&$variables) {
        // easier for other preprocess functions to append to it.
        $variables['link_options']['attributes']['rel'] = 'nofollow';
   -    $variables['link_path'] = $account->homepage;
   -    $variables['homepage'] = $account->homepage;
   +    $variables['link_path'] = $account->homepage->value;
   +    $variables['homepage'] = $account->homepage->value;
   

   Ah, this I guess?

   Hm, this seems a trick that comment.module uses to render that, and I would imagine that my fix in the constructor would fix that too.

Ok, fixed the documentation so that it IMHO makes sense and reverted the homepage changes.

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -164,6 +162,69 @@ public function hasController($entity_type, $controller_type) {
   +   * Returns an empty entity object. If you need an entity object with default
   

   I think we could do better with the differentiation. What about:

   createInstance():

   Instantiates an entity object based on its values.

   (.i.e. avoid using the word "create" as creating involvnes something new, and people should not think about new entities here)

   If you want to create a new entity, use EntityManager::create() instead.

   (having EM::create() apply default values is one thing that both method differs, but it should not be the main reason to go with one method or the other. The mean reason should be what you are supposed to do: create a new entity, or instantiate an entity object)

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -164,6 +162,69 @@ public function hasController($entity_type, $controller_type) {
   +   * @see entity_create()
   

   I'd suggest stopping to "advertise" that function.

3. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -164,6 +162,69 @@ public function hasController($entity_type, $controller_type) {
   +   * Constructs a new entity object with default values, without saving it.
   +   *
   

   I'd suggest:

   "Constructs an entity object for creating a new entity."

   "Note that for the permanent creation of the new entity, the returned entity object needs to be saved first."

   (imho it's enough to mention applied default values in the @return description)

Is there a reason we need to be breaking the createInstance() signature? Seems like bundle could easily exist in the $values since that's what it ultimately gets merged into anyway.

Eclipse

78: entity-factory-1867228-78.patch queued for re-testing.

Re-roll, updated the comments.

@EclipseGc: We discussed this at length in Prague and agreed that this is the best approach. EntityManager still isn't really a plugin manager and there are still issues to move away further.

@plach: I'm completely lost in regards to the language related changes that happened and how it affects this issue. Care to have a look?

Rerolled after #2005716: Promote EntityType to a domain object. This should also fix the language-related failures.

Mmh, those exceptions might be related to #2156945: Clean up installer entity defaults following bugfix.

This should be green.

1. +++ b/core/includes/entity.inc
   @@ -373,23 +373,26 @@ function entity_delete_multiple($entity_type, array $ids) {
   - * Constructs a new entity object, without permanently saving it.
   + * Constructs a new entity object with default values, without saving it.
     *
   - * @param $entity_type
   ...
   + * entity object use \Drupal\Core\Entity\EntityManager::createInstance()
   + * instead.
   

   The docs should be aligned with createEntity() docs.

2. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
   @@ -15,6 +16,25 @@
   +   *   An empty new entity object.
   ...
   +  public static function createInstance(ContainerInterface $container, array $values, $entity_type, $bundle = FALSE, $translations = array());
   

   nope, it's not necessarily empty - it depends on the value. It should say "The instantiated entity object."

3. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -167,6 +165,68 @@ public function hasController($entity_type, $controller_type) {
   +   *   An empty new entity object.
   +   */
   +  public function createInstance($entity_type, array $values = array(), $bundle = FALSE, array $translations = array()) {
   

   same here.

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -143,20 +143,27 @@ public function __construct(array $values, $entity_type, $bundle = FALSE, $trans
   +    if (!empty($values['isDefaultRevision'])) {
   +      $this->isDefaultRevision($values['isDefaultRevision'][Language::LANGCODE_DEFAULT]);
   

   Hm, we could just handle isDefaultRevision internally as a regular field? Not necessarily stuff for this issue though.

Just a re-roll for now, did not change anything I didn't have to. Had some noticed locally, let's try the testbot.

91: entity-factory-1867228-91.patch queued for re-testing.

Another re-roll, review still not addressed.

This will reduce *some* of the special casing and burden on storage controllers.

Re-roll.

Trying to fix that, but now we have the interesting problem that file normalization does no longer add default values, which is I think by design but the tests are expecting that. So someone else would need to do that?

100: entity-factory-1867228-100.patch queued for re-testing.

updating the patch with reroll.

Fixing a getStorageController() call.

106: entity-factory-1867228-106.patch queued for re-testing.

Given the missing factory results in problems again and again (e.g. at #2458817: Creating new user entities for anonymous users is very slow), I think this easily qualifies as major.

Re-roll.

Rerolled. Looking into fails...

... Amateescu and I talked about the idea to compile the entity information into a class for each entity type, given that this class could then potentially
have the storage code, this could somehow invalidate some of the work of this issue as this compiled class could hold that factory.

@fago @entity_people
Feel free to tell my I'm thinking bullshit.

#2553853: Optimize and/or cache dummy/empty entity creation will be closed in favor of this issue.

Probably could be closed as outdated
there's 2 "creators" already, so having 3rd useless

EntityManager is deprecated, isn't it? https://www.drupal.org/node/2549139

Sure, but that just means it moves to EntityTypeManager.

I think this is still relevant, because the method right now exists but is broken. This different to creating a new entity, it's about standardising on how an entity object is created, whether it is loading an existing one, creating a new example and special cases like content entity normalization for patching one, which is the best example why this would be nice to have.

I think this would encapsulate/mark duplicated #2998471: Make the entity storage accessible from the entity object in so far as if entities have a factory, they could receive services from the dependency injection container such as the entity storage...right?

as API change it should go to 10.1

also there's 3 TODOs

core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueEntityStorage.php:101:    //   https://www.drupal.org/node/1867228 improves this but does not solve it
core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1671:          // factory, see https://www.drupal.org/node/1867228.
core/modules/user/src/Entity/User.php:425:      //   https://www.drupal.org/node/1867228.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Seems some failures in MR 3240