Properly deprecate entity_load() and friends

Adding patch.

$ git apply 2723593_1-6.patch 
error: patch failed: core/modules/config/src/Tests/ConfigInstallWebTest.php:185
error: core/modules/config/src/Tests/ConfigInstallWebTest.php: patch does not apply

Rerolled.

~Chirag

Patch applies to 8.3.x. Superficially removes all calls to entity_load('config_test').

Re-running tests to see if they magically pass.

Un-assigning chishah92. Please re-assign yourself if you'd like.

Hopefully this will fix all the testbot errors.

+++ b/core/modules/config/src/Tests/ConfigEntityStatusUITest.php
@@ -31,7 +31,9 @@ function testCRUD() {
-    $entity = entity_load('config_test', $id);
+    $storage = $this->container->get('entity_type.manager')->getStorage('config_test');
+    $storage->resetCache([$id]);
+    $entity = $storage->load($id);

+++ b/core/modules/config/src/Tests/ConfigEntityTest.php
@@ -315,7 +315,9 @@ function testCRUDUI() {
-    $this->assertFalse(entity_load('config_test', '0'), 'Test entity deleted');
+    $storage = $this->container->get('entity_type.manager')->getStorage('config_test');
+    $storage->resetCache([0]);
+    $this->assertFalse($storage->load(0), 'Test entity deleted');

@@ -352,7 +354,8 @@ function testCRUDUI() {
-    $entity = entity_load('config_test', $id);
+    $storage->resetCache([$id]);
+    $entity = $storage->load($id);

+++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
@@ -185,7 +185,9 @@ public function testUnmetDependenciesInstall() {
-    $this->assertTrue(entity_load('config_test', 'other_module_test_with_dependency'), 'The config_test.dynamic.other_module_test_with_dependency configuration has been created during install.');
+    $storage = $this->container->get('entity_type.manager')->getStorage('config_test');
+    $storage->resetCache(['other_module_test_with_dependency']);
+    $this->assertTrue($storage->load('other_module_test_with_dependency'), 'The config_test.dynamic.other_module_test_with_dependency configuration has been created during install.');

+++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigEntityStatusTest.php
@@ -35,7 +35,9 @@ function testCRUD() {
-    $entity = entity_load('config_test', $entity->id());
+    $storage = $this->container->get('entity_type.manager')->getStorage('config_test');
+    $storage->resetCache([$entity->id()]);
+    $entity = $storage->load($entity->id());

This is changing the test - it did not do a reset before why now? It is wrong to change what is tested in these patches.

Patch no longer applies.

Re-rolled.

Removed calls to $storage->resetCache(), as per @alexpott's comments in #17.

Although it should be noted that the same method has been accepted into core in Drupal\Tests\config\Functional\ConfigOtherModuleTest::testUninstall().

Entity->load() returns NULL (not FALSE) when an entity is not found.

@Jo Fitzgerald's patch in #25 no longer applied to 8.6.x. A reroll with no further modifications is attached.

Compared the patch with the patch in #15 and it looks like I made a mistake in my re-roll in #21.

Failure appears to have been a testbot bug.

+++ b/core/modules/config/tests/src/Functional/ConfigOtherModuleTest.php
@@ -26,7 +26,8 @@ public function testInstallOtherModuleFirst() {
-    $this->assertTrue(entity_load('config_test', 'other_module_test', TRUE), 'Default configuration has been installed.');
+    $storage = $this->container->get('entity_type.manager')->getStorage('config_test');
+    $this->assertTrue($storage->load('other_module_test'), 'Default configuration has been installed.');

@@ -37,7 +38,8 @@ public function testInstallOtherModuleFirst() {
-    $other_module_config_entity = entity_load('config_test', 'other_module_test', TRUE);
+    $storage = $this->container->get('entity_type.manager')->getStorage('config_test');
+    $other_module_config_entity = $storage->load('other_module_test');

@@ -47,10 +49,12 @@ public function testInstallOtherModuleFirst() {
-    $this->assertTrue(entity_load('config_test', 'other_module_test', TRUE), 'Default configuration for other modules is not removed when the module that provides it is uninstalled.');
+    $storage = $this->container->get('entity_type.manager')->getStorage('config_test');
+    $this->assertTrue($storage->load('other_module_test'), 'Default configuration for other modules is not removed when the module that provides it is uninstalled.');
...
-    $this->assertTrue(entity_load('config_test', 'dotted.default', TRUE), 'The configuration is not deleted.');
+    $storage = $this->container->get('entity_type.manager')->getStorage('config_test');
+    $this->assertTrue($storage->load('dotted.default'), 'The configuration is not deleted.');

@@ -59,15 +63,16 @@ public function testInstallOtherModuleFirst() {
-    $this->assertNull(entity_load('config_test', 'other_module_test_unmet', TRUE), 'The optional configuration config_test.dynamic.other_module_test_unmet whose dependencies are not met is not created.');
-    $this->assertNull(entity_load('config_test', 'other_module_test_optional_entity_unmet', TRUE), 'The optional configuration config_test.dynamic.other_module_test_optional_entity_unmet whose dependencies are not met is not created.');
-    $this->installModule('config_test_language');
+    $storage = $this->container->get('entity_type.manager')->getStorage('config_test');
+    $this->assertNull($storage->load('other_module_test_unmet'), 'The optional configuration config_test.dynamic.other_module_test_unmet whose dependencies are not met is not created.');
+    $this->assertNull($storage->load('other_module_test_optional_entity_unmet'), 'The optional configuration config_test.dynamic.other_module_test_optional_entity_unmet whose dependencies are not met is not created.');    $this->installModule('config_test_language');
...
-    $this->assertTrue(entity_load('config_test', 'other_module_test_unmet', TRUE), 'The optional configuration config_test.dynamic.other_module_test_unmet whose dependencies are met is now created.');
+    $storage = $this->container->get('entity_type.manager')->getStorage('config_test');
+    $this->assertTrue($storage->load('other_module_test_unmet'), 'The optional configuration config_test.dynamic.other_module_test_unmet whose dependencies are met is now created.');
...
-    $this->assertNull(entity_load('config_test', 'other_module_test_optional_entity_unmet', TRUE), 'The optional configuration config_test.dynamic.other_module_test_optional_entity_unmet whose dependencies are met is not created.');
+    $this->assertNull($storage->load('other_module_test_optional_entity_unmet'), 'The optional configuration config_test.dynamic.other_module_test_optional_entity_unmet whose dependencies are met is not created.');

@@ -75,10 +80,12 @@ public function testInstallOtherModuleFirst() {
-    $this->assertFalse(entity_load('config_test', 'other_module_test', TRUE), 'Default configuration provided by config_other_module_config_test does not exist.');
+    $storage = $this->container->get('entity_type.manager')->getStorage('config_test');
+    $this->assertFalse($storage->load('other_module_test'), 'Default configuration provided by config_other_module_config_test does not exist.');
...
-    $this->assertTrue(entity_load('config_test', 'other_module_test', TRUE), 'Default configuration provided by config_other_module_config_test has been installed.');
+    $storage = $this->container->get('entity_type.manager')->getStorage('config_test');
+    $this->assertTrue($storage->load('other_module_test'), 'Default configuration provided by config_other_module_config_test has been installed.');

OK, so the function signature of entity_load() looks like this:

/**
 * Loads an entity from the database.
 *
 * @param string $entity_type
 *   The entity type to load, e.g. node or user.
 * @param mixed $id
 *   The id of the entity to load.
 * @param bool $reset
 *   Whether to reset the internal cache for the requested entity type.
 *
 * @return \Drupal\Core\Entity\EntityInterface|null
 *   The entity object, or NULL if there is no entity with the given ID.
[..]
 */
function entity_load($entity_type, $id, $reset = FALSE) {

If $reset is FALSE (which it is by default), then our modifications shouldn't reset the cache, like @alexpott points out in #17.

However, If $reset is TRUE, then the changes should reset the cache in order to be like the original test code.

So please reset the cache for the tests with TRUE passed in for $reset.

Re-instate the cache resets removed in error.

No longer applies.

Re-rolled

Let's rescope this issue into something that will eventually deliver the same change but do so in a way that means we won't add anymore usages in. See https://www.drupal.org/core/scope for why the current scope doesn't really work. So let's scope this to deal with properly deprecating entity_load()

Patch replaces all entity_load() calls in core.

1. +++ b/core/modules/config/tests/src/Functional/ConfigEntityStatusUITest.php
   @@ -31,7 +31,8 @@ public function testCRUD() {
    
   -    $entity = entity_load('config_test', $id);
   +    $storage = $this->container->get('entity_type.manager')->getStorage('config_test');
   +    $entity = $storage->load($id);
    
   
   +++ b/core/modules/config/tests/src/Functional/ConfigEntityTest.php
   @@ -316,7 +316,8 @@ public function testCRUDUI() {
        $this->drupalPostForm('admin/structure/config_test/manage/0/delete', [], 'Delete');
   -    $this->assertFalse(entity_load('config_test', '0'), 'Test entity deleted');
   +    $storage = $this->container->get('entity_type.manager')->getStorage('config_test');
   +    $this->assertNull($storage->load(0), 'Test entity deleted');
    
   

   I think we pretty much agreed on not using $this->container anymore in tests? Update to \Drupal:entityTypeManager()?

   quite a few more of these below.

2. +++ b/core/modules/config/tests/src/Functional/ConfigOtherModuleTest.php
   @@ -59,17 +68,22 @@ public function testInstallOtherModuleFirst() {
        // installed once all the dependencies are met.
   -    $this->assertNull(entity_load('config_test', 'other_module_test_unmet', TRUE), 'The optional configuration config_test.dynamic.other_module_test_unmet whose dependencies are not met is not created.');
   -    $this->assertNull(entity_load('config_test', 'other_module_test_optional_entity_unmet', TRUE), 'The optional configuration config_test.dynamic.other_module_test_optional_entity_unmet whose dependencies are not met is not created.');
   -    $this->installModule('config_test_language');
   -    $this->assertNull(entity_load('config_test', 'other_module_test_optional_entity_unmet2', TRUE), 'The optional configuration config_test.dynamic.other_module_test_optional_entity_unmet2 whose dependencies are not met is not created.');
   +    $storage = $this->container->get('entity_type.manager')->getStorage('config_test');
   +    $storage->resetCache(['other_module_test_unmet']);
   +    $this->assertNull($storage->load('other_module_test_unmet'), 'The optional configuration config_test.dynamic.other_module_test_unmet whose dependencies are not met is not created.');
   +    $storage->resetCache(['other_module_test_optional_entity_unmet']);
   +    $this->assertNull($storage->load('other_module_test_optional_entity_unmet'), 'The optional configuration config_test.dynamic.other_module_test_optional_entity_unmet whose dependencies are not met is not created.');    $this->installModule('config_test_language');
   +    $storage->resetCache(['other_module_test_optional_entity_unmet']);
   +    $this->assertNull($storage->load('other_module_test_optional_entity_unmet'), 'The optional configuration config_test.dynamic.other_module_test_optional_entity_unmet whose dependencies are met is not created.');
        $this->installModule('config_install_dependency_test');
   

   config_test doesn't use a static cache, so those resetCache() calls shouldn't be needed and if we really want to keep them then we could do a single resetCache() without arguments first, should make this more readable again? Like we actually do below on the last call..

@Berdir good points - patch addresses them.

1. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigEntityNormalizeTest.php
   @@ -35,8 +35,9 @@ public function testNormalize() {
        $this->assertNotIdentical($config_entity->toArray(), $config->getRawData(), 'Stored config entity is not is equivalent to config schema.');
   -
   -    $config_entity = entity_load('config_test', 'system', TRUE);
   +    $storage = $this->container->get('entity_type.manager')->getStorage('config_test');
   +    $storage->resetCache(['system']);
   +    $config_entity = $storage->load('system');
        $config_entity->save();
    
   

   another $this->container, this is a kernel test, so this definitely doesn't require a cache clear either.

   (Btw, I had the idea to open an issue to reset the global entity static cache service after POST requests, just like we reset config and other things.. that would probably remove about 90% of the cases where we need resetCache() in tests..)

2. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigEntityStorageTest.php
   @@ -47,7 +47,8 @@ public function testUUIDConflict() {
    
        // Ensure that the config entity was not corrupted.
   -    $entity = entity_load('config_test', $entity->id(), TRUE);
   +    $storage->resetCache();
   +    $entity = $storage->load($entity->id());
        $this->assertIdentical($entity->toArray(), $original_properties);
   

   I guess this is the one that fails, $storage probably doesn't exist here? or dies it above?

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityRevisionTranslationTest.php
   @@ -178,12 +179,14 @@ public function testSetNewRevision() {
          $entity_id = $entity->id();
          $entity_rev_id = $entity->getRevisionId();
   -      $entity = entity_load($entity_type, $entity_id, TRUE);
   +      $storage->resetCache([$entity_id]);
   +      $entity = $storage->load($entity_id);
   

   another kernel tes twith resetCache(), only reason it would be required is if we'd somehow change the stored data by hand above, but it looks like a regular save based on the visible context.

1. Fixed
2. $storage is defined above
3. It doing this in foreach (entity_test_entity_types(ENTITY_TEST_TYPES_REVISABLE) as $entity_type) { I guess we can swap to loadUnchanged() and then we have to think about but we know we're dealing with what is in the DB. I guess we could use loadUnchanged() in a couple of other places - ie after // Ensure that the config entity was not corrupted.

Looks good, just needs an explicit deprecation test and this should be done. Unless we also want to deprecate node_load() and user_load() which both are usage-free in core so they'd be easy. Found editor_load() as a somewhat strange case that still has a few usages.

I think as node_load() and user_load() are not in entity.inc and have no usages they are outside this scope. We can do them in yet-another-issue.

Added a test.

Works for me, just mentioned that because that's what you did with the _load_multiple() functions :)

@Berdir good point - well made :)

I think as editor_load() has usages, is a bit different and is not deprecated we shouldn't touch it here. Done the reset and added tests.

Missed some... even test entity types have load functions! Deprecated them for good measure but didn't add tests of the test functions.

1. +++ b/core/modules/file/file.module
   @@ -102,13 +102,13 @@ function file_load_multiple(array $fids = NULL, $reset = FALSE) {
     */
    function file_load($fid, $reset = FALSE) {
   +  @trigger_error('file_load() is deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0. Use \Drupal\file\Entity\File::load(). See https://www.drupal.org/node/2266845', E_USER_DEPRECATED);
      if ($reset) {
        \Drupal::entityManager()->getStorage('file')->resetCache([$fid]);
   

   hm, the file load functions recommend File::load*() while the others recommend the entity storage load method.

2. +++ b/core/modules/node/node.module
   @@ -468,10 +471,13 @@ function node_load_multiple(array $nids = NULL, $reset = FALSE) {
     */
    function node_load($nid = NULL, $reset = FALSE) {
   +  @trigger_error('node_load() is deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0. Use \Drupal\node\Entity\Node::load(). See https://www.drupal.org/node/2266845', E_USER_DEPRECATED);
   

   oh well, so does node_load() and many others. I guess the argument is that an entity_load() was likely dynamic and couldn't be hardcoded like that.

3. +++ b/core/modules/system/tests/modules/entity_test/entity_test.module
   @@ -361,8 +361,14 @@ function entity_test_form_node_form_alter(&$form, FormStateInterface $form_state
     */
    function entity_test_load($id, $reset = FALSE) {
   +  @trigger_error('entity_test_load() is deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0. Use \Drupal::entityTypeManager()->getStorage(\'entity_test\')->load(). See https://www.drupal.org/node/2266845', E_USER_DEPRECATED);
      $storage = \Drupal::entityTypeManager()->getStorage('entity_test');
   

   I think it would have been OK to just remove those :)

Looks good to me.

Committed a3a1663 and pushed to 8.7.x. Thanks!