Posted by xjm. Updated: Comment #2

Problem/Motivation

1). Followup from #1995868: Fatal when using a role contextual filter. Entity::entityInfo() looks like this:

<?php
public function entityInfo() {
  return
entity_get_info($this->entityType);
}
?>

This is entity_get_info():

<?php
function entity_get_info($entity_type = NULL) {
  if (empty(
$entity_type)) {
    return
Drupal::entityManager()->getDefinitions();
  }
  else {
    return
Drupal::entityManager()->getDefinition($entity_type);
  }
}
?>

This adds a dependency on the entity manager and requires the container and entity.inc to be available to use this method. #1995868: Fatal when using a role contextual filter changes it to use Drupal::entityManager()->getDefinition() directly.

2), That EntityControllerInterface::createInstance() is our only factory method not named create() is very very confusing. see comment #11.

Proposed resolution

See comment #12

     
  • EntityStorageControllerInterface::create() renaming to EntityStorageControllerInterface::createEntity() to create an entity instance;
  •  

  • EntityControllerInterface::createInstance() renaming to EntityControllerInterface::create() to instantiate controller itself.
  •  

  • Add ::create() to EntityInterface to replace the original way of instantiating entity, i.e., $entity = new $entity_class($values, $this->entityType);. We could use the standard way -- $entity = $entity_class::create($container, $values, $this->entityType); which is the proper way to inject dependency.

This change will affect in following ways:
1).

<?php
function entity_create($entity_type, array $values) {
  return Drupal::entityManager()->getStorageController($entity_type)->create($values);
}
?>

becomes
<?php
function entity_create($entity_type, array $values) {
  return Drupal::entityManager()->getStorageController($entity_type)->createEntity($values);
}
?>

2). Change the instantiating process of class implementing EntityControllerInterface from $class::createInstance to $class::create.

An example here is to change $class::createInstance($this->container, $entity_type, $this->getDefinition($entity_type)); to $class::create($this->container, $entity_type, $this->getDefinition($entity_type)); in Entitymanager:getController() to instantiate entity type's (storage)Controllers.

3). Instead of calling $entity = new $entity_class($values, $this->entityType); to instantiate entity in EntityStorageController's createEntity method(this method has been renamed from create to createEntity in this comment), we call $entity = $entity_class::create($container, $values, $this->entityType); to better handle the dependency injection.
**I guess we have a dependency on the entity manager and need to inject it? This seems wrong somehow.

Remaining tasks

API changes

TBD

Files: 
CommentFileSizeAuthor
#30 improve_instantiation_entity_controllers-do-not-test-2015535-30.txt97.72 KBsmiletrl
#30 interdiff-27-30.txt19.09 KBsmiletrl
#27 better_instantiate_entity_controllers-2015535-27.patch82.04 KBsmiletrl
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch better_instantiate_entity_controllers-2015535-27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#27 interdiff-25-27.txt1.43 KBsmiletrl
#25 better_instantiate_entity_controllers-2015535-25.patch80.61 KBsmiletrl
FAILED: [[SimpleTest]]: [MySQL] 57,084 pass(es), 160 fail(s), and 51 exception(s).
[ View ]
#25 interdiff-21-25.txt704 bytessmiletrl
#21 better_instantiate_entity_controllers-2015535-21.patch79.92 KBsmiletrl
FAILED: [[SimpleTest]]: [MySQL] 57,004 pass(es), 240 fail(s), and 189 exception(s).
[ View ]
#21 interdiff-19-21.txt39 KBsmiletrl
#19 better_instantiate_entity_controllers-2015535-19.patch41.55 KBsmiletrl
FAILED: [[SimpleTest]]: [MySQL] 48,237 pass(es), 2,296 fail(s), and 2,967 exception(s).
[ View ]
#19 interdiff-16-19.txt6.73 KBsmiletrl
#17 better_instantiate_entity_controllers-2015535-16.patch35.05 KBsmiletrl
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#17 interdiff-14-16.txt8.83 KBsmiletrl
#14 better_instantiate_entity_controllers-2015535-14.patch26.81 KBsmiletrl
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#6 drupal-entity_inject_entity_manager-2015535-6.patch32.17 KBiamEAP
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

Title:Entity::entityInfo() calls entity_get_info() which wraps Drupal::entityManager()->getDefinitions(), and this is sillyInject the entity manager in Entity (?)
Assigned:xjm» Unassigned

Never mind. Foiled again by typed data's getDefinition() having the same name as the plugin system's method.

Issue summary:View changes

Update summary. --xjm

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated summary. --xjm

Yes, we probably need to do this, also for the save/delete methods. But it scares me a bit ;)

Issue tags:+VDC, +Entity Field API

