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 now deprecated so we shouldn't use it anymore. When the entity type is known we should directly call <EntityType>::create(). When the entity type is variable, we should use \Drupal::entityManager()->getStorage($entity_type)->create($values);.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task, because Drupal will continue working during and after the beta without this change.
Issue priority Normal because it's just about code cleanup and good practices
Prioritized changes This is not prioritized because it does not "improve Drupal 8's stability or move Drupal 8 closer to a releasable state".
Disruption This change could be disruptive to many pending patches because there are hundreds of files in core that call entity_create(). In addition, some usages may require injection, which again is more disruptive than a simple find and replace. Simply it "will require changes across many subsystems and patches in order to make core consistent."

Proposed resolution

Replace the deprecated call to entity_create() by a proper call to <EntityType>::create().

Before:

entity_create('field_config', $field_values)->save();

After:

use Drupal\field\Entity\FieldConfig;
FieldConfig::create($field_values)->save();

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Instructions Done
Manually test the patch Novice 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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DuaelFr’s picture

Title: [Meta] Replace deprecated usage of entity_create('filter_format') with a direct call to FilterFormat::create() » Replace deprecated usage of entity_create('filter_format') with a direct call to FilterFormat::create()
Assigned: DuaelFr » Unassigned
Issue summary: View changes
Status: Active » Needs review
FileSize
26.7 KB

Please also give commit attribution to trwad, jmolivas and rbp as they worked on the previous version of this patch (that was splitted by module and not by entity type)

Status: Needs review » Needs work

The last submitted patch, 1: replace_deprecated-2503379-1.patch, failed testing.

DuaelFr’s picture

Status: Needs work » Needs review
FileSize
608 bytes
26.7 KB
DuaelFr’s picture

Issue tags: +Avoid commit conflicts

Status: Needs review » Needs work

The last submitted patch, 3: replace_deprecated-2503379-3.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: replace_deprecated-2503379-3.patch, failed testing.

DuaelFr’s picture

Status: Needs work » Needs review
FileSize
28.35 KB
2.2 KB

Solved conflict between FilterFormat entity type and FilterFormat data type plugin.

cilefen’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue summary: View changes
Status: Needs review » Postponed

@DuaelFr Nice work on getting rid of these deprecated usages. I'd like to triage this issue and consider whether it is suitable for the beta phase:

  • It is not an "unfrozen" change.
  • Is it a prioritized change? I think it is not. The function is marked as deprecated and it will be removed before Drupal 9, not Drupal 8.0. Usages in core can be removed later.
  • Is it disruptive? It "will require changes across many subsystems and patches in order to make core consistent". With this patch applied there are more than 300 files using entity_create() in core. Many other issues may have conflicts with this one.

I updated the beta evaluation accordingly. Ultimately, the scale of this change in terms of the amount of files affected suggests to me that it should be postponed to 8.1.x. If it were only a handful of files in an isolated system, things would be different. The beta phase rules are not only intended to increase stability, they are also for focusing the attention of contributors and committers on issues that put Drupal 8 in a releaseable state. @xjm can be consulted if anyone disagrees with me.

cilefen’s picture

Version: 8.1.x-dev » 8.0.x-dev
Status: Postponed » Needs review

Let's ignore my comment here and talk about it on the META.

jmolivas’s picture

I test this path and it works fine

Drupal test run
---------------

Tests to be run:
  - Drupal\filter\Tests\FilterAPITest

Test run started:
  Thursday, June 11, 2015 - 03:54

Test summary
------------

Drupal\filter\Tests\FilterAPITest                             81 passes

Test run duration: 42 sec

I believe when can take advantage of this update and replace some array() with the short syntax, uploading a patch in a few

cilefen’s picture

@jmolivas Do you not trust the testbots?

jmolivas’s picture

@cilefen Yes I do, but is a good practice to test locally instead of wait for the testbots to complain, and also I am sure my code is ok before uploading a patch.

Like in this case with the patch I am uploading.

Changes:
- Replace array() with short array syntax.
- Remove unused class Drupal\Component\Utility\SafeMarkup.

Drupal test run
---------------

Tests to be run:
  - Drupal\filter\Tests\FilterAPITest

Test run started:
  Thursday, June 11, 2015 - 04:15

Test summary
------------

