Problem/Motivation

\Drupal::entityManager() is deprecated, because the entity_manager service itself is deprecated.

The Entity base class however still uses the entity_manager service.

We should change it to use the non-deprecated entity services.

Proposed resolution

Use the entity_type.manager and other non-deprecated services for these methods.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Mile23’s picture

Issue summary: View changes

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mile23’s picture

Title: Drupal\Core\Entity\Entity::load() etc. use deprecated \Drupal::entityManager() » Remove usages of deprecated EntityManager from Entity class
Issue summary: View changes
Status: Active » Needs review
FileSize
11.64 KB

Rescoping.

First pass focusing on the static factory methods (load, loadAll, create).

Unit tests pass... Let's see what fails.

Mile23’s picture

Mile23’s picture

All done, for unit tests anyway. Testbot may have different opinion.

Mile23’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 6: 2782833_6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Mile23’s picture

Status: Needs work » Needs review
FileSize
69.82 KB
1.28 KB

Not sure how that change to ConfigManager got in...

Also fixing the coding standards error.

martin107’s picture

Status: Needs review » Reviewed & tested by the community

I like this issue, as it moves the codebase towards D9.

After a slow a steady visual scan of the patch :-

Everything looks ok, all changes are directly related to the issue.

+1 from me.

PS

'There are a few conversions of strings like 'EntityManager' to semantically meaningful string like EntityManager:class.
but to my way of thinking as long as those changed are tightly coupled to the task at hand then it is a welcome 'best practise' update.

Mile23’s picture

Yes, I only changed to the EntityManagerInterface::class pattern where I was making changes.

There's another pattern in the test mocks that might need some explanation:

EntityManager calls out to non-deprecated services, so rather than mock EntityManager in parallel, I'd make an instance of EntityManger which I register as a service and then inject with the container, so it can use the other services. This allows it to use the other mocked services, so we don't have to worry about two sets of mocks, for code that ends up using EntityManager. You can see this in the patch changes for ContentEntityBaseUnitTest.

In the future, when those other classes (such as ContentEntityBase) remove their usages of EntityManager, the tests should still work and pass.

Then, when EntityManager eventually triggers E_USER_DEPRECATED after #2886622: Deprecate all EntityManager methods with E_USER_DEPRECATED, the tests will fail only because the tests are using deprecated classes. Ideally. :-) At that time we can just remove EntityManager from the mocking pattern.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work