Issue summary:View changes

Updated issue summary.

Regarding the comment of berdir.

In an ideal world, none entity should override the logic of the save method and then you would actually also never call $entity->save() but $entityStorageController->save($entity);

Linking a sort of related issue: #2005716: Promote EntityType to a domain object

Status:Active» Needs review
StatusFileSize
new32.17 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

This is not going to be pleasant. Entity and all of its derivatives get created in myriad different ways across many different storage controllers and their derivatives. We'll have to modify the constructors for every derivative of Entity and find all of the odd ways in which entities become instantiated and tack on the extra EntityManager argument (finding them is not easy, since they're often done dynamically, e.g. $foo = new $entity_class(...);)

Here's a work in progress that will almost certainly fail. Guessing we'll also probably hit this pretty hard: #2004282: Injected dependencies and serialization hell

Also, I worked on a not-so-up-to-date commitref. For those looking to re-roll in the future: 79347b882ae08a0d18a1d6f0205a83ed913a36a7

Status:Needs review» Needs work

The last submitted patch, drupal-entity_inject_entity_manager-2015535-6.patch, failed testing.

Also, this would represent an API change, since we'd be adding a new argument to the Entity constructor (and all its derivatives).

It's not so bad, because I don't think we officially encourage or even support $entity = new Entity(...); in user-land. However, this gets called plenty in entity storage controllers; anyone extending one (or otherwise providing their own) would be in trouble.

Issue summary:View changes

Updated issue summary.

Just as a note: We don't treat changed constructors as API changes at the moment.

Will review in detail later, just this for now:

- See my comment in #2053415-22: Allow typed data plugins to receive injected dependencies, I guess we want to do the same here for the entity manager.

- We really really need a proper factory to create an entity object. Which is not the same as entity_create() (That's create a *new* entity, with default value magic and stuff). Then we'll have to update all these places too, but we can then handle the injection in a single place. Possibly something like EntityManager::something(). We need to make sure to properly document this and the difference between $storagecontroller->create(). I'm also not yet sure how these two will play together. We already moved pre/post to the entity class, that was the main reason it was on the storage controller before. And at least NG doesn't need that many custom stuff there, you can just define default values of your fields.

It might be out of scope, but if it works out that renaming EntityStorageControllerInterface::create() helps out in this issue, that would be ideal.
That EntityControllerInterface::createInstance() is our only factory method not named create() is very very confusing.

Updated accroding to comment #13.

Yeah, the two methods names are confusing here. There seems to be a consensus in community that class's ::create should instantiate itself, i.e.,

<?php
return new static($param);
?>

In this case here, EntityStorageController's create method is responsible for instantiating an Entity, instead of itself. '::createInstance' has taken over the duty of intantiating itself.

So, my suggestion would be

  • EntityStorageControllerInterface::create() renaming to EntityStorageControllerInterface::createEntity() to create an entity instance;
  • EntityControllerInterface::createInstance() renaming to EntityControllerInterface::create() to instantiate controller itself.
  • Add ::create() to EntityInterface to replace the original way of instantiating entity, i.e., $entity = new $entity_class($values, $this->entityType);. We could use the standard way -- $entity = $entity_class::create($container, $values, $this->entityType); which is the proper way to inject dependency.

This change will affect in following ways:
1).

<?php
function entity_create($entity_type, array $values) {
  return
Drupal::entityManager()->getStorageController($entity_type)->create($values);
}
?>

becomes
<?php
function entity_create($entity_type, array $values) {
  return
Drupal::entityManager()->getStorageController($entity_type)->createEntity($values);
}
?>

2). Change the instantiating process of class implementing EntityControllerInterface from $class::createInstance to $class::create.

An example here is to change $class::createInstance($this->container, $entity_type, $this->getDefinition($entity_type)); to $class::create($this->container, $entity_type, $this->getDefinition($entity_type)); in Entitymanager:getController() to instantiate entity type's (storage)Controllers.

3). Instead of calling $entity = new $entity_class($values, $this->entityType); to instantiate entity in EntityStorageController's createEntity method(this method has been renamed from create to createEntity in this comment), we call $entity = $entity_class::create($container, $values, $this->entityType); to better handle the dependency injection.

In a word, to keep naming consistency here:)

I'd go with createEntity() for the new method name, to distance itself from FactoryInterface::createInstance().

Otherwise, +1

Status:Needs work» Needs review
StatusFileSize
new26.81 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Here's a patch to apply changes 1) and 2) from comment #12.

If the two changes work well, then goto the third part, i.e., add create to EntityInterface

Title:Inject the entity manager in Entity (?)Better instantiate entity and entity controllers

update title.

Status:Needs review» Needs work

