Follow up from #1735118: Convert Field API to CMI

Conversion happens in sandbox
Log of commits

Problem/Motivation

field_create_*(), field_update_*() and field_delete_*() duplicates entity functions.

Proposed resolution

Replace usage of "field CUD" functions with proper entity API

Remaining tasks

Commit #1973324: Mark field_create_*(), field_update_*() and field_delete_*() deprecated in favor of just using the ConfigEntity API
Convert all usage all over core
Review patches

API changes

function to replace new code
field_create_field() entity_create('field_entity', $field_definition)->save()
field_create_instance() entity_create('field_instance', $instance_definition)->save()
field_update_field() $field->save()
field_update_instance() $field_instance->save()
field_delete_field() $field->delete()
field_delete_instance() $field_instance->delete()

To make it easy to review patches filed as separate issues:
#1981292: Drop procedural usage of fields in field_sql_storage & field_ui tests
#1981306: Drop procedural usage of fields in [a-e] modules
#1981314: Drop procedural usage of fields in field module - This one will effectively remove the functions
#1981316: Drop procedural usage of fields in [f-l] modules
#1981334: Drop procedural usage of fields in [n-r] modules
#1981336: Drop procedural usage of fields in [t*] modules
#1981342: Drop procedural usage of fields in [s-u-v] modules

#1970006: Make FileFieldTestBase::createFileField to return Field entity

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

Issue tags: +Field API

tagging

yched’s picture

Status: Postponed » Active

Un-postponing.

andypost’s picture

Status: Active » Needs review
FileSize
2.08 KB

Grep against source code gives:
field_create_ 344
field_update_ 107
field_delete_ 58

Is there a branch in sandbox for the issue? or better to split this one to 3 issues?

At least all this functions needs to be marked as @deprecated

yched’s picture

#3 is a good first step, sure, let's do this.

No branch in the sandbox yet, feel free to create one :-)

Dealing with the three functions in one pass should be easier IMO. So rather than separate patches for C, U, D, I'd suggest splitting the alphabet in 3 or 4, and attacking modules from a to f, then g to p or whatever, etc...

yched’s picture

Status: Needs review » Reviewed & tested by the community

For #3 right now.

andypost’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
29.01 KB

Fixed field_delete_instance() and field_delete_field() - some comments are changed too (needs review of my bad English)

Filed #1970006: Make FileFieldTestBase::createFileField to return Field entity to clean-up file tests and minimize a patch

yched’s picture

@andypost ; I gave you commit access to the sandbox

yched’s picture

[edit: deleted, wrong issue]

Status: Needs review » Needs work

The last submitted patch, 1953410-field-cud-4.patch, failed testing.

andypost’s picture

Fixed broken tests and pushed 1953410-field-cud branch

andypost’s picture

Status: Needs work » Needs review
FileSize
4.31 KB

I think this needs clear terminology for comments and doc blocks

This doc-blocks are updated in patch.
Should we use "field" or "field entity"? field_inctance or field instance entity?

andypost’s picture

FileSize
48.09 KB

I found no way to split conversion on each function because each test needs special attention to convert
So each commit is per test file seems easy to review

First converted CRUD tests

andypost’s picture

Forget interdiffs

Status: Needs review » Needs work

The last submitted patch, 1953410-field-cud-12.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
81.13 KB
1.59 KB

Test current state, added workaround for #1970110: Should ConfigEntity::isNew() be based on id or uuid ? see workaround.txt

andypost’s picture

FileSize
0 bytes

somehow this does not pass locally

andypost’s picture

FileSize
3.21 KB

hm, again

andypost’s picture

Suppose all converted, now let's see the bot's feedback

Also let's discuss how to split the patch - I think better to split on module first letters...

EDIT preg ( field_create_(field|instance)| field_update_(field|instance)| field_delete_(field|instance))

yched’s picture

Also let's discuss how to split the patch - I think better to split on module first letters

Yup, that's what I suggested in #4 above :-) :

Dealing with the three functions in one pass should be easier IMO. So rather than separate patches for C, U, D, I'd suggest splitting the alphabet in 3 or 4, and attacking modules from a to f, then g to p or whatever, etc...

andypost’s picture

what size of patches preferable?
Patch with removal of functions

difstat
113 files changed, 599 insertions(+), 721 deletions(-)

yched’s picture

If overall patch is ~200k, then splitting in 4 patches sounds good IMO.

swentel’s picture

Yeah, 4 seems fine to me too, nice stuff @andypost!

andypost’s picture

So here we go!
I think field patch better to save as one

yched’s picture

Awesome !

Should we split in different sub issues then ?

