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() 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.

CommentFileSizeAuthor
#24 2641528-24.patch10.32 KBheykarthikwithu
#20 2641528-9.patch9.02 KBheykarthikwithu
#17 2641528-15.patch9.01 KBnaveenvalecha
#9 interdiff-2641528-4-9.txt905 bytesheykarthikwithu
#9 2641528-9.patch9.02 KBheykarthikwithu
#4 editor-2641528-4.patch9.78 KBmac_weber
#2 editor-2641528-2.patch9.77 KBmac_weber

Comments

Mac_Weber created an issue. See original summary.

mac_weber’s picture

mac_weber commented
Status: Active » Needs review
StatusFileSize
new editor-2641528-2.patch9.77 KB

Status: Needs review » Needs work

The last submitted patch, 2: editor-2641528-2.patch, failed testing.

mac_weber’s picture

mac_weber commented
Status: Needs work » Needs review
StatusFileSize
new editor-2641528-4.patch9.78 KB

Fixed class aliasing

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Reviewed & tested by the community

Patch in #4 applies, IDE says it replaces entity_create('editor and with the Editor::create() method.

Setting RTBC and rerunning the tests.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
@@ -204,7 +204,7 @@ public function settingsForm(array $form, FormStateInterface $form_state, Editor
+    $fake_editor = EditorEntity::create(array(

Why is it EditorEntity here and not Editor? Do we have a conflict with the Editor classname in that file or is it unnecessarily aliased?

heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
commented
Assigned: Unassigned » heykarthikwithu

looking into this.

heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
commented

@catch, their is a use statement aliasing for Editor.
use Drupal\editor\Entity\Editor as EditorEntity;

heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
commented
Assigned: heykarthikwithu » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 2641528-9.patch9.02 KB
new interdiff-2641528-4-9.txt905 bytes
catch’s picture

catch
he/him
Primary language English
commented

@heykarthikwithu yes it looks like we could remove the alias - there's no conflict with another class there. Your patch is using the same class twice which doesn't simplify things.

However that should probably be done in a separate issue - ideally we'd make that change first and postpone this patch on it.

heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
commented
Related issues: +#2668828: Remove EditorEntity aliasing in editor & ckeditor module
catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Postponed

Thanks! Postponing on that, then this patch is a quick re-roll once that's in.

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Status: Postponed » Needs review

This would be N/W when this #2668828: Remove EditorEntity aliasing in editor & ckeditor module went in.

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Status: Needs review » Postponed
naveenvalecha’s picture

naveenvalecha
He/Him
commented
Status: Postponed » Needs work

unpostponing this one as the editor aliasing issue went in #2668828: Remove EditorEntity aliasing in editor & ckeditor module

heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
commented
Assigned: Unassigned » heykarthikwithu

working on this.

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 2641528-15.patch9.01 KB

Straight reroll.
Reviewed the patch, Well this patch does not have my code.So RTBC

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Status: Reviewed & tested by the community » Needs work

Setting N/W , patch needs to take care of removing the existing use statements

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

heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
commented
Assigned: heykarthikwithu » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 2641528-9.patch9.02 KB

Rerolled as per #15.

Status: Needs review » Needs work

The last submitted patch, 20: 2641528-9.patch, failed testing.

naveenvalecha’s picture

naveenvalecha
He/Him
commented 
+++ b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
@@ -13,6 +13,7 @@
 use Drupal\editor\Entity\Editor as EditorEntity;

Remove this statement as this has been already taken care in #2668828: Remove EditorEntity aliasing in editor & ckeditor module.Patch needs rework

heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
commented
Assigned: Unassigned » heykarthikwithu

wrong patch added in #20. needs a reroll.

heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
commented
Assigned: heykarthikwithu » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 2641528-24.patch10.32 KB
naveenvalecha’s picture

naveenvalecha
He/Him
commented
Component: edit.module » editor.module
Status: Needs review » Reviewed & tested by the community

Patch looks good, Thanks!
RTBC if green
Changing the component b/c it should be editor.module and ckeditor.module

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Priority: Normal » Minor
Issue tags: +Quickfix

lgtm

  • catch committed c3d6658 on 8.1.x 
    Issue #2641528 by heykarthikwithu, Mac_Weber, naveenvalecha: Replace...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x, thanks!

Status: Fixed » Closed (fixed)

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