- Patch #364620 by bjaspan, yched: allow creating a field with a deleted name.

5 files changed, 117 insertions, 62 deletions

diff --git a/modules/field/field.crud.inc b/modules/field/field.crud.inc
index aad0e9aa8..97b3d8a70 100644
--- a/modules/field/field.crud.inc
+++ b/modules/field/field.crud.inc
@@ -197,6 +197,8 @@
  *     carefully, for it might seriously affect the site's performance.
  *   - settings: each omitted setting is given the default value defined in
  *     hook_field_info().
+ * @return
+ *   The $field structure with the id property filled in.
  * @throw
  *   FieldException
  */
@@ -222,9 +224,10 @@ function field_create_field($field) {
     throw new FieldException(t('Attempt to create a field of unknown type %type.', array('%type' => $field['type'])));
   }
 
-  // Ensure the field name is unique. We also check disabled or deleted fields.
+  // Ensure the field name is unique over active and disabled fields.
+  // We do not care about deleted fields.
   // TODO : do we want specific messages when clashing with a disabled or inactive field ?
-  $prior_field = field_read_field($field['field_name'], array('include_inactive' => TRUE, 'include_deleted' => TRUE));
+  $prior_field = field_read_field($field['field_name'], array('include_inactive' => TRUE));
   if (!empty($prior_field)) {
     throw new FieldException(t('Attempt to create field name %name which already exists.', array('%name' => $field['field_name'])));
   }
@@ -255,9 +258,6 @@ function field_create_field($field) {
   );
   $field['indexes'] += $schema['indexes'];
 
-  // Inform the storage engine.
-  module_invoke(variable_get('field_storage_module', 'field_sql_storage'), 'field_storage_create_field', $field);
-
   // The serialized 'data' column contains everything from $field that does not
   // have its own column and is not automatically populated when the field is
   // read.
