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
Related Issues
#1970006: Make FileFieldTestBase::createFileField to return Field entity
Comment | File | Size | Author |
---|---|---|---|
#50 | interdiff.txt | 858 bytes | andypost |
#50 | 1953410-field-cud-50.patch | 177.91 KB | andypost |
#47 | 1953410-field-cud-47.patch | 177.91 KB | andypost |
#39 | interdiff.txt | 1.07 KB | andypost |
#39 | 1953410-field-cud-39.patch | 184.24 KB | andypost |
Comments
Comment #1
swentel CreditAttribution: swentel commentedtagging
Comment #2
yched CreditAttribution: yched commentedUn-postponing.
Comment #3
andypostGrep 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
Comment #4
yched CreditAttribution: yched commented#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...
Comment #5
yched CreditAttribution: yched commentedFor #3 right now.
Comment #6
andypostFixed 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
Comment #7
yched CreditAttribution: yched commented@andypost ; I gave you commit access to the sandbox
Comment #8
yched CreditAttribution: yched commented[edit: deleted, wrong issue]
Comment #10
andypostFixed broken tests and pushed 1953410-field-cud branch
Comment #11
andypostI 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?
Comment #12
andypostI 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
Comment #13
andypostForget interdiffs
Comment #15
andypostTest current state, added workaround for #1970110: Should ConfigEntity::isNew() be based on id or uuid ? see workaround.txt
Comment #16
andypostsomehow this does not pass locally
Comment #17
andyposthm, again
Comment #18
andypostSuppose 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))
Comment #19
yched CreditAttribution: yched commentedYup, that's what I suggested in #4 above :-) :
Comment #20
andypostwhat size of patches preferable?
Patch with removal of functions
difstat
113 files changed, 599 insertions(+), 721 deletions(-)
Comment #21
yched CreditAttribution: yched commentedIf overall patch is ~200k, then splitting in 4 patches sounds good IMO.
Comment #22
swentel CreditAttribution: swentel commentedYeah, 4 seems fine to me too, nice stuff @andypost!
Comment #23
andypostSo here we go!
I think field patch better to save as one
Comment #24
yched CreditAttribution: yched commentedAwesome !
Should we split in different sub issues then ?
Comment #25
swentel CreditAttribution: swentel commentedTalked 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 :/
Comment #26
swentel CreditAttribution: swentel commentedOne thing I'm wondering: shouldn't we do entity_load()->delete() here ?
Comment #27
yched CreditAttribution: yched commented"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)
Comment #28
andypostAlso @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 inFieldUnitTestBase::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
Comment #29
yched CreditAttribution: yched commented1953410-field.txt removes the functions completely, so it should be last to go.
Reviewing 1953410-field_other.txt for now.
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)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.
Same here, $field and $instance can be inlined.
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.
Commented lines ?
(next hunk as well)
Comment #30
andypostaddressed #29 in commit 00613d18
Comment #31
andypostremoved another 2 commented lines
Comment #32
andypostFiled first sub-issue #1973324: Mark field_create_*(), field_update_*() and field_delete_*() deprecated in favor of just using the ConfigEntity API
Will collect 'em in summary
Comment #32.0
andypostUpdated summary
Comment #34
yched CreditAttribution: yched commentedEr, 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 ?
Comment #35
andypostmerged in sandbox, just to get current state
Comment #36
andypostFix broken tests
Comment #37
andypostFiled follow-ups (added to head)
#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
#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
Also testing sandbox
Comment #37.0
andypostUpdated issue summary.
Comment #39
andypostShould be fine, pushed
Comment #40
aspilicious CreditAttribution: aspilicious commentedThis 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?
:)
Comment #41
aspilicious CreditAttribution: aspilicious commentedhttp://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
Comment #42
aspilicious CreditAttribution: aspilicious commentedfield_create_field
field_update_field
field_delete_field
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.
Comment #43
swentel CreditAttribution: swentel commented@aspilicious: FieldStorageController.php doesn't even exist anymore ...
Comment #44
aspilicious CreditAttribution: aspilicious commentedhmmm...
Lets see what happened...
Comment #45
aspilicious CreditAttribution: aspilicious commentedSo 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
Comment #46
amateescu CreditAttribution: amateescu commentedNo, it's not. This will break #1875992: Add EntityFormDisplay objects for entity forms in so many places that it's not even funny.
Comment #47
andypostMerged after #1875992: Add EntityFormDisplay objects for entity forms and pushed
Comment #48
andypostComment #49
aspilicious CreditAttribution: aspilicious commentedSmall spacing issue
Can someone fix this so I can rtbc this. I don't want to rtbc my own reroll :)
Comment #50
andypost@aspilicious thanx for review, here's fixed patch
Comment #51
swentel CreditAttribution: swentel commentedSo, 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 :/
Comment #52
andypostWe got a sprint this weekend so - is it ok to split the patch and re-open issues?
Comment #53
swentel CreditAttribution: swentel commented@andypost - that's fine - sorry for the trouble.
Comment #53.0
swentel CreditAttribution: swentel commentedadded issues
Comment #54
swentel CreditAttribution: swentel commentedLet's keep this as the meta and just active.
Comment #55
andypostI'd better use the issue to test sandbox code
Comment #55.0
andypostUpdated issue summary.
Comment #56
fubhy CreditAttribution: fubhy commentedPlease 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).
Comment #57
pcambraAdding 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.
Comment #58
swentel CreditAttribution: swentel commentedThis is done, woot, thanks all!
Comment #59
andypostIs there any change notice about?
Comment #60
swentel CreditAttribution: swentel commentedI guess the convert field api to cmi one
Comment #61
andypostUpdated here
Comment #62.0
(not verified) CreditAttribution: commentedUpdated issue summary.