Follow-up to #2490966: [Meta] Replace deprecated usage of entity_create with a direct call to the entity type class

Problem/Motivation

According to #2346261: Deprecate entity_create() in favor of a <EntityType>::create($values) or \Drupal::entityManager()->getStorage($entity_type)->create($values), entity_create() function is going to be deprecated so we shouldn't use it anymore. When the entity type is known we should directly call <EntityType>::create(). What to do when the entity type is not known or is variable is upon discussions.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task
Issue priority Normal because it's just about code cleanup and good practices
Prioritized changes The main goal of this issue is DX, performance and removing code already deprecated for 8.0.0. (Direct calls to EntityType::create are better than generic calls to entity_create for readability)
Disruption This change is not disruptive at all as it only replaces deprecated functions call by their exact equivalent.

Proposed resolution

Replace the deprecated call to entity_create('config_test') by a proper call to ConfigTest::create().

Edit: 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. See #40 for a better explanation, and #38 with 2 similar patches using the different methods, failing only with the short one.

Before:

entity_create('config_test', $params)->save();

After:

\Drupal::entityTypeManager()->getStorage('config_test')->create($params)->save();

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Instructions Done
Manually test the patch Instructions
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#62 interdiff-59-60.txt9.58 KBanya_m
#62 replace_deprecated-2641518-60.patch9.49 KBanya_m
#59 interdiff-57-59.txt644 bytespk188
#59 replace_deprecated-2641518-59.patch9.36 KBpk188
#57 interdiff-54-57.txt4.39 KBpk188
#57 replace_deprecated-2641518-57.patch9.33 KBpk188
#54 interdiff-2641518-53-54.txt10.62 KBdimaro
#54 replace_deprecated-2641518-54.patch9.72 KBdimaro
#53 interdiff-2641518-51-53.txt835 bytesdimaro
#53 replace_deprecated-2641518-53.patch10 KBdimaro
#51 replace_deprecated-2641518-51.patch10 KBdimaro
#42 replace_deprecated-2641518-42.patch10.06 KBdimaro
#38 2641518_38_PASS.patch831 bytesMile23
#38 2641518_38_FAIL.patch1005 bytesMile23
#32 2641518-32.patch10.75 KBnaveenvalecha
#30 config_test-2641518-30.patch9.88 KBsdstyles
#27 replace_deprecated-2641518-27.patch13.32 KBdimaro
#25 config_test-2641518-25.patch14.72 KBMac_Weber
#23 replace_Deprcated-2641518-21.patch12.02 KBanchal29
#20 2641518-20.patch12.03 KBheykarthikwithu
#17 2641518-15.patch11.16 KBaditya_anurag
#15 2641518-15.patch11.16 KBrakesh.gectcr
#10 replace_deprecated-2641518-10.patch9.45 KBnesta_
#5 config_test-2641518-5.patch10.36 KBMac_Weber
#2 config_test-2641518-2.patch11.01 KBMac_Weber
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Mac_Weber created an issue. See original summary.

Mac_Weber’s picture

Status: Active » Needs review
FileSize
11.01 KB
Mac_Weber’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 2: config_test-2641518-2.patch, failed testing.

Mac_Weber’s picture

Status: Needs work » Needs review
FileSize
10.36 KB

As @xano pointed at another issue Tests must use $this->container->get('entity_type.manager')->getStorage('config_test')->create().

chx’s picture

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.

Mile23’s picture

Status: Needs review » Needs work

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.

Mile23’s picture

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]	
naveenvalecha’s picture

Issue tags: +Novice, +@deprecated, +deprecated, +Needs reroll

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

nesta_’s picture

Status: Needs work » Needs review
FileSize
9.45 KB

Applying patch #5 and change line 43 for #8

dimaro’s picture

Issue tags: -Needs reroll

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

RTBC?

Mile23’s picture

Status: Needs review » Needs work
+++ 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.

Sriparna Khatua’s picture

Sriparna Khatua’s picture

Assigned: Unassigned » Sriparna Khatua
rakesh.gectcr’s picture

Assigned: Sriparna Khatua » Unassigned
Status: Needs work » Needs review
FileSize
11.16 KB

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(),
-    ));

Status: Needs review » Needs work

The last submitted patch, 15: 2641518-15.patch, failed testing.

aditya_anurag’s picture

Status: Needs work » Needs review
FileSize
11.16 KB

Status: Needs review » Needs work

The last submitted patch, 17: 2641518-15.patch, failed testing.

heykarthikwithu’s picture

