Deprecate all EntityManager methods with E_USER_DEPRECATED

+1 on the idea behind the issue...

@Mile23

Is you plan to fix the test fails in other issues ... or sort out the 67 fails here.... I want to help out where I can

Also note that the deprecation message is incorrect.

"EntityManager has been split into 11 classes" -> https://www.drupal.org/node/2549139

It was not replaced solely by EntityTypeManager

Also, shouldn't this use a service-level deprecation instead?

Actually, even that would be impossible to get rid of in core as we have to support something like \Drupal\Core\Entity\EntityFormInterface::setEntityManager() which will always require us to instantiate and inject that service until Drupal 9.

So maybe we need to add @trigger_errors() to all the methods so we can trigger it when it is actually called?

I think a method-level deprecation makes sense for both of those reasons.

Yeah, I think so too. That might also allow us to split things up, e.g. per replacement service or even methods in specific ones?

Pinging the followers her for #3006409: Update deprecated usages of Entity::getEntityManager(), fairly simple part that should be ready if it still applies, just started a retest.

Starting with method deprecations: #3023799: Add @trigger_error() to deprecated EntityManager::clearCachedDefinitions() method

Next chunk is #3023981: Add @trigger_error() to deprecated EntityManager->EntityRepository methods, but already postponed on #2691675: Replace deprecated entityManager() in ControllerBase descendents, overlaps too much with that and also the module service one I guess.

The next one is up: #3025427: Add @trigger_error() to deprecated EntityManager->EntityTypeBundleInfo methods, EntityRepository above is now green and ready for reviews.

This counts all method calls on entity_manager, entityManager, entityManager():

grep -ir 'entity_\?manager(\?)\?-' core/ | wc -l

8.6.x: 1327
HEAD: 1059
With the 3 pending patches applied: 797

Ok, the clearCachedDefinitions() issue is in, updated #3023981: Add @trigger_error() to deprecated EntityManager->EntityRepository methods which should be pretty much ready (but is very big).

Also created two new issues which are smaller: #3028656: Add @trigger_error() to deprecated EntityManager listener methods and #3028671: Add @trigger_error() to deprecated and already (almost) unused EntityManager methods.

Status report for those following along:

* the form classes patch got in
* Created #3038926: Add @trigger_error() to deprecated EntityManager->EntityDisplayRepository methods, that should be ready for reviews, it's pretty small.
* Also created #3035953: Add @trigger_error() to deprecated EntityManager->EntityFieldManager methods a while ago, but is postponed on #3035383: Replace deprecated usages of entityManager in list builder classes, which is RTBC.

With those, all conversions to all services except EntityTypeManager should be done, that seems like a nice goal for 8.7, lets see :)

After applying the 3 patches, we're done to 592 usages. about 370 of those are in tests, most of the others are in module and include files, there's also a quite a bit in deprecated code and legacy tests like EntityManager and EntityManagerTest that we won't update.

Once the issues above are in, I will likely start doing the remaining ones per method or possibly a few rarely used methods grouped.

#3035953: Add @trigger_error() to deprecated EntityManager->EntityFieldManager methods is ready I think, cleaning up the remaining injected entity managers in #3042545: Update last injected entity managers.

With those two patches:

$ grep -ir 'entity_\?manager(\?)\?-' core/ | wc -l
544

After that, I'll switch to search & replace patches for each of the remaining methods.

Playing around with the rules module ... suggests to me that for contrib modules which create lots of plugins - that this becomes relevant.

#3045525: EntityDeriver: deprecate entityManager.

Sorry if this is a cross post ... I was looking for a meta issue talking about how we prioritize fixing deprecated entityManager issues.

Ended up being a bit more than I expected, but this should be close. Did also quite a bit of cleanups, leftovers in unit tests and so on, so that blew up the patch a bit in the end, but I hope it's not too hard to review, just long.

What's left is mostly from legacy tests, already deprecated code and stuff like that.

Noticed a considerable amount of construction injections where we updated the type hint but still inject the entity.manager service, not quite sure where that happened ;)

Missed a very prominent one, didn't look for service('entity.manager')..

That went better than I expected.

removed the smallest of problems

This look great.

A) Sat down with a pot of coffee and visually scanned this patch

1) All trigger messages follow the approved pattern.
2) All create/constructor changes look correct.
3) Line by line - all other changes look OK.
3) Its is good to see so many $entityManager properties of test classes go away.
4) Nothing extraneous to the issues objectives.

B) Visually inspected the EntityManagerTest class in isolation because this is where the new code is.

in short all looks correct and justified.

C) Inspected the test results .. no additional coding standard violations.

