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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

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

andypost’s picture

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

yched’s picture

Also there's some conflict with "node body" field that created automatically and I found no way to ship it as config

Is that really related ? You're not telling much.

effulgentsia’s picture

Issue tags: +beta deadline

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

yched’s picture

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

andypost’s picture

For contrib there could be a script to rename all config entities.
+1 to rename

swentel’s picture

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

alexpott’s picture

Issue summary: View changes
Issue tags: +API change

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.

Given 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 what did not register for me was that field.override.* would be a field.instance for base fields

So, reluctantly, I'm also +1 - will get consensus from other committers before approving.

catch’s picture

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

alexpott’s picture

Assigned: Unassigned » alexpott

I'm having a go at FieldConfig to FieldStorageConfig - btw I definitely think we should do this in two parts.

alexpott’s picture

Status: Active » Needs review
FileSize
388.15 KB

So here is the part 1 - renaming FieldConfig to FieldStorageConfig. Notable things:

  • config files field.field.* to field.storage.*
  • hook_ENTITY_TYPE_* change from hook_field_config_* to hook_field_storage_config_*

Status: Needs review » Needs work

The last submitted patch, 11: 2287727.11.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
27.07 KB
417.23 KB

Forget to commit some stuff... lol.

yched’s picture

w00t ! Awesome , thanks @alexpott !!!

Hem, un(?)fortunately, I'm not where I can review this for the next couple days... Any takers ? ;-)

yched’s picture

@alexpott :

I wish we'd bit the bullet and completely removed the distinction between configurable fields and base fields

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 ?

effulgentsia’s picture

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 ?

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

yched’s picture

@alexpott: is this patch on a sandbox somewhere ?

Berdir’s picture

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

yched’s picture

Yup, agreed that it would be best to get #2144263: Decouple entity field storage from configurable fields in first.

yched’s picture

Rerolled 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

yched’s picture

Forgot 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]

Status: Needs review » Needs work

The last submitted patch, 21: 2287727-rename_FieldConfig-21.patch, failed testing.

yched’s picture

Also - FieldConfigStorage will need to be renamed to ... FieldStorageConfigStorage :-/

yched’s picture

Status: Needs work » Needs review
FileSize
358.34 KB
2.2 KB

A couple "use" errors. Should fix install.

Status: Needs review » Needs work

The last submitted patch, 24: 2287727-rename_FieldConfig-24.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
358.35 KB
894 bytes

Missed one s/->uuid/->uuid()/ in the merge

Status: Needs review » Needs work

The last submitted patch, 26: 2287727-rename_FieldConfig-26.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
358.34 KB
867 bytes

Drats. Merge error, too. That one should be back to green.

Status: Needs review » Needs work

The last submitted patch, 28: 2287727-rename_FieldConfig-28.patch, failed testing.

yched’s picture

yched’s picture

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

yched’s picture

Status: Needs work » Needs review
FileSize
451.17 KB
213.1 KB

- 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

Status: Needs review » Needs work

The last submitted patch, 32: 2287727-rename_FieldConfig-32.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
451.18 KB

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

yched’s picture

Renames :

- 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

Status: Needs review » Needs work

The last submitted patch, 35: 2287727-rename_FieldConfig-35.patch, failed testing.

yched’s picture

Must have made a mistake while rolling #35. Interdiff is the same.

yched’s picture

Status: Needs work » Needs review

The last submitted patch, 30: 2287727-rename_FieldConfig-30.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 37: 2287727-rename_FieldConfig-37.patch, failed testing.

yched’s picture

Leftover URL

yched’s picture

Status: Needs work » Needs review
yched’s picture

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

yched’s picture

For the next reroll : leftover "// @todo update error messages.." that is actually done.

yched’s picture

