As was decided in Prague in conclusion to #2051923: Rename \Drupal\Core\Entity\Field\Field, related subclasses and interfaces to *FieldItemList
(clarify the use of "Field" - value object or definition object ? see discussion notes)

The "definition objects for configurable fields and instances" are renamed FieldConfig / FieldInstanceConfig

API changes

This means the following renames

- classes / interfaces :
Fieldinterface -> FieldConfigInterface
Field -> FieldConfig
FieldStorageController -> FieldConfigStorageController
FieldInstanceInterfce -> FieldInstanceConfigInterafce
FieldInstance -> FieldInstanceConfig
FieldInstanceStorageController -> FieldInstanceConfigStorageController

- entity types
field_entity -> field_config
field_instance -> field_instance_config

- hooks:
hook_field_entity_(create|insert|update|delete)() -> hook_field_config_(create|insert|update|delete)()
hook_field_instance_(create|insert|update|delete)() -> hook_field_instance_config_(create|insert|update|delete)()

- yml files : unchanged

Remaining tasks

- Rename the storage controller classes
- Rename the interfaces (might be deferred to a followup to avoid the patch size exploding ?)

Change references to old names in the following change records:
- field_entity:

- field_instance:

- FieldInstance:

- FieldInterface:

Follow ups:
- #2200821: Rename Fieldinterface and FieldInstanceInterface
- #2201087: Rename hook_field_update_forbid to hook_field_config_update_forbid

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

Issue tags: +Field API

tagging

swentel’s picture

Status: Active » Needs review
FileSize
109.19 KB

Just testing .. currently discussing also whether to rename field_entity / field_instance to different id's or not.

swentel’s picture

Title: Rename 'field_entity' to 'field' in the field config entity » Rename 'field_entity' to 'field_config' and 'field_instance' to 'field_instance_config'

Also rename the classes to FieldConfig en FieldInstanceConfig.

Status: Needs review » Needs work

The last submitted patch, 2095303-2.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
227.91 KB

With the renames.

yched’s picture

- _entity_reference_field_instance_settings_ajax_process_element($element[$key], $main_form);
+ _entity_reference_field_instance_config_settings_ajax_process_element($element[$key], $main_form);

That's probably the automated search / replace, but I'm not sure we want to rename those (& a couple other ones in entity_ref)

Status: Needs review » Needs work

The last submitted patch, 2095303-5.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
225.45 KB

Fixed that - not sure why Drupal can't install though yet.

Status: Needs review » Needs work
Issue tags: -Field API

The last submitted patch, 2095303-8.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review

#8: 2095303-8.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Field API

The last submitted patch, 2095303-8.patch, failed testing.

jthorson’s picture

Testbot debugging:

During install, after the configuration page where you enter the site name, admin username/password, timezone, etc, and submit ... the site returns with "The field_config entity type does not exist".

This shows up as text on the install page, with nothing written to the apache exception log.

swentel’s picture

Status: Needs work » Needs review
FileSize
268.43 KB

Thank you! Found it. Just giving a testbot run now, need to rename the storage controller and interfaces as well.

Status: Needs review » Needs work

The last submitted patch, 2095303-13.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
2.92 KB
268.46 KB

Status: Needs review » Needs work

The last submitted patch, 2095303-15.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
270.27 KB

Status: Needs review » Needs work

The last submitted patch, 2095303-17.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
229.9 KB

rerolling

Status: Needs review » Needs work

The last submitted patch, 2095303-19.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
278.85 KB

Status: Needs review » Needs work

The last submitted patch, 2095303-21.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
271.54 KB

Major reroll

Status: Needs review » Needs work

The last submitted patch, 2095303-23.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
12.21 KB
201.74 KB

Re-roll, cleaning up the array BC removal, fixing some tests. The field instance edit form problem was related to some symfony black magic that maps request attributes to the name of the variable of the buildForm() method, had to change field_config there to field_config_instance, that should fix a lot of fails.

Not sure why the patch is so much smaller, there are a few instances where changes are no longer necessary, .e.g removed use's.

Status: Needs review » Needs work

The last submitted patch, 2095303-25.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.33 KB
203.85 KB

