Posted by xjm. Updated: Comment #2

Problem/Motivation

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

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

This is entity_get_info():

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

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

becomes

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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Title: Entity::entityInfo() calls entity_get_info() which wraps Drupal::entityManager()->getDefinitions(), and this is silly » Inject 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.

xjm’s picture

Issue summary: View changes

Update summary. --xjm

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated summary. --xjm

Berdir’s picture

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

xjm’s picture

Issue tags: +VDC, +Entity Field API
xjm’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

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

iamEAP’s picture

Status: Active » Needs review
FileSize
32.17 KB

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.

iamEAP’s picture

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.

iamEAP’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

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

Berdir’s picture

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.

tim.plunkett’s picture

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.

smiletrl’s picture

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

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

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

becomes

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

tim.plunkett’s picture

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

Otherwise, +1

smiletrl’s picture

Status: Needs work » Needs review
FileSize
26.81 KB

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

smiletrl’s picture

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.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
8.83 KB
35.05 KB

more fix.

Status: Needs review » Needs work

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

smiletrl’s picture

Status: Needs work » Needs review
FileSize
6.73 KB
41.55 KB

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.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
39 KB
79.92 KB

More fix.

smiletrl’s picture

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.

tim.plunkett’s picture

Title: Better instantiate entity and entity controllers » Improve instantiation of entity classes and entity controllers
smiletrl’s picture

Status: Needs work » Needs review
FileSize
704 bytes
80.61 KB

updated fix.

Status: Needs review » Needs work

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

smiletrl’s picture

Status: Needs work » Needs review
FileSize
1.43 KB
82.04 KB

fix again.

smiletrl’s picture

Issue summary: View changes

Remove myself from author field to unfollow.

dawehner’s picture

  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

smiletrl’s picture

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.

      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.

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.

smiletrl’s picture

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:

 $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

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

andypost’s picture

yched’s picture

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

chx’s picture

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

smiletrl’s picture

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?

yched’s picture

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

fago’s picture

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 EntityTypeManager provide an entity factory.

yched’s picture

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

fago’s picture

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

fago’s picture

Issue summary: View changes

Updated issue summary.

jibran’s picture

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

anavarre’s picture

Status: Needs review » Needs work

Looks like status is not accurate.

andypost’s picture

Seems most of suggestions are implemented or outdated

tim.plunkett’s picture

Version: 8.0.x-dev » 9.x-dev

I don't see a way to do this while preserving BC.

catch’s picture

Status: Needs work » Closed (won't fix)

Not sure there's anything left here, so I'm going to move this to won't fix. If that's wrong, I think we should start again in a new issue.

Version: 9.x-dev » 9.0.x-dev

The 9.0.x branch will open for development soon, and the placeholder 9.x branch should no longer be used. Only issues that require a new major version should be filed against 9.0.x (for example, removing deprecated code or updating dependency major versions). New developments and disruptive changes that are allowed in a minor version should be filed against 8.9.x, and significant new features will be moved to 9.1.x at committer discretion. For more information see the Allowed changes during the Drupal 8 and 9 release cycles and the Drupal 9.0.0 release plan.