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
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
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.
Comment | File | Size | Author |
---|---|---|---|
#31 | interdiff-2503379-29-31.txt | 728 bytes | lluvigne |
#31 | replace_deprecated-2503379-31.patch | 27.6 KB | lluvigne |
#29 | replace_deprecated-2503379-29.patch | 27.23 KB | lluvigne |
#21 | filter_format-2503379-21.patch | 26.8 KB | Mac_Weber |
#19 | interdiff-15to19.txt | 1.37 KB | Mac_Weber |
Comments
Comment #1
DuaelFrPlease 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)Comment #3
DuaelFrComment #4
DuaelFrComment #8
DuaelFrSolved conflict between FilterFormat entity type and FilterFormat data type plugin.
Comment #9
cilefen CreditAttribution: cilefen commented@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:
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.
Comment #10
cilefen CreditAttribution: cilefen commentedLet's ignore my comment here and talk about it on the META.
Comment #11
jmolivas CreditAttribution: jmolivas at FFW commentedI test this path and it works fine
I believe when can take advantage of this update and replace some array() with the short syntax, uploading a patch in a few
Comment #12
cilefen CreditAttribution: cilefen commented@jmolivas Do you not trust the testbots?
Comment #13
jmolivas CreditAttribution: jmolivas at FFW commented@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.
Comment #14
DuaelFrAs @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.
Comment #15
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commented@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.
Comment #17
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedFixed class aliasing
Comment #19
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedwrong patch file sent
Comment #20
naveenvalechaReplaced the static function call in tests with chain functions.
$this->container->get('entity_type.manager')->getStorage('filter_format')->
Comment #21
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedThere is no need for a "use" statement when you are not using the class.
Patch fixed.
Comment #22
DuaelFr@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?
Comment #23
Wim Leers+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.
Comment #24
naveenvalechaThe appropriate service would have to be injected instead.
+1
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 ?
Comment #25
Wim Leers@chx says this is not true in #2641584-9: Replace deprecated usage of entity_create('field_config') with a direct call to FieldConfig::create() and in #2641518-6: Replace deprecated usage of entity_create('config_test') with a direct call to ConfigTest::create().
Comment #26
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commented@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
Comment #27
Mile23Neither #19 or #21 apply.
Let's use the approach in #19 for now, since we're trying to deprecate entity_create().
Comment #28
naveenvalechaPlease reroll the patch from #19
Comment #29
lluvigneRerolled #19.
Comment #30
Wim LeersOne occurrence is still left in
\Drupal\filter\Tests\FilterCrudTest::testTextFormatCrud()
after applying #29.Once that's done, this is RTBC :)
Comment #31
lluvigneReplace the last occurrence in \Drupal\filter\Tests\FilterCrudTest::testTextFormatCrud(). Hope it works!
Comment #32
Wim LeersIt totally does :)
Thank you!
Comment #33
catchCommitted/pushed to 8.1.x, thanks!