@@ -265,23 +265,32 @@ function field_create_field($field) {
   unset($data['columns'], $data['field_name'], $data['type'], $data['locked'], $data['module'], $data['active'], $data['deleted']);
   $field['data'] = $data;
 
+  // Store the field and create the id.
   drupal_write_record('field_config', $field);
 
+  // Invoke hook_field_storage_create_field after the field is
+  // complete (e.g. it has its id).
+  module_invoke(variable_get('field_storage_module', 'field_sql_storage'), 'field_storage_create_field', $field);
+
   // Clear caches
   field_cache_clear(TRUE);
+
+  return $field;
 }
 
 /**
  * Read a single field record directly from the database. Generally,
  * you should use the field_info_field() instead.
  *
+ * This function will not return deleted fields. Use
+ * field_read_fields() instead for this purpose.
+ *
  * @param $field_name
  *   The field name to read.
  * @param array $include_additional
  *   The default behavior of this function is to not return a field that
- *   is inactive or has been deleted. Setting
- *   $include_additional['include_inactive'] or
- *   $include_additional['include_deleted'] to TRUE will override this
+ *   is inactive. Setting
+ *   $include_additional['include_inactive'] to TRUE will override this
  *   behavior.
  * @return
  *   A field structure, or FALSE.
@@ -303,7 +312,9 @@ function field_read_field($field_name, $include_additional = array()) {
  *   $include_additional['include_deleted'] to TRUE will override this
  *   behavior.
  * @return
- *   An array of fields matching $params.
+ *   An array of fields matching $params. If
+ *   $include_additional['include_deletd'] is TRUE, the array is keyed
+ *   by field id, otherwise it is keyed by field name.
  */
 function field_read_fields($params = array(), $include_additional = array()) {
   $query = db_select('field_config', 'fc', array('fetch' => PDO::FETCH_ASSOC));
@@ -316,7 +327,8 @@ function field_read_fields($params = array(), $include_additional = array()) {
   if (!isset($include_additional['include_inactive']) || !$include_additional['include_inactive']) {
     $query->condition('fc.active', 1);
   }
-  if (!isset($include_additional['include_deleted']) || !$include_additional['include_deleted']) {
+  $include_deleted = (isset($include_additional['include_deleted']) && $include_additional['include_deleted']);
+  if (!$include_deleted) {
     $query->condition('fc.deleted', 0);
   }
 
@@ -335,7 +347,11 @@ function field_read_fields($params = array(), $include_additional = array()) {
     $schema += array('columns' => array(), 'indexes' => array());
     $field['columns'] = $schema['columns'];
 
-    $fields[$field['field_name']] = $field;
+    $field_name = $field['field_name'];
+    if ($include_deleted) {
+      $field_name = $field['id'];
+    }
+    $fields[$field_name] = $field;
   }
   return $fields;
 }
@@ -348,18 +364,21 @@ function field_read_fields($params = array(), $include_additional = array()) {
  *   The field name to delete.
  */
 function field_delete_field($field_name) {
-  // Mark the field for deletion.
-  db_update('field_config')
+  // Mark field storage for deletion.
+  module_invoke(variable_get('field_storage_module', 'field_sql_storage'), 'field_storage_delete_field', $field_name);
+
+  // Mark any instances of the field for deletion.
+  db_update('field_config_instance')
     ->fields(array('deleted' => 1))
     ->condition('field_name', $field_name)
     ->execute();
 
-  // Mark any instances of the field for deletion.
-  db_update('field_config_instance')
+  // Mark the field for deletion.
+  db_update('field_config')
     ->fields(array('deleted' => 1))
     ->condition('field_name', $field_name)
     ->execute();
-  module_invoke(variable_get('field_storage_module', 'field_sql_storage'), 'field_storage_delete_field', $field_name);
+
   // Clear the cache.
   field_cache_clear(TRUE);
 }
@@ -391,6 +410,8 @@ function field_delete_field($field_name) {
  *     - type: the default formatter specified in hook_field_info().
  *     - settings: each omitted setting is given the default value specified in
  *       hook_field_formatter_info().
+ * @return
+ *   The $instance structure with the id property filled in.
  * @throw
  *   FieldException
  */
@@ -414,7 +435,7 @@ function field_create_instance($instance) {
 
   // Ensure the field instance is unique.
   // TODO : do we want specific messages when clashing with a disabled or inactive instance ?
-  $prior_instance = field_read_instance($instance['field_name'], $instance['bundle'], array('include_inactive' => TRUE, 'include_deleted' => TRUE));
+  $prior_instance = field_read_instance($instance['field_name'], $instance['bundle'], array('include_inactive' => TRUE));
   if (!empty($prior_instance)) {
     throw new FieldException('Attempt to create a field instance which already exists.');
   }
@@ -427,7 +448,7 @@ function field_create_instance($instance) {
   // Invoke external hooks after the cache is cleared for API consistency.
   module_invoke_all('field_create_instance', $instance);
 
-  return FALSE;
+  return $instance;
 }
 
 /*
@@ -561,15 +582,17 @@ function _field_write_instance($instance, $update = FALSE) {
  * Read a single instance record directly from the database. Generally,
  * you should use the field_info_instance() instead.
  *
+ * This function will not return deleted instances. Use
+ * field_read_instances() instead for this purpose.
+ *
  * @param $field_name
  *   The field name to read.
  * @param $bundle
  *   The bundle to which the field is bound.
  * @param array $include_additional
  *   The default behavior of this function is to not return an instance that
- *   is inactive or has been deleted. Setting
- *   $include_additional['include_inactive'] or
- *   $include_additional['include_deleted'] to TRUE will override this
+ *   is inactive. Setting
+ *   $include_additional['include_inactive'] to TRUE will override this
  *   behavior.
  * @return
  *   An instance structure, or FALSE.
diff --git a/modules/field/field.install b/modules/field/field.install
index b13db7514..ee8a0a26e 100644
--- a/modules/field/field.install
+++ b/modules/field/field.install
@@ -29,7 +29,7 @@ function field_schema() {
         'type' => 'varchar',
         'length' => 32,
         'not null' => TRUE,
-        'description' => 'The name of this field',
+        'description' => 'The name of this field. Non-deleted field names are unique, but multiple deleted fields can have the same name.',
       ),
       'type' => array(
         'type' => 'varchar',
@@ -61,13 +61,13 @@ function field_schema() {
         'type' => 'int',
         'size' => 'tiny',
         'not null' => TRUE,
-          'default' => 0,
+        'default' => 0,
       ),
       'active' => array(
         'type' => 'int',
         'size' => 'tiny',
         'not null' => TRUE,
-          'default' => 0,
+        'default' => 0,
       ),
       'deleted' => array(
         'type' => 'int',
@@ -77,13 +77,14 @@ function field_schema() {
       ),
     ),
     'primary key' => array('id'),
-    'unique keys' => array('field_name' => array('field_name')),
     'indexes' => array(
-      // used by field_read_fields
+      // used by field_delete_field() among others
+      'field_name' => array('field_name'),
+      // used by field_read_fields()
       'active_deleted' => array('active', 'deleted'),
-      // used by field_modules_disabled
+      // used by field_modules_disabled()
       'module' => array('module'),
-      // used by field_associate_fields
+      // used by field_associate_fields()
       'type' => array('type'),
     ),
   );
@@ -129,14 +130,14 @@ function field_schema() {
     ),
     'primary key' => array('id'),
     'unique keys' => array(
-      'field_name_bundle' => array('field_name', 'bundle'),
+      'field_id_bundle' => array('field_id', 'bundle'),
     ),
     'indexes' => array(
-      // used by field_read_instances
+      // used by field_read_instances()
       'widget_active_deleted' => array('widget_active', 'deleted'),
-      // used by field_modules_disabled
+      // used by field_modules_disabled()
       'widget_module' => array('widget_module'),
-      // used by field_associate_fields
+      // used by field_associate_fields()
       'widget_type' => array('widget_type'),
     ),
   );
diff --git a/modules/field/field.test b/modules/field/field.test
index 429b8e77e..c67eb72ab 100644
--- a/modules/field/field.test
+++ b/modules/field/field.test
@@ -1074,10 +1074,10 @@ class FieldFormTestCase extends DrupalWebTestCase {
   }
 }
 
-class FieldTestCase extends DrupalWebTestCase {
+class FieldCrudTestCase extends DrupalWebTestCase {
   public static function getInfo() {
     return array(
-      'name'  => t('Field tests'),
+      'name'  => t('Field CRUD tests'),
       'description'  => t("Create / read /update a field."),
       'group' => t('Field')
     );
@@ -1121,7 +1121,7 @@ class FieldTestCase extends DrupalWebTestCase {
       'field_name' => 'field_2',
       'type' => 'test_field',
     );
-    field_create_field($field_definition);
+    $field_definition = field_create_field($field_definition);
 
     $field = field_read_field($field_definition['field_name']);
 
@@ -1240,7 +1240,8 @@ class FieldTestCase extends DrupalWebTestCase {
 
     // Make sure that the field is marked as deleted when it is specifically
     // loaded.
-    $field = field_read_field($this->field['field_name'], array('include_deleted' => TRUE));
+    $fields = field_read_fields(array(), array('include_deleted' => TRUE));
+    $field = current($field);
     $this->assertTrue(!empty($field['deleted']), t('A deleted field is marked for deletion.'));
 
     // Make sure that this field's instance is marked as deleted when it is
@@ -1261,6 +1262,30 @@ class FieldTestCase extends DrupalWebTestCase {
     $this->assertTrue(!empty($another_field) && empty($another_field['deleted']), t('A non-deleted field is not marked for deletion.'));
     $another_instance = field_read_instance($this->another_instance_definition['field_name'], $this->another_instance_definition['bundle']);
     $this->assertTrue(!empty($another_instance) && empty($another_instance['deleted']), t('An instance of a non-deleted field is not marked for deletion.'));
+
+    // Try to create a new field the same name as a deleted field and
+    // write data into it.
+    field_create_field($this->field);
+    field_create_instance($this->instance_definition);
+    $field = field_read_field($this->field['field_name']);
+    $this->assertTrue(!empty($field) && empty($field['deleted']), t('A new field with a previously used name is created.'));
+    $instance = field_read_instance($this->instance_definition['field_name'], $this->instance_definition['bundle']);
+    $this->assertTrue(!empty($instance) && empty($instance['deleted']), t('A new instance for a previously used field name is created.'));
+
+    // Save an object with data for the field
+    $entity = field_test_create_stub_entity(0, 0, $instance['bundle']);
+    $values[0]['value'] = mt_rand(1, 127);
+    $entity->{$field['field_name']} = $values;
+    $entity_type = 'test_entity';
+    field_attach_insert($entity_type, $entity);
+
+    // Verify the field is present on load
+    $entity = field_test_create_stub_entity(0, 0, $this->instance_definition['bundle']);
+    field_attach_load($entity_type, array(0 => $entity));
+    $this->assertIdentical(count($entity->{$field['field_name']}), count($values), "Data in previously deleted field saves and loads correctly");
+    foreach ($values as $delta => $value) {
+      $this->assertEqual($entity->{$field['field_name']}[$delta]['value'], $values[$delta]['value'], "Data in previously deleted field saves and loads correctly");
+    }
   }
 }
 
diff --git a/modules/field/modules/field_sql_storage/field_sql_storage.module b/modules/field/modules/field_sql_storage/field_sql_storage.module
index add36edf1..18f89adb8 100644
--- a/modules/field/modules/field_sql_storage/field_sql_storage.module
+++ b/modules/field/modules/field_sql_storage/field_sql_storage.module
@@ -20,25 +20,25 @@ function field_sql_storage_help($path, $arg) {
 /**
  * Generate a table name for a field data table.
  *
- * @param $name
- *   The name of the field
+ * @param $field
+ *   The field structure.
  * @return
  *   A string containing the generated name for the database table
  */
-function _field_sql_storage_tablename($name) {
-  return 'field_data_' . $name;
+function _field_sql_storage_tablename($field) {
+  return "field_data_{$field['field_name']}_{$field['id']}";
 }
 
 /**
  * Generate a table name for a field revision archive table.
  *
  * @param $name
- *   The name of the field
+ *   The field structure.
  * @return
  *   A string containing the generated name for the database table
  */
-function _field_sql_storage_revision_tablename($name) {
-  return 'field_data_revision_' . $name;
+function _field_sql_storage_revision_tablename($field) {
+  return "field_revision_{$field['field_name']}_{$field['id']}";
 }
 
 /**
@@ -102,8 +102,9 @@ function _field_sql_storage_etid($obj_type) {
  *   One or more tables representing the schema for the field.
  */
 function _field_sql_storage_schema($field) {
+  $deleted = $field['deleted'] ? 'deleted ' : '';
   $current = array(
-    'description' => 'Data storage for field ' . $field['field_name'],
+    'description' => "Data storage for {$deleted}field {$field['id']} ({$field['field_name']})",
     'fields' => array(
       'etid' => array(
         'type' => 'int',
@@ -166,13 +167,13 @@ function _field_sql_storage_schema($field) {
   // revision_id but not entity_id so that multiple revision loads can
   // use the IN operator.
   $revision = $current;
-  $revision['description'] = 'Revision archive storage for field ' . $field['field_name'];
+  $revision['description'] = "Revision archive storage for {$deleted}field {$field['id']} ({$field['field_name']})";
   $revision['revision_id']['description'] = 'The entity revision id this data is attached to';
   $revision['primary key'] = array('etid', 'revision_id', 'deleted', 'delta');
 
   return array(
-    _field_sql_storage_tablename($field['field_name']) => $current,
-    _field_sql_storage_revision_tablename($field['field_name']) => $revision,
+    _field_sql_storage_tablename($field) => $current,
+    _field_sql_storage_revision_tablename($field) => $revision,
   );
 }
 
@@ -191,7 +192,8 @@ function field_sql_storage_field_storage_create_field($field) {
  */
 function field_sql_storage_field_storage_delete_field($field_name) {
   // Mark all data associated with the field for deletion.
-  $table = _field_sql_storage_tablename($field_name);
+  $field = field_info_field($field_name);
+  $table = _field_sql_storage_tablename($field);
   db_update($table)
     ->fields(array('deleted' => 1))
     ->execute();
@@ -220,7 +222,7 @@ function field_sql_storage_field_storage_load($obj_type, &$objects, $age, $skip_
 
   foreach ($field_ids as $field_name => $ids) {
     $field = field_info_field($field_name);
-    $table = $load_current ? _field_sql_storage_tablename($field_name) : _field_sql_storage_revision_tablename($field_name);
+    $table = $load_current ? _field_sql_storage_tablename($field) : _field_sql_storage_revision_tablename($field);
 
     $results = db_select($table, 't')
       ->fields('t')
@@ -261,9 +263,9 @@ function field_sql_storage_field_storage_write($obj_type, $object, $op, $skip_fi
       continue;
     }
 
-    $table_name = _field_sql_storage_tablename($field_name);
-    $revision_name = _field_sql_storage_revision_tablename($field_name);
     $field = field_info_field($field_name);
+    $table_name = _field_sql_storage_tablename($field);
+    $revision_name = _field_sql_storage_revision_tablename($field);
 
     // Leave the field untouched if $object comes with no $field_name property.
     // Empty the field if $object->$field_name is NULL or an empty array.
@@ -334,8 +336,9 @@ function field_sql_storage_field_storage_delete($obj_type, $object) {
   $instances = field_info_instances($bundle);
   foreach ($instances as $instance) {
     $field_name = $instance['field_name'];
-    $table_name = _field_sql_storage_tablename($field_name);
-    $revision_name = _field_sql_storage_revision_tablename($field_name);
+    $field = field_read_field($field_name);
+    $table_name = _field_sql_storage_tablename($field);
+    $revision_name = _field_sql_storage_revision_tablename($field);
     db_delete($table_name)
       ->condition('etid', $etid)
       ->condition('entity_id', $id)
@@ -360,7 +363,8 @@ function field_sql_storage_field_storage_delete_revision($obj_type, $object) {
     $instances = field_info_instances($bundle);
     foreach ($instances as $instance) {
       $field_name = $instance['field_name'];
-      $revision_name = _field_sql_storage_revision_tablename($field_name);
+      $field = field_read_field($field_name);
+      $revision_name = _field_sql_storage_revision_tablename($field);
       db_delete($revision_name)
         ->condition('etid', $etid)
         ->condition('entity_id', $id)
@@ -376,8 +380,9 @@ function field_sql_storage_field_storage_delete_revision($obj_type, $object) {
  * This function simply marks for deletion all data associated with the field.
  */
 function field_sql_storage_field_storage_delete_instance($field_name, $bundle) {
-  $table_name = _field_sql_storage_tablename($field_name);
-  $revision_name = _field_sql_storage_revision_tablename($field_name);
+  $field = field_read_field($field_name);
+  $table_name = _field_sql_storage_tablename($field);
+  $revision_name = _field_sql_storage_revision_tablename($field);
   db_update($table_name)
     ->fields(array('deleted' => 1))
     ->condition('bundle', $bundle)
@@ -394,8 +399,9 @@ function field_sql_storage_field_storage_delete_instance($field_name, $bundle) {
 function field_sql_storage_field_storage_rename_bundle($bundle_old, $bundle_new) {
   $instances = field_info_instances($bundle_old);
   foreach ($instances as $instance) {
-    $table_name = _field_sql_storage_tablename($instance['field_name']);
-    $revision_name = _field_sql_storage_revision_tablename($instance['field_name']);
+    $field = field_read_field($instance['field_name']);
+    $table_name = _field_sql_storage_tablename($field);
+    $revision_name = _field_sql_storage_revision_tablename($field);
     db_update($table_name)
       ->fields(array('bundle' => $bundle_new))
       ->condition('bundle', $bundle_old)
diff --git a/modules/field/modules/field_sql_storage/field_sql_storage.test b/modules/field/modules/field_sql_storage/field_sql_storage.test
index df278f00c..0ac174fae 100644
--- a/modules/field/modules/field_sql_storage/field_sql_storage.test
+++ b/modules/field/modules/field_sql_storage/field_sql_storage.test
@@ -25,14 +25,14 @@ class FieldSqlStorageTestCase extends DrupalWebTestCase {
     parent::setUp('field_sql_storage', 'field', 'field_test');
     $this->field_name = drupal_strtolower($this->randomName() . '_field_name');
     $this->field = array('field_name' => $this->field_name, 'type' => 'test_field', 'cardinality' => 4);
-    field_create_field($this->field);
+    $this->field = field_create_field($this->field);
     $this->instance = array(
       'field_name' => $this->field_name,
       'bundle' => 'test_bundle'
     );
-    field_create_instance($this->instance);
-    $this->table = _field_sql_storage_tablename($this->field_name);
-    $this->revision_table = _field_sql_storage_revision_tablename($this->field_name);
+    $this->instance = field_create_instance($this->instance);
+    $this->table = _field_sql_storage_tablename($this->field);
+    $this->revision_table = _field_sql_storage_revision_tablename($this->field);
 
   }