swentel’s picture

Talked quickly to xjm in IRC, she suggested todo a patch per function - so one for create, one for update, one for delete. While it makes sense, this might be annoying though for this patch as we have all the changes already :/

swentel’s picture

+++ b/core/modules/comment/comment.installundefined
@@ -10,7 +10,7 @@
+  field_info_field('comment_body')->delete();

One thing I'm wondering: shouldn't we do entity_load()->delete() here ?

yched’s picture

"one patch per function" has been discussed above. It's the wrong approach IMO. Will make things artificially tedious because the same code blocks often use two or even the three of the functions, and so will need several passes, possibly with temporary adaptations only added to be removed by the next pass. It seems @andypost tried it and changed gears for those very reasons.

Splitting core in chunks, and go for the three functions at once, sounds much easier.
And it's done :-). So the only question left is : should the patches in #24 go in separate sub issues that can be reviewed and committed separately ? (I'd vote yes)

andypost’s picture

Title: Remove field_create_*(), field_update_*() and field_delete_*() in favor of just using the ConfigEntity API » [Meta] Remove field_create_*(), field_update_*() and field_delete_*() in favor of just using the ConfigEntity API

Also @xjm suggested to provide a helper method in separate issue a-la #1872540: Provide a test helper method for creating block instances
@berdir pointed that there's a lot of collisions with #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest that will require second pass to remove $entity->getBCEntity()

as I pointed in #12 there's no way to make this conversion on per function basis because some places needs refactoring

So the only hard patch 1953410-field.txt that big and depends on changes in FieldUnitTestBase::createFieldWithInstance()

Another approach would be per-file-based conversion as #500866: [META] remove t() from assert message

Anyway much more changes will be required to remove ArrayAccess shim #1953408: Remove ArrayAccess BC layer from field config entities so I think better to make refactoring of field tests in one pass

yched’s picture

1953410-field.txt removes the functions completely, so it should be last to go.

Reviewing 1953410-field_other.txt for now.

