Use more specific entity.manager services in module .services.yml files

Also, we have too many REST tests, the amount of fails due to a wrong check in BasicAuth is pretty crazy.

I think @Wim Leers would disagree. :)

+++ b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php
@@ -55,14 +63,20 @@ class BasicAuth implements AuthenticationProviderInterface, AuthenticationProvid
-  public function __construct(ConfigFactoryInterface $config_factory, UserAuthInterface $user_auth, FloodInterface $flood, EntityManagerInterface $entity_manager) {
+  public function __construct(ConfigFactoryInterface $config_factory, UserAuthInterface $user_auth, FloodInterface $flood, EntityTypeManagerInterface $entity_type_manager) {
     $this->configFactory = $config_factory;
     $this->userAuth = $user_auth;
     $this->flood = $flood;
-    $this->entityManager = $entity_manager;
+    if ($entity_type_manager instanceof EntityManagerInterface) {
+      @trigger_error('Passing the entity.manager service to BasicAuth::__construct() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Pass the new dependencies instead. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);
+      $this->entityTypeManager = \Drupal::entityTypeManager();
+    }
+    else {
+      $this->entityTypeManager = $entity_type_manager;
+    }

I don't think this is worth it. Because EntityManagerInterface implements EntityTypeManagerInterface so if something is override and passing in the entity manager then it'll work. Once we can properly deprecate the entity manager service custom or contrib code that does this will get a deprecation message anyway so there's not a lot gained as far as I can tell.

+++ b/core/modules/comment/src/CommentManager.php
@@ -71,8 +89,21 @@ class CommentManager implements CommentManagerInterface {
-  public function __construct(EntityManagerInterface $entity_manager, ConfigFactoryInterface $config_factory, TranslationInterface $string_translation, ModuleHandlerInterface $module_handler, AccountInterface $current_user) {
-    $this->entityManager = $entity_manager;
+  public function __construct(EntityTypeManagerInterface $entity_type_manager, ConfigFactoryInterface $config_factory, TranslationInterface $string_translation, ModuleHandlerInterface $module_handler, AccountInterface $current_user, EntityFieldManagerInterface $entity_field_manager = NULL) {
+    if ($entity_type_manager instanceof EntityManagerInterface) {
...
+    if ($entity_field_manager) {
+      $this->entityFieldManager = $entity_field_manager;
+    }
+    else {
+      @trigger_error('The entity_field.manager service must be passed to CommentManager::__construct(), it is required before Drupal 9.0.0. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);
+      $this->entityFieldManager = \Drupal::service('entity_field.manager');
+    }

In the case where we are adding an new argument I think we need code to handle the new argument. So anything that extends this will not be broken.

I think we need an @trigger_error() because unlike #16 service deprecation messages won't help us.

1. +++ b/core/modules/book/src/BookManager.php
   @@ -97,7 +115,7 @@ protected function loadBooks() {
   +      $nodes = $this->entityTypeManager->getStorage('node')->loadMultiple($nids);
   
   @@ -413,7 +431,7 @@ protected function recurseTableOfContents(array $tree, $indent, array &$toc, arr
   +    $nodes = $this->entityTypeManager->getStorage('node')->loadMultiple($nids);
   
   @@ -449,7 +467,7 @@ public function deleteFromBook($nid) {
   +      $children = $this->entityTypeManager->getStorage('node')->loadMultiple(array_keys($result));
   
   @@ -576,7 +594,7 @@ protected function buildItems(array $tree) {
   +      $node = $this->entityTypeManager->getStorage('node')->load($data['link']['nid']);
   
   @@ -995,14 +1013,14 @@ public function bookLinkTranslate(&$link) {
   +      $node = $this->entityTypeManager->getStorage('node')->load($link['nid']);
   ...
   +        $node = $this->entityTypeManager->getStorage('node')
   

   do you reckon its worth a novice followup to add a nodeStorage protected property and initialize that in the constructor and avoid this duplication? (out of scope here)

2. +++ b/core/modules/book/tests/src/Unit/BookManagerTest.php
   @@ -57,7 +58,7 @@ class BookManagerTest extends UnitTestCase {
   -    $this->entityManager = $this->getMock('Drupal\Core\Entity\EntityManagerInterface');
   +    $this->entityManager = $this->createMock(EntityTypeManagerInterface::class);
   

   with this one you switch to using ::class constant, but for the rest you retain the class as a string.

   Should we standardize? I'd be plus one on making them all use the class constant.

3. +++ b/core/modules/comment/src/CommentManager.php
   --- a/core/modules/comment/src/CommentStatistics.php
   +++ b/core/modules/comment/src/CommentStatistics.php
   
   +++ b/core/modules/comment/src/CommentStatistics.php
   @@ -3,15 +3,23 @@
   +use Drupal\Core\Entity\EntityManagerInterface;
   

   unneeded?

4. +++ b/core/modules/comment/tests/src/Unit/CommentLinkBuilderTest.php
   @@ -70,10 +70,10 @@ class CommentLinkBuilderTest extends UnitTestCase {
   +    $this->entityTypeManager = $this->getMock('\Drupal\Core\Entity\EntityTypeManagerInterface');
   
   +++ b/core/modules/comment/tests/src/Unit/CommentManagerTest.php
   @@ -28,9 +28,10 @@ public function testGetFields() {
   +    $entity_field_manager = $this->getMock('Drupal\Core\Entity\EntityFieldManagerInterface');
   +    $entity_type_manager = $this->getMock('Drupal\Core\Entity\EntityTypeManagerInterface');
   

   example where you've kept the class as a string (there are many more)

5. +++ b/core/modules/serialization/serialization.services.yml
   @@ -6,17 +6,17 @@ services:
   -      class: Drupal\serialization\Normalizer\ContentEntityNormalizer
   -      tags:
   -        - { name: normalizer }
   -      arguments: ['@entity.manager']
   +    class: Drupal\serialization\Normalizer\ContentEntityNormalizer
   +    tags:
   +      - { name: normalizer }
   +    arguments: ['@entity_type.manager', '@entity_type.repository', '@entity_field.manager']
   

   hah, nice cleanup

6. +++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
   @@ -22,11 +24,25 @@ class EntityNormalizer extends ComplexDataNormalizer implements DenormalizerInte
   +    $this->entityTypeRepository = $entity_type_repository;
   ...
   +    $this->entityFieldManager = $entity_field_manager;
   

   are these even used? not seeing them in this class so is it child classes only?

7. +++ b/core/modules/system/src/SystemManager.php
   @@ -79,8 +81,14 @@ class SystemManager {
   +    if ($request_stack instanceof EntityManagerInterface) {
   

   ouch

I'm actually not a big fan of "injecting" entity storages
me neither since I spent 3 hrs this working working out why after saving a contact form all of its field definitions went missing.

Turned out there was a ->getQuery() to get an entity query call in a constructor of a service that was doing inbound/outbound route processing before modules were loaded resulting in field_entity_storage_info not being loaded and being dropped from the module implements cache

I was unable to find any occurrence other then core/core.services.yml

grep -inr 'entity.manager' core/*.yml
core/core.services.yml:546:  entity.manager:

+++ b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php
@@ -18,6 +19,12 @@
+  use DeprecatedServicePropertyTrait;
+
+  /**
+   * {@inheritdoc}
+   */
+  protected $deprecatedProperties = ['entityManager' => 'entity.manager'];

I like this.

I'm actually not a big fan of "injecting" entity storages

I'd like to know more about it. Can we document it somewhere?

Patch looks good to me so setting it RTBC.

I'm actually not a big fan of "injecting" entity storages

me neither

Same!

EDIT: also, EPIC patch! 😵

+++ b/core/modules/system/src/SystemManager.php
@@ -79,8 +81,14 @@ class SystemManager {
+  public function __construct(ModuleHandlerInterface $module_handler, $request_stack, $menu_tree, $menu_active_trail) {
     $this->moduleHandler = $module_handler;
+    if ($request_stack instanceof EntityManagerInterface) {
+      @trigger_error('Passing the entity.manager service to SystemManager::__construct() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0.. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);
+      $request_stack = \Drupal::service('request_stack');
+      $menu_tree = \Drupal::menuTree();
+      $menu_active_trail = \Drupal::service('menu.active_trail');
+    }

We need assert on the interfaces here or we lose type safety. I think the thing to do here is to remove the entity manager type hint and pass a NULL in via the .services file and then file a follow-up for Drupal 9 to fix this. Because then we can make a breaking change... or maybe we have auto-wiring at that point.

#34 is moved to follow-up so back to RTBC.

Crediting myself, @jibran, @larowlan for reviews.

Committed 7dcda1f and pushed to 8.7.x. Thanks!

diff --git a/core/modules/comment/src/CommentManager.php b/core/modules/comment/src/CommentManager.php
index 2e2a8d8e00..18eb329ffb 100644
--- a/core/modules/comment/src/CommentManager.php
+++ b/core/modules/comment/src/CommentManager.php
@@ -77,8 +77,6 @@ class CommentManager implements CommentManagerInterface {
    *
    * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
    *   The entity type manager service.
-   * @param \Drupal\Core\Entity\EntityFieldManagerInterface $entity_field_manager
-   *   The entity field manager service.
    * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
    *   The config factory.
    * @param \Drupal\Core\StringTranslation\TranslationInterface $string_translation
@@ -87,6 +85,8 @@ class CommentManager implements CommentManagerInterface {
    *   The module handler service.
    * @param \Drupal\Core\Session\AccountInterface $current_user
    *   The current user.
+   * @param \Drupal\Core\Entity\EntityFieldManagerInterface $entity_field_manager
+   *   The entity field manager service.
    */
   public function __construct(EntityTypeManagerInterface $entity_type_manager, ConfigFactoryInterface $config_factory, TranslationInterface $string_translation, ModuleHandlerInterface $module_handler, AccountInterface $current_user, EntityFieldManagerInterface $entity_field_manager = NULL) {
     $this->entityTypeManager = $entity_type_manager;
diff --git a/core/modules/forum/src/ForumManager.php b/core/modules/forum/src/ForumManager.php
index 340fddecb3..6e95ff0447 100644
--- a/core/modules/forum/src/ForumManager.php
+++ b/core/modules/forum/src/ForumManager.php
@@ -127,14 +127,14 @@ class ForumManager implements ForumManagerInterface {
    *   The config factory service.
    * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
    *   The entity type manager.
-   * @param \Drupal\Core\Entity\EntityFieldManagerInterface $entity_field_manager
-   *   The entity field manager.
    * @param \Drupal\Core\Database\Connection $connection
    *   The current database connection.
    * @param \Drupal\Core\StringTranslation\TranslationInterface $string_translation
    *   The translation manager service.
    * @param \Drupal\comment\CommentManagerInterface $comment_manager
    *   The comment manager service.
+   * @param \Drupal\Core\Entity\EntityFieldManagerInterface $entity_field_manager
+   *   The entity field manager.
    */
   public function __construct(ConfigFactoryInterface $config_factory, EntityTypeManagerInterface $entity_type_manager, Connection $connection, TranslationInterface $string_translation, CommentManagerInterface $comment_manager, EntityFieldManagerInterface $entity_field_manager = NULL) {
     $this->configFactory = $config_factory;
diff --git a/core/modules/hal/src/LinkManager/RelationLinkManager.php b/core/modules/hal/src/LinkManager/RelationLinkManager.php
index 9280aef6fc..8db87a8e91 100644
--- a/core/modules/hal/src/LinkManager/RelationLinkManager.php
+++ b/core/modules/hal/src/LinkManager/RelationLinkManager.php
@@ -72,7 +72,7 @@ class RelationLinkManager extends LinkManagerBase implements RelationLinkManager
    * @param \Drupal\Core\Entity\EntityFieldManagerInterface $entity_field_manager
    *   The entity field manager.
    */
-  public function __construct(CacheBackendInterface $cache, EntityTypeManagerInterface $entity_type_manager, ModuleHandlerInterface $module_handler, ConfigFactoryInterface $config_factory, RequestStack $request_stack,  EntityTypeBundleInfoInterface $entity_type_bundle_info = NULL, EntityFieldManagerInterface $entity_field_manager = NULL) {
+  public function __construct(CacheBackendInterface $cache, EntityTypeManagerInterface $entity_type_manager, ModuleHandlerInterface $module_handler, ConfigFactoryInterface $config_factory, RequestStack $request_stack, EntityTypeBundleInfoInterface $entity_type_bundle_info = NULL, EntityFieldManagerInterface $entity_field_manager = NULL) {
     $this->cache = $cache;
     $this->entityTypeManager = $entity_type_manager;
     if (!$entity_type_bundle_info) {
diff --git a/core/modules/hal/src/LinkManager/TypeLinkManager.php b/core/modules/hal/src/LinkManager/TypeLinkManager.php
index 897731fe8d..95df24b03b 100644
--- a/core/modules/hal/src/LinkManager/TypeLinkManager.php
+++ b/core/modules/hal/src/LinkManager/TypeLinkManager.php
@@ -59,10 +59,10 @@ class TypeLinkManager extends LinkManagerBase implements TypeLinkManagerInterfac
    *   The config factory service.
    * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack
    *   The request stack.
-   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
-   *   The entity type manager.
    * @param \Drupal\Core\Entity\EntityTypeBundleInfoInterface $bundle_info_service
    *   The bundle info service.
+   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
+   *   The entity type manager.
    */
   public function __construct(CacheBackendInterface $cache, ModuleHandlerInterface $module_handler, ConfigFactoryInterface $config_factory, RequestStack $request_stack, EntityTypeBundleInfoInterface $bundle_info_service, EntityTypeManagerInterface $entity_type_manager = NULL) {
     $this->cache = $cache;

Fixed the above coding standards / documentation issues on commit.