Reroll (and fixed #44)

yched’s picture

Reroll

Status: Needs review » Needs work

The last submitted patch, 46: 2287727-rename_FieldConfig-46.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

Reroll

yched’s picture

swentel’s picture

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

  1. +++ b/core/modules/config/src/Tests/ConfigImportAllTest.php
    @@ -69,8 +69,8 @@ public function testInstallUninstall() {
    +    $fields = \Drupal::entityManager()->getStorage('field_storage_config')->loadMultiple();
    

    For consistency, should be $field_storages then ? (even though it's in a test)

  2. +++ b/core/modules/entity/src/Tests/EntityDisplayTest.php
    @@ -140,15 +140,15 @@ public function testFieldComponent() {
    +    // Create a field storage and an instance.
    

    Should this then become 'Create a field storage and a field' ?

  3. +++ b/core/modules/entity/src/Tests/EntityDisplayTest.php
    @@ -317,15 +317,15 @@ public function testDeleteFieldInstance() {
    +    // Create a field storage and an instance.
    

    same as in 2

  4. +++ b/core/modules/entity/src/Tests/EntityFormDisplayTest.php
    @@ -49,16 +49,16 @@ public function testEntityGetFromDisplay() {
    +    // Create a field storage and an instance.
    

    same

  5. +++ b/core/modules/entity/src/Tests/EntityFormDisplayTest.php
    @@ -180,15 +180,15 @@ public function testDeleteFieldInstance() {
    +    // Create a field storage and an instance.
    

    and again :)

  6. +++ b/core/modules/field/field.api.php
    @@ -249,47 +249,47 @@ function hook_field_info_max_weight($entity_type, $bundle, $context, $context_mo
    + * to allow them to respond to the field being purged.
    

    'to the field storage' ?

  7. +++ b/core/modules/field/src/ConfigImporterFieldPurger.php
    @@ -31,27 +32,27 @@ public static function process(array &$context, ConfigImporter $config_importer)
           // case when the field deletion is staged.
    

    'field storage deletion' ?

  8. +++ b/core/modules/field/src/Entity/FieldInstanceConfig.php
    @@ -214,10 +214,11 @@ class FieldInstanceConfig extends ConfigEntityBase implements FieldInstanceConfi
        *   - field_name: The field name.
    

    Wondering: should we rename this to 'field_storage_name' or is that for part 2 ?

  9. +++ b/core/modules/field/src/FieldStorageConfigStorage.php
    @@ -97,34 +95,34 @@ public function loadByProperties(array $conditions = array()) {
    +    // for deleted storages only, this can be skipped, because they will be retrieved
    

    over 80 chars

  10. +++ b/core/modules/field/src/Tests/BulkDeleteTest.php
    @@ -252,34 +250,34 @@ function testPurgeInstance() {
    +    // The field storage still exists, not deleted, because it has a second instance.
    

    80 chars

  11. +++ b/core/modules/field/tests/modules/field_test_config/config/install/field.instance.entity_test.entity_test.field_test_import.yml
    @@ -11,3 +11,6 @@ default_value_function: ''
    +    - field.storage.entity_test.field_test_import_2
    

    Should this dependency not be on 'field_test_import' ?

  12. +++ b/core/modules/field/tests/modules/field_test_config/config/install/field.instance.entity_test.entity_test.field_test_import_2.yml
    @@ -11,3 +11,6 @@ default_value_function: ''
    +    - field.storage.entity_test.field_test_import
    

    And this one on '2' ?

  13. +++ b/core/modules/field/tests/src/FieldStorageConfigEntityUnitTest.php
    @@ -77,7 +78,7 @@ public function testCalculateDependencies() {
    +    // entity type and once in FieldStorageConfig::calculateDependencies() to get the
    

    80 chars

  14. +++ b/core/modules/forum/src/Tests/ForumUninstallTest.php
    @@ -87,13 +87,13 @@ function testForumUninstallWithField() {
    +    $this->assertNotNull($field_storage, 'The taxonomy_forums field exists.');
    

    '... field storage exists.' ? (nitpick as it's a test)

  15. +++ b/core/modules/taxonomy/taxonomy.views.inc
    @@ -432,7 +432,7 @@ function taxonomy_field_views_data_views_data_alter(array &$data, FieldConfigInt
    +    'field table' => ContentEntityDatabaseStorage::_fieldTableName($field_storage),
    

    Hmm, my initial reaction was, this should be _fieldStorageTableName() then, but I guess the class already has, so we're ok ?

swentel’s picture

new patch - although one tests fails which I can't reproduce locally for now :/
Addressed 9, 10 and 13 already too and pushed to branch.

yched’s picture

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

yched’s picture

Issue summary: View changes

Reshaped the issue summary and added a list of API changes.

yched’s picture

Issue summary: View changes
yched’s picture

Issue summary: View changes
swentel’s picture

Found this one.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
@@ -85,8 +85,8 @@ public function status();
+   * make sure that the storage tables are created or updated for the field
    * configuration entity, which is not a configuration change, and it must be

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

swentel’s picture

Chasing HEAD

swentel’s picture

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

Review from alexpott, done on IRC, mostly renames (variables $field -> $field_storage, FieldConfigListBuilder -> FieldStorageConfigListBuilder)

alexpott’s picture

+1 to rtbc

alexpott’s picture

yched’s picture

Yay, 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 ? ;-)

catch’s picture

Status: Reviewed & tested by the community » Fixed

For updates I think it's OK to do after.

Committed/pushed to 8.x, thanks!

  • catch committed a57630c on 8.x
    Issue #2287727 by yched, swentel, alexpott: Rename Field[instance]Config...
yched’s picture

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

anavarre’s picture

In 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" @

fago’s picture

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

swentel’s picture

Don't think so yet - it would be an ideal thing to work on next week during Drupalaton.

effulgentsia’s picture

Assigned: alexpott » Unassigned
Status: Fixed » Needs work

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

xjm’s picture

Status: Needs work » Fixed

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

effulgentsia’s picture

Dreditor++ :)

effulgentsia’s picture

Title: Rename Field[instance]Config classes and entity types (again) » Rename FieldConfig to FieldStorageConfig

retroactive title change for symmetry with the new issue

Status: Fixed » Closed (fixed)

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

swentel’s picture