Assigned: Unassigned » heykarthikwithu
heykarthikwithu’s picture

Assigned: heykarthikwithu » Unassigned
Status: Needs work » Needs review
FileSize
12.03 KB

Status: Needs review » Needs work

The last submitted patch, 20: 2641518-20.patch, failed testing.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

anchal29’s picture

Status: Needs work » Needs review
FileSize
12.02 KB

Here's a patch for it.

Status: Needs review » Needs work

The last submitted patch, 23: replace_Deprcated-2641518-21.patch, failed testing.

Mac_Weber’s picture

Status: Needs work » Needs review
FileSize
14.72 KB

It seems the tests work fine for this issue with the long chaining method.

I am not sure if we should use it or not, as (at least) most of the other patches use the short method to call.

Mile23’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

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?

dimaro’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
13.32 KB

Rerolled #25.

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

arunkumark’s picture

Hi,

patch #21 is works fine as per depricated.

Mac_Weber’s picture

Status: Needs review » Needs work

Patch #27 does not fix the problems pointed at #26

sdstyles’s picture

Status: Needs work » Needs review
FileSize
9.88 KB
naveenvalecha’s picture

Status: Needs review » Needs work

The patch is not applying anymore.

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
10.75 KB

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

Status: Needs review » Needs work

The last submitted patch, 32: 2641518-32.patch, failed testing.

naveenvalecha’s picture

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

Mac_Weber’s picture

use the long chain method as in #25 and it will work

tstoeckler’s picture

Issue tags: +DrupalBCDays
dimaro’s picture

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

Mile23’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Needs work » Needs review
FileSize
1005 bytes
831 bytes

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.

The last submitted patch, 38: 2641518_38_FAIL.patch, failed testing.

Mac_Weber’s picture

Status: Needs review » Needs work

@Mile23 IKR. I found this same weird behavior in another issue of this meta. I had pinged other core developers at #drupal-contribute on that time and they told me that EntityTypeRepository::getEntityTypeFromClass() is expected to work in 90% of the cases, but for the other 10% we should use the long chaining way. I am still think this is weird, but this is the way it is right now.

Changing it back to needs work because there are many other entries that have arguments and are not included in the previous patch.

Sutharsan’s picture

Issue summary: View changes
Issue tags: -Novice

Removing Novice tag because of the length of the issue.

dimaro’s picture

Status: Needs work » Needs review
FileSize
10.06 KB

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!

Mac_Weber’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, passes on tests, and do not have any missing occurrences.
Thank you @dimaro

heykarthikwithu’s picture

+1 RTBC
Looks good, passes on tests :)

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.

cilefen’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs issue summary update

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?

xjm’s picture

Issue tags: +DrupalCampNJ2017
Mac_Weber’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

@cilefen #40 has the explanation I got from other core maintainers. The patches at #38 shows that this issue with the short/long method is a real issue sometimes.

I'm marking the issue as RTBC, as other similar ones with long chain method are already applied to core by now and this one also looks good.

Issue summary updated, too.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

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!

xjm’s picture

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.

dimaro’s picture

Assigned: Unassigned » dimaro
Status: Needs work » Needs review
FileSize
10 KB

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

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

Status: Needs review » Needs work

The last submitted patch, 51: replace_deprecated-2641518-51.patch, failed testing.

dimaro’s picture

Status: Needs work » Needs review
FileSize
10 KB
835 bytes

OMG!
Upload again

dimaro’s picture

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.

dimaro’s picture

Assigned: dimaro » Unassigned

Ups! I forgot check as unassigned.

Mile23’s picture

Status: Needs review » Needs work
+++ 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 .. ]);
pk188’s picture

Status: Needs work » Needs review
FileSize
9.33 KB
4.39 KB

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

Status: Needs review » Needs work

The last submitted patch, 57: replace_deprecated-2641518-57.patch, failed testing. View results

pk188’s picture

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.

andypost’s picture

Status: Needs review » Needs work
  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

anya_m’s picture

Status: Needs work » Needs review
FileSize
9.49 KB
9.58 KB
andypost’s picture

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

anya_m’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
andypost’s picture

Status: Needs review » Reviewed & tested by the community

Good to go now

  • xjm committed 00befe6 on 8.4.x
    Issue #2641518 by dimaro, Mac_Weber, pk188, Mile23, anya_m,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • xjm committed 7185860 on 8.5.x
    Issue #2641518 by dimaro, Mac_Weber, pk188, Mile23, anya_m,...
rakesh.gectcr’s picture

Status: Fixed » Closed (fixed)

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