Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#13 | interdiff.txt | 2.87 KB | Mile23 |
#13 | 2782833_13.patch | 69.89 KB | Mile23 |
#9 | interdiff.txt | 1.28 KB | Mile23 |
#9 | 2782833_9.patch | 69.82 KB | Mile23 |
#6 | interdiff.txt | 66.08 KB | Mile23 |
Comments
Comment #2
Mile23Comment #4
Mile23Rescoping.
First pass focusing on the static factory methods (load, loadAll, create).
Unit tests pass... Let's see what fails.
Comment #5
Mile23Comment #6
Mile23All done, for unit tests anyway. Testbot may have different opinion.
Comment #7
Mile23Comment #9
Mile23Not sure how that change to ConfigManager got in...
Also fixing the coding standards error.
Comment #10
martin107 CreditAttribution: martin107 as a volunteer commentedI 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.
Comment #11
Mile23Yes, 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 mockEntityManager
in parallel, I'd make an instance ofEntityManger
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 usingEntityManager
. You can see this in the patch changes forContentEntityBaseUnitTest
.In the future, when those other classes (such as
ContentEntityBase
) remove their usages ofEntityManager
, the tests should still work and pass.Then, when
EntityManager
eventually triggersE_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 removeEntityManager
from the mocking pattern.Comment #12
effulgentsia CreditAttribution: effulgentsia at Acquia commentedOverall, this patch looks great. Thank you!
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.Since this pattern is duplicated in a bunch of tests, what do you think of adding a protected method to
UnitTestCase
? Maybe namedgetContainerWithEntityManager()
(similar togetContainerWithCacheTagsInvalidator
)?Here and elsewhere in the patch, can we move this to the
setUp()
method to avoid a\Drupal::getContainer()
call within a test?Comment #13
Mile23Thanks.
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 haveKernelTestBase
, but these tests are alreadyUnitTestCase
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 likeContentLanguageSettingsUnitTest
have a whole bunch of other expectations regarding anEntity
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 throwingE_USER_DEPRECATED
.Comment #14
martin107 CreditAttribution: martin107 as a volunteer commentedI 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.
Comment #16
Mile23Well 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.
Comment #19
michaellenahan CreditAttribution: michaellenahan at erdfisch commentedI 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?
Comment #20
Mile23We just need to update the testbot to run against 8.5.x.
Comment #21
xjmI definitely did not expect
\Drupal::service()
hardcoded like this, but I guess HEAD already does that with the entity manager service itself.Comment #22
larowlan#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.
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
Comment #23
Mile23Those 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
Comment #24
larowlanthanks @Mile23 - and yeah missed the fact they were static.
Comment #25
catchCommitted 3bc858e and pushed to 8.5.x. Thanks!