Use more specific entity.manager services in core.services.yml

I don't see how a novice would know how to proceed on this issue. I just about barely understand it.

This will change some class constructor signatures and also some interfaces. For example \Drupal\Core\Config\ConfigManagerInterface:

• First argument in __construct() is now an entity manager object but will be an entity type manager.
• ::getEntityManager() will return an entity type manager object instead of entity manager. Course, this method will be deprecated and we'll add a clone ::getEntityTypeManager() but it still return other object than now in HEAD.

Is this OK for 8.1.x?

I'm illustrating with this patch.

This is keeping the interface BC but not constructor signature.

This is 100% BC. Maybe we need to default the new arguments from signature to NULL?

@dawehner, as we've discussed on IRC, I picked up other example. There's only one use case (I will extend the patch to all use cases after discussing).

Question: Does the service constructor signature change (in this case ContentUninstallValidator::__construct()) represent a BC break? I think, yes. So, I'm not sure that this can go as replacement. In this case I think we need to deprecate the entity.manager service argument and add the entity_type.manager as optional argument at the end of signature, so we don't break existing classed that are extending ContentUninstallValidator.

I guess such a change can break existing sites that are using sites/default/services.yml. Moving to 9.x

You mean: If an argument expects an EntityTypeManagerInterface object but a custom module an existing site will still pass the old EntityManagerInterface object, this is OK because EntityManagerInterface inherits EntityTypeManagerInterface?

I picked-up only those where i can do a 1-to-1 replacement.

Forgot the core modules.

