Rename FieldInstanceConfig to FieldConfig

Will start on this and aim for finish during Drupalaton next week.

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

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

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

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

Added name of branch - will update summary later this week.

First time green - still need to rename variables though.

Green, more renames, getting close. Unassigning for now, leaving tomorrow. Please use the branch if someone else takes over!

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

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

re-rolled and pushed to sandbox.

1) Let's move bikeshed about settings merge to separate issue, #3 suggests to rename
FieldItemInterface::defaultSettings() to defaultStorageSettings()
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 storage

2) Seems we need another follow-up to clean-up test messages

+++ b/core/modules/comment/src/Tests/CommentNodeChangesTest.php
@@ -28,10 +28,10 @@ function testNodeDeletion() {
     $this->assertNotNull(entity_load('field_storage_config', 'node.comment'), 'Comment field exists');
-    $this->assertNotNull(entity_load('field_instance_config', 'node.article.comment'), 'Comment instance exists');
+    $this->assertNotNull(entity_load('field_config', 'node.article.comment'), 'Comment instance exists');
...
     $this->assertNull(entity_load('field_storage_config', 'node.comment'), 'Comment field deleted');
-    $this->assertNull(entity_load('field_instance_config', 'node.article.comment'), 'Comment instance deleted');
+    $this->assertNull(entity_load('field_config', 'node.article.comment'), 'Comment instance deleted');

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

+++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
@@ -62,7 +62,7 @@ public static function create($type) {
  public static function create($type) {
    $field_definition = new static(array());
    $field_definition->type = $type;
    $field_definition->itemDefinition = FieldItemDataDefinition::create($field_definition);
    // Create a definition for the items, and initialize it with the default
    // settings for the field type.
    // @todo Cleanup in https://drupal.org/node/2116341.
    $field_type_manager = \Drupal::service('plugin.manager.field.field_type');
-    $default_settings = $field_type_manager->getDefaultSettings($type) + $field_type_manager->getDefaultInstanceSettings($type);
+    $default_settings = $field_type_manager->getDefaultSettings($type) + $field_type_manager->getDefaultFieldSettings($type);
     $field_definition->itemDefinition->setSettings($default_settings);
     return $field_definition;

  /**
   * Helper to retrieve the field item class.
   *
   * @todo: Remove once getClass() adds in defaults. See
   * https://drupal.org/node/2116341.
   */
  protected function getFieldItemClass() {
    if ($class = $this->getItemDefinition()->getClass()) {
      return $class;
    }
    else {
      $type_definition = \Drupal::typedDataManager()
        ->getDefinition($this->getItemDefinition()->getDataType());
      return $type_definition['class'];
    }
  }

no idea how to clean-up this properly

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

New green version

Keeping up with HEAD

Keeping up with HEAD

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

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

Yeah merging is sometimes tedious with this patch.

So who's going to review and RTBC this now ? :)

I'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?

Fixed an occurrence of entity_create('field_instance_config') that got added in the last HEAD merge.

Yeah merging is sometimes tedious with this patch

Lol. This just got nominated for the "Understatement Of The Year" award :-D

So who's going to review and RTBC this now ? :)

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.

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.

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

but who will help the commiter to check a whole codebase for comments that we probably forget to fix

Well, 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" ?

IMO doing a full-text search for "instance" within field.module / field_ui.module, and doing the correct replaces, would be that "best effort" ?

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

Removes the remaining uses of "(field) instance" within field and field_ui modules.

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

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

Agreed, "field settings" / "storage settings" might be the cleanest. Feel like doing the renames, I'm mosly done for today :-) ?

Reroll after #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration

Agreed, "field settings" / "storage settings" might be the cleanest. Feel like doing the renames, I'm mosly done for today :-) ?

Not before monday evening, I can do it then. Unless someone beats me to it.

"field settings" / "storage settings" makes a lot of sense to me as well.

OK, 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

A bit too much trust in PhpStorm's rename, didn't catch all of #41's method renames.
This should at least install.

Should be green.

A couple more var renames, and a few form fixes.

Keeping up with HEAD. Also renamed most of the documentation. I think we're ready to go here.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
   @@ -16,8 +16,8 @@
     * ensure that configuration entities are imported in the correct order. For
   - * example, node types are created before their fields and the fields are
   - * created before their field instances.
   + * example, node types are created before their field storages and the field
   + * storages are created before their fields.
   

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

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
   @@ -1754,7 +1754,7 @@ public static function _fieldSqlSchema(FieldStorageDefinitionInterface $storage_
              'default' => '',
   -          'description' => 'The field instance bundle to which this row belongs, used when deleting a field instance',
   +          'description' => 'The field bundle to which this row belongs, used when deleting a field',
            ),
   

   This is weird, what is a "field bundle" ? Maybe use "entity bundle" ?

3. +++ b/core/modules/field/src/Tests/ConfigFieldDefinitionTest.php
   index ec9b4d7..770155a 100644
   --- a/core/modules/field/src/Tests/CrudTest.php
   
   --- a/core/modules/field/src/Tests/CrudTest.php
   +++ b/core/modules/field/src/Tests/CrudTest.php
   

   FieldInstanceCrudTest is renamed to FieldCrudTest, what about CrudTest? Should we rename that to FieldStorageCrudTest?

4. +++ b/core/modules/field/src/Tests/CrudTest.php
   @@ -212,13 +212,13 @@ function testRead() {
   -    // Create an instance of the field.
   -    $instance_definition = array(
   +    // Create a field from the field storage.
   

   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.

5. +++ b/core/themes/bartik/templates/node.html.twig
   @@ -61,12 +61,12 @@
     *
   - * In field variables, each field instance attached to the node a corresponding
   - * variable is defined; for example, 'node.body' becomes 'body'. When needing to
   - * access a field's raw values, developers/themers are strongly encouraged to
   - * use these variables. Otherwise they will have to explicitly specify the
   - * desired field language; for example, 'node.body.en', thus overriding any
   - * language negotiation rule that may have been applied previously.
   + * In field variables, each field attached to the node a corresponding variable
   + * is defined; for example, 'node.body' becomes 'body'. When needing to access
   + * a field's raw values, developers/themers are strongly encouraged to use
   + * these variables. Otherwise they will have to explicitly specify the desired
   + * field language; for example, 'node.body.en', thus overriding any language
   + * negotiation rule that may have been applied previously.
     *
   

   This seems to be a left-over that I think I removed in the original node.html.twig. Those no longer exist => Remove it.

Yeah, documentation updates in the patch are tedious :)
Yched also suggested to move these to a follow up, I'd be in favor of that :)

no changes, just a reroll

Keeping up .. up .. up

Merged 8.0.x and made further fixes.

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

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -137,7 +135,7 @@ public function purgeFieldData(FieldDefinitionInterface $field_definition, $batc
+   * onFieldDelete() or onFieldDelete() has previously run.

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

+1 to the RTBC. Some followup material:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -137,7 +135,7 @@ public function purgeFieldData(FieldDefinitionInterface $field_definition, $batc
   -   * onFieldDelete() or onFieldInstanceDelete() has previously run.
   +   * onFieldDelete() or onFieldDelete() has previously run.
   

   Huh?

2. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -32,7 +32,7 @@
   +   * The name of the field attached to the bundle by this field.
   

   Huh?

3. +++ b/core/lib/Drupal/Core/Field/FieldConfigStorageBase.php
   @@ -39,7 +39,7 @@ protected function mapFromStorageRecords(array $records) {
   -    $record['settings'] = $class::instanceSettingsToConfigData($record['settings']);
   +    $record['settings'] = $class::fieldSettingsFromConfigData($record['settings']);
   

   Should be fieldSettingsToConfigData? Do we not have test coverage for this?

4. +++ b/core/modules/user/user.js
   @@ -174,25 +174,4 @@
   -  /**
   -   * Field instance settings screen: force the 'Display on registration form'
   -   * checkbox checked whenever 'Required' is checked.
   -   */
   -  Drupal.behaviors.fieldUserRegistration = {
   -    attach: function (context, settings) {
   -      var $checkbox = $('form#field-ui-field-edit-form input#edit-instance-settings-user-register-form');
   -
   -      if ($checkbox.length) {
   -        $(context).find('input#edit-instance-required').once('user-register-form-checkbox', function () {
   -          $(this).on('change', function (e) {
   -            if ($(this).prop('checked')) {
   -              $checkbox.prop('checked', true);
   -            }
   -          });
   -        });
   -
   -      }
   -    }
   -  };
   -
   

   Why is this JS no longer needed?

I'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!

+++ b/core/lib/Drupal/Core/Field/FieldConfigStorageBase.php
@@ -39,7 +39,7 @@ protected function mapFromStorageRecords(array $records) {
-    $record['settings'] = $class::instanceSettingsToConfigData($record['settings']);
+    $record['settings'] = $class::fieldSettingsFromConfigData($record['settings']);

Should be fieldSettingsToConfigData? Do we not have test coverage for this?

both methods simply return $settings; so it works

The test coverage needs separate issue.

Re #58:

1. Yep needs fixing
2. Yep needs fixing
3. Yep
4. Because the "user-register-form-checkbox" no longer exists this - this js been redundant this became a form mode :)

Yay ! Thanks all !

In 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 {} \;

Filed #2343483: Doc cleanup after FieldConfig

Related : #2344821: Unify 'name' / 'field_name' in FieldConfig / FieldStorageConfig
(beta deadline :-/)

Updated all change record listed in https://www.drupal.org/node/2287727#comment-8985027 + a couple more.

we need to update https://www.drupal.org/node/2064123 as well.

Here is the updated CR - https://www.drupal.org/node/2064123/revisions/view/7363139/7774515

Looks good. Thanks @vijaycs85 !

+++ b/core/modules/image/src/Tests/ImageFieldDefaultImagesTest.php
@@ -37,13 +37,13 @@ public function testDefaultImages() {
-    foreach (array('field', 'instance', 'instance2', 'field_new', 'instance_new') as $image_target) {
+    foreach (array('field', 'field', 'field2', 'field_new', 'field_new') as $image_target) {

This 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