Provide a method of DI for entities

REST serialization has nothing to do with this/php serialization.

This will conflict with #2027795: Optimize content entity serialization. I'd rather duplicate those lines and add it to the sleep/wakeup methods added by the referenced than making the entity class hierarchy even more complicated.

Note: "REST serialization" has no issues with services in the object. It's only PHP serialize() that's problematic.

+ I'm not sure DependencySerialization will be needed for entities:
#2027795: Optimize content entity serialization provides dedicated/optimized serialization for ContentEntities
#1977206: Default serialization of ConfigEntities does the same for ConfigEntities (a reroll would be welcome though...)

We'd still need something if we have injected services, because we need to get them back when we unserialize, though.

Calling this a duplicate of #2208115: Add DependencySerializationTrait

Feel free to re-open if it's not. :-)

It's not. Entities still don't support dependency injection.

OK.

Why should we inject services into entities?

The various layers of entity management classes have injection points, and can manipulate entities based on those services.

Shouldn't we expand on the repository pattern and inject there?

The EntityBase class uses the DependencySerialization trait now. I think the bigger point of contention is just where we want to inject services to these entities as we cannot use the symfony create function. Perhaps we can just add a function to the entity base class called 'InjectServices' or similar and it just takes in the container and allows the developer to pull in whichever services they want.

We would also need to change the entity base classes to remove functions that call \Drupal::service() and convert entities that currently do not inject services as properties (might be big enough to be a separate issue).

I think this is necessary to do for two main reasons. Consistency and performance. Consistency with the rest of Drupal DI, though the method here might be different the methodology can stay the same. And performance by avoiding excess calls to the \Drupal class.

#20: many core classes already have createInstance() static methods specifically for dependency injection - including EntityHandlerInterface::createInstance() - so I would suggest that name for consistency, unless there's naming conflict I've missed.

#20: many core classes already have createInstance() static methods specifically for dependency injection - including EntityHandlerInterface::createInstance() - so I would suggest that name for consistency, unless there's naming conflict I've missed.

+1!

++1

Now that we have bundle classes available, and people start (enthusiastically) adopting those without any prior exposure to defining entity classes, it's a very WTF moment to discover that you can't inject dependencies into your shiny new bundle class...

Typed entities is wrapping the entities to provide dependency injection on them:
https://www.drupal.org/project/typed_entity Can be used in conjunction with bundle classes.

Still it's a wrapper, I think it would be nice to be able to do it straight on the entity.

Still it's a wrapper, I think it would be nice to be able to do it straight on the entity.

+ in core, without further contrib module :)

+1 -- started implementing some bundle classes and then realized that there's no DI available, so back to calling \Drupal in classes. :(

It's easy enough to add createInstance() on EntityBase, but I'm not totally sure where it would need to actually be implemented. I'd imagine it would need to be implemented in EntityStorageBase wherever an entity is returned, since the save/create/load methods in EntityBase all rely on $entity_type_manager->getStorage anyway.

For instance

  /**
   * Performs storage-specific creation of entities.
   *
   * @param array $values
   *   An array of values to set, keyed by property name.
   *
   * @return \Drupal\Core\Entity\EntityInterface
   */
  protected function doCreate(array $values) {
    $entity_class = $this->getEntityClass();
    return new $entity_class($values, $this->entityTypeId);
  }

I'd imagine we'd just want to return an $entity_class::createInstance() instead?

Then there's EntityStorageBase::load(), which eventually calls the abstract EntityStorageBase::doLoadMultiple(). Obviously since it's abstract, we'd need to add the handling somewhere else. Not sure where that would be. Same with doSave().

Digging a bit more, it certainly seems more complex. For instance, Drupal\Core\Entity\KeyValueStore\KeyvalueEntityStorage::doLoadMultiple() internally calls EntityStorageBase::mapFromStorageRecords.

  /**
   * Maps from storage records to entity objects.
   *
   * @param array $records
   *   Associative array of query results, keyed on the entity ID.
   *
   * @return \Drupal\Core\Entity\EntityInterface[]
   *   An array of entity objects implementing the EntityInterface.
   */
  protected function mapFromStorageRecords(array $records) {
    $entities = [];
    foreach ($records as $record) {
      $entity_class = $this->getEntityClass();
      /** @var \Drupal\Core\Entity\EntityInterface $entity */
      $entity = new $entity_class($record, $this->entityTypeId);
      $entities[$entity->id()] = $entity;
    }
    return $entities;
  }

Easy enough, just another call to $entity_class::createInstance(). But then you have NodeStorage, which extends Drupal\Core\Entity\Sql\SqlContentEntityStorage, whose mapFromStorageRecords() which returns new instances of the entities but without invoking EntityStorageBase::mapFromStorageRecords(). So I suspect we'd essentially need to go through all EntityStorage classes looking for versions of new $entity_class() to be replaced.

Ah actually, searching all of core for the string new $entity_class only shows 6 results, so it might not be so bad at all, assuming that that string is used consistently in all relevant cases.

@mrweiner Yup, injecting dependencies via the storage class is how I've gotten it to work on Omnipedia. Having Drupal core support this officially would be awesome.

Did some initial work on this based on my earlier comments. Anybody care to give things a look to see that they pass a sniff test? I'm assuming that some tests will need to be updated as well.

At the moment I'm also doing $this->container = \Drupal::getContainer() in EntityStorageBase::__construct() for proof of concept, but might need to add the container as a constructor argument and update all of the extending classes, though I can see this as being potential BC for contrib.

Marked the MR as ready just to see if it would kick off tests to see what happens.

