Problem/Motivation
In the past couple weeks, the Core/Field API has settled on a terminology of :
- FieldStorageDefinition : the "shared" properties that define a storage bucket for field data, relevant for a given entity type
- FieldDefinition : the definition of an actual field atached to an actual bundle of the above entity type
This exactly replicates the split between "field" and "field instance" that comes from D7, and that is still used for configurable fields :
- FieldConfig is exactly what the Core/Field API uses as a FieldStorageDefinition
- FieldInstanceConfig is exactly what the Core/Field API uses as a FieldDefinition
This terminology split produces some very confusing artifacts (var names, class hierarchy), and forces you to switch brain modes when crossing between Core/Field and field.module.
#2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields is adding a FieldConfigBase, with
- FieldConfig *not* extends FieldConfigBase
- FieldInstanceConfig extends FieldConfigBase
Proposed resolution
We should bite the bullet and apply our terminology consistently :
- what D7 called a $field is a FieldStorageDefinition
- what D7 called an $instance is a FieldDefinition
This is not a proposal at this point, that's what HEAD does already. We just have one subpart of the system that still uses the old terminology, while the system itself uses a new terminology (with one word, "field", being used as "that part" in one case and "the other part" in the other case - making it the perfect WTF :-)
Thus :
- FieldConfig should be renamed FieldStorageConfig
- entity type : field_storage_config,
- CMI names : field.storage.[entity_type].[field_name]
- FieldInstanceConfig should be renamed FieldConfig
- entity type : field_config,
- CMI names : field.field.[entity_type].[bundle].[field_name]
Given we're renaming A to B and C to A, it's much saner to approach this in two successive patches :
1. Rename FieldConfig to FieldStorageConfig
2. Rename FieldInstanceConfig to FieldConfig
Current patch does 1.
API changes
- renames FieldConfig to FieldStorageConfig
- renames FieldConfigInterface to FieldStorageConfigInterface
- renames FieldConfigStorage to FieldStorageConfigStorage
- renames entity type ID from 'field_config' to 'field_storage_config'
- renames CMI records from field.field.* to field.storage.*
- renames hook_field_config_*() to hook_field_storage_config_*() (hook_ENTITY_TYPE_*)
- renames $values['field'] to $values['field_storage'] in entity_create('field_instance_config', $values)
- renames $conditions['field_uuid'] to $conditions['field_storage_uuid'] in entity_load_multiple_by_properties('field_instance_config', $conditions)
- renames state('field.field.deleted') to state('field.storage.deleted')
- renames (hook_)field_purge_field() to (hook_)field_purge_field_storage()
- renames FieldEditForm to FieldStorageEditForm
form_id : field_edit_form --> field_storage_edit_form
URL: .../fields/[instance_id]/field --> .../fields/[instance_id]/storage
route name: field_ui.field_edit_$entity_type_id --> field_ui.storage_edit_$entity_type_id
Comment | File | Size | Author |
---|---|---|---|
#58 | interdiff.txt | 35.04 KB | swentel |
#58 | 2287727-rename_FieldConfig-58.patch | 521.88 KB | swentel |
#11 | 2287727.11.patch | 388.15 KB | alexpott |
#13 | 11-13-interdiff.txt | 27.07 KB | alexpott |
#13 | 2287727.13.patch | 417.23 KB | alexpott |
Comments
Comment #1
yched CreditAttribution: yched commentedOne interesting consequence would be :
CMI config sync for fields currently works because field.field.* happens to come before field.instance.* alphabetically :-)
We would have to force the ordering somehow if the above move to field.storage.* & field.field.* respectively.
Comment #2
andypostSuppose dependencies should solve ordering of import.
Also there's some conflict with "node body" field that created automatically and I found no way to ship it as config.
Comment #3
yched CreditAttribution: yched commentedIs that really related ? You're not telling much.
Comment #4
effulgentsia CreditAttribution: effulgentsia commented+1 to the rename being logical. Unsure about whether the improvement to clarity is worth the disruption it would cause at this point in the cycle. Seems to me like it would be way too disruptive to do after beta though, so tagging as beta deadline, meaning we should either fix it or "won't fix" it before then.
Comment #5
yched CreditAttribution: yched commentedAgreed that this needs to happen before beta or not at all (= D9).
To fuel my position :
I think the actual code impact of such a patch would be much smaller than it would have been a couple months ago.
Most (and more and more) of the code base already works on the generic Core/Field API interfaces. It's mostly our tests that are filled with configurable "fields and instances", for practical and historical reasons.
The disruption in terms of actual contrib code breaking would mostly be limited to modules that ship fields & instances in their default config (or create them dynamically like the body field on nodes).
The "conceptual disruption" ("what you knew as Field and Instance are now respectively FieldStorage and Field") has already happened, and is something that people coming from D7 will need to make with no matter what. Needing to add "well, except for configurable fields, that still use the old, semi-reversed terminology" to that explanation would put D8 in a really awkward position IMO.
Comment #6
andypostFor contrib there could be a script to rename all config entities.
+1 to rename
Comment #7
swentel CreditAttribution: swentel commented+1, this makes a lot of sense. From feedback I've had, even the D7 field an field instance terminology confuses a lot of people, this should make it way more clearer.
How do we a green light ? And a vacation after that ? :)
Comment #8
alexpottGiven the importance of the Entity Field system to the success of Drupal I think that we are justified in spending the time to make sure it is grokable as easily as possible. The fact that we have "semi-reversed terminology" I think is unacceptable since just when you think you know the system you have to flip your mind around to get the other half. Anything that brings base fields and configurable fields closer together and make more logical sense is good for me. I wish we'd bit the bullet and completely removed the distinction between configurable fields and base fields - this continues to add complexity.
So whilst I hate the idea we're renaming all the things again. I think that this will help people immensely. The current situation confuses even the most seasoned core developers...
So, reluctantly, I'm also +1 - will get consensus from other committers before approving.
Comment #9
catchIt feels like the main change here for module maintainers would be the CMI file rename for default field config. That should be limited to git mv though so it's an easy change to make (and generally specific default fields tends to be something that custom modules do a lot more than contrib).
If the other changes are mostly internal then that seems pretty harmless, but definitely needs to be beta deadline.
Comment #10
alexpottI'm having a go at FieldConfig to FieldStorageConfig - btw I definitely think we should do this in two parts.
Comment #11
alexpottSo here is the part 1 - renaming FieldConfig to FieldStorageConfig. Notable things:
Comment #13
alexpottForget to commit some stuff... lol.
Comment #14
yched CreditAttribution: yched commentedw00t ! Awesome , thanks @alexpott !!!
Hem, un(?)fortunately, I'm not where I can review this for the next couple days... Any takers ? ;-)
Comment #15
yched CreditAttribution: yched commented@alexpott :
Well, I'm interested in anything that narrows the gap, but they still have one inherent major difference : one is defined in code and one is defined in config...
Curious to know whether you have something specific in mind ?
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedSee #2289551: Clarify what 'locked' means for a config entity and whether it's okay for code to rely on a locked config entity existing for a related discussion on that. I think it's tangential to this issue though, so not adding that as a related issue; it's really just an aside.
Comment #17
yched CreditAttribution: yched commented@alexpott: is this patch on a sandbox somewhere ?
Comment #18
BerdirI know this is a beast to re-roll but I'd love to get #2144263: Decouple entity field storage from configurable fields in first, it's a beta blocker and it converts usage of these interfaces to the generic field definition interfaces, then those will just no longer have to be touched in this patch, instead of renaming them and then having to merge this into the other issue.
I hope we're getting close on that one...
Comment #19
yched CreditAttribution: yched commentedYup, agreed that it would be best to get #2144263: Decouple entity field storage from configurable fields in first.
Comment #20
yched CreditAttribution: yched commentedRerolled on current HEAD. Will need another reroll after #2144263: Decouple entity field storage from configurable fields, but at least we're current.
Also, pushed to the 2287727-rename_FieldConfig branch in my sandbox
Comment #21
yched CreditAttribution: yched commentedForgot the patch.
[edit : patch size difference with #13 is ppbly because my patch is rolled with "rename = copies", wich reduces the renames of FieldConfig.php]
Comment #23
yched CreditAttribution: yched commentedAlso - FieldConfigStorage will need to be renamed to ... FieldStorageConfigStorage :-/
Comment #24
yched CreditAttribution: yched commentedA couple "use" errors. Should fix install.
Comment #26
yched CreditAttribution: yched commentedMissed one s/->uuid/->uuid()/ in the merge
Comment #28
yched CreditAttribution: yched commentedDrats. Merge error, too. That one should be back to green.
Comment #30
yched CreditAttribution: yched commentedAnd #2144263: Decouple entity field storage from configurable fields landed meanwhile :-)
Comment #31
yched CreditAttribution: yched commentedSigh. Somehow forgot to resolve conflicts in the most important file (ContentEntityDatabaseStorage) :-p
Opened #2295803: Helper issue for #2287727 - Rename FieldConfig to stop polluting this one.
Comment #32
yched CreditAttribution: yched commented- reolled on current HEAD,
- changed $values['field'] to $values['field_storage'] in entity_create('field_instance_config', $values)
- More $field / $field_storage var renames - I think I got all those that can be found by pattern searching
@todo :
Rename FieldConfigStorage to FieldStorageConfigStorage... unless someone has a better idea :-/
Rename FieldEditForm to FieldStorageEditForm
Do a pass on exception/error messages
Comment #34
yched CreditAttribution: yched commentedNever say "rerolled on current HEAD" unless you just did a pull.
Also, it would help if #2282627: Remove field_uuid from field instance config records got committed (one less "field"y thing to rename)
Comment #35
yched CreditAttribution: yched commentedRenames :
- FieldConfigStorage --> FieldStorageConfigStorage
- FieldEditForm --> FieldStorageEditForm
form_id : field_edit_form --> field_storage_edit_form
URL: .../fields/[instance_id]/field --> .../fields/[instance_id]/storage
route name: field_ui.field_edit_$entity_type_id --> field_ui.storage_edit_$entity_type_id
Comment #37
yched CreditAttribution: yched commentedMust have made a mistake while rolling #35. Interdiff is the same.
Comment #38
yched CreditAttribution: yched commentedComment #41
yched CreditAttribution: yched commentedLeftover URL
Comment #42
yched CreditAttribution: yched commentedComment #43
yched CreditAttribution: yched commented- rename state() 'field.field.deleted' entry, & update associated var names
- rename field_purge_field() & hook_field_purge_field()
- rephrase exception messages in FieldStorageConfig
- rename 'field_uuid' condition in entity_load_multiple_by_properties
- leftover var renames & type hinting in views hooks
Aside from running after HEAD, I think this is ready.
Comment #44
yched CreditAttribution: yched commentedFor the next reroll : leftover "// @todo update error messages.." that is actually done.
Comment #45
yched CreditAttribution: yched commentedReroll (and fixed #44)
Comment #46
yched CreditAttribution: yched commentedReroll
Comment #48
yched CreditAttribution: yched commentedReroll
Comment #49
yched CreditAttribution: yched commentedComment #50
swentel CreditAttribution: swentel commentedWent through it. Found mostly nitpicks, posting them for now, will try to fix a couple of them already in the branch (and update here if done)
For consistency, should be $field_storages then ? (even though it's in a test)
Should this then become 'Create a field storage and a field' ?
same as in 2
same
and again :)
'to the field storage' ?
'field storage deletion' ?
Wondering: should we rename this to 'field_storage_name' or is that for part 2 ?
over 80 chars
80 chars
Should this dependency not be on 'field_test_import' ?
And this one on '2' ?
80 chars
'... field storage exists.' ? (nitpick as it's a test)
Hmm, my initial reaction was, this should be _fieldStorageTableName() then, but I guess the class already has, so we're ok ?
Comment #51
swentel CreditAttribution: swentel commentednew patch - although one tests fails which I can't reproduce locally for now :/
Addressed 9, 10 and 13 already too and pushed to branch.
Comment #52
yched CreditAttribution: yched commented#50.2 to .5 :
"Should this then become 'Create a field storage and a field' ?"
Eventually, but that's for part 2 :-). Instances are still instances atm.
#50.8 :
No, I think "field_name" can stay "field_name". There are FieldStorage and FieldDefinition objects, they are connected through a field name. Still males sense IMO.
#50.11 & .12 :
Indeed.
#50.14 :
Fixed.
#50.15 :
That's fine IMO, a "field table" is still a "field table". A "field storage table" would be tedious, and I don't think there's much ambiguity.
Comment #53
yched CreditAttribution: yched commentedReshaped the issue summary and added a list of API changes.
Comment #54
yched CreditAttribution: yched commentedComment #55
yched CreditAttribution: yched commentedComment #56
swentel CreditAttribution: swentel commentedFound this one.
'... updated for the field storage configuration entity ..' ?
Talked to Alex on IRC and we'll go over the patch tomorrow during the day somewhere and fix any remaining nitpicks in case we find them and then hopefully commit it, since this is as good as RTBC anyway.
Comment #57
swentel CreditAttribution: swentel commentedChasing HEAD
Comment #58
swentel CreditAttribution: swentel commentedReview from alexpott, done on IRC, mostly renames (variables $field -> $field_storage, FieldConfigListBuilder -> FieldStorageConfigListBuilder)
Comment #59
alexpott+1 to rtbc
Comment #60
alexpotthttps://www.drupal.org/list-changes/published/drupal?keywords_descriptio...
https://www.drupal.org/list-changes/published/drupal?keywords_descriptio...
need checking.
Comment #61
yched CreditAttribution: yched commentedYay, thanks !
There might be a couple field.field.* that sneaked in meanwhile in config schemas (#2226063: Merge ListBooleanItem from options module into BooleanItem added / removed some recently)
@alexpott: do we go through the change records before or after the commit ? ;-)
Comment #62
catchFor updates I think it's OK to do after.
Committed/pushed to 8.x, thanks!
Comment #64
alexpottI've updated:
Second looks appreciated :)
Comment #65
yched CreditAttribution: yched commented*Thanks* @alexpott !
For now, stopped at https://www.drupal.org/node/2078765 in the list above - wow, that one is badly outdated :-/ Will try to overhaul it tomorrow.
Comment #66
anavarreIn case you got bitten by this like I was, here's a one-liner to fix things. Run the two below shell commands from within any config/install dir for any of your custom modules after you've backed up your files, just in case.
find . -name '*field.field*' -execdir bash -c 'mv -i "$1" "${1//field.field/field.storage}"' bash {} \; && grep -lir "field.field" * | xargs -i@ sed -i "s/field.field/field.storage/I" @
Comment #67
fagoGreat to see this land - thanks @yched and others.
It mentions step 2 - 2. Rename FieldInstanceConfig to FieldConfig. Is there already an issue for it? (even if targeted to d9...)
Comment #68
swentel CreditAttribution: swentel commentedDon't think so yet - it would be an ideal thing to work on next week during Drupalaton.
Comment #69
effulgentsia CreditAttribution: effulgentsia commented"needs work" for the FieldInstanceConfig part. If someone want to make a new issue for it instead, please copy over this one's summary and tags.
Comment #70
xjmFYI, Dreditor has a "clone issue" button that does just that (see next to view/edit/revisions under the title). :) Was one click, plus changing the title, setting the status to active, and indicating which part was already done in the summary. Edit: It's #2312093: Rename FieldInstanceConfig to FieldConfig.
Thanks @alexpott for the CR updates.
Comment #71
effulgentsia CreditAttribution: effulgentsia commentedDreditor++ :)
Comment #72
effulgentsia CreditAttribution: effulgentsia commentedretroactive title change for symmetry with the new issue
Comment #74
swentel CreditAttribution: swentel commentedReally quick follow up: #2344669: Rename the field to field storage in the annotation of fieldstorage