Remove usage of deprecated \Drupal::entityManager() in core

1. +++ b/core/includes/entity.inc
   @@ -51,10 +51,10 @@ function entity_render_cache_clear() {
      if (isset($entity_type)) {
   -    return \Drupal::entityManager()->getBundleInfo($entity_type);
   +    return \Drupal::service('entity_type.bundle.info')->getBundleInfo($entity_type);
      }
      else {
   -    return \Drupal::entityManager()->getAllBundleInfo();
   +    return \Drupal::service('entity_type.bundle.info')->getAllBundleInfo();
      }
    }
    
   

   note that we kind of on purpose didn't update many calls that are in already deprecated code. getBundleInfo() would trigger a deprecation message, but nothing in core is calling this.

2. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
   @@ -211,7 +211,7 @@ public function processForm($element, FormStateInterface $form_state, $form) {
    
        // Hide extra fields.
   -    $extra_fields = \Drupal::entityManager()->getExtraFields($this->targetEntityType, $this->bundle);
   +    $extra_fields = \Drupal::service('entity_field.manager')->getExtraFields($this->targetEntityType, $this->bundle);
        $extra_fields = isset($extra_fields['form']) ? $extra_fields['form'] : [];
   

   I'd definitely suggest to postpone this on the entity field manager issue: #3035953: Add @trigger_error() to deprecated EntityManager->EntityFieldManager methods.

   After that, I'm fine with finishing this, my plan was to do it per method, because then we can actually add @trigger_error() to those and be sure that there is nothing left.

   I noticed you did a change record, but we kind of agreed to not do separate CR's for constructor changes, there have been dozens of these already. Also, that part should already have been taken care of in my issue.

   I assume you didn't look at $this->entityManager, which I think still exists quite a lot in tests.

This patch is duplicate of the work in some existing issues like #3041656: Replace deprecated usages of entityManager in field item classes and #3030689: Deprecated entityManager and possibly many others - I didn't do a deep search. Regardless, if you want to handle it here, you should go through the issue queue and consolidate all those issues and make sure that the people who provided patches months or even years ago for some of these changes get credit for their contribution.

I had a discussion with the framework managers at DrupalCon, and they aren't interested in patches that remove one or two usages of a deprecated function. Basically, each patch to remove deprecated code should remove all usages in core of one particular deprecated call, and include a new @trigger_error for that call to ensure we don't ever use that call again, so any of the small patches attempting to remove one or two usages should be closed either as a duplicate of a larger issue, or as won't fix in favor of a larger patch similar to this one.

Yes, I do understand that this is now the current thinking and the 'rules' for what will be considered have changed. But for many, many years large patches have been rejected outright; the push back was to narrow the scope so that the patch would be reviewable and not have the potential to conflict with other patches across core. Previous contributions under the old 'rules' should be recognized and acknowledged if the scope gets broadened. (Neither of the patches I linked to is from me, BTW, and as I said I know there are others like those.)

Specifically, the entity.manager split has been fixed in much of contrib for 4 years, but previous attempts to fix it in core have failed as too broad.

It's great that we're finally seeing progress with the entity manager - better late than never. I don't mean to imply that this issue should be closed - I'm just saying that if you are going to broaden the scope and tackle this in a large issue please take the time to search the issue queue, properly consolidate the issues by marking the others as duplicate, and give credit/acknowledgement to those previous contributors where credit is due.

@TR For the record, patch scope has nothing do with the patch size. I've been working on the parent issue that I referenced now for months, and there were several ~200k patches and various 100kb patches in those child issue, which all had a pretty clear scope, e.g. update methods for one specific replacmenet service, update it in controllers, core services, module services, .. and the scope of this is update everything that goes through \Drupal::entityManager(). "but previous attempts to fix it in core have failed as too broad." might have been true 6 month ago, but I've been pretty active since then on this topic.

I'm not 100 sure yet if this specific scope will work well, didn't review yet, it should get a good bit smaller once the entity field manager issue is in and I'll do a review then.

Recent months have shown that a good scope for deprecation issues is to enforce deprecation warnings for a certain method/service/function, and then update everything, which allows to make sure that we are finding and fixing every usage.

the blocker is in, this is probably going to be a fun reroll.

Lets do a minimal reroll and then I'll do a review and assess whether this approach makes sense or if we should instead do it per method.

Updated the patch by applying against an old commit and rebasing as git apply -3 failed due to renamed and removed files.

Patch got about 50kb smaller due to other issues getting committed. Had quite a lot of conflicts but git rebase did a good job. Mostly kept the HEAD version unless this converted additional cases. Removed a few in deprecated files (e.g. old test traits) because we agreed on that earlier but only where it conflicted.

Also fixed two wrong calls that caused test fails.

1. +++ b/core/includes/entity.inc
   @@ -20,10 +20,10 @@
     */
    function entity_render_cache_clear() {
      @trigger_error(__FUNCTION__ . '() is deprecated. Use \Drupal\Core\Entity\EntityViewBuilderInterface::resetCache() on the required entity types or invalidate specific cache tags instead. See https://www.drupal.org/node/3000037', E_USER_DEPRECATED);
   -  $entity_manager = Drupal::entityManager();
   -  foreach ($entity_manager->getDefinitions() as $entity_type => $info) {
   -    if ($entity_manager->hasHandler($entity_type, 'view_builder')) {
   -      $entity_manager->getViewBuilder($entity_type)->resetCache();
   +  $entity_type_manager = Drupal::entityTypeManager();
   +  foreach ($entity_type_manager->getDefinitions() as $entity_type => $info) {
   +    if ($entity_type_manager->hasHandler($entity_type, 'view_builder')) {
   +      $entity_type_manager->getViewBuilder($entity_type)->resetCache();
        }
      }
   

   entity.inc is an example that I think we shouldn't touch at all. Some cases like entity_get_bundles() aren't called at all in core (because these methods already have @trigger_error() and we need to officially and fully deprecate all functions in that file.

2. +++ b/core/modules/comment/src/Plugin/views/field/NodeNewComments.php
   @@ -166,8 +202,7 @@ protected function renderLink($data, ResultRow $values) {
          //   https://www.drupal.org/node/2594201
   -      $entity_type_manager = \Drupal::entityTypeManager();
   -      $field_map = \Drupal::service('entity_field.manager')->getFieldMapByFieldType('comment');
   +      $field_map = $this->entityFieldManager->getFieldMapByFieldType('comment');
          $comment_field_name = 'comment';
   

   I kept the injection of this despite the calls already being converted in the other issue, but it does make it bigger and harder to review. It's done now, but I'd suggest to keep such changes to a minimum.

3. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationMetadataFieldsTest.php
   @@ -98,7 +102,10 @@ public function testSkipUntranslatable() {
        $this->drupalLogin($this->translator);
   -    $fields = \Drupal::service('entity_field.manager')->getFieldDefinitions($this->entityTypeId, $this->bundle);
   +    $entity_type_manager = \Drupal::entityTypeManager();
   +    /** @var \Drupal\Core\Entity\EntityFieldManagerInterface $entity_field_manager */
   +    $entity_field_manager = \Drupal::service('entity_field.manager');
   +    $fields = $entity_field_manager->getFieldDefinitions($this->entityTypeId, $this->bundle);
   

   This was another conflict where I kept this version, but actually, I'd suggest to drop those changes after reviewing again. There is only a single entity type manager call below and it's another 2 chunks that we can remove from this.

With a few exceptions, the changes in this patch are absolutely straight-forward and easy to review. I think this issue does make sense as a next step, in combination with a @trigger_error() in Drupal::getEntityManager(). Looking at the remaining calls, a few will need to work around it, by calling get('entity.manager'), for example \Drupal\Core\Entity\EntityTypeManager::getFormObject().

There are also still a handful of calls that we can fix, $this->entityManager in \Drupal\Tests\content_translation\Functional\Update\ContentTranslationUpdateTest::setUp() and several other update tests looks unused and safe to remove (might have been used for the trait once). \Drupal\image\Plugin\Field\FieldType\ImageItem::getEntityManager() can be deprecated too. \Drupal\views\FieldAPIHandlerTrait::getEntityManager() can also be deprecated explicitly, unused in core but search_api seems to call it still. core.api.php also has a call to it in documentation.

Once this is in, there might be few enough calls to do the rest at once, we'll see. Still 300 get('entity.manager') calls after that.

+++ b/core/lib/Drupal/Core/Entity/EntityForm.php
@@ -2,6 +2,7 @@
@@ -41,7 +42,7 @@ class EntityForm extends FormBase implements EntityFormInterface {

@@ -41,7 +42,7 @@ class EntityForm extends FormBase implements EntityFormInterface {
    *
    * @see https://www.drupal.org/node/2549139
    */
-  protected $entityManager;
+  private $entityManager;

I thought about doing that too, but did plan to do that in a separate issue, my idea was to simply access it from the container here.

*If* we do this, then you don't have to reinvent this, there's a trait that can handle that generically, I wrote that for other entity manager deprecations, look for DeprecatedServicePropertyTrait and how it is used.

That is in, one small conflict in node.module.

1. +++ b/core/lib/Drupal.php
   @@ -270,6 +270,7 @@ public static function currentUser() {
       *   correct interface or service.
       */
      public static function entityManager() {
   +    @trigger_error("\Drupal::entityManager() is deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0. Use \Drupal::entityTypeManager() instead in most cases. If the needed method is not on \Drupal\Core\Entity\EntityTypeManagerInterface, see the deprecated \Drupal\Core\Entity\EntityManager to find the correct interface or service. See https://www.drupal.org/node/2549139", E_USER_DEPRECATED);
        return static::getContainer()->get('entity.manager');
      }
   

   Honestly, I think this is a bit wordy. Every method not on EntityTypeManager is already fully deprecated, that means calling e.g. \Drupal::entityManager()->getFieldDefinitions() will then give you two deprecation messages, one for entityManager() and the second for getFieldDefinitions() that tells you that you have to use the entity field manager.

   3 options:

   a) Keep as is now.
   b) Simplify the wording, rely on the duplicated message.
   c) Remove this @trigger_error(), because we *will* add it also to every single remaining method, so you will always get two.

   Not sure, I'll let a framework/release manager decide that. we could even do combinations, add it so we don't cause regressions (as in, add new usages of this), but remove it when we are done deprecating it.

2. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalTest.php
   @@ -748,7 +748,7 @@ public function testWrite() {
    
   -    $node = \Drupal::entityManager()->loadEntityByUuid('node', $uuid);
   +    $node = \Drupal::service('entity.repository')->loadEntityByUuid('node', $uuid);
        $this->assertEquals(1, $node->get('status')->value, 'Node status was not changed.');
        // 9. Successful POST to related endpoint.
        $body = [
   

   This one is strange. EntityManager::loadEntityByUuid() has @trigger_error(), why is this not failing?

3. +++ b/core/modules/system/tests/src/Functional/Entity/Update/SqlContentEntityStorageSchemaConverterTestBase.php
   @@ -14,13 +14,6 @@ abstract class SqlContentEntityStorageSchemaConverterTestBase extends UpdatePath
    
   -  /**
   -   * The entity manager service.
   -   *
   -   * @var \Drupal\Core\Entity\EntityManagerInterface
   -   */
   -  protected $entityManager;
   

   I'd say just removing this is fine, it is only a test and one that is pretty much internal to the entity system, I think it's very unlikely that contrib subclasses this, just two subclasses currently exist in core.

I want through the whole patch again with word-diff and it is simple and IMHO ready apart from the question about the trigger message above. Calling entity_load() or similar in contrib will be fun case once we are done, as that will result in 3 different deprecation messages when we are done :)

I think #29.1 is a good point. Can't this issue add an @trigger_error() to every method on \Drupal\Core\Entity\EntityManager already? I.e. has it not removed all the usages of the methods that don't already have an @trigger_error()

No, we can't add @trigger_error() yet, there are still a few hundred calls left, this just deals with those accessing it through \Drupal::entityManager() as opposed to $container->get().

> It's perfectly valid to call this function and then not do anything else with the entityManager you get back

That seems pretty far fetched, worst case it's a trivial fatal error to fix once you actually start testing on the 9.x branch :)

I wasn't aware that this method is just copied from the docblock, fine to keep it like this then, or we would have to change that too. As mentioned, if we end up confusing people by having 2-3 deprecation messages for a single call in the end, we can still improve it, on the plus side, they will be happy if they can get rid of 3 messages at a time with one change :p

Credited some folks who worked on parallel efforts with smaller scope

Committed ba509af and pushed to 8.8.x. Thanks!

completed and published the change record