Overall, this patch looks great. Thank you!

  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -508,24 +518,30 @@ public function getCacheMaxAge() {
    +    $entity_repository = \Drupal::service('entity_type.repository');
    

    Here and elsewhere in the patch, the local variable should be named $entity_type_repository, since this is the 'entity_type.repository' service, rather than the 'entity.repository' service.

  2. +++ b/core/modules/editor/tests/src/Unit/EditorConfigEntityUnitTest.php
    @@ -78,10 +80,16 @@ protected function setUp() {
    +    $entity_manager = new EntityManager();
         $container = new ContainerBuilder();
    +    $container->set('entity.manager', $entity_manager);
    +    // Inject the container into entity.manager so it can defer to
    +    // entity_type.manager.
    +    $entity_manager->setContainer($container);
    

    Since this pattern is duplicated in a bunch of tests, what do you think of adding a protected method to UnitTestCase? Maybe named getContainerWithEntityManager() (similar to getContainerWithCacheTagsInvalidator)?

  3. +++ b/core/modules/language/tests/src/Unit/ContentLanguageSettingsUnitTest.php
    @@ -254,16 +269,20 @@ public function testLoadByEntityTypeBundle($config_id, ContentLanguageSettings $
    +    \Drupal::getContainer()->set('entity_type.repository', $entity_type_repository);
    

    Here and elsewhere in the patch, can we move this to the setUp() method to avoid a \Drupal::getContainer() call within a test?

Mile23’s picture

Status: Needs work » Needs review
FileSize
69.89 KB
2.87 KB

Thanks.

1. Well spotted, thanks.

2. Adding methods to UnitTestCase for mocks which are directly related to the code under test isn't such a great habit to get into. Because if you look closely at those different mocked containers, they're not all the same... You'd have to have local properties for them and modify their expectations anyway. Also, we already have KernelTestBase, but these tests are already UnitTestCase tests.

3. I tried to stay consistent with the existing test. Refactoring the test that way loses the value of the existing test, since we might be changing the expectations of the test without really understanding it. For this issue, we're only changing Entity, which then changes some expectations for mocking, but tests like ContentLanguageSettingsUnitTest have a whole bunch of other expectations regarding an Entity subclasses. So do we change it in place which is relatively easy to write and review, and is also consistent? Or do we essentially re-write the test for a class which is out of scope for this issue?

Also, I could have just changed those Entity subclasses to use the non-deprecated services, but that's out of scope here, too.

And also: We'll be undoing many of these changes anyway, as EntityManager is more fully deprecated from other entity base classes, and when it starts throwing E_USER_DEPRECATED.

martin107’s picture

Status: Needs review » Reviewed & tested by the community

I think

#13 is a good response to all the issue raised in #12.

All the appropriate changes have been made to the patch .. an all the other issues raised defended ... in a very constructive way .. it is good to see differences of approach aired in the issue queue...

I dont want this issue to stall --- I think we are at the stage where committing this patch will be a positive step forward.

I appreciate D9 is at best 12 months away but once we get this into core, as Mile23, has pointed out in other issues there is alot of other conversions to be done for example EntityViewBuilder::__construct() and all it offspring such as BlockViewBuilder.

public function __construct(EntityTypeInterface $entity_type, EntityManagerInterface $entity_manager, LanguageManagerInterface $language_manager, ModuleHandlerInterface $module_handler) {

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 2782833_13.patch, failed testing. View results

Mile23’s picture

Status: Needs work » Reviewed & tested by the community

Well that's weird... The failing test results are here: https://www.drupal.org/pift-ci-job/725604

It's a fail on ContentModerationWorkflowConfigTest which is a brand-new test and not touched by this patch.

I can't repro the fail locally. Re-running the test here and setting to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 2782833_13.patch, failed testing. View results

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

michaellenahan’s picture

Status: Needs work » Reviewed & tested by the community

I re-ran the test, it's green now: https://www.drupal.org/pift-ci-job/764188

I guess this means we can go back to RTBC?

Mile23’s picture

We just need to update the testbot to run against 8.5.x.

xjm’s picture

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -89,6 +89,15 @@ protected function entityTypeManager() {
+    return \Drupal::service('entity_type.bundle.info');

@@ -508,24 +518,30 @@ public function getCacheMaxAge() {
-    $entity_manager = \Drupal::entityManager();
-    return $entity_manager->getStorage($entity_manager->getEntityTypeFromClass(get_called_class()))->load($id);
+    $entity_type_repository = \Drupal::service('entity_type.repository');
+    $entity_type_manager = \Drupal::entityTypeManager();

I definitely did not expect \Drupal::service() hardcoded like this, but I guess HEAD already does that with the entity manager service itself.

larowlan’s picture

#2053415: Allow typed data plugins to receive injected dependencies is the issue to allow DI in typed data, so \Drupal is all we have here.

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -508,24 +518,30 @@ public function getCacheMaxAge() {
+    $entity_type_repository = \Drupal::service('entity_type.repository');
+    $entity_type_manager = \Drupal::entityTypeManager();
+    $storage = $entity_type_manager->getStorage($entity_type_repository->getEntityTypeFromClass(get_called_class()));
...
+    $entity_type_repository = \Drupal::service('entity_type.repository');
+    $entity_type_manager = \Drupal::entityTypeManager();
+    $storage = $entity_type_manager->getStorage($entity_type_repository->getEntityTypeFromClass(get_called_class()));
...
+    $entity_type_repository = \Drupal::service('entity_type.repository');
+    $entity_type_manager = \Drupal::entityTypeManager();
+    $storage = $entity_type_manager->getStorage($entity_type_repository->getEntityTypeFromClass(get_called_class()));

feels like we need a protected function getStorageHandler but that would be a follow up - can someone create that?

In terms of the duplication in tests, agree that we shouldn't add to UnitTestCase, if anything a trait would help - but that's also an out of scope follow up

Mile23’s picture

#2053415: Allow typed data plugins to receive injected dependencies is the issue to allow DI in typed data, so \Drupal is all we have here.

+    $entity_type_repository = \Drupal::service('entity_type.repository');
+    $entity_type_manager = \Drupal::entityTypeManager();
+    $storage = $entity_type_manager->getStorage($entity_type_repository->getEntityTypeFromClass(get_called_class()));

feels like we need a protected function getStorageHandler but that would be a follow up - can someone create that?

Those are called from static methods, or we'd use Entity::entityTypeManager() and then get the storage. It's also the real reason we're stuck with \Drupal.

Having a protected/private static would aid with DRY though. I'm pretty sure this is not the place to change Entity to have a service accessor, so here's the follow up: #2911162: Refactor Entity static methods for DRY

larowlan’s picture

thanks @Mile23 - and yeah missed the fact they were static.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3bc858e and pushed to 8.5.x. Thanks!

  • catch committed 3bc858e on 8.5.x
    Issue #2782833 by Mile23, martin107, larowlan, effulgentsia: Remove...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.