This is now a huge patch :(

Still needs work.

OK, ready for a review. But I think this should go in 8.0.x. Reason: The module developers are using core (and core modules) as a primary inspiration source. They will continue to use @entity.manager because in YAML their IDE do not mark that value as deprecated. Very few of them will go the the service class and see the deprecation note. We have to encourage them to use the new entity services instead of EntityManager.

Created #2627938: Add docs to *.services.yml to tell that @entity.manager is deprecated for 8.0.x.

Moving to 8.0.x as per #21.

Rerolled for 8.0.x.

Previous patch is out of date. Rerolling with mostly the same contents, with exception for some of the patched files that are already deleted.

+++ b/core/core.services.yml
@@ -871,7 +871,7 @@ services:
   entity.query:
     class: Drupal\Core\Entity\Query\QueryFactory
-    arguments: ['@entity.manager']
+    arguments: ['@entity_type.manager']
     calls:

entity.query itself is deprecated too, so maybe we want to leave that out to avoid conflicts with #2389335: Deprecate entity.query service and replace with using the entity storage's getQuery() method?

There's a BC risk with this patch. Suppose there's a class extending a service where @entity.manage is injected and we replace entity manager with @entity_type.manager. If that extension will use a method that is part of @entity.manager but not part of @entity_type.manager, will crash.

The constructor for a service object (one in the container), plugin, or controller is considered internal unless otherwise marked, and may change in a minor release

https://www.drupal.org/core/d8-bc-policy#constructors

Yeah, that's the official policy, but we still end up breaking modules if we change constructors of subclassed classes.

That said that's not even the point of #34, but the fact that we *change* a property of those classes, so subclasses that do $this->entityManager will then break. But those are also considered @internal. Still, we end up breaking stuff, so at least on base classes like e.g. an EntityForm, we can't just change it. But I think most services here are usually not extended.

None of those replaced properties are tagged with @internal and they are not private so there is nothing that would stop developers from using them in subclasses. The question is how likely is it? Should we check every single contrib module on d.org for this kind of subclasses? That way we will know that this patch at least won't break sites that do not contain custom code.

If this BC risk is considered plausible, this patch can be committed only in Drupal 9. Unless #2575081: [policy, no patch] Use E_USER_DEPRECATED in Drupal 8 minor releases is accepted sooner, in which case we will have a lot of errors.

so at least on base classes like e.g. an EntityForm, we can't just change it.

That's fine, but this issue is about services only. There is no BC break here.

So I actually did download all contrib modules with Drupal 8 versions on d.org, and – who would have though! – found at least 3 modules that do exactly what @claudiu.cristea describes in #34: entity_pilot, replication, subsite

Here is an (edited) example from dev version of subsite module:

subsite-8.x-1.x-dev/src/SubsiteBookManager.php:

<?php

namespace Drupal\subsite;

use Drupal\book\BookManager;
use Drupal\Core\Template\Attribute;


class SubsiteBookManager extends BookManager {

  /* ... snip ... */

  /**
   * {@inheritdoc}
   */
  public function bookTreeOutput(array $tree) {
    /* ... snip ... */

    $num_items = count($items);
    foreach ($items as $i => $data) {
      /* ... snip ... */
      $node = $this->entityManager->getStorage('node')->load($data['link']['nid']);

  /* ... snip ... */

Drupal\book\BookManager is one of the files that are updated by the patch and it does not have entityManager property anymore.

And even more modules extend some of the patched classes and override class constructor. Example from default_content module:

default_content-8.x-1.0-alpha3/src/Normalizer/TermEntityNormalizer.php:

<?php

namespace Drupal\default_content\Normalizer;

use Drupal\Core\Entity\EntityManagerInterface;
use Drupal\Core\Extension\ModuleHandlerInterface;
use Drupal\hal\Normalizer\ContentEntityNormalizer;
use Drupal\rest\LinkManager\LinkManagerInterface;

/**
 * Defines a class for normalizing terms to ensure parent is stored.
 */
class TermEntityNormalizer extends ContentEntityNormalizer {

  /* ... snip ... */

  /**
   * {@inheritdoc}
   */
  public function __construct(LinkManagerInterface $link_manager, EntityManagerInterface $entity_manager, ModuleHandlerInterface $module_handler) {
    parent::__construct($link_manager, $entity_manager, $module_handler);
    $this->termStorage = $entity_manager->getStorage('taxonomy_term');
  }

The child constructor passes the wrong type of object to the parent but this does not seem to cause any errors actually.

So what will it be: postponed or won't fix?

Those aren't protected by the BC policy. Idk how else to explain it.

@tim.plunkett
So basically, it is a bug in contrib modules that do not use Drupal API correctly?

No, it is not a bug. They're simply doing something is not covered by the BC policy. That's OK, they simply have to live with the consequences.

I recently wrote a blog post on how to override a service with additional dependencies and avoiding to touch the constructor to avoid a case like this, works only for services though: http://www.md-systems.ch/de/blog/techblog/2016/12/17/how-safely-inject-a...

If you want to be nice, you could write a patch for default_content and other modules that do this and rewrite their code like this, then they should be compatible both with HEAD and this patch.

This will need a reroll now after #2389335: Deprecate entity.query service and replace with using the entity storage's getQuery() method got in. That probably conflicted on quite a bunch of services and some were already updated to ue entity_type.manager as part of that issue too.

Already working on it. Not a bunch, just 3 actually.

Work still in progress, uploading to run the tests.

Oops..

+++ b/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php
@@ -23,14 +23,27 @@
-   * The entity type CRUD listener.
+   * The entity manager service.
+   *
+   * This class still has a dependency on 'entity.manager' service to control
+   * caching in all relevant entity services.
+   *
+   * @todo Stop injecting entity.manager service once
+   *   https://www.drupal.org/node/2549143 is in.
+   *
+   * @var \Drupal\Core\Entity\EntityManagerInterface
+   */
+  protected $entityManager;
+
+  /**
+   * The entity type listener service.
    *

Hm. If this is needed then that sounds like a bug.

It is certainly a bug in this part of EntityManager:

  public function clearCachedDefinitions() {
    $this->container->get('entity_type.manager')->clearCachedDefinitions();

    // @todo None of these are plugin managers, and they should not co-opt
    //   this method for managing its caches. Remove in
    //   https://www.drupal.org/node/2549143.
    $this->container->get('entity_type.bundle.info')->clearCachedBundles();
    $this->container->get('entity_field.manager')->clearCachedFieldDefinitions();
    $this->container->get('entity_type.repository')->clearCachedDefinitions();
  }

I don't know how to deal with this part right now. So instead of copy-pasting this code into EntityDefinitionUpdateManager, I've decided to keep dependency on 'entity.manager'.

Good, they are all finally passing. Issue summary update is on its way.

Reroll, conflicted in hal and seriaization.services.yml (plus moved files that git rebase handled automatically)

+++ b/core/lib/Drupal/Core/Config/ConfigManagerInterface.php
@@ -34,14 +34,25 @@ public function loadConfigEntityByName($name);
    * @return \Drupal\Core\Entity\EntityManagerInterface
    *   The entity manager.
+   *
+   * @deprecated in Drupal 8.4.x, will be removed before Drupal 9.0.0. Use
+   *   \Drupal\Core\Config\ConfigManagerInterface::getEntityTypeManager() instead.
    */
   public function getEntityManager();

wondering if we should already updated those usages here as well but this patch is big enough, lets make a follow-up?

that said, It is also not clear to me why we inject this through the config manager and don't inject the entity type manager into those services directly? Then we could just deprecate it without replacement. Probably @alexpott knows more.

re #68 The reason the ConfigManager exposed the entity manager is because it exposes a method getEntityTypeIdByName() often this is followed by doing something with the entity type. I think when I wrote this I felt that injecting both was overkill because the ConfigManager had a clear dependency on the entity manager. Also, \Drupal\Core\Config\ConfigImporter has enough injected already.

+++ b/core/modules/hal/hal.services.yml
@@ -16,10 +16,10 @@ services:
   serializer.normalizer.entity.hal:
     class: Drupal\hal\Normalizer\ContentEntityNormalizer
-    arguments: ['@hal.link_manager', '@entity.manager', '@module_handler']
+    arguments: ['@hal.link_manager', '@entity.manager', '@module_handler', '@entity_type.repository', '@entity_field.manager']
     tags:

shouldn't this also be the entity_type.manager?

Yep. Missed it.

Is it allowed to reorder arguments in a class constructor? Instead of this order in ContentEntityNormalizer, for example:

public function __construct(
    LinkManagerInterface $link_manager,
    EntityTypeManagerInterface $entity_type_manager,
    ModuleHandlerInterface $module_handler,
    EntityTypeRepositoryInterface $entity_type_repository,
    EntityFieldManagerInterface $entity_field_manager
) {
  $this->linkManager = $link_manager;
  $this->entityTypeManager = $entity_type_manager;
  $this->moduleHandler = $module_handler;
  $this->entityTypeRepository = $entity_type_repository;
  $this->entityFieldManager = $entity_field_manager;
}

I would rather see services logically grouped like this:

public function __construct(
    LinkManagerInterface $link_manager,
    EntityTypeManagerInterface $entity_type_manager,
    EntityTypeRepositoryInterface $entity_type_repository,
    EntityFieldManagerInterface $entity_field_manager,
    ModuleHandlerInterface $module_handler
) {
  $this->linkManager = $link_manager;
  $this->entityTypeManager = $entity_type_manager;
  $this->entityTypeRepository = $entity_type_repository;
  $this->entityFieldManager = $entity_field_manager;
  $this->moduleHandler = $module_handler;
}

Another reroll after #2751325: All serialized values are strings, should be integers/booleans when appropriate

Adding correct description to updated class properties and constructor agrument. And replacing one instance of \Drupal::entityManager() call with a service injection.

1. +++ b/core/lib/Drupal/Core/Config/ConfigManagerInterface.php
   @@ -34,14 +34,25 @@ public function loadConfigEntityByName($name);
   +   *   \Drupal\Core\Config\ConfigManagerInterface::getEntityTypeManager() instead.
   

   Line exceeding 80 characters.

2. +++ b/core/modules/search/tests/src/Unit/SearchPageRepositoryTest.php
   @@ -56,13 +56,14 @@ protected function setUp() {
   +    /** @var \Drupal\Core\Entity\EntityTypeManagerInterface|\PHPUnit_Framework_MockObject_MockObject $entity_type_manager */
   

   The Multiline comments should not be in single line as per coding standards.

The patch #77 getting error: patch failed.

Another reroll after #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase and #2854529: Fix Drupal.Scope.MethodScope - all methods should have scopes

#78.1: Fixed, thanks!

#78.2: That is not a multiline comment, it's a variable type hint which is a bit special.

Reroll and minor code clean-up: missing doc comments; order of constructor arguments; and removed unused service injection into SystemManager.

Related: #2886622: Deprecate all EntityManager methods with E_USER_DEPRECATED

Also, I think this stands a better chance of getting in if we increase consistency by limiting scope, such as only changing the deprecated entity.manager to entity_type.manager in services.yml files.

No longer applies.

Rerolled the patch from #83.

A few minor corrections to the re-roll.

Address some test failures.
Fix coding standards error.

This was the patch I meant to upload.

Patch no longer applies. Needs reroll.

It's a huge patch, I reviewed visually at some 50+ places. These are my two most serious concerns ... and both are minor :)

1. +++ b/core/lib/Drupal/Core/Config/ConfigManagerInterface.php
   @@ -34,14 +34,26 @@ public function loadConfigEntityByName($name);
   +   * @deprecated in Drupal 8.4.x, will be removed before Drupal 9.0.0. Use
   +   *   \Drupal\Core\Config\ConfigManagerInterface::getEntityTypeManager()
   

   Outdated. Sigh, why do we do these things to ourselves

2. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -30,13 +30,13 @@ class EntityResolverManager {
       * @param \Drupal\Core\DependencyInjection\ClassResolverInterface $class_resolver
   
   +++ b/core/modules/comment/tests/src/Unit/CommentLinkBuilderTest.php
   diff --git a/core/modules/comment/tests/src/Unit/CommentManagerTest.php b/core/modules/comment/tests/src/Unit/CommentManagerTest.php
   index 3a6e201..7b4b669 100644
   
   +++ b/core/modules/serialization/tests/src/Unit/Normalizer/EntityNormalizerTest.php
   @@ -15,11 +15,25 @@
   +   * @var \Druapl\Core\Entity\EntityTypeRepositoryInterface|\PHPUnit_Framework_MockObject_MockObject
   

   Typo in class name.

Re-rolled

No error is shown in terminal while applying patch , don't know why it's failing test !!!

Reroll.

@Mohit Malik are you sure you were applying the patch to the latest version of 8.6.x?

Patch applies on 8.6.x.
Comments from #95 have not been addressed. Note the typo is in "\Druapl"

+++ b/core/core.services.yml
@@ -515,7 +515,13 @@ services:
-    arguments: ['@entity.manager', '@string_translation']
+    arguments: ['@entity_type.manager', '@string_translation']
+    lazy: true
+  field_uninstall_validator:
+    class: Drupal\Core\Field\FieldModuleUninstallValidator
+    tags:
+      - { name: module_install.uninstall_validator }
+    arguments: ['@entity_type.manager', '@entity_field.manager', '@string_translation']
     lazy: true

I think this went wrong in the reroll. field_uninstall_validator was not part of previous patches.

@Sutharsan , Thanks for reviewing the patch . Did required changes and attached interdiff against patch #101 :)

Unfortunately the patch still does not apply.
My first comment about "@deprecated in Drupal 8.4.x" has not yet been addressed. Pls change to 8.6.x.

@Mohit Malik, you are working from an old 8.6.x version. Your patch contains:

@@ -870,7 +870,7 @@ services:
       - { name: event_subscriber }
   entity.query:
     class: Drupal\Core\Entity\Query\QueryFactory
-    arguments: ['@entity.manager']

The last line (arguments: ...) changed in 24/01/2018.
Always create a patch from the HEAD of the applicable branch (git pull / git rebase). See https://www.drupal.org/node/707484 for more info about making patches.

1. Remove service included in error.
2. Correct version number in deprecation message.
3. Correct typo.
4. Remove redundant use statement.
5. Replace parameter removed in error.

Catastrophic test failure!

Correction in core.services.yml

Correct a few more tests.

A rebase and fixed the shared tempstore deprecation thing, was incorrectly merged.

This will still need a lot of work. A mistake that we did here was to change the order of injected services to keep the entity manager replacements together. While a nice idea, it is an unecessary BC change, all the new services should be optional constructor arguments with a fallback to \Drupal::service().

Also wondering if we should split this up as it is crazy large.

Here at the Front-end United conference I have discussed the status of this issue with Alex Pott, because I had concerns about how to proceed to get this patch committed.

The patch size is large (250K) and Drupal core is a moving target. But the major point is that the patch contains backward compatibility (BC) breaking changes. With all in one patch, these BC-breaking changes will block simpler and easier to accept parts of the patch. Futher the changes do affects many parts of core not all of them may be discovered by tests. We should allow ample time to discover the any unforseen effects.

To addres the first point, I propose to break-up the patch along lines of BC-breaking and difficulty of committing. That will allow progress on easier parts while BC discussions can take place. The patch currently contains both BC and non-BC breaking changes. Some of the BC-breaking changes may be mittigated or (better) refactored to be non-BC breaking. Inidividual API changes require change records to be written.

Split the patch

Proposed break-down of the patch:
1. Non-BC breaking changes; Classes with only 1-on-1 replacement of EntityManager by EntityTypeManager and with no other constructor signature changes. One patch for this category.
2. Changes in test-classes that have no relation to other changes in the patch. One patch for this category. (Perhaps this category is small or non-existend.)
3. Classes with more complex constructor changes; Other BC-breaking changes. Changes that need their own decicated change record. One patch per class.
4. Changed Base class; Classed that are (frequently) inherited; Changes that are expected to raise discussion about API-change(s). One patch per class.

Schedule the patch

To have enough time to discover unforseen effects, I propose to aim for committing the non-BC breaking code (parts 1 and 2) in the window between 6.0-alpha and 6.0.0. At 6.0-alpha release (the week of July 18, 2018) the 7.0.x branch will be opened. That will provide a period of 6-7 months before core is released with this change.

Parts 3 and 4 are smaller in scope, but may take more time to discuss. These can be committed later.

On Backward Compatibility

See also the Drupal 8 backwards compatibility and internal API policy.

Constructors

Changing EntityManagerInterface to EntityTypeManagerInterface is not BC breaking. When a modified constructor is called with EntityTypeManger, it will still work.
New constructor arguments should be made optional (defaults to NULL) and the constructor should check if the argument implements the desired interface.

Protected properties

Protected properties on a class are always considered @internal and should not change between patch versions. Protected properties in base classes may not be allowed to change.

How about we do this one last, after we've changed all the code usages to the extent we can without changing service definitions?

There are about a zillion smaller-scope issues dealing with EntityManager deprecation.

For instance: #2691675: Replace deprecated entityManager() in ControllerBase descendents

This deprecation really needs a meta.

I never followed up here but I quickly discussed this in Slack as well. What I proposed there is that we do split it up, but simply by doing patch for core.services.yml and one for the module services.yml.

I'm aware that this currently has BC breaking changes including tons of constructor changes. I have no intention of getting it committed like that. Most of those things can be handled in a way that doesn't break contrib modules and that includes constructor changes. We found a way to deal with ContentEntityForm for example.

For the constructor changes, that means appending new arguments and not changing the existing order. And doing it as optional arguments.

> This deprecation really needs a meta.

IMHO that's what your own #2886622: Deprecate all EntityManager methods with E_USER_DEPRECATED kind of is.

Trying that, rebased and moved the changes in core/modules to #2977107: Use more specific entity.manager services in module .services.yml files.

Expecting some test fails. I already updated the entity converted services to be backwards compatible so that the one in views_ui doesn't break.

Fixed the failing test and updated the one other service like that.

EntityDefinitionUpdateManager might be complex enough of a change to need a follow-up, as in #62 and #63.

It has a lot of calls to $this->entityManager->clearCachedDefinitions();, which it gets from EntityTypeRepositoryInterface, and $this->entityManager->useCaches(FALSE); which it gets from EntityFieldManagerInterface

So it probably needs to also inject EntityTypeRepositoryInterface and then use it to clear caches (if that's still a thing we do), and convert $this->entityManager->useCaches(FALSE); to use the existing $this->entityFieldManager.

The rest is +1 because it's pretty straightforward.

Updated IS for scope.

Edited
Re-roll
Working on splitting of the patch...
Sorry, did not see comments coming in. Reading...

@Sutharsan: Yeah, I was worried you'd be working on it, but it wasn't been assigned to you and you weren't in slack :)

@Mile23:
Yeah, that one is a bit annoying.

Since that service specifically deals with entity type and field definitions, I think it's fair to expect that we have to invalidate entity type and field definitions and also use the useCaches() method on EntityFieldManager (which is currently deprecated but seems like we might need to undeprecate that, we'll see). The bundles and entity type listener I'm not sure yet, lets see if something fails.

Kernel tests seem to pass with this, lets see. If this isn't working at all in update tests or so then we can split it off.

That passed :)

Also restoring issue title & summary.

(which is currently deprecated but seems like we might need to undeprecate that, we'll see)

Thankfully, that's out of scope here. :-)

1. This scope certainly seems manageable

2. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -83,21 +91,24 @@ class ConfigManager implements ConfigManagerInterface {
   +    $this->entityRepository = $entity_repository ?: \Drupal::service('entity.repository');
   

   Needs to trigger a deprecation message if not set.

3. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -120,7 +131,14 @@ public function loadConfigEntityByName($name) {
       * {@inheritdoc}
       */
      public function getEntityManager() {
   -    return $this->entityManager;
   +    return \Drupal::service('entity.manager');
   +  }
   

   Should throw a deprecation message if used. This one is fun to sort out...

4. +++ b/core/lib/Drupal/Core/Entity/EntityCreateAccessCheck.php
   @@ -14,11 +14,11 @@
   -  protected $entityManager;
   +  protected $entityTypeManager;
   

   These changes make me wonder if we should have an entityManagerProperty deprecation trait that triggers a deprecation warning and uses a magic getter. It could even be dynamic so the something like DeprecatedServicePropertyTrait and use a class property thats an array like ['entityManager', 'entity.manager'].

5. +++ b/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php
   @@ -14,21 +15,74 @@
      /**
       * The entity manager service.
       *
   +   * This class still has a dependency on 'entity.manager' service to control
   +   * caching in all relevant entity services.
   +   *
   +   * @todo Stop injecting entity.manager service once
   +   *   https://www.drupal.org/node/2549143 is in.
   +   *
       * @var \Drupal\Core\Entity\EntityManagerInterface
       */
      protected $entityManager;
   

   As far as I can see this has been removed.

6. +++ b/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php
   @@ -14,21 +15,74 @@
   +    $this->entityFieldManager = $entity_field_manager;
   +    $this->entityLastInstalledSchemaRepository = $entity_last_installed_schema_repository;
   +    $this->entityTypeListener = $entity_type_listener;
   +    $this->fieldStorageDefinitionListener = $field_storage_definition_listener;
   

   Should these have the NULL thingy? And deprecations. Or is the EntityDefintionUpdateManager not API :)

7. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -82,13 +91,21 @@ class EntityConverter implements ParamConverterInterface {
   +    // If the passed in service is the entity manager, then set it additionally
   +    // as the old, removed property.
   +    if ($entity_type_manager instanceof EntityManagerInterface) {
   +      $this->entityManager = $entity_type_manager;
   +    }
   

   This is weird. I think the trait idea up above is a better solution. As with the patch this would become public.

Thanks for the review.

1. Well, getting a bit bigger now..
2. Added.
3. Added, also converted the calls to it.
4. Interesting idea. Lets see if this works. Like with 2, this forces us to already convert the subclasses, e.g. in views_ui as well (or add it to the skipped exceptions). This is how the message then looks (we can't be quite as helpful as with non-generic ones as we don't know about how to replace it, but it's better than a fatal):

The property entityManager (entity.manager service) is deprecated in Drupal\Core\ParamConverter\EntityConverter and will be removed before Drupal 9.0.0.

5. Yep.
6. Well. It makes sense to be as nice as possible on classes that are likely candidates for replacements/subclasses. I don't think this is one of them. We certainly haven't done it before, especially the EntityConverter class history (which actually is being subclassed quite a bit) was a mess, cleaned it up a bit but only added deprecation messages for entity manager related changes. It does result in quite a bit of cruft though.
7. Well, actually, __get() must be public, so this doesn't change anything. And we still need to trigger a deprecation message. But updated with the trait.

Tagging for tracking per #2949018-10: [policy, no patch] Make removal of deprecated usages a feature release blocker. Please correct me if there's a different tag we should use for this.

Reroll. I'd love to finally get this in, it frequently conflicts, for example most recently with #2976103: Make it possible to retrieve all the last installed entity type definitions at once from the update manager which injected one of the several needed dependencies into EntityDefinitionUpdateManager.

@effulgentsia: Yeah, that tag make sense, also commented over there. This itself and various other issues is part/prerequisite of #2886622: Deprecate all EntityManager methods with E_USER_DEPRECATED.

Soooo close...

+++ b/core/lib/Drupal/Core/DependencyInjection/DeprecatedServicePropertyTrait.php
@@ -0,0 +1,30 @@
+trait DeprecatedServicePropertyTrait {

This needs its own CR, so we can tell people how to use it, which means adding the URL to the logic exception message.

1. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -120,7 +131,15 @@ public function loadConfigEntityByName($name) {
   +    @trigger_error('ConfigManagerInterface::getEntityManager() is deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0. Use ::getEntityTypeManager() instead. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);
   
   +++ b/core/lib/Drupal/Core/Config/ConfigManagerInterface.php
   @@ -34,14 +34,26 @@ public function loadConfigEntityByName($name);
   +   * @deprecated in Drupal 8.6.x, will be removed before Drupal 9.0.0. Use
   
   +++ b/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php
   @@ -31,19 +59,33 @@ class EntityDefinitionUpdateManager implements EntityDefinitionUpdateManagerInte
   +      @trigger_error('Passing the entity.manager service to EntityDefinitionUpdateManager::__construct() is deprecated in Drupal 8.6.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);
   
   +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -82,13 +98,25 @@ class EntityConverter implements ParamConverterInterface {
   +      @trigger_error('Passing the entity.manager service to EntityConverter::__construct() is deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0. Pass the entity_type.manager service instead. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);
   

   I think we need to change these messages to mention 8.7.0 instead.

2. +++ b/core/lib/Drupal/Core/Config/ConfigManagerInterface.php
   @@ -34,14 +34,26 @@ public function loadConfigEntityByName($name);
   -   *   The entity manager.
   +   *   The entity config factory.
   

   Isn't this just the config factory? Without the "entity" part :)

3. +++ b/core/lib/Drupal/Core/DependencyInjection/DeprecatedServicePropertyTrait.php
   @@ -0,0 +1,30 @@
   +  public function __get($name) {
   +
   +    if (!isset($this->deprecatedProperties)) {
   ...
   +    }
   +
   +  }
   

   We could remove these blank newlines.

4. +++ b/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php
   @@ -2,24 +2,52 @@
   +  use StringTranslationTrait, DeprecatedServicePropertyTrait;
   

   Don't we usually put these on multiple lines?

5. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -82,13 +98,25 @@ class EntityConverter implements ParamConverterInterface {
   +      @trigger_error('The entity.repository service must be passed to EntityConverter::__construct() required before Drupal 9.0.0. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);
   

   This sentence sounds a bit weird around EntityConverter::__construct() and required.

Addressed comments in #143.

Still requires CR.

Created https://www.drupal.org/node/3002321, not sure that a link to that belongs into the exception, don't think I've ever seen that done?

Agreed with #145, I think this looks ready now.

+++ b/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php
@@ -31,19 +59,33 @@ class EntityDefinitionUpdateManager implements EntityDefinitionUpdateManagerInte
-  public function __construct(EntityManagerInterface $entity_manager, EntityLastInstalledSchemaRepositoryInterface $entity_last_installed_schema_repository = NULL) {
...
+      $this->entityLastInstalledSchemaRepository = \Drupal::service('entity.last_installed_schema.repository');

Minor nitpick: Why not use $entity_last_installed_schema_repository as that one remains unchanged and is passed in both pre- and post-patch?

Not sure I follow, it was there before but it was also optional already then. That additional option argument was added in 8.6 and conflicted in a recent reroll. It's only there because the previous BC approach was different, it always set the variable instead of assigning it, but I removed that part, so it actually isn't guaranteed to be set anymore.

1. +++ b/core/core.services.yml
   @@ -570,12 +570,12 @@ services:
        class: Drupal\Core\Entity\EntityDefinitionUpdateManager
   -    arguments: ['@entity.manager', '@entity.last_installed_schema.repository']
   +    arguments: ['@entity_type.manager', '@entity.last_installed_schema.repository', '@entity_field.manager', '@entity_type.listener', '@field_storage_definition.listener']
      entity.last_installed_schema.repository:
   

   Sets the last installed schema repository both before and after the patch.

2. +++ b/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php
   @@ -31,19 +60,33 @@ class EntityDefinitionUpdateManager implements EntityDefinitionUpdateManagerInte
   -    if (!isset($entity_last_installed_schema_repository)) {
   -      @trigger_error('The $entity_last_installed_schema_repository parameter was added in Drupal 8.6.x and will be required in 9.0.0. See https://www.drupal.org/node/2973262.', E_USER_DEPRECATED);
   -      $entity_last_installed_schema_repository = \Drupal::service('entity.last_installed_schema.repository');
   

   Here, the optional-because-of-BC variable is populated when left out.

3. +++ b/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php
   @@ -31,19 +60,33 @@ class EntityDefinitionUpdateManager implements EntityDefinitionUpdateManagerInte
   +    if ($entity_type_manager instanceof EntityManagerInterface) {
   +      @trigger_error('Passing the entity.manager service to EntityDefinitionUpdateManager::__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();
   +      $this->entityFieldManager = \Drupal::service('entity_field.manager');
   +      $this->entityLastInstalledSchemaRepository = \Drupal::service('entity.last_installed_schema.repository');
   +      $this->entityTypeListener = \Drupal::service('entity_type.listener');
   +      $this->fieldStorageDefinitionListener = \Drupal::service('field_storage_definition.listener');
   +    }
   +    else {
   +      $this->entityTypeManager = $entity_type_manager;
   +      $this->entityFieldManager = $entity_field_manager;
   +      $this->entityLastInstalledSchemaRepository = $entity_last_installed_schema_repository;
   +      $this->entityTypeListener = $entity_type_listener;
   +      $this->fieldStorageDefinitionListener = $field_storage_definition_listener;
   

   But here, that deprecation notice is gone and we assume people did not pass the repository if they are still passing the old entity manager. A fair assumption to make, but it seems safer to keep both deprecation warnings?

Edit: I'm thinking of the outside chance someone updated their code after 8.6.0 to properly pass in the repository and now they are not seeing the passed in repository being used because it is replaced with the one from the container. Very much an edge case so feel free to handle this whichever way your judgement tells you to.

Agreed with @kristiaanvandeneynde , we can compare the hunk above to how EntityConverter is doing it:

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
@@ -82,13 +98,25 @@ class EntityConverter implements ParamConverterInterface {
-    $this->entityManager = $entity_manager;
+  public function __construct(EntityTypeManagerInterface $entity_type_manager, LanguageManagerInterface $language_manager = NULL, EntityRepositoryInterface $entity_repository = NULL) {
+    if ($entity_type_manager instanceof EntityManagerInterface) {
+      @trigger_error('Passing the entity.manager service to EntityConverter::__construct() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Pass the entity_type.manager service instead. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);
+    }
+    $this->entityTypeManager = $entity_type_manager;
+    if ($entity_repository) {
+      $this->entityRepository = $entity_repository;
+    }
+    else {
+      @trigger_error('The entity.repository service must be passed to EntityConverter::__construct(), it is required before Drupal 9.0.0. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);
+      $this->entityRepository = \Drupal::service('entity.repository');
+    }

So I think EntityDefinitionUpdateManager should follow the same pattern as here - i.e. separate variable checks and deprecation messages.

Otherwise this looks great.

Agreed with #150/#151. A few of my patches depend on this one, so here's a quick update which implements #151.

A bit overly verbose but I guess that's the whole idea with these deprecation notices :) Looks good to me now!

That said, I went over the other constructors again just to make sure we are fully BC and I think there are quite a few which need the same treatment. If I'm wrong, feel free to go right back to RTBC!

1. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -83,21 +91,24 @@ class ConfigManager implements ConfigManagerInterface {
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, ConfigFactoryInterface $config_factory, TypedConfigManagerInterface $typed_config_manager, TranslationInterface $string_translation, StorageInterface $active_storage, EventDispatcherInterface $event_dispatcher, EntityRepositoryInterface $entity_repository = NULL) {
   +    $this->entityTypeManager = $entity_type_manager;
        $this->configFactory = $config_factory;
        $this->typedConfigManager = $typed_config_manager;
        $this->stringTranslation = $string_translation;
        $this->activeStorage = $active_storage;
        $this->eventDispatcher = $event_dispatcher;
   +    $this->entityRepository = $entity_repository ?: \Drupal::service('entity.repository');
      }
   

   ConfigManager is not marked as internal. So don't we need to add the deprecation notice here as well? And use the DeprecatedServicePropertyTrait?

2. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -89,8 +89,8 @@ class DateFormatter implements DateFormatterInterface {
   -  public function __construct(EntityManagerInterface $entity_manager, LanguageManagerInterface $language_manager, TranslationInterface $translation, ConfigFactoryInterface $config_factory, RequestStack $request_stack) {
   -    $this->dateFormatStorage = $entity_manager->getStorage('date_format');
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, LanguageManagerInterface $language_manager, TranslationInterface $translation, ConfigFactoryInterface $config_factory, RequestStack $request_stack) {
   +    $this->dateFormatStorage = $entity_type_manager->getStorage('date_format');
        $this->languageManager = $language_manager;
   

   This will still allow people to pass an entity manager as both managers have a getStorage() method, but I guess that's fine? Or do we need to throw a deprecation notice as well here?

3. +++ b/core/lib/Drupal/Core/Entity/ContentUninstallValidator.php
   @@ -14,22 +14,22 @@ class ContentUninstallValidator implements ModuleUninstallValidatorInterface {
   -  public function __construct(EntityManagerInterface $entity_manager, TranslationInterface $string_translation) {
   -    $this->entityManager = $entity_manager;
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, TranslationInterface $string_translation) {
   +    $this->entityTypeManager = $entity_type_manager;
   

   ContentUninstallValidator isn't internal, so do we need to throw a warning here too? And use the DeprecatedServicePropertyTrait?

4. +++ b/core/lib/Drupal/Core/Entity/EntityCreateAccessCheck.php
   @@ -30,11 +30,11 @@ class EntityCreateAccessCheck implements AccessInterface {
   -   * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
   -   *   The entity manager.
   +   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   +   *   The entity type manager service.
       */
   -  public function __construct(EntityManagerInterface $entity_manager) {
   -    $this->entityManager = $entity_manager;
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager) {
   +    $this->entityTypeManager = $entity_type_manager;
   

   EntityCreateAccessCheck isn't internal, so same comment re warning and trait.

5. +++ b/core/lib/Drupal/Core/Entity/EntityFormBuilder.php
   @@ -27,13 +27,13 @@ class EntityFormBuilder implements EntityFormBuilderInterface {
   -  public function __construct(EntityManagerInterface $entity_manager, FormBuilderInterface $form_builder) {
   -    $this->entityManager = $entity_manager;
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, FormBuilderInterface $form_builder) {
   +    $this->entityTypeManager = $entity_type_manager;
        $this->formBuilder = $form_builder;
   

   EntityFormBuilder isn't internal, so same comment re warning and trait.

6. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -30,13 +30,13 @@ class EntityResolverManager {
   -  public function __construct(EntityManagerInterface $entity_manager, ClassResolverInterface $class_resolver) {
   -    $this->entityManager = $entity_manager;
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, ClassResolverInterface $class_resolver) {
   +    $this->entityTypeManager = $entity_type_manager;
   

   EntityResolverManager isn't internal, so same comment re warning and trait.

7. +++ b/core/lib/Drupal/Core/Entity/Event/BundleConfigImportValidate.php
   @@ -21,23 +21,23 @@ class BundleConfigImportValidate extends ConfigImportValidateEventSubscriberBase
   -  public function __construct(ConfigManagerInterface $config_manager, EntityManagerInterface $entity_manager) {
   +  public function __construct(ConfigManagerInterface $config_manager, EntityTypeManagerInterface $entity_type_manager) {
        $this->configManager = $config_manager;
   -    $this->entityManager = $entity_manager;
   +    $this->entityTypeManager = $entity_type_manager;
   

   BundleConfigImportValidate isn't internal, so same comment re warning and trait.

8. +++ b/core/lib/Drupal/Core/Entity/HtmlEntityFormController.php
   @@ -26,12 +26,12 @@ class HtmlEntityFormController extends FormController {
   -  public function __construct(ArgumentResolverInterface $argument_resolver, FormBuilderInterface $form_builder, EntityManagerInterface $manager) {
   +  public function __construct(ArgumentResolverInterface $argument_resolver, FormBuilderInterface $form_builder, EntityTypeManagerInterface $entity_type_manager) {
        parent::__construct($argument_resolver, $form_builder);
   -    $this->entityManager = $manager;
   +    $this->entityTypeManager = $entity_type_manager;
   

   HtmlEntityFormController isn't internal, so same comment re warning and trait.

9. +++ b/core/lib/Drupal/Core/Entity/Query/QueryFactory.php
   @@ -26,20 +26,20 @@ class QueryFactory implements ContainerAwareInterface {
   -  public function __construct(EntityManagerInterface $entity_manager) {
   -    $this->entityManager = $entity_manager;
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager) {
   +    $this->entityTypeManager = $entity_type_manager;
   

   QueryFactory isn't internal, so same comment re warning and trait.

10. +++ b/core/lib/Drupal/Core/EventSubscriber/EntityRouteProviderSubscriber.php
    @@ -14,20 +14,20 @@
    -  public function __construct(EntityManagerInterface $entity_manager) {
    -    $this->entityManager = $entity_manager;
    +  public function __construct(EntityTypeManagerInterface $entity_type_manager) {
    +    $this->entityTypeManager = $entity_type_manager;
    

    EntityRouteProviderSubscriber isn't internal, so same comment re warning and trait.

11. +++ b/core/lib/Drupal/Core/Menu/MenuParentFormSelector.php
    @@ -24,25 +24,25 @@ class MenuParentFormSelector implements MenuParentFormSelectorInterface {
    -    $this->entityManager = $entity_manager;
    +    $this->entityTypeManager = $entity_type_manager;
    

    MenuParentFormSelector isn't internal, so same comment re warning and trait.

Ha, that was very thorough :p At least that stuff can mostly be copy & pasted.

2. Left it at that. At some point, we'll add a @trigger_error to all entity manager methods, if someone really still passes entity.manager, then it will do a deprecation message then.
9. QueryFactory is completely deprecated, not bothering to add another a check there, it will go away and if someone still uses that then they already get deprecation messages. Did add the trait through to be extra thorough.

Added both the constructor check and the trait everywhere else.

Looks great :)

Agreed, sorry for having so many nitpicks at the eleventh hour :D

Committed ac63952 and pushed to 8.7.x. Thanks!