The last submitted patch, better_instantiate_entity_controllers-2015535-14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.83 KB
new35.05 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

more fix.

Status:Needs review» Needs work

The last submitted patch, better_instantiate_entity_controllers-2015535-16.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.73 KB
new41.55 KB
FAILED: [[SimpleTest]]: [MySQL] 48,237 pass(es), 2,296 fail(s), and 2,967 exception(s).
[ View ]

More fix.

Although, ::createInstance of class EntityFormController seems redundant and useless here. It should probably needs to be deleted too.

Status:Needs review» Needs work

The last submitted patch, better_instantiate_entity_controllers-2015535-19.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new39 KB
new79.92 KB
FAILED: [[SimpleTest]]: [MySQL] 57,004 pass(es), 240 fail(s), and 189 exception(s).
[ View ]

More fix.

Issue tags:+API change

Add tag. This could be API change for #2015535-12: Improve instantiation of entity classes and entity controllers. Needs approval I guess.

Status:Needs review» Needs work

The last submitted patch, better_instantiate_entity_controllers-2015535-21.patch, failed testing.

Title:Better instantiate entity and entity controllersImprove instantiation of entity classes and entity controllers

Status:Needs work» Needs review
StatusFileSize
new704 bytes
new80.61 KB
FAILED: [[SimpleTest]]: [MySQL] 57,084 pass(es), 160 fail(s), and 51 exception(s).
[ View ]

updated fix.

Status:Needs review» Needs work

The last submitted patch, better_instantiate_entity_controllers-2015535-25.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.43 KB
new82.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch better_instantiate_entity_controllers-2015535-27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

fix again.

Issue summary:View changes