+++ b/core/modules/field_ui/field_ui.admin.incundefined
@@ -756,11 +756,11 @@ function field_ui_field_delete_form_submit($form, &$form_state) {
-    drupal_set_message(t('The field %field has been deleted from the %type content type.', array('%field' => $instance['label'], '%type' => $bundle_label)));
+    $instance->delete();
+    drupal_set_message(t('The field %field has been deleted from the %type content type.', array('%field' => $instance->label(), '%type' => $bundle_label)));

Slippery slope :-). No biggie, but let's keep the $instance['foo'] / $instance->foo changes for #1953408: Remove ArrayAccess BC layer from field config entities.
(same in the else { block below)

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/AlterTest.phpundefined
@@ -50,10 +50,10 @@ function setUp() {
   function testDefaultWidgetPropertiesAlter() {

This test has a series of :
$instance = array(...);
entity_create('field_instance', $instance);
where $instance is not reused but successively overwritten
Let's just inline those in the entity_create() calls.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageFieldsTest.phpundefined
@@ -43,7 +43,7 @@ function setUp() {
     $field = array(
 (...)
-    field_create_field($field);
+    entity_create('field_entity', $field)->save();
 
     $instance = array(
 (...)
-    field_create_instance($instance);
+    entity_create('field_instance', $instance)->save();

Same here, $field and $instance can be inlined.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.phpundefined
@@ -570,8 +570,8 @@ public function submitForm(array &$form, array &$form_state) {
+        entity_create('field_entity', $field)->save();
+        entity_create('field_instance', $instance)->save();

Same here, the $field and $instance variables can be inlined.
There's a dsm() in the catch { branch that uses $instance['label'], it can switch back to $values['label'].

(same thing a couple lines below after // Re-use existing field.

+++ b/core/modules/field_sql_storage/lib/Drupal/field_sql_storage/Tests/FieldSqlStorageTest.phpundefined
@@ -374,15 +394,19 @@ function testFieldUpdateIndexesWithData() {
+    //$field = array('field_name' => $field_name, 'indexes' => array('value' => array(array('value', 255))));
+    $field['indexes'] = array('value' => array(array('value', 255)));
+    //field_update_field($field);

Commented lines ?
(next hunk as well)

andypost’s picture

FileSize
18.95 KB
8.29 KB

addressed #29 in commit 00613d18

andypost’s picture

FileSize
18.78 KB
1.02 KB

removed another 2 commented lines

andypost’s picture

andypost’s picture

Issue summary: View changes

Updated summary

Status: Needs review » Needs work

The last submitted patch, 1953410-field_other.patch, failed testing.

yched’s picture

Er, so yeah, this will clash with #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest all over the place :-/.
That one seems hard enough that we should probably not make their life even more difficult with hell rerolls.
Then again, it's difficult to isolate what will clash and what won't...
Does it mean we'd better postpone this whole issue on #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest ?

andypost’s picture

Status: Needs work » Needs review
FileSize
185.49 KB

merged in sandbox, just to get current state

andypost’s picture

FileSize
185.66 KB
2.33 KB

Fix broken tests

andypost’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, 1953410-field-cud-37.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
184.24 KB
1.07 KB

Should be fine, pushed

aspilicious’s picture

This shouldn't be split into 7 issues, certainly not when you leave this one open :).
I'm perfectly fine with reviewing this one. Can we please close the others? Or close this one? Or at least make clear those issues are no followup issues but just this one patch splitted in pieces?

:)

aspilicious’s picture

+++ b/core/modules/comment/comment.installundefined
@@ -10,7 +10,7 @@
+  field_info_field('comment_body')->delete();

http://api.drupal.org/api/drupal/core%21modules%21field%21field.info.inc... is saying it returns an array. Is this incorrect? Or is api.drupal.org not adjusted yet... Looking at the procedural function this should be fine. ALthough there is an extra check in the procedural...

But maybe that check is part of the delete() so in the end this is ok ;)

I can't find any other problems...
Will check if you convert every procedural listed in the summary in the next review

aspilicious’s picture

Status: Needs review » Needs work

field_create_field

1 match in C:\xampp\htdocs\drupal8\core\modules\field\lib\Drupal\field\FieldStorageController.php
23 field_create_field($config);

field_update_field

1 match in C:\xampp\htdocs\drupal8\core\modules\field\lib\Drupal\field\FieldStorageController.php
      31      field_update_field($config);

field_delete_field

1 match in C:\xampp\htdocs\drupal8\core\modules\field\lib\Drupal\field\FieldStorageController.php
      39      field_delete_field($config['field_name']);

So FieldStorageController.php is the only file left containing procedural calls. This probably means this is untested :). We can use a followup for that. Besides of that file this is rtbc.

swentel’s picture

@aspilicious: FieldStorageController.php doesn't even exist anymore ...

aspilicious’s picture

Status: Reviewed & tested by the community » Needs work

hmmm...

Lets see what happened...

aspilicious’s picture

Status: Needs work » Reviewed & tested by the community

So apparently I had 4 files not yet added to core (garbage of other patches, git reset --hard doesn't remove those).
So in the end this patch is ready to go :)

:D

amateescu’s picture

Status: Needs work » Postponed

No, it's not. This will break #1875992: Add EntityFormDisplay objects for entity forms in so many places that it's not even funny.

andypost’s picture

Status: Postponed » Needs work
FileSize
177.91 KB
andypost’s picture

Status: Needs work » Needs review
aspilicious’s picture

+++ b/core/modules/field/lib/Drupal/field/Tests/FormTest.phpundefined
@@ -616,8 +616,9 @@ function testFieldFormHiddenWidget() {
+    $this->instance= entity_create('field_instance', $this->instance);

Small spacing issue

Can someone fix this so I can rtbc this. I don't want to rtbc my own reroll :)

andypost’s picture

FileSize
177.91 KB
858 bytes

@aspilicious thanx for review, here's fixed patch

swentel’s picture

So, we agreed moving this into chunks again, sorry I closed the other ones on this, but we're waiting for #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest to land first though :/

andypost’s picture

We got a sprint this weekend so - is it ok to split the patch and re-open issues?

swentel’s picture

@andypost - that's fine - sorry for the trouble.

swentel’s picture

Issue summary: View changes

added issues

swentel’s picture

Status: Needs review » Active

Let's keep this as the meta and just active.

andypost’s picture

I'd better use the issue to test sandbox code

andypost’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

Please make sure that hook_uninstall() occurrences of field_delete_field() get an if() check when replacing with $field->delete(). Same with $instance->delete(). The fields or instances are not guaranteed to exist in those places e.g. in case they have been deleted previously or never actually existed (as in the case of comment module e.g. if there are no node types).

pcambra’s picture

Adding a reference to the issue mentioned in #56: #2022769: Fix conversion of field_delete_field() in hook_uninstall()

I've checked all the issues we're dealing with in this one and none uses field deletion on uninstall except comment and forum and those are addresses in the one mentioned above.

swentel’s picture

Status: Active » Fixed

This is done, woot, thanks all!

andypost’s picture

Is there any change notice about?

swentel’s picture

I guess the convert field api to cmi one

andypost’s picture

Updated here

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.