Make the entity storage accessible from the entity object

Other frameworks often provide similar functionality, but to prevent global access, they inject their equivalent of our entity manager/storage into the entities when they are loaded through said entity manager. You'd get something like this:

$apple = \Drupal::entityTypeManager()->getStorage('apple')->create();
# AppleStorage then sets itself on the entity.
$apple->save();
# ::save() uses $this->storage rather than \Drupal::....

I like your patch because your ::getStorage() provides a really good place for BC-compatibility behavior, if we want it. We can make it return any injected storage if it exists, and fall back to calling \Drupal and trigger a deprecation error when it does, to encourage people to the entity storage rather than all kinds of global access shortcuts like Entity::create().

The last submitted patch, 2: 2998471-2.patch, failed testing. View results →

1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -47,6 +47,13 @@
   +  /**
   +   * Holds a reference to the entity storages, keyed by entity type ID.
   +   *
   +   * @var array
   +   */
   +  protected static $storages = [];
   

   Why make this static?

2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -662,4 +659,45 @@ public function getConfigTarget() {
   +  public function getEntityStorage() {
   ...
   +  protected static function getStorage($entity_type_id = NULL) {
   

   I'm not necessarily opposed to making this method public, but not exactly sure if it's really necessary.

   If we do want to make it public, though, I think it should just be getStorage().

   Having two separate methods is a bit confusing, and I don't think the current names do a good job to help with that. I don't really have a better suggestion, though. Maybe we can just leave the static methods as is, and just introduce a single getStorage()?

@tstoeckler, the $storage property is static, because this way it lives on the class and not on instantiated objects.

I am also not really sure about the naming, I've just written the first that came to my mind. We need two methods - one public getter method and one helper protected static like the current getStorage($entity_type_id), which would check for the current class if the entity type ID isn't provided. But thinking once more about it probably it would be better to make it more explicit and have three methods by dividing the current getStorage into two methods - getStorageByClass and getStorageById and renaming the getEntityStorage into getStorage:

1. public getStorage() - returns the entity storage of the current entity object
2. protected static getStorageByClass($class = NULL) - returns the entity storage for the given entity class or for the called entity class.
3. protected static getStorageById($entity_type_Id) - returns the entity storage for the given entity type ID.

Here's a tiny patch that hopefully illustrates my previous comment. My main problem with our entities is that there's too much logic on them for anyone's good (they're entities, data carriers, not services). This results in entity classes requiring many services, including the entity storage for what are probably the few sane functional bits we do want on entities: CRUD operations. I can't fix entities requiring all kinds of non-entity services, but perhaps this issue is the right place to introduce an improvement for those CRUD operations. For similar reasons, I'm not too happy with static entity storages on the Entity class. Too much coupling.

The patch is probably broken, incomplete, and it implements my idea rather crudely, so if anyone ends up liking this I'll put in some more work to make it pretty :)

@Xano, so your patch is doing the same as mine, but mine is introducing some performance optimizations. Is there anything different beside that, which I am missing?

Drupal 9.2.0-alpha1 → will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.1.0-alpha1 → will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule → and the Allowed changes during the Drupal 9 release cycle → .

Drupal 8.9.0-beta1 → was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule → and the Allowed changes during the Drupal 8 and 9 release cycles → .

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule → and the Allowed changes during the Drupal 8 and 9 release cycles → .

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule → and the Allowed changes during the Drupal 8 release cycle → .

The difference is less with your patch than with how core has been doing this for years. These are entities. They are primarily data containers, and shouldn't really contain too much logic. Ours do, and we violate our dependency injection principles everywhere. Seeing as the entity storage is actually one of the few services other ORMs inject into their entities exactly for the purpose of CRUD methods, I wanted to pitch (hence the less invasive do-not-test patch) if anyone else thought this would be a good place to improve this part of entity API. As fixing this was obviously not part of your scope, your patch doesn't address this issue. I figured if we're changing that part of the code anyway, could we introduce a possibly better approach for these dependencies?

IMO this is not a duplicate.

The other issue is talking about methods on the entity type manager, not access to the manager (or storage) from an entity context.

2 related, but different scenarios.

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

The linked issue would allow entities to be proper plugin instances, which would mean you could instantiate them with whatever container services would be needed, e.g. the storage. I think this is (probably?) the best approach and would solve this issue in the course of being implemented. Which is why I marked it a duplicate.

ah, that makes sense, thanks for the explanation.
i guess i'll keep waiting on the other thread...