Actually, realizing that I already made that $container constructor update to ContentEntityStorageBase and SqlContentEntityStorageBase, which may be equally problematic from a BC perspective.

looks interesting!

Updating title and description

Well, 4 failures is certainly better than 212! https://www.drupal.org/pift-ci-job/2468490. Remaining failures are related to the $container itself.

There were 15 errors:

1) Drupal\Tests\Core\Entity\KeyValueStore\KeyValueEntityStorageTest::testCreateWithPredefinedUuid
ArgumentCountError: Too few arguments to function Drupal\Core\Entity\KeyValueStore\KeyValueEntityStorage::__construct(), 5 passed in /var/www/html/core/tests/Drupal/Tests/Core/Entity/KeyValueStore/KeyValueEntityStorageTest.php on line 137 and exactly 6 expected

This is simply because I forgot to remove the container as an argument on KeyValueEntityStorage::__construct(). How this is addressed really depends on how we address...

1) Drupal\Tests\Core\Config\Entity\ConfigEntityStorageTest::testCreateWithPredefinedUuid
Drupal\Core\DependencyInjection\ContainerNotInitializedException: \Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container.

Apparently my WIP solution of using $this->container = \Drupal::getContainer(); in EntityStorageBase is not an option. I was hoping it would be, because it avoids us needing to update constructors for entity classes all over the place, in tests, etc. I'd imagine that this could potentially cause issues in userland code as well, although I do not know.

Anybody more knowledgeable care to chime in? Would love the input.

Needs rebase over 10.1.x

Is this still actively being pursued or is there a workaround?

@drupals.user There haven't been any updates in 2+ years, so no, it isn't actively being updated.

As a workaround, you can use \Drupal calls without DI.

I used to think DI on entities was a good idea and even implemented a way to do it via overriding the storage class, but I've honestly completely come around to the Typed Entity module approach of using a (very clever) way of wrapping entities; see #26 (#2142515-26: Provide a method of DI for entities). I over-engineered a solution to use DI on node entities, and it turned into a maintenance nightmare on multiple occasions, until I'd had enough and moved all that code to entity wrappers.

Here's the big problem with adding DI to entities: entities are already complex, and this just loads more complexity into them, making them more tedious to test, mock up, etc.

I know the thought of having to add an additional step every time you act on an entity to wrap it may seem annoying - and it certainly held me back for a while - but the benefits end up far outweighing that by allowing you to mock up and test parts of the logic without baking it directly into entity classes. If you want to see a concrete example (and one I'm very proud of), here's an absolutely massive merge request from last year where I did such a refactor to replace overloaded entity class logic with entity wrappers.

I don't totally understand the benefits of Typed Entity approach in #26 -- which is fine! If it works for folks then that is great! (Though I wish there were some bite-sized, practical example on that module page that show the benefits. Your massive MR is...well...massive :D).

But the trouble with is -- per #27 -- this is still a contrib approach. It feels like something that should be addressed in core, if possible, for folks who just want to stick with Core. Whether bundle classes or basic entity classes, we end up with a de facto workaround being to just call \Drupal directly. The alternative being to accept the required services as method parameters, which works but isn't as nice of DX.

This is actually one of the things OOP hooks could help with. Let's take a look at Drupal\system\Entity\Menu. All of the overridden methods (preDelete(), save(), delete()) could be extracted to a hook class.

So, my suggestion would be to first try to reduce close coupling of entities with services by leveraging OOP hooks or the event system (whatever works best for the given entity type).

@znerol yesh I think you're correct for some cases but not all. For instance in Drupal Commerce, Order entities have this:

  /**
   * {@inheritdoc}
   */
  public function getCalculationDate() {
    $timezone = $this->getStore()?->getTimezone();
    $timestamp = $this->getPlacedTime() ?: \Drupal::time()->getRequestTime();
    $date = DrupalDateTime::createFromTimestamp($timestamp, $timezone);

    return $date;
  }

It's just a standard method unrelated to entity lifecycle.

Yes, sure. I am suggesting incremental improvements.

I have mixed opinions on this.

One one side, DI for things that are being serialized has some problematic drawbacks (and entities are serialized _a lot_), it requires trickery such as DependencySerializationTrait. And there's discussions around whether or not entities should be value object, which would imply they shouldn't do stuff that requires DI.

But also, we have bundle classes now (although they probably mostly serve as nice getters and setters), pre/post methods (Re #48, Hooks being services changes things bit, but I still generally prefer to have entity related logic on the entity itself) and ContentEntityBase itself has a number of \Drupal:: calls.

One problem is that, unlike other plugins, we never got the factory stuff working, so we don't have a single thing responsible for creating entity objects. We did make it work for bundle classes though, it's fairly well isolated in the storage handler.

To summarize, not against it, but I do very much expect that there will be cases where it will come back to bite us.

@mrweiner I know it's contrib and I would love it to go into core, but I don't feel like that's a downside - a lot of really awesome and super useful things are implemented in contrib, prove their worth there, and are adopted into core. The biggest benefit in using Typed Entity (in my opinion) is that you now have a clean separation of your data (entities, their fields) and the logic that allows you to interact with it (the wrapped entity plug-ins), which leads to more testable code and hopefully away from spaghetti code.

Edit: here's an example of a simpler wrapped entity plug-in I wrote that used to live on a node class back when we used to it that so we could inject a few services; we no longer do that and instead inject them into the plug-in which also provides methods to interact with the data on the nodes.

@berdir That's also a good point that entities are serialized/deserialized a lot which adds even more potential headaches.