field_instance_config_config... yeah... ;)

Status: Needs review » Needs work

The last submitted patch, 2095303-27.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
694 bytes
238.05 KB

Fix in hook presave of image.module

amateescu’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinitionInterface.php
    @@ -26,16 +26,16 @@
    + * "field_instance_config" configuration entities. The former for storing configuration
    ...
    + * and bundle. The class that implements "field_instance_config" configuration entities
    ...
    + * field.module and its "field_config"/"field_instance_config" configuration entities.
    

    These comments need to be rewrapped.

  2. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockTypeTest.php
    @@ -143,8 +143,8 @@ public function testCustomBlockTypeDeletion() {
    -    $type = $this->createCustomBlockType('foo');
    -    $type = $this->createCustomBlockType('bar');
    +    $this->createCustomBlockType('foo');
    +    $this->createCustomBlockType('bar');
    

    How it this related? :)

  3. +++ b/core/modules/image/lib/Drupal/image/Tests/ImageFieldDisplayTest.php
    @@ -247,7 +247,8 @@ function testImageFieldDefaultImage() {
    +    debug($field->getFieldSetting('default_image'));
    

    Needs to go..

Note for further reviewers: This patch will bore you to death!

Status: Needs review » Needs work

The last submitted patch, 2095303-29.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
81.57 KB
307.83 KB

Renamed interfaces

Status: Needs review » Needs work

The last submitted patch, 2095303-29.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
204.36 KB

Let's not rename the interfaces just yet...

This is #27 + the interdiff of #29 (the patch in #29 seems to be too big?)

Berdir’s picture

+++ b/core/scripts/generate-d7-content.sh
+++ b/core/scripts/generate-d7-content.sh
@@ -93,7 +93,7 @@

@@ -93,7 +93,7 @@
       ),
     ),
   );
-  entity_create('field_entity', $field)->save();
+  entity_create('field_config', $field)->save();
   $node_types = $i > 11 ? array('page') : array_keys(node_type_get_types());
   foreach ($node_types as $bundle) {
     $instance = array(
@@ -132,7 +132,7 @@

@@ -132,7 +132,7 @@
         'settings' => array(),
       );
     }
-    entity_create('field_instance', $instance)->save();
+    entity_create('field_instance_config', $instance)->save();
   }

This script is meant to run in a 7.x environment I think and generate content that then can be exported into a dump to be used for upgrade path tests. But it was already broken before this change, so not sure what to do.

amateescu’s picture

I'd say revert the changes done by this patch and open a new issue to bring the old code back..

Berdir’s picture

34: 2095303-34.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 34: 2095303-34.patch, failed testing.

swentel’s picture

Assigned: swentel » Unassigned
Issue summary: View changes

unassinging for now

plopesc’s picture

Status: Needs work » Needs review
FileSize
303.62 KB

New patch from scratch. Let's push this issue.

This patch does not rename the interfaces or the Storage Controller as suggested in #13 yet.

Let's see testbot against this monster patch.

Regards.

plopesc’s picture

FileSize
308.17 KB

Re-rolling the monster patch...

yched’s picture

Reroll - should be a tad smaller because D7 -> D8 hook_update_N()s have been removed from core, so patch has a bit less hunks.

Also, pushed to the field_config_entities_rename-2095303 branch in the sandbox, should help for later rerolls.

Will try to review this asap.

yched’s picture

Better with an upload...

plopesc’s picture

Thank you for re-rolling @yched!

Before reviewing, maybe we should decide if we have to rename also the interfaces and the storage controller as proposed @swentel in #13 or keep them as they are now, as suggested @Berdir in #34 (maybe to avoid an even bigger patch).

Any thoughts?

yched’s picture

Yes, we do want to rename the interfaces & storage controller in the end.
For the interfaces, might be wiser to defer to a followup patch, or at least wait for the current patch to be RTBC. The storage controllers probably have a much smaller impact, maybe we could do that now ?

yched’s picture

