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 toEntityStorageControllerInterface::createEntity()
to create an entity instance;EntityControllerInterface::createInstance()
renaming toEntityControllerInterface::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
- Once this is fixed, we also need to update
RolesRidTest
from #1995868: Fatal when using a role contextual filter.
API changes
TBD
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#30 | improve_instantiation_entity_controllers-do-not-test-2015535-30.txt | 97.72 KB | smiletrl |
#30 | interdiff-27-30.txt | 19.09 KB | smiletrl |
#27 | better_instantiate_entity_controllers-2015535-27.patch | 82.04 KB | smiletrl |
#27 | interdiff-25-27.txt | 1.43 KB | smiletrl |
#25 | better_instantiate_entity_controllers-2015535-25.patch | 80.61 KB | smiletrl |
Comments
Comment #2
xjmNever mind. Foiled again by typed data's
getDefinition()
having the same name as the plugin system's method.Comment #2.0
xjmUpdate summary. --xjm
Comment #2.1
xjmUpdated issue summary.
Comment #2.2
xjmUpdated summary. --xjm
Comment #3
BerdirYes, we probably need to do this, also for the save/delete methods. But it scares me a bit ;)
Comment #4
xjmComment #4.0
xjmUpdated issue summary.
Comment #5
dawehnerRegarding 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
Comment #6
iamEAP CreditAttribution: iamEAP commentedThis 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
Comment #8
iamEAP CreditAttribution: iamEAP commentedAlso, 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.Comment #8.0
iamEAP CreditAttribution: iamEAP commentedUpdated issue summary.
Comment #9
dawehnerJust as a note: We don't treat changed constructors as API changes at the moment.
Comment #10
BerdirWill 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.
Comment #11
tim.plunkettIt 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.Comment #12
smiletrl CreditAttribution: smiletrl commentedUpdated 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.,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 toEntityStorageControllerInterface::createEntity()
to create an entity instance;EntityControllerInterface::createInstance()
renaming toEntityControllerInterface::create()
to instantiate controller itself.::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).
becomes
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:)
Comment #13
tim.plunkettI'd go with createEntity() for the new method name, to distance itself from FactoryInterface::createInstance().
Otherwise, +1
Comment #14
smiletrl CreditAttribution: smiletrl commentedHere'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
toEntityInterface
Comment #15
smiletrl CreditAttribution: smiletrl commentedupdate title.
Comment #17
smiletrl CreditAttribution: smiletrl commentedmore fix.
Comment #19
smiletrl CreditAttribution: smiletrl commentedMore fix.
Although,
::createInstance
of class EntityFormController seems redundant and useless here. It should probably needs to be deleted too.Comment #21
smiletrl CreditAttribution: smiletrl commentedMore fix.
Comment #22
smiletrl CreditAttribution: smiletrl commentedAdd tag. This could be API change for #2015535-12: Improve instantiation of entity classes and entity controllers. Needs approval I guess.
Comment #24
tim.plunkettComment #25
smiletrl CreditAttribution: smiletrl commentedupdated fix.
Comment #27
smiletrl CreditAttribution: smiletrl commentedfix again.
Comment #27.0
smiletrl CreditAttribution: smiletrl commentedRemove myself from author field to unfollow.
Comment #28
dawehnerPotentially changes like that could be replaced with {@inheritdoc}
I don't see why this is not @inheritdoc
Comment #29
smiletrl CreditAttribution: smiletrl commentedinherit from whom?:) See class hierarchy here.
1). Personally speaking, I would like to make
EntityFormControllerInterface
extendsEntityControllerInterface
and thenEntityFormController
implements '::create' fromEntityControllerInterface
The next work will be removing
EntityControllerInterface
from all entity form controllers classes, because they don't have to implementEntityControllerInterface
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 fromEntityFormController
. We bind entity form controllers instantiation to default __construct, i.e., new $class().See
getFormController($entity_type, $operation)
from entitymanager.And create method from EntityFormController.
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.
Comment #30
smiletrl CreditAttribution: smiletrl commentedThis 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:
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 useinside 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?Comment #31
andypostRelated #2059245-18: Add a FormBase class containing useful methods
Comment #32
yched CreditAttribution: yched commentedIf 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...
Comment #33
chx CreditAttribution: chx commented>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!
Comment #34
smiletrl CreditAttribution: smiletrl commentedSome 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?
Comment #35
yched CreditAttribution: yched commentedFWIW, 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
Comment #36
fagoI 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.
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.
Comment #37
yched CreditAttribution: yched commentedWhat I was suggesting in #36 was actually StorageController::createNew() - and keep create() for something that is a real "factory" method.
Comment #38
fago@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() ?
Comment #38.0
fagoUpdated issue summary.
Comment #39
jibran27: better_instantiate_entity_controllers-2015535-27.patch queued for re-testing.
Comment #41
anavarreLooks like status is not accurate.
Comment #42
andypostSeems most of suggestions are implemented or outdated
Comment #43
tim.plunkettI don't see a way to do this while preserving BC.
Comment #44
catchNot 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.