Drupal\filter\Tests\FilterAPITest                             81 passes

Test run duration: 40 sec
DuaelFr’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs review » Postponed
Issue tags: -Avoid commit conflicts

As @alexpott said in #2494775-8: Replace deprecated usage of entity_create('action') with a direct call to Action::create(), this issue and it's meta are now postponed to Drupal 8.1.

Mac_Weber’s picture

Status: Postponed » Needs review
FileSize
28.78 KB

@jmolivas talking with alexpott in IRC about a similar issue he said we should not take care of converting array() to [] at this time.

Patches do not apply anymore. Rerolled.

Status: Needs review » Needs work

The last submitted patch, 15: filter_format-2503379-15.patch, failed testing.

Mac_Weber’s picture

Status: Needs work » Needs review
FileSize
28.78 KB
1.37 KB

Fixed class aliasing

Status: Needs review » Needs work

The last submitted patch, 17: filter_format-2503379-16.patch, failed testing.

Mac_Weber’s picture

Status: Needs work » Needs review
FileSize
29.43 KB
1.37 KB

wrong patch file sent

naveenvalecha’s picture

Replaced the static function call in tests with chain functions. $this->container->get('entity_type.manager')->getStorage('filter_format')->

Mac_Weber’s picture

+++ b/core/modules/filter/src/Tests/FilterAPITest.php
@@ -11,7 +11,8 @@
-use Drupal\filter\Plugin\DataType\FilterFormat;
+use Drupal\filter\Entity\FilterFormat;
+use Drupal\filter\Plugin\DataType\FilterFormat as FilterFormatDataType;
 use Drupal\filter\Plugin\FilterInterface;
 use Drupal\system\Tests\Entity\EntityUnitTestBase;
 use Symfony\Component\Validator\ConstraintViolationListInterface;
@@ -36,7 +37,7 @@ protected function setUp() {

@@ -36,7 +37,7 @@ protected function setUp() {
    */
   function testCheckMarkupFilterOrder() {
     // Create crazy HTML format.
-    $crazy_format = entity_create('filter_format', array(
+    $crazy_format = $this->container->get('entity_type.manager')->getStorage('filter_format')->create(array(

There is no need for a "use" statement when you are not using the class.

Patch fixed.

DuaelFr’s picture

Status: Needs review » Needs work

@naveenvalecha Why did you choose to use that long chained function against the Issue Summary and the Meta issue?
The purpose here is to increase DX by adding consistency and reducing verbosity of Core.

I think we should stick to #19's patch.
@Mac_Weber could you, please, post it again so the maintainers know the last one is the good one?

Wim Leers’s picture

The purpose here is to increase DX by adding consistency and reducing verbosity of Core.

+1

Also, if we'd not go with that shorthand, accessing the container would also be utterly unacceptable. The appropriate service would have to be injected instead.

naveenvalecha’s picture

The appropriate service would have to be injected instead.
+1

The purpose here is to increase DX by adding consistency and reducing verbosity of Core.

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

But for consistency we can stick with the static calls for the time.have we made the deal with the entity maintainers about static functions ?

Mac_Weber’s picture

@Wim Leers @naveenvalecha @DuaelFr I think the best place to discuss it would be #2644752: [policy] Decide how to create/load entities in procedural code and test classes

Mile23’s picture

Neither #19 or #21 apply.

Let's use the approach in #19 for now, since we're trying to deprecate entity_create().

naveenvalecha’s picture

Issue tags: +Novice, +Needs reroll

Please reroll the patch from #19

lluvigne’s picture

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

Rerolled #19.

Wim Leers’s picture

Status: Needs review » Needs work

One occurrence is still left in \Drupal\filter\Tests\FilterCrudTest::testTextFormatCrud() after applying #29.

Once that's done, this is RTBC :)

lluvigne’s picture

Status: Needs work » Needs review
FileSize
27.6 KB
728 bytes

Replace the last occurrence in \Drupal\filter\Tests\FilterCrudTest::testTextFormatCrud(). Hope it works!

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

It totally does :)

Thank you!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x, thanks!

  • catch committed e825a16 on 8.1.x
    Issue #2503379 by Mac_Weber, DuaelFr, lluvigne, naveenvalecha, jmolivas...

Status: Fixed » Closed (fixed)

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