side note : git diff --word-diff really helps reviewing this :-)

  1. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -208,12 +208,12 @@ public function addBodyField($entity_type, $field_name) {
    -    $field_instance = $this->entityManager
    -      ->getStorageController('field_instance')
    +    $field_instance_config = $this->entityManager
    

    No need to rename $field_instance to $field_instance_config if the $field variable in the same method isn't renamed similarly (and I'm not in favor of renaming it - keeping variables named $field / $instance or $field_instance is fine IMO)

  2. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
    @@ -187,7 +187,7 @@ class Field extends ConfigEntityBase implements FieldInterface {
       /**
        * The original field.
        *
    -   * @var \Drupal\field\Entity\Field
    +   * @var \Drupal\field\Entity\FieldConfig
        */
       public $original = NULL;
    

    We should actually remove that property now. Earlier in the D8 cycle we used to populate this property ourselves, but it's now populated automatically by the Entity::save() flow, it's not a real property of our own anymore.

    Same in FieldInstanceConfig.

  3. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
    @@ -563,7 +563,7 @@ public function isTranslatable() {
    -   * @return \Drupal\field\Entity\Field
    +   * @return \Drupal\field\Entity\FieldConfig
        *   The object itself for chaining.
    

    We should switch to the new core convention instead (#2158497: [policy, no patch] Use "$this" and "static" in documentation for @return types) :

    @return $this (with no description text - look for other examples of the above in core)

  4. +++ b/core/modules/image/image.module
    @@ -423,10 +422,10 @@ function image_filter_keyword($value, $current_pixels, $new_pixels) {
    -  if ($entity instanceof FieldInstance) {
    +  if ($entity instanceof FieldInstanceConfig) {
         $field = $entity->getField();
       }
    -  elseif ($entity instanceof Field) {
    +  elseif ($entity instanceof FieldConfig) {
    

    Actually, those should rather test $entity->getEntityTypeId()

  5. +++ b/core/modules/image/image.module
    @@ -495,18 +494,18 @@ function image_field_entity_update(FieldInterface $field) {
    -function image_field_instance_update(FieldInstanceInterface $field_instance) {
    -  $field = $field_instance->getField();
    +function image_field_instance_config_update(FieldInstanceInterface $field_instance_config) {
    

    the var name in image_field_config_update() / image_field_config_delete() is still $field
    so similarly, we could just keep $field_instance here.

    Same for image_field_instance_config_delete()

    Also, while we're in there, those hooks should hint at the interfaces instead.

  6. +++ b/core/modules/image/lib/Drupal/image/Tests/ImageFieldTestBase.php
    @@ -84,8 +84,8 @@ function createImageField($name, $type_name, $field_settings = array(), $instanc
         $instance['settings'] = array_merge($instance['settings'], $instance_settings);
    -    $field_instance = entity_create('field_instance', $instance);
    -    $field_instance->save();
    +    $field_instance_config = entity_create('field_instance_config', $instance);
    +    $field_instance_config->save();
    

    We could cleanup that code a bit.
    The $field and $instance *arrays* are useless, they could be inlined in the entity_create() calls

    Then the instance *object* that the method returns can just be named $instance.

  7. +++ b/core/modules/system/tests/modules/entity_test/entity_test.module
    @@ -494,14 +494,14 @@ function entity_test_entity_test_mul_translation_delete(EntityInterface $transla
    -function entity_test_field_default_value(EntityInterface $entity, Field $field, FieldInstance $instance, $langcode) {
    +function entity_test_field_default_value(EntityInterface $entity, FieldConfig $field, FieldInstanceConfig $instance, $langcode) {
    

    Same, should hint at the interfaces

  8. +++ b/core/modules/views/views.module
    @@ -534,25 +534,25 @@ function views_language_list($field = 'name', $flags = Language::STATE_ALL) {
    -function views_field_instance_create(FieldInstanceInterface $field_instance) {
    +function views_field_instance_config_create(FieldInstanceInterface $field_instance_config) {
    

    I'd tend to keep $field_instance (same for the other hooks below)

yched’s picture

Issue summary: View changes

Filled in a real issue summary.

yched’s picture

Fill in missing tags - the change has been approved in Prague

andypost’s picture

Controllers should be addressed in follow-up issue, probably in one of #1976158: Rename entity storage/list/form/render "controllers" to handlers

Berdir’s picture

No, that issue is not related. but it could conflict with it, so maybe separate issue?

yched’s picture

Hm - #1976158: Rename entity storage/list/form/render "controllers" to handlers has no patch to conflict with ?
I'd rather keep a consistent set of classes here (FieldConfig / FieldConfigStorage), rather than saying "some patch is going to rename those classes some day, let's not touch them and break consistency meanwhile" ?

Berdir’s picture

Ah, we currently only have a storage controller for each of them and nothing else. Let's do it then ;) I thought there would be more. Shouldn't there be?

yched’s picture

@berdir:
field_ui_entity_info() adds:
- FieldListController - "list" controller for fields
- FieldDeleteForm - "delete" form controller for *instances* (so it's kind of misnamed already)

I guess we might rename them as well...
FieldConfigListController
FieldInstanceConfigDeleteForm
?

plopesc’s picture

New round addressing comments in #46 and #53.

Let's see testbot...

yched’s picture

Issue tags: +Avoid commit conflicts

Needs a reroll after #2119905: Provide @ConfigEntityType and @ContentEntityType to have better defaults
(@plopesc already merged latest HEAD in the sandbox, just needs re-uploading here)

Other than that, this is RTBC IMO. Thanks @plopesc !
Adding the "avoid commit conflict" tag.

plopesc’s picture

New Re-roll

Status: Needs review » Needs work

The last submitted patch, 56: field_config_entities_rename-2095303-56.patch, failed testing.

The last submitted patch, 56: field_config_entities_rename-2095303-56.patch, failed testing.

plopesc’s picture

swentel’s picture

Status: Needs work » Reviewed & tested by the community

Shamelessly pulling the trigger.

alexpott’s picture

swentel’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Avoid commit conflicts

Committed 40dc8da and pushed to 8.x. Thanks!

Followups:

During the commit I made the following fixes:

diff --git a/core/modules/config_translation/config_translation.module b/core/modules/config_translation/config_translation.module
index 3161a6e..62f014f 100644
--- a/core/modules/config_translation/config_translation.module
+++ b/core/modules/config_translation/config_translation.module
@@ -96,7 +96,7 @@ function config_translation_entity_info_alter($entity_info) {
       }
       elseif ($entity_type_id == 'field_instance_config') {
         $class = 'Drupal\config_translation\Controller\ConfigTranslationFieldInstanceListController';
-        // Will be filled in dynamically, see \Drupal\field\Entity\FieldInstance::linkTemplates().
+        // Will be filled in dynamically, see \Drupal\field\Entity\FieldInstanceConfig::linkTemplates().
         $entity_type->setLinkTemplate('drupal:config-translation-overview', 'config_translation.item.overview.');
       }
       else {
diff --git a/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php b/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
index 2d2db1f..cef84e5 100644
--- a/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
+++ b/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
@@ -189,7 +189,7 @@ class FieldConfig extends ConfigEntityBase implements FieldInterface {
   protected $itemDefinition;
 
   /**
-   * Constructs a Field object.
+   * Constructs a FieldConfig object.
    *
    * @param array $values
    *   An array of field properties, keyed by property name. Most array
@@ -294,8 +294,8 @@ protected function preSaveNew(EntityStorageControllerInterface $storage_controll
     // Assign the ID.
     $this->id = $this->id();
 
-    // Field name cannot be longer than Field::NAME_MAX_LENGTH characters. We
-    // use Unicode::strlen() because the DB layer assumes that column widths
+    // Field name cannot be longer than FieldConfig::NAME_MAX_LENGTH characters.
+    // We use Unicode::strlen() because the DB layer assumes that column widths
     // are given in characters rather than bytes.
     if (Unicode::strlen($this->name) > static::NAME_MAX_LENGTH) {
       throw new FieldException(format_string(
@@ -379,7 +379,7 @@ public static function preDelete(EntityStorageControllerInterface $storage_contr
     $instance_controller = \Drupal::entityManager()->getStorageController('field_instance_config');
 
     // Delete instances first. Note: when deleting a field through
-    // FieldInstance::postDelete(), the instances have been deleted already, so
+    // FieldInstanceConfig::postDelete(), the instances have been deleted already, so
     // no instances will be found here.
     $instance_ids = array();
     foreach ($fields as $field) {
diff --git a/core/modules/field/lib/Drupal/field/Entity/FieldInstanceConfig.php b/core/modules/field/lib/Drupal/field/Entity/FieldInstanceConfig.php
index eec718c..acc01fa 100644
--- a/core/modules/field/lib/Drupal/field/Entity/FieldInstanceConfig.php
+++ b/core/modules/field/lib/Drupal/field/Entity/FieldInstanceConfig.php
@@ -215,7 +215,7 @@ class FieldInstanceConfig extends ConfigEntityBase implements FieldInstanceInter
   protected $itemDefinition;
 
   /**
-   * Constructs a FieldInstance object.
+   * Constructs a FieldInstanceConfig object.
    *
    * @param array $values
    *   An array of field instance properties, keyed by property name. Most
diff --git a/core/modules/field/lib/Drupal/field/Tests/BulkDeleteTest.php b/core/modules/field/lib/Drupal/field/Tests/BulkDeleteTest.php
index 94f2717..145fab0 100644
--- a/core/modules/field/lib/Drupal/field/Tests/BulkDeleteTest.php
+++ b/core/modules/field/lib/Drupal/field/Tests/BulkDeleteTest.php
@@ -247,7 +247,7 @@ function testPurgeInstance() {
     }
 
     // Check hooks invocations.
-    // hook_field_delete() should have been called once for each entity in the
+    // FieldItemInterface::delete() should have been called once for each entity in the
     // bundle.
     $actual_hooks = field_test_memorize();
     $hooks = array();
@@ -288,7 +288,7 @@ function testPurgeField() {
     $instance = field_info_instance($this->entity_type, $field->name, $bundle);
     $instance->delete();
 
-    // Assert that hook_field_delete() was not called yet.
+    // Assert that FieldItemInterface::delete() was not called yet.
     $mem = field_test_memorize();
     $this->assertEqual(count($mem), 0, 'No field hooks were called.');
 
@@ -296,7 +296,7 @@ function testPurgeField() {
     field_purge_batch(10);
 
     // Check hooks invocations.
-    // hook_field_delete() should have been called once for each entity in the
+    // FieldItemInterface::delete() should have been called once for each entity in the
     // bundle.
     $actual_hooks = field_test_memorize();
     $hooks = array();
@@ -318,7 +318,7 @@ function testPurgeField() {
     $instance = field_info_instance($this->entity_type, $field->name, $bundle);
     $instance->delete();
 
-    // Assert that hook_field_delete() was not called yet.
+    // Assert that FieldItemInterface::delete() was not called yet.
     $mem = field_test_memorize();
     $this->assertEqual(count($mem), 0, 'No field hooks were called.');
 
diff --git a/core/modules/field/tests/modules/field_test/field_test.module b/core/modules/field/tests/modules/field_test/field_test.module
index 6fb541a..fb9d263 100644
--- a/core/modules/field/tests/modules/field_test/field_test.module
+++ b/core/modules/field/tests/modules/field_test/field_test.module
@@ -77,7 +77,7 @@ function field_test_menu() {
  *   // retrieve and reset the memorized hook call data
  *   $mem = field_test_memorize();
  *
- *   // make sure hook_field_create_field() is invoked correctly
+ *   // make sure hook_field_config_create() is invoked correctly
  *   assertEqual(count($mem['field_test_field_config_create']), 1);
  *   assertEqual($mem['field_test_field_config_create'][0], array($field));
  * @endcode
swentel’s picture

plopesc’s picture

After a search word by word in the change records list, here is the full list of change records containing the names that has been changed in this issue and it follow-ups
- field_entity:

- field_instance:

- FieldInstance:

- FieldInterface:

- FieldInstanceInterface:

  • None

- FieldStorageController:

  • None

- FieldInstanceStorageController:

  • None

From tomorrow, I will start to replace word by word all the references in the issues listed above.

Regards

plopesc’s picture

Issue summary: View changes
plopesc’s picture

Reviewed and changed references in change records referenced in #65

Status: Fixed » Closed (fixed)

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