Replace deprecated usage of entity_create('config_test') with a direct call to ConfigTest::create()

Xano is wrong. https://api.drupal.org/api/drupal/core%21includes%21entity.inc/function/...

Use The method overriding Entity::create() for the entity type, e.g. \Drupal\node\Entity\Node::create() if the entity type is known.

Depends on what you're testing.

In this case, the test depends on a test module which implements the entity. If we're testing the service discovery mechanism to that entity, then we'd say something like $this->container->get('entity_type.manager')->getStorage('config_test')->create(). If we're using config_test as a fixture to, for instance, feed into some other system, then we don't need (and probably shouldn't use) the service container or the type manager explicitly.

So in the patch:

+++ b/core/modules/config/src/Tests/ConfigDiffTest.php
@@ -81,7 +81,7 @@ function testDiff() {
     // Test diffing a renamed config entity.
     $test_entity_id = $this->randomMachineName();
-    $test_entity = entity_create('config_test', array(
+    $test_entity = $this->container->get('entity_type.manager')->getStorage('config_test')->create(array(

Here, we only care about the ability of the diff component to accurately diff two entities, so we don't need the rest of it, so it's proper to say EntityTest::create().

But let's look at the create() static used by config_test we'll be calling (It lives in \Drupal\Core\Entity\Entity):

  /**
   * {@inheritdoc}
   */
  public static function create(array $values = array()) {
    $entity_manager = \Drupal::entityManager();
    return $entity_manager->getStorage($entity_manager->getEntityTypeFromClass(get_called_class()))->create($values);
  }

See how it just grabs the deprecated entity manager, gets its storage, and then uses it to create an entity, from the static \Drupal? The SimpleTest test base pokes the same fixture container as $this->container into \Drupal pseudo-global, enabling sloppy code like that. Meaning: Drupal 8 refuses to be opinionated on how you generate your entities, even though it should, as exemplified here: Not only what are we testing, but how can we test it without inadvertently testing a bunch of other things at the same time?

Anyway. @Xano is mostly right in principle, but we'd have to be careful what we're really testing here, rather than explicitly exercising the service container in all cases.

I'd err on the side of using ContainerTest::create() because it's easier to type, and things will break more instructively later.

As for a review: The patch in #5 applies and removes all but one occurrence of entity_create('config_test:

ConfigEntityTest.php	
    $empty = entity_create('config_test');      [position 43:14]	

As we are sticking with the static methods for now. So please reroll the patch from #2

Applying patch #5 and change line 43 for #8

I checked with my IDE and its fine, there's no occurance left.

RTBC?

+++ b/core/modules/config/src/Tests/ConfigDiffTest.php
@@ -81,7 +81,7 @@ function testDiff() {
-    $test_entity = entity_create('config_test', array(
+    $test_entity = $this->container->get('entity_type.manager')->getStorage('config_test')->create(array(

This kind of change still works but we want to use ConfigTest::create() rather than poking around the service container.

We do this because the deprecation notes for entity_create() look like this:

 * @deprecated in Drupal 8.0.x, will be removed before Drupal 9.0.0. Use
 *   The method overriding Entity::create() for the entity type, e.g.
 *   \Drupal\node\Entity\Node::create() if the entity type is known. If the
 *   entity type is variable, use the entity storage's create() method to
 *   construct a new entity:
 * @code
 * \Drupal::entityManager()->getStorage($entity_type)->create($values)
 * @endcode

So:

1. Re-roll #2.
2. Check why #2 failed tests above. (You can click on the test result and see a list of failing tests)
3. Fix those tests locally so they pass.
4. Upload the patch.

Thanks.

Didn't get update on this issue from more than 8days.

So i rerolled with patch.

According to our scope its should be replaced in the following way.

--- a/core/modules/config/src/Tests/ConfigDiffTest.php
+++ b/core/modules/config/src/Tests/ConfigDiffTest.php
@@ -7,6 +7,7 @@
 
 namespace Drupal\config\Tests;
 
+use Drupal\config_test\Entity\ConfigTest;
 use Drupal\simpletest\KernelTestBase;
 
 /**
@@ -81,11 +82,10 @@ function testDiff() {
 
     // Test diffing a renamed config entity.
     $test_entity_id = $this->randomMachineName();
-    $test_entity = entity_create('config_test', array(
+    $test_entity = ConfigTest::create(array(
       'id' => $test_entity_id,
       'label' => $this->randomMachineName(),
-    ));

Here's a patch for it.

Patch no longer applies:

$ git apply config_test-2641518-25.patch 
error: core/modules/system/src/Tests/Entity/ConfigEntityQueryTest.php: No such file or directory

Also it's true that $this->container is preferred for tests.

Some other review stuff:

1. +++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceIntegrationTest.php
   @@ -198,9 +198,9 @@ protected function assertFieldValues($entity_name, $referenced_entities) {
      protected function getTestEntities() {
   -    $config_entity_1 = entity_create('config_test', array('id' => $this->randomMachineName(), 'label' => $this->randomMachineName()));
   +    $config_entity_1 = $this->container->get('entity_type.manager')->getStorage('config_test')->create(array('id' => $this->randomMachineName(), 'label' => $this->randomMachineName()));
        $config_entity_1->save();
   -    $config_entity_2 = entity_create('config_test', array('id' => $this->randomMachineName(), 'label' => $this->randomMachineName()));
   +    $config_entity_2 = $this->container->get('entity_type.manager')->getStorage('config_test')->create(array('id' => $this->randomMachineName(), 'label' => $this->randomMachineName()));
        $config_entity_2->save();
   

   There are probably other places like this where you could get the storage service once and then re-use it, but it's not a huge problem to leave it this way.

2. +++ b/core/modules/file/file.module
   @@ -18,6 +18,7 @@
   +use Drupal\config_test\Entity\ConfigTest;
   
   +++ b/core/modules/file/src/Element/ManagedFile.php
   @@ -16,6 +16,7 @@
   +use Drupal\config_test\Entity\ConfigTest;
   
   +++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
   @@ -21,6 +21,7 @@
   +use Drupal\config_test\Entity\ConfigTest;
   
   +++ b/core/modules/file/tests/file_test/file_test.module
   @@ -9,6 +9,7 @@
   +use Drupal\config_test\Entity\ConfigTest;
   
   +++ b/core/modules/image/image.module
   @@ -11,6 +11,7 @@
   +use Drupal\config_test\Entity\ConfigTest;
   
   +++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
   @@ -15,6 +15,7 @@
   +use Drupal\config_test\Entity\ConfigTest;
   
   +++ b/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
   @@ -14,6 +14,7 @@
   +use Drupal\config_test\Entity\ConfigTest;
   

   Why are we adding use statements but making no other changes in these files?

Rerolled #25.

I will work in points 1 and 2 on #25.

Hi,

patch #21 is works fine as per depricated.

The patch is not applying anymore.

The patch in #31, used the chain function but we are using the Entity static function for keeping the consistency throughout the core.

The patch failed b/c the https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

    $entity_manager = \Drupal::entityManager();
    return $entity_manager->getStorage($entity_manager->getEntityTypeFromClass(get_called_class()))->create($values);

got the multiple definitions of ConfigTest entity.

Any suggested approach how to proceed with it with static functions ? instead of chain methods

Reading the comments above on #34 and #35.
Finally, what we should use?

What's weird about this is that it works in all the other replacements from the meta.

The problem arises during Entity::create(), which ends up calling EntityTypeRepository::getEntityTypeFromClass() based on the calling class.

Somehow config_test can't be discovered properly during this process, from its class name.

However, as pointed out, if you use the plugin type ID, all is well.

Perhaps there's a problem with the plugin definition?

Setting to NR to trigger the testbot.

Removing Novice tag because of the length of the issue.

I upload this patch using the chaining way.
I checked in my IDE all occurrences and everything looks fine.

Does this patch go on the right way?
Thanks!

+1 RTBC
Looks good, passes on tests :)

My first reaction to the current patch is that the issue title doesn't match the current plan (i.e. using the container). I see there was discussion and work done as to whether we should use the container or the static method. It looks like #38 contains explanations. In addition, in the very few other children of the parent I looked at, it was the static method. Is it correct that the static method was causing the test failures? If so, if the container is the way forward, could we add a brief explanation in the issue summary please?

Thanks @Mac_Weber.

I read over the thread here as well as the meta issue, and discussed this with @alexpott and @cilefen. Here's my feedback:

1. Thanks for the summary update. The summary still says the proposed resolution is to use the actual class, though, which contradicts the patch. The title also does not reflect the current patch.
    

2. Regarding:

   as other similar ones with long chain method are already applied to core by now and this one also looks good.

   What is an example? All the ones we looked at use the actual class, which is an architectural decision the framework and release managers made for these conversions.
    

3. Regarding:

   in this specific issue we must use the long chaining method, as the EntityTypeRepository::getEntityTypeFromClass() is expected to work in 90% of the cases, but for the other 10% we should use the long chaining way.

   That's not really an explanation, unfortunately.
    

4. Regarding:

   @cilefen #40 has the explanation I got from other core maintainers

   Who recommended this? I'm fairly certain none of alexpott, catch, nor I would have recommended $this->container in tests since we're all aware of the bugs that have been caused because of it.

@alexpott said regarding this issue:

The reason ConfigTest::create() does not work is because config_test_entity_type_alter() creates a second entity type which means that \Drupal\Core\Entity\EntityTypeRepository::getEntityTypeFromClass() can’t resolve the class - because 2 entity types use the same class. \Drupal::entityTypeManager() would work just as well as $this->container->get(‘entity_type.manager’).

In Kernel tests it is not dangerous - there’s code to keep everything in sync. In WebTestBase / BrowserTestBase there’s nothing to keep the child / parent containers in sync and $this->container can get more out-of-sync than \Drupal

So, we should use \Drupal here rather than introduce a risky pattern. Thanks!

To clarify, we should replace it with ConfigTest::create() except where the alter hook leads to the exception... and try \Drupal::entityTypeManager() only for the calls that would fail. And, possibly, file a followup to give the alter hook its own test implementation so it doesn't pollute other stuff.

The latest patch no longer applies.
Reroll against 8.3.x

I will work on this to fix #49 and #50.

OMG!
Upload again

Looking at #49 and #50 I try to replace all ocurrences to ConfigTest::create(), however when i run the test locally I have some fails, so I try using \Drupal::entityTypeManager(). I'm not sure if we want this, but could be a first approach.

Ups! I forgot check as unassigned.

+++ b/core/modules/config/src/Tests/ConfigEntityTest.php
@@ -35,7 +35,7 @@ class ConfigEntityTest extends WebTestBase {
+    $empty = \Drupal::entityTypeManager()->getStorage('config_test')->create();

Use this pattern for methods that have a lot of calls to entity_create():

$storage = \Drupal::entityTypeManager()->getStorage('config_test');

Then whenever the entity is needed, you'd do this:

$entity = $storage->create([ .. stuff .. ]);

I have changed it in two files where it is using multiple times. Please check whether i have done it correctly?

1. +++ b/core/modules/config_translation/src/Tests/ConfigTranslationOverviewTest.php
   @@ -90,7 +90,7 @@ public function testMapperListPage() {
        foreach ($labels as $label) {
   -      $test_entity = entity_create('config_test', [
   +      $test_entity = \Drupal::entityTypeManager()->getStorage('config_test')->create([
   

   Please move storage retrieval out of loop

2. +++ b/core/modules/field/tests/src/Functional/EntityReference/EntityReferenceIntegrationTest.php
   @@ -197,9 +197,9 @@ protected function assertFieldValues($entity_name, $referenced_entities) {
   -    $config_entity_1 = entity_create('config_test', ['id' => $this->randomMachineName(), 'label' => $this->randomMachineName()]);
   +    $config_entity_1 = \Drupal::entityTypeManager()->getStorage('config_test')->create(['id' => $this->randomMachineName(), 'label' => $this->randomMachineName()]);
        $config_entity_1->save();
   -    $config_entity_2 = entity_create('config_test', ['id' => $this->randomMachineName(), 'label' => $this->randomMachineName()]);
   +    $config_entity_2 = \Drupal::entityTypeManager()->getStorage('config_test')->create(['id' => $this->randomMachineName(), 'label' => $this->randomMachineName()]);
   

   get storage only once

There's no places left, so +1RTBC but needs to update summary

Good to go now

So, this patch does not follow the instructions in #49 and #50. Please be sure to read the comments on an issue carefully as they will often contain important instructions about the patch.

In the case of this issue, the replacements are at least consistent since it's used only in tests, and the patch is small enough to be manageable. So, I decided to commit it as-is. Also, after this patch, there are only a few entity_create() calls left in core. Nice work!

Committed and pushed to 8.5.x. I also cherry-picked it to 8.4.x since it only cleans up deprecated code usage in tests.

Note that a few people didn't receive issue credit when their uploaded patches didn't seem to help the issue's progress. To get credit in the future, make sure to read and take into account other comments on the issue, post your own comment explaining what you did and why, and provide an interdiff to help reviewers see the changes you made. These things will help everyone collaborate on fixing the issue.

Next up: We need to remove a few stray documentation references to entity_create() as well as a few calls that use a variable entity type. We can do that in new child issues of #2490966: [Meta] Replace deprecated usage of entity_create with a direct call to the entity type class. Hooray!