Berdir++

My patch did not really contribute to this work in any meaningful way.I Just moved a linting banana peel out the way. So I am bending the rules and moving this to RTBC

With reference to #29.1 just thought I should point out that the trigger_error standard is not actually being checked in core tests yet. The texts used do broadly follow the pattern, but there is one small difference:

@trigger_error('EntityManagerInterface::getDefinition() is deprecated in drupal:8.0.0 and will be removed before drupal:9.0.0. Use \Drupal\Core\Entity\EntityTypeManager::getDefinition() instead. See https://www.drupal.org/node/2549139', E_USER_DEPRECATED);

"will be removed before" should be chaged to "is removed from" to align with the standard agreed on #2908391: Add a rule for expected format of @deprecated and @trigger_error() text and which will soon be checked in Coder.

I see that as my mistake...

I will fix this tonight.

A little later than advertised. sorry for that...

Note to self: Nothing but a search and replace inside of the original patch file. I had to prevent one 'replace' from functioning because that would have been fixing an existing error in core not associated with the changes here.

Whew, that's a lot of changed files. I only found a couple things that need to be adjusted, and a few styling nits here and there that we might as well fix while we are in there.

1. +++ b/core/lib/Drupal/Core/Entity/EntityDeleteMultipleAccessCheck.php
   @@ -16,7 +16,7 @@ class EntityDeleteMultipleAccessCheck implements AccessInterface {
   diff --git a/core/lib/Drupal/Core/Entity/EntityManager.php b/core/lib/Drupal/Core/Entity/EntityManager.php
   
   diff --git a/core/lib/Drupal/Core/Entity/EntityManager.php b/core/lib/Drupal/Core/Entity/EntityManager.php
   index 13f8ccc407..c1755b1288 100644
   
   index 13f8ccc407..c1755b1288 100644
   --- a/core/lib/Drupal/Core/Entity/EntityManager.php
   
   --- a/core/lib/Drupal/Core/Entity/EntityManager.php
   +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   
   +++ b/core/modules/action/tests/src/Functional/ActionListTest.php
   @@ -27,7 +27,7 @@ public function testEmptyActionList() {
   -    $storage = $this->container->get('entity.manager')->getStorage('action');
   

   At the top of the entityManager class, there is a @todo that needs to be removed

2. +++ b/core/modules/aggregator/src/Plugin/views/argument/Iid.php
   @@ -50,7 +50,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   -    return new static($configuration, $plugin_id, $plugin_definition, $container->get('entity.manager'));
   +    return new static($configuration, $plugin_id, $plugin_definition, $container->get('entity_type.manager'));
   

   nit: we usually break the arguments to new static() onto separate lines in factory methods when they are over 80 characters. lets do that here since we are touching this line.

3. +++ b/core/modules/block/src/BlockForm.php
   @@ -110,7 +110,7 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager, Exe
   -      $container->get('entity.manager'),
   +      $container->get('entity_type.manager'),
          $container->get('plugin.manager.condition'),
   

   It's interesting here we pass the EntityTypeManager to the constructor and then extract the block storage from it and throw the manager away, while in other places we need storage, we pass the storage directly from the factory method. Probably fine for purposes of this patch, but what's the best practice/standard for injecting storage when the manager is not otherwise needed?

4. +++ b/core/modules/block/tests/src/Unit/BlockFormTest.php
   @@ -74,10 +74,10 @@ protected function setUp() {
   +    $this->entityTypeManager = $this->getMock('Drupal\Core\Entity\EntityTypeManagerInterface');
   
   +++ b/core/modules/block_content/src/Plugin/Derivative/BlockContent.php
   @@ -33,9 +33,9 @@ public function __construct(EntityStorageInterface $block_content_storage) {
   +    $entity_type_manager = $container->get('entity_type.manager');
   

   Leaving deprecated getMock in here is fine, many other usages around it and out of scope for this patch.

5. +++ b/core/modules/block_content/src/Controller/BlockContentController.php
   @@ -37,10 +37,10 @@ class BlockContentController extends ControllerBase {
   +    $enitty_type_manager = $container->get('entity_type.manager');
   ...
   +      $enitty_type_manager->getStorage('block_content'),
   +      $enitty_type_manager->getStorage('block_content_type'),
   

   s/enitty/entity/

6. +++ b/core/modules/block_content/src/Plugin/Derivative/BlockContent.php
   @@ -33,9 +33,9 @@ public function __construct(EntityStorageInterface $block_content_storage) {
   -    $entity_manager = $container->get('entity.manager');
   +    $entity_type_manager = $container->get('entity_type.manager');
        return new static(
   -      $entity_manager->getStorage('block_content')
   +      $entity_type_manager->getStorage('block_content')
        );
   
   +++ b/core/modules/block_content/tests/src/Functional/BlockContentListTest.php
   @@ -72,7 +72,7 @@ public function testListing() {
        $blocks = $this->container
   

   This can just be combined into one line.

7. +++ b/core/modules/book/src/Form/BookAdminEditForm.php
   @@ -51,9 +51,9 @@ public function __construct(EntityStorageInterface $node_storage, BookManagerInt
   -    $entity_manager = $container->get('entity.manager');
   +    $entity_type_manager = $container->get('entity_type.manager');
        return new static(
   -      $entity_manager->getStorage('node'),
   +      $entity_type_manager->getStorage('node'),
   
   +++ b/core/modules/book/tests/src/Functional/BookTest.php
   @@ -340,7 +340,7 @@ public function testNavigationBlockOnAccessModuleInstalled() {
   -    $node_storage = $this->container->get('entity.manager')->getStorage('node');
   

   nit: we really don't need the variable here.

8. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationTestBase.php
   @@ -222,8 +222,8 @@ protected function createEntity($values, $langcode, $bundle_name = NULL) {
   -    $controller = $this->container->get('entity.manager')->getStorage($this->entityTypeId);
   -    if (!($controller instanceof SqlContentEntityStorage)) {
   +    $storage = $this->container->get('entity_type.manager')->getStorage($this->entityTypeId);
   +    if (!($storage instanceof SqlContentEntityStorage)) {
   
   +++ b/core/modules/field/tests/src/Kernel/FieldAttachOtherTest.php
   @@ -203,7 +203,7 @@ public function testEntityCache() {
   -    $controller = $this->container->get('entity.manager')->getStorage($entity->getEntityTypeId());
   +    $controller = $this->container->get('entity_type.manager')->getStorage($entity->getEntityTypeId());
   

   In some places we changed the name of the storage variable from $controller to $storage and in other places we didn't. I feel like it should be done, but looking at how many instances there are, it probably shouldn't be done in this patch.

9. +++ b/core/modules/node/src/Plugin/views/argument/Type.php
   @@ -42,12 +42,12 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   -    $entity_manager = $container->get('entity.manager');
   +    $entity_type_manager = $container->get('entity_type.manager');
        return new static(
          $configuration,
          $plugin_id,
          $plugin_definition,
   -      $entity_manager->getStorage('node_type')
   +      $entity_type_manager->getStorage('node_type')
   

   nit: Don't really need the $entity_type_manager variable here.

10. +++ b/core/modules/system/tests/src/Functional/Update/EntityUpdateAddRevisionDefaultTest.php
    @@ -39,7 +32,6 @@ protected function setUp() {
         // Do not use this property after calling ::runUpdates().
    -    $this->entityManager = \Drupal::entityManager();
         $this->state = \Drupal::state();
       }
    

    And here.

11. +++ b/core/modules/system/tests/src/Functional/Update/EntityUpdateAddRevisionTranslationAffectedTest.php
    @@ -44,7 +37,6 @@ protected function setUp() {
         // Do not use this property after calling ::runUpdates().
    -    $this->entityManager = \Drupal::entityManager();
         $this->state = \Drupal::state();
    

    Does the comment refer to the entityManager specifically? Should it be removed too, or is state also stale after updates?

12. +++ b/core/modules/taxonomy/src/Plugin/views/argument/IndexTidDepth.php
    @@ -40,7 +40,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
    -    return new static($configuration, $plugin_id, $plugin_definition, $container->get('entity.manager')->getStorage('taxonomy_term'));
    +    return new static($configuration, $plugin_id, $plugin_definition, $container->get('entity_type.manager')->getStorage('taxonomy_term'));
    

    nit: would be more readable to split arguments onto separate lines

13. +++ b/core/modules/user/src/Plugin/views/argument/RolesRid.php
    @@ -44,7 +44,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
    -    return new static($configuration, $plugin_id, $plugin_definition, $container->get('entity.manager'));
    +    return new static($configuration, $plugin_id, $plugin_definition, $container->get('entity_type.manager'));
    

    and here

14. +++ b/core/modules/user/src/Plugin/views/argument/Uid.php
    @@ -44,7 +44,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
    -      $container->get('entity.manager')->getStorage('user'));
    +      $container->get('entity_type.manager')->getStorage('user'));
    
    +++ b/core/modules/user/src/Plugin/views/field/Permissions.php
    @@ -57,7 +57,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
    -    return new static($configuration, $plugin_id, $plugin_definition, $container->get('module_handler'), $container->get('entity.manager'));
    +    return new static($configuration, $plugin_id, $plugin_definition, $container->get('module_handler'), $container->get('entity_type.manager'));
    

    Couple more

15. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUrlTest.php
    @@ -576,12 +576,12 @@ public function testUriRelationships() {
        * @param array $methods
        *   (optional) An array of additional methods to mock on the entity object.
    -   *   The getEntityType() and entityManager() methods are always mocked.
    +   *   The getEntityType() and entityTypeManager() methods are always mocked.
    

    This just feels off to me. The method mocks the getEntityType and the entityTypeBundleInfo() which is not mentioned in the doc, but doesn't actually mock the return value of entityTypeManager like the documentation suggests. I think the documentation needs to be adjusted.

1. Removed the @todo, not quite sure what to do about the rest of the docblock, the todo kind of belonged to that.
2. Fixed.
3. As discussed, I prefer injecting and keeping the entity type manager, not the storage, but way out of scope.
5. Fixed.
6. / 7. / 9. There are lot of these cases, I'd rather not change that here.
8. Yeah, weird but IMHO also out of scope. controller is actually unused. But it's a base class, there might be more subclasses of that in contrib? Should have don that when adding a new base class for phpunit tests ;)
10. / 11. I think it technically applies to both, state also has a static cache that could interfere so I kept it.
12. / 13. / 14. Fixed.
15. Yes, removed entityTypeManager() and updated the comment to mention entityTypeBundleInfo()

All my feedback has been addressed, RTBC from me pending green tests.

Committed 408c540 and pushed to 8.8.x. Thanks!

Note I ignored the changes core/modules/comment/src/Form/CommentTypeDeleteForm.php to because I committed #2509652: Remove unused and deprecated entity.manager service injection in CommentTypeDeleteForm first.

diff --git a/core/modules/editor/tests/src/Unit/EditorConfigEntityUnitTest.php b/core/modules/editor/tests/src/Unit/EditorConfigEntityUnitTest.php
index a463727869..45ebe37882 100644
--- a/core/modules/editor/tests/src/Unit/EditorConfigEntityUnitTest.php
+++ b/core/modules/editor/tests/src/Unit/EditorConfigEntityUnitTest.php
@@ -3,7 +3,6 @@
 namespace Drupal\Tests\editor\Unit;
 
 use Drupal\Core\DependencyInjection\ContainerBuilder;
-use Drupal\Core\Entity\EntityManager;
 use Drupal\Core\Entity\EntityTypeManagerInterface;
 use Drupal\editor\Entity\Editor;
 use Drupal\Tests\UnitTestCase;
@@ -81,7 +80,6 @@ protected function setUp() {
       ->getMock();
 
     $container = new ContainerBuilder();
-    //$container->set('entity.manager', $entity_manager);
     $container->set('entity_type.manager', $this->entityTypeManager);
     $container->set('uuid', $this->uuid);
     $container->set('plugin.manager.editor', $this->editorPluginManager);
diff --git a/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
index 71b677acc9..c12a6b04d7 100644
--- a/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
+++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
@@ -604,7 +604,7 @@ public function testGetFormDisplay() {
    */
   public function testGetDefinition() {
     $entity_type = $this->prophesize(EntityTypeInterface::class)->reveal();
-    $this->entityTypeManager->getDefinition('entity_test', true)->shouldBeCalled()->willReturn($entity_type);
+    $this->entityTypeManager->getDefinition('entity_test', TRUE)->shouldBeCalled()->willReturn($entity_type);
     $this->assertInstanceOf(EntityTypeInterface::class, $this->entityManager->getDefinition('entity_test'));
   }
 
@@ -665,8 +665,8 @@ public function testGetStorage() {
    * @expectedDeprecation EntityManagerInterface::getFormObject() is deprecated in drupal:8.0.0 and is removed from drupal:9.0.0. Use \Drupal\Core\Entity\EntityTypeManager::getFormObject() instead. See https://www.drupal.org/node/2549139
    */
   public function testGetFormObject() {
-    $FormObject = $this->prophesize(EntityFormInterface::class)->reveal();
-    $this->entityTypeManager->getFormObject('entity_test', 'edit')->shouldBeCalled()->willReturn($FormObject);
+    $form_object = $this->prophesize(EntityFormInterface::class)->reveal();
+    $this->entityTypeManager->getFormObject('entity_test', 'edit')->shouldBeCalled()->willReturn($form_object);
     $this->assertInstanceOf(EntityFormInterface::class, $this->entityManager->getFormObject('entity_test', 'edit'));
   }
 

Fixed some coding standards on commit.