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

Files: 
CommentFileSizeAuthor
#50 interdiff.txt858 bytesandypost
#50 1953410-field-cud-50.patch177.91 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,894 pass(es).
[ View ]
#47 1953410-field-cud-47.patch177.91 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 56,600 pass(es).
[ View ]
#39 interdiff.txt1.07 KBandypost
#39 1953410-field-cud-39.patch184.24 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,406 pass(es).
[ View ]
#37 1953410-field-cud-37.patch184.03 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 55,872 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#36 interdiff.txt2.33 KBandypost
#36 1953410-field-cud-36.patch185.66 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,355 pass(es).
[ View ]
#35 1953410-field-cud-35.patch185.49 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 55,313 pass(es), 154 fail(s), and 30 exception(s).
[ View ]
#31 interdiff.txt1.02 KBandypost
#31 1953410-field_other.patch18.78 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 54,339 pass(es), 141 fail(s), and 29 exception(s).
[ View ]
#30 interdiff.txt8.29 KBandypost
#30 1953410-field_other.txt18.95 KBandypost
#23 1953410-field.txt66.77 KBandypost
#23 1953410-field_other.txt16.47 KBandypost
#23 1953410-a-e.txt20.14 KBandypost
#23 1953410-f-l.txt24.92 KBandypost
#23 1953410-n-r.txt22.46 KBandypost
#23 1953410-t.txt21.37 KBandypost
#23 1953410-s_.txt19.01 KBandypost
#23 1953410-field-cud-20_.patch191.14 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,439 pass(es).
[ View ]
#20 1953410-drop_cud.txt7.16 KBandypost
#20 1953410-field-cud-20.patch191.14 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,649 pass(es).
[ View ]
#18 1953410-field-cud-18-final.patch186.54 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,625 pass(es).
[ View ]
#17 date-field.patch3.21 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,300 pass(es).
[ View ]
#16 date-field.patch0 bytesandypost
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#15 workaround.txt1.59 KBandypost
#15 1953410-field-cud-15.patch81.13 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,297 pass(es).
[ View ]
#13 FieldInstanceCrudTest.txt8.71 KBandypost
#13 CrudTest.txt11.21 KBandypost
#13 FieldUnitTestBase.txt1.21 KBandypost
#12 1953410-field-cud-12.patch48.09 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 54,156 pass(es), 49 fail(s), and 39 exception(s).
[ View ]
#11 comments.txt4.31 KBandypost
#6 1953410-field-cud-4.patch29.01 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 54,194 pass(es), 57 fail(s), and 42 exception(s).
[ View ]
#3 1953410-field-cud-3.patch2.08 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 54,268 pass(es), 38 fail(s), and 0 exception(s).
[ View ]

Comments

Issue tags:+Field API

tagging

Status:Postponed» Active

Un-postponing.

Status:Active» Needs review
StatusFileSize
new2.08 KB
FAILED: [[SimpleTest]]: [MySQL] 54,268 pass(es), 38 fail(s), and 0 exception(s).
[ View ]

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

#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...

Status:Needs review» Reviewed & tested by the community

For #3 right now.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new29.01 KB
FAILED: [[SimpleTest]]: [MySQL] 54,194 pass(es), 57 fail(s), and 42 exception(s).
[ View ]

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

@andypost ; I gave you commit access to the sandbox

[edit: deleted, wrong issue]

Status:Needs review» Needs work

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

Fixed broken tests and pushed 1953410-field-cud branch

Status:Needs work» Needs review
StatusFileSize
new4.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?

StatusFileSize
new48.09 KB
FAILED: [[SimpleTest]]: [MySQL] 54,156 pass(es), 49 fail(s), and 39 exception(s).
[ View ]

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

StatusFileSize
new1.21 KB
new11.21 KB
new8.71 KB

Forget interdiffs

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new81.13 KB
PASSED: [[SimpleTest]]: [MySQL] 54,297 pass(es).
[ View ]
new1.59 KB

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

StatusFileSize
new0 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

somehow this does not pass locally

StatusFileSize
new3.21 KB
PASSED: [[SimpleTest]]: [MySQL] 54,300 pass(es).
[ View ]

hm, again

StatusFileSize
new186.54 KB
PASSED: [[SimpleTest]]: [MySQL] 54,625 pass(es).
[ View ]

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

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

StatusFileSize
new191.14 KB
PASSED: [[SimpleTest]]: [MySQL] 54,649 pass(es).
[ View ]
new7.16 KB

what size of patches preferable?
Patch with removal of functions

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

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

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

StatusFileSize
new191.14 KB
PASSED: [[SimpleTest]]: [MySQL] 54,439 pass(es).
[ View ]
new19.01 KB
new21.37 KB
new22.46 KB
new24.92 KB
new20.14 KB
new16.47 KB
new66.77 KB

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

Awesome !

Should we split in different sub issues then ?

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 :/

+++ 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 ?

"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)

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

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)

StatusFileSize
new18.95 KB
new8.29 KB

addressed #29 in commit 00613d18

StatusFileSize
new18.78 KB
FAILED: [[SimpleTest]]: [MySQL] 54,339 pass(es), 141 fail(s), and 29 exception(s).
[ View ]
new1.02 KB

removed another 2 commented lines

Issue summary:View changes

Updated summary

Status:Needs review» Needs work

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

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 ?

Status:Needs work» Needs review
StatusFileSize
new185.49 KB
FAILED: [[SimpleTest]]: [MySQL] 55,313 pass(es), 154 fail(s), and 30 exception(s).
[ View ]

merged in sandbox, just to get current state

StatusFileSize
new185.66 KB
PASSED: [[SimpleTest]]: [MySQL] 55,355 pass(es).
[ View ]
new2.33 KB

Fix broken tests

Issue summary:View changes

Updated issue summary.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new184.24 KB
PASSED: [[SimpleTest]]: [MySQL] 55,406 pass(es).
[ View ]
new1.07 KB

Should be fine, pushed

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?

:)

+++ 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

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.

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

Status:Reviewed & tested by the community» Needs work

hmmm...

Lets see what happened...

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

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.

Status:Postponed» Needs work
StatusFileSize
new177.91 KB
PASSED: [[SimpleTest]]: [MySQL] 56,600 pass(es).
[ View ]

Status:Needs work» Needs review

+++ 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 :)

StatusFileSize
new177.91 KB
PASSED: [[SimpleTest]]: [MySQL] 55,894 pass(es).
[ View ]
new858 bytes

@aspilicious thanx for review, here's fixed patch

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 :/

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

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

Issue summary:View changes

added issues

Status:Needs review» Active

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

I'd better use the issue to test sandbox code

Issue summary:View changes

Updated issue summary.

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

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.

Status:Active» Fixed

This is done, woot, thanks all!

Is there any change notice about?

I guess the convert field api to cmi one

Updated here

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

Issue summary:View changes

Updated issue summary.