Follow-up to #2287727: Rename FieldConfig to FieldStorageConfig
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 -- done in #2287727: Rename FieldConfig to FieldStorageConfig
2. Rename FieldInstanceConfig to FieldConfig
Current patch does 1.
API changes
- renames FieldInstanceConfig to FieldConfig
- renames FieldInstanceConfigInterface to FieldConfigInterface
- renames FieldInstanceConfigStorage to FieldConfigStorage
- renames entity type ID from 'field_instance_config' to 'field_config'
- renames CMI records from field.instance.* to field.field.*
- renames hook_field_instance_config_*() to hook_field_config_*() (hook_ENTITY_TYPE_*)
- renames FieldInstanceConfigDeleteForm to FieldConfigDeleteForm
- renames FieldInstanceEditForm to FieldEditForm
form_id : field_ui_field_instance_edit_form --> field_ui_field_edit_form
route name: field_ui.instance_edit_$entity_type_id --> field_ui.field_edit_$entity_type_id
For field types,
"settings" are renamed "storage settings",
"instance settings" are renamed "field settings"
In practice:
FieldItemInterface:
- renames defaultSetiings() to defaultStorageSettings()
- renames defaultInstanceSettings() to defaultFieldsettings()
- renames settingsForm() to storageSettingsForm()
- renames instanceSettingsForm() to fieldSettingsForm()
- renames settingsToConfigData() to storageSettingsToConfigData()
- renames instanceSettingsToConfigData() to fieldSettingsToConfigData()
- renames settingsFromConfigData() to storageSettingsFromConfigData()
- renames instanceSettingsFromConfigData() to fieldSettingsFromConfigData()
FieldTypePluginManager:
- renames getDefaultSettings() to getDefaultStorageSettings()
- renames getDefaultInstanceSettings() to getDefaultFieldSettings()
field types schema:
- field.[type].settings renamed to field.[type].storage_settings
- field.[type].instance_settings renamed to field.[type].field_settings
-----
Work happens in 2312093-rename-FieldInstanceConfig - helper issue is over at #2317047: Helper issue for #2312093: rename FieldInstanceConfig to FieldConfig
Comment | File | Size | Author |
---|---|---|---|
#55 | 2312093-yched.55.patch | 626.24 KB | alexpott |
Comments
Comment #1
xjmComment #2
swentel CreditAttribution: swentel commentedWill start on this and aim for finish during Drupalaton next week.
Comment #3
yched CreditAttribution: yched commentedThe final naming here is connected to the wider naming issue in #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields, which introduces a sister config entity type with a common interface and base class. Would be good if the names for those were consistent. I still think that FieldConfig is a valid contender here, but would be worth figuring out in the big picture.
Also, one aspect we overlooked in the 1st part of the rename (FieldConfig -> FieldStorageConfig) is about settings.
A field type class (FieldItemInterface) defines separate "settings" at a) the storage level and b) the field level. The corresponding API is still shaped in the old "$field / $instance" nomenclature :
- FieldItemInterface::defaultSettings(), FieldTypePluginManager::getDefaultSettings() = "storage settings"
- FieldItemInterface::defaultInstanceSettings(), FieldTypePluginManager::getDefaultInstanceSettings() = "field settings"
With the current rename plan, this should move to defaultSettings() + defaultStorageSettings()...
Comment #4
Berdir@yched: Speaking of those settings, I noticed today that FieldInstanceConfig settings still automatically fall back to FieldStorageConfig. With the explicit separation, we should possibly no longer do that but widgets/formatters and so on should be explicit about whether they want an field definition or a field storage definition setting?
Comment #5
yched CreditAttribution: yched commented@Berdir : yeah, I wondered about the "merge of settings" too, wasn't sure of the outcome...
This merge was initially made because, back then, "EntityNG fields don't have the $field / $instance split" ;-), and so for base fields all you had was a single bag of settings on the "field" object.
This assumption is stale now, but it's not clear to me where that puts us in terms of merging the settings...
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedI agree with #5: FDI and FSDI are now linked at the API level for both configurable and nonconfigurable fields, since FDI has a getFieldStorageDefinition() method defined on it. Therefore, I think we can remove the merging of the two settings in FDI and FSDI implementations.
However, widgets and formatters usually don't need to know or care whether a setting is a field-level or storage-level setting (and sometimes the split is arbitrary because some field types define a storage-level setting that has nothing to do with storage, but is just a shortcut for "better UX if it does not vary by bundle"). So perhaps we can implement the merging within
(Formatter|Widget)Base::getFieldSetting(s)()
instead?Comment #7
swentel CreditAttribution: swentel commentedAdded name of branch - will update summary later this week.
Comment #8
swentel CreditAttribution: swentel commentedComment #9
swentel CreditAttribution: swentel commentedComment #10
swentel CreditAttribution: swentel commentedComment #11
swentel CreditAttribution: swentel commentedFirst time green - still need to rename variables though.
Comment #13
swentel CreditAttribution: swentel commentedComment #15
swentel CreditAttribution: swentel commentedComment #16
swentel CreditAttribution: swentel commentedGreen, more renames, getting close. Unassigning for now, leaving tomorrow. Please use the branch if someone else takes over!
Comment #17
yched CreditAttribution: yched commentedW00t! Thanks @swentel!
Didn't review atm, just a note for now: #3 and "settings / instance_settings" also means renaming the config schema entries : field.[field_type].settings / field.[field_type].instance_settings :-/
Comment #18
swentel CreditAttribution: swentel commented@yched config schema has been renamed as well :) - luckily that was an easy search and replace - the hardest part are all occurences in comments and variable names.
However, I did one thing different: defaultInstanceSettings() -> defaultFieldSettings() - overlooked 3
But this should be an easy re-rename though :)
Comment #19
andypostre-rolled and pushed to sandbox.
1) Let's move bikeshed about settings merge to separate issue, #3 suggests to rename
FieldItemInterface::defaultSettings()
todefaultStorageSettings()
but patch does only
FieldItemInterface::defaultInstanceSettings() to ::defaultFieldSettings()
and I think that's enough because
defaultSettings()
are related to FieldType definition that currently used without storage2) Seems we need another follow-up to clean-up test messages
some strings already affected in the patch but let's decide the reasons to clean-up 'em in the patch or not!
3) I found that there's still 2 todo with link #2116341: Apply defaults to definition objects
no idea how to clean-up this properly
Comment #20
yched CreditAttribution: yched commentedWill try to review asap, but FWIW, the "API changes" section in #2287727: Rename FieldConfig to FieldStorageConfig's issue summary provides a good list of the related "side renames" we had to do for FieldStorageConfig (e.g. field_purge_field()...).
Comment #21
swentel CreditAttribution: swentel commentedNew green version
Comment #22
swentel CreditAttribution: swentel commentedKeeping up with HEAD
Comment #23
swentel CreditAttribution: swentel commentedKeeping up with HEAD
Comment #24
yched CreditAttribution: yched commentedAwesome work !
- fixed incorrect doc for FieldItemInterface::settingsForm() (field settings -> storage settings - that was a overlook from the FieldStorage rename)
- $storages var in FieldConfigStorage::loadByProperties() should be $fields
- replaced a couple leftover "instance"s in FieldConfig comments, docs & exception messages
- renamed a couple leftover $instance vars
- renamed createFieldWithInstance() test helper to createFieldWithStorage()
(+ merged HEAD and pushed to the sandbox)
Comment #25
yched CreditAttribution: yched commented- There was a leftover conflict marker in content_translation.module (in the middle of a comment, so no syntax error), which showed that the patch reintroduced content_translation_entity_bundle_field_info_alter(), that was removed in #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields
- We will need to be careful when merging with #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration, as it touches content_translation_form_field_ui_field_instance_edit_form_alter(), and does stuff in $form['instance']. Here we rename form_id and thus the associated alter hooh, and move to $form['field']
Comment #27
swentel CreditAttribution: swentel commentedYeah merging is sometimes tedious with this patch.
So who's going to review and RTBC this now ? :)
Comment #28
andypostI'm following/reviewing changes and last 60k changes in comments are great!
It does not matter who pull the trigger... but who will help the commiter to check a whole codebase for comments that we probably forget to fix...
Maybe it make sense to update summary with a steps to help reviewer to go through?
Comment #29
yched CreditAttribution: yched commentedFixed an occurrence of entity_create('field_instance_config') that got added in the last HEAD merge.
Lol. This just got nominated for the "Understatement Of The Year" award :-D
Yeah, I hesitated a bit before jumping in and pushing code myself, but it would have been tedious. For such patches, I think we have to relax the rules a bit and state that it's ok to RTBC based on people reviewing each other's changes.
Comment #30
swentel CreditAttribution: swentel commentedYeah, I guess. Also, with the previous patch, I've did the final patch together while Alex was reviewing on IRC and then he assigned it to catch after. I think this is the way to go again.
Comment #32
yched CreditAttribution: yched commentedWell, IMO we have to make a best effort here, but we have to accept the fact that there will probably be leftovers that we will stumble on and fix later on.
A couple thoughts:
- The word "instance" is used for non-Field-related stuff pretty heavily in core
- But most Field-related code now uses Core/Field interfaces, in which the notion of "field instance" is not used
- Thus, the last uses of "instance" in a Field context are in code specifically related to configurable fields
--> IMO doing a full-text search for "instance" within field.module / field_ui.module, and doing the correct replaces, would be that "best effort" ?
Comment #33
yched CreditAttribution: yched commentedDoing this. I'll probably leave out the "Field API explanation" docblock at the top of field.module, opened #2329665: Move the "Field API doc" topic out of field.module for this.
Comment #34
yched CreditAttribution: yched commentedRemoves the remaining uses of "(field) instance" within field and field_ui modules.
Comment #35
yched CreditAttribution: yched commentedSo the last thing that worries me here is how we name "settings" - mostly in method names in FieldItemInterface & FieldTypePluginManager :
FII:default*Settings()
FII:*settingsToConfigData()
FII:*settingsFromConfigData()
FII:*settingsForm()
FTPM::getDefault*Settings()
The current patch does:
"settings" = the storage settings
"field settings" = the field (ex-instance) settings,
1. It means the storage is the primary notion when you code your FieldItem class, which IMO is not the most natural option. The whole Field API D8 uses FieldDefinition objects as the primary notion.
2. "a field type has settings and field settings" feels like a WTF.
"a field type has settings and storage settings" seems more natural
or "a field type has field settings and storage settings" if we want to be unambiguous, and not put one upfront ?
Comment #36
swentel CreditAttribution: swentel commented+ 1 for "a field type has field settings and storage settings"
IMO if we then also go for say defaultFieldSettings() and defaultStorageSettings(), Field{whatever} and {Storage}whatever, there is really no way to get confused. It might be a little verbose but it feels really clear to me then.
Comment #37
yched CreditAttribution: yched commentedAgreed, "field settings" / "storage settings" might be the cleanest. Feel like doing the renames, I'm mosly done for today :-) ?
Comment #38
yched CreditAttribution: yched commentedReroll after #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration
Comment #39
swentel CreditAttribution: swentel commentedNot before monday evening, I can do it then. Unless someone beats me to it.
Comment #40
fago"field settings" / "storage settings" makes a lot of sense to me as well.
Comment #41
yched CreditAttribution: yched commentedOK, did it, then :-)
Renamed "settings" methods in FieldItemInterface and FieldTypePluginManagerInterface as discussed in #35 - #37 :
FII::defaultStorageSettings()
FII::defaultFieldSettings()
FII::storageSettingsToConfigData()
FII::fieldSettingsToConfigData()
FII::storageSettingsFromConfigData()
FII::fieldSettingsFromConfigData()
FII::storageSettingsForm()
FII::fieldSettingsForm()
FTPM::getDefaultStorageSettings()
FTPM::getDefaultFieldSettings()
Added that to the issue summary
Comment #43
yched CreditAttribution: yched commentedA bit too much trust in PhpStorm's rename, didn't catch all of #41's method renames.
This should at least install.
Comment #46
yched CreditAttribution: yched commentedShould be green.
Comment #47
yched CreditAttribution: yched commentedA couple more var renames, and a few form fixes.
Comment #48
swentel CreditAttribution: swentel commentedKeeping up with HEAD. Also renamed most of the documentation. I think we're ready to go here.
Comment #49
BerdirWhen adding FieldStorageDefinitionInterface, there was some discussion that it is not the field storage. It is the definition for the field storage. There are a lot of documentation changes that do "fields" => "field storages", I'm not sure that's really correct? Don't have better suggestions, though.
This is weird, what is a "field bundle" ? Maybe use "entity bundle" ?
FieldInstanceCrudTest is renamed to FieldCrudTest, what about CrudTest? Should we rename that to FieldStorageCrudTest?
Not sure about the comment, similar to above. We do this a million times, so maybe just remove the comment? ;)
Also, this is the only test that is using _definition for the variables.
This seems to be a left-over that I think I removed in the original node.html.twig. Those no longer exist => Remove it.
Comment #50
swentel CreditAttribution: swentel commentedYeah, documentation updates in the patch are tedious :)
Yched also suggested to move these to a follow up, I'd be in favor of that :)
Comment #51
swentel CreditAttribution: swentel commentedno changes, just a reroll
Comment #52
swentel CreditAttribution: swentel commentedKeeping up .. up .. up
Comment #54
swentel CreditAttribution: swentel commentedComment #55
alexpottMerged 8.0.x and made further fixes.
Comment #56
swentel CreditAttribution: swentel commentedOk, back to green thanks to Alex. Has been green a couple of times and has been +1'd by yched as well.
Things that can be nitpicked in this patch is terminology in some docs or variable names, but should not hold up this patch as it's big enough already.
These things can be cleaned up afterwards and are ideal novice issues which are perfect for Amsterdam.
The list of change records to update are more or less the same ones as in #2287727: Rename FieldConfig to FieldStorageConfig - a full list is available in the comments there, see https://www.drupal.org/node/2287727#comment-8985027 - will scan for more if needed.
Comment #57
tim.plunkettIs this right? I don't see any methods called that, either in HEAD or changed in this patch.
Not to hold up commit, just that its very confusing.
Comment #58
effulgentsia CreditAttribution: effulgentsia commented+1 to the RTBC. Some followup material:
Huh?
Huh?
Should be
fieldSettingsToConfigData
? Do we not have test coverage for this?Why is this JS no longer needed?
Comment #59
webchickI'm high on cold meds atm so am not able to review this thoroughly, but in glancing through it looks pretty straight-forward. Thanks for identifying issues to fix in follow-ups.
Committed and pushed to 8.x. Thanks!
Comment #61
andypostboth methods simply
return $settings;
so it worksThe test coverage needs separate issue.
Comment #62
alexpottRe #58:
Comment #63
yched CreditAttribution: yched commentedYay ! Thanks all !
Comment #64
xjmCan we please file followups? The 600K beta deadline part of this is done so let's keep that queue clean.
Also, can someone confirm when the change record updates are completed? :)
Comment #65
anavarreIn the same vein as #2287727 (#66), if you have config files from a custom module you wish to mass-update, run the below command from within your custommodule/config/install dir after you've backed up everything first:
$ find . -name 'field.instance*' -execdir bash -c 'mv -i "$1" "${1//field.instance/field.field}"' bash {} \;
Comment #66
andypostFiled #2343483: Doc cleanup after FieldConfig
Comment #67
yched CreditAttribution: yched commentedRelated : #2344821: Unify 'name' / 'field_name' in FieldConfig / FieldStorageConfig
(beta deadline :-/)
Comment #68
swentel CreditAttribution: swentel commentedUpdated all change record listed in https://www.drupal.org/node/2287727#comment-8985027 + a couple more.
Comment #70
vijaycs85we need to update https://www.drupal.org/node/2064123 as well.
Comment #71
vijaycs85Here is the updated CR - https://www.drupal.org/node/2064123/revisions/view/7363139/7774515
Comment #72
yched CreditAttribution: yched commentedLooks good. Thanks @vijaycs85 !
Comment #73
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis change caused a regression in the test coverage of the default images, here's a followup for it: #2903332: Regression: lost test coverage for handling default images in the Image field