Remove myself from author field to unfollow.

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
    @@ -307,9 +307,9 @@ protected function attachLoad(&$queried_entities, $revision_id = FALSE) {
    -   * Implements Drupal\Core\Entity\EntityStorageControllerInterface::create().
    +   * Implements Drupal\Core\Entity\EntityStorageControllerInterface::createEntity().
    +++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
    @@ -360,9 +360,9 @@ protected function attachLoad(&$queried_entities, $load_revision = FALSE) {
    -   * Implements \Drupal\Core\Entity\EntityStorageControllerInterface::create().
    +   * Implements \Drupal\Core\Entity\EntityStorageControllerInterface::createEntity().

    Potentially changes like that could be replaced with {@inheritdoc}

  2. +++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
    @@ -52,9 +52,24 @@ public function __construct(ModuleHandlerInterface $module_handler) {
    -   * {@inheritdoc}
    +   * Instantiates a new instance of this entity form controller.
    +   *
    +   * This is a factory method that returns a new instance of this object. The
    +   * factory should pass any needed dependencies into the constructor of this
    +   * object, but not the container itself. Every call to this method must return
    +   * a new instance of this object; that is, it may not implement a singleton.
    +   *
    +   * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
    +   *   The service container this object should use.
    +   * @param string $entity_type
    +   *   The entity type which the controller handles.
    +   * @param array $entity_info
    +   *   An array of entity info for the entity type.
    +   *
    +   * @return static
    +   *   A new instance of the entity form controller.
        */
    -  public static function createInstance(ContainerInterface $container, $entity_type, array $entity_info) {
    +  public static function create(ContainerInterface $container, $entity_type, array $entity_info) {

    I don't see why this is not @inheritdoc

I don't see why this is not @inheritdoc

inherit from whom?:) See class hierarchy here.

1). Personally speaking, I would like to make EntityFormControllerInterface extends EntityControllerInterface and then EntityFormController implements '::create' from EntityControllerInterface

The next work will be removing EntityControllerInterface from all entity form controllers classes, because they don't have to implement EntityControllerInterface any more. This will lead to another bunch of removing work. Anyway, it will clean the code.

2). The other option is to remove EntityControllerInterface from all entity form controllers classes. And delete ::create method from EntityFormController. We bind entity form controllers instantiation to default __construct, i.e., new $class().

See getFormController($entity_type, $operation) from entitymanager.

<?php
     
if (in_array('Drupal\Core\Entity\EntityControllerInterface', class_implements($class))) {
       
$this->controllers['form'][$operation][$entity_type] = $class::createInstance($this->container, $entity_type, $this->getDefinition($entity_type));
      }
      else {
       
$this->controllers['form'][$operation][$entity_type] = new $class($this->container->get('module_handler'));
      }
?>

And create method from EntityFormController.
<?php
public static function createInstance(ContainerInterface $container, $entity_type, array $entity_info) {
  return new static(
$container->get('module_handler'));
}
?>

So, for entity form controllers, no matter entitymanager calls create method or use new $class, it does the same thing -- pass 'module_handler' to __construct of controller class. Using create just makes it looks better that we are injecting something:)

@dawehner I didn't mean to change code for this part. But since you bing it in. Which option do you like:)?
I will update your first {@inheritdoc} issue though.

This is a trial for the third part of this issue, i.e., add $container to EntityInterface to better handle instantiation of Entity.

I might get into a dilemma situation.

I use code like this:

<?php
$entity
= $entity_class::create(\Drupal::getContainer(), $values, $this->entityType);
?>

in EntityStorageController's createEntity method to inject the container into entity class.

However, \Drupal::getContainer() is not recommended way to get container. I could think of a better way is to inject container to the storage controller's __construct, and then use

<?php
$entity
= $entity_class::create($this->container, $values, $this->entityType);
?>

inside StorageController's createEntity method to instantiate Entity. $this means the StorageController itself. But this could lead another big change of controller class.

So my question is -- is it appropriate to use \Drupal::getContainer() to get current container inside EntityStorageControllers? Or there're other better ways to get the container?

If entities start to hold services from the container, we'll need to make sure they extend #2004282: Injected dependencies and serialization hell 's DependencySerialization. Same for entity controllers...

>EntityStorageControllerInterface::create() renaming to EntityStorageControllerInterface::createEntity() to create an entity instance;

You have load, and not loadEntity and you have save and not saveEntity. This seems a bit illogical to me.

> EntityControllerInterface::createInstance() renaming to EntityControllerInterface::create() to instantiate controller itself.

well, in the light above, perhaps let's not do that?

> Add ::create() to EntityInterface

That's great!

Some discussions from IRC:

[11:25] timplunkett: and EntityStorageController::create doesn't tell me "i make entities"
[11:26] timplunkett: it means "i make storage controllers"
[11:26] chx: timplunkett: so, sure, if you feel like it , rename create to createEntity but do NOT rename createInstance to create
[11:26] timplunkett: EntityStorageController::createEntity says that
[11:26] chx: timplunkett: that's not a good idea t the same time

[11:31] smiletrl3864: hhm, I think it means something like this: in dependency inject pattern, we should use create. createInstance is ok to use, but it's not meant for inject stuff.

[11:33] chx: so it's more for me that i do not necessarily understand the complcations here
[11:33] msonnabaum: chx is totally right about create()
[11:34] msonnabaum: it's just the typical name of a factory method, controller interface doesn't own that

Perhaps, some other ideas?

FWIW, I agree with the initial comments here that the "factory method" being create() everywhere else but createInstance() for entity controllers is a WTF - bigger IMO than createEntity() vs. "load() / save(), not loadEntity() / saveEntity()".

in other words, I'm +1 on create() -> createEntity(), static createInstance() -> static create()

Also, as @Berdir pointed, the current StorageController::create() is *not* a factory for entity objects, it's only involved when creating *new* (non existing in storage) entities. Entity objects loaded storage do not go through this - and that's intentional.

So one possibility would be to rename the current StorageController::create() to createNew() (relates to $entity->isNew()). End result would be:

Entity
- static create() : factory method, instantiates an entity object, allows dependency injection
StorageController
- static create() : factory method, instantiates a storage object, allows dependency injection
- createNew() : creates a *new* entity in the system - calls Entity::create() at some point
- load() : loads an existing entity - calls Entity::create() at some point

in other words, I'm +1 on create() -> createEntity(), static createInstance() -> static create()

I don't think we should do this. entity-create is already working and known terminology, so of course would I assume Node::create() to be entity create. Having createEntity() also would not clarify that, however createInstance() is imho clear.

Also, as @Berdir pointed, the current StorageController::create() is *not* a factory for entity objects, it's only involved when creating *new* (non existing in storage) entities. Entity objects loaded storage do not go through this - and that's intentional.

Exactly. entity create has been mis-used as factory quite a bit, that's why we really need a real factory. We've been working on the entity factory in #1867228: Make EntityManager provide an entity factory.

What I was suggesting in #36 was actually StorageController::createNew() - and keep create() for something that is a real "factory" method.

@yched: I see. That would me still worry that people miss the difference between createNew() and create() - in particular as Drupal 7 terminology is already that entity_create() <=> create a new permanent entity.

Personally, I actually like createInstance() as it is descriptive about what it is to create and we already have it on $manager->createInstance(). Having Entity::crea.. to autocomplete to create() and createInstance() should already work out - imo. Or should we try to be even clearer and make it e.g. Entity::createNew() and Entity::createInstance() ?

Issue summary:View changes

Updated issue summary.

The last submitted patch, 27: better_instantiate_entity_controllers-2015535-27.patch, failed testing.