Bring back the initial functionality of _field_write_instance function

[alanmels]

Description of the need

An interesting issue popped up when porting the Comment Alter module. When adapting the comment_alter_form_field_ui_field_edit_form_alter(&$form, &$form_state, $form_id) function (see https://git.drupalcode.org/project/comment_alter/-/blob/7.x-1.x/comment_alter.module#L16) to Backdrop, I've traced why it was not saving new values for custom fields on a content type edit page such as, for example, admin/structure/types/manage/ticket/fields/field_status to the following difference between Drupal 7 and Backdrop cores.

Here is how the original function looks like on https://git.drupalcode.org/project/drupal/-/blob/7.x/modules/field/field.crud.inc#L578

/**
 * Stores an instance record in the field configuration database.
 *
 * @param $instance
 *   An instance structure.
 * @param $update
 *   Whether this is a new or existing instance.
 */
function _field_write_instance($instance, $update = FALSE) {
  $field = field_read_field($instance['field_name']);
  $field_type = field_info_field_types($field['type']);

  // Set defaults.
  $instance += array(
    'settings' => array(),
    'display' => array(),
    'widget' => array(),
    'required' => FALSE,
    'label' => $instance['field_name'],
    'description' => '',
    'deleted' => 0,
  );

  // Set default instance settings.
  $instance['settings'] += field_info_instance_settings($field['type']);

  // Set default widget and settings.
  $instance['widget'] += array(
    // TODO: what if no 'default_widget' specified ?
    'type' => $field_type['default_widget'],
    'settings' => array(),
  );
  // If no weight specified, make sure the field sinks at the bottom.
  if (!isset($instance['widget']['weight'])) {
    $max_weight = field_info_max_weight($instance['entity_type'], $instance['bundle'], 'form');
    $instance['widget']['weight'] = isset($max_weight) ? $max_weight + 1 : 0;
  }
  // Check widget module.
  $widget_type = field_info_widget_types($instance['widget']['type']);
  $instance['widget']['module'] = $widget_type['module'];
  $instance['widget']['settings'] += field_info_widget_settings($instance['widget']['type']);

  // Make sure there are at least display settings for the 'default' view mode,
  // and fill in defaults for each view mode specified in the definition.
  $instance['display'] += array(
    'default' => array(),
  );
  foreach ($instance['display'] as $view_mode => $display) {
    $display += array(
      'label' => 'above',
      'type' => isset($field_type['default_formatter']) ? $field_type['default_formatter'] : 'hidden',
      'settings' => array(),
    );
    if ($display['type'] != 'hidden') {
      $formatter_type = field_info_formatter_types($display['type']);
      $display['module'] = $formatter_type['module'];
      $display['settings'] += field_info_formatter_settings($display['type']);
    }
    // If no weight specified, make sure the field sinks at the bottom.
    if (!isset($display['weight'])) {
      $max_weight = field_info_max_weight($instance['entity_type'], $instance['bundle'], $view_mode);
      $display['weight'] = isset($max_weight) ? $max_weight + 1 : 0;
    }
    $instance['display'][$view_mode] = $display;
  }

  // The serialized 'data' column contains everything from $instance that does
  // not have its own column and is not automatically populated when the
  // instance is read.
  $data = $instance;
  unset($data['id'], $data['field_id'], $data['field_name'], $data['entity_type'], $data['bundle'], $data['deleted']);

  $record = array(
    'field_id' => $instance['field_id'],
    'field_name' => $instance['field_name'],
    'entity_type' => $instance['entity_type'],
    'bundle' => $instance['bundle'],
    'data' => $data,
    'deleted' => $instance['deleted'],
  );
  // We need to tell drupal_update_record() the primary keys to trigger an
  // update.
  if ($update) {
    $record['id'] = $instance['id'];
    $primary_key = array('id');
  }
  else {
    $primary_key = array();
  }
  drupal_write_record('field_config_instance', $record, $primary_key);
}

Pay special attention to the comment and the line following it:

// The serialized 'data' column contains everything from $instance that does
  // not have its own column and is not automatically populated when the
  // instance is read.
  $data = $instance;

Now see how the same function looks on https://github.com/backdrop/backdrop/blob/7cc9968d3c1f8885a9d126668b1a8c0766b8e3d6/core/modules/field/field.crud.inc#L628

/**
 * Stores an instance record in the field configuration database.
 *
 * @param $instance
 *   An instance structure.
 * @param $update
 *   Whether this is a new or existing instance.
 */
function _field_write_instance($instance, $update = FALSE) {
  $field = field_read_field($instance['field_name']);
  $field_type = field_info_field_types($field['type']);

  // Set default widget and settings.
  $instance['widget'] += array(
    // TODO: what if no 'default_widget' specified ?
    'type' => $field_type['default_widget'],
    'settings' => array(),
  );
  // If no weight specified, make sure the field sinks at the bottom.
  if (!isset($instance['widget']['weight'])) {
    $max_weight = field_info_max_weight($instance['entity_type'], $instance['bundle'], 'form');
    $instance['widget']['weight'] = isset($max_weight) ? $max_weight + 1 : 0;
  }
  // Check widget module.
  $widget_type = field_info_widget_types($instance['widget']['type']);
  $instance['widget']['module'] = isset($widget_type['module']) ? $widget_type['module'] : '';
  $instance['widget']['settings'] += field_info_widget_settings($instance['widget']['type']);

  // Make sure there are at least display settings for the 'default' display
  // mode, and fill in defaults for each display mode specified in the
  // definition.
  $instance['display'] += array(
    'default' => array(),
  );
  foreach ($instance['display'] as $view_mode => $display) {
    $display += array(
      'label' => 'hidden',
      'type' => isset($field_type['default_formatter']) ? $field_type['default_formatter'] : 'hidden',
      'settings' => array(),
    );
    if ($display['type'] != 'hidden') {
      $formatter_type = field_info_formatter_types($display['type']);
      $display['module'] = isset($formatter_type['module']) ? $formatter_type['module'] : '';
      $display['settings'] += field_info_formatter_settings($display['type']);
    }
    // If no weight specified, make sure the field sinks at the bottom.
    if (!isset($display['weight'])) {
      $max_weight = field_info_max_weight($instance['entity_type'], $instance['bundle'], $view_mode);
      $display['weight'] = isset($max_weight) ? $max_weight + 1 : 0;
    }
    $instance['display'][$view_mode] = $display;
  }

  // Include only defined data in the configuration file.
  $instance_data = array_intersect_key($instance, field_defaults_instance());

  $config = config('field.instance.' . $instance['entity_type'] . '.' . $instance['bundle'] . '.' . $instance['field_name']);
  $config->setData($instance_data);
  $config->save();
}

As you see the very end of the Backdrop version of the function is quite restrictive, in fact it's just the opposite of inclusive Drupal 7 version:

// Include only defined data in the configuration file.
  $instance_data = array_intersect_key($instance, field_defaults_instance());

Git blaming took me to https://github.com/backdrop/backdrop/commit/3ab41c27beaf214a2902803a9fb27ed3636275dc#r91927316 showing the change was done as part of https://github.com/backdrop/backdrop/pull/178

Basically, with this change the value of custom fields stored within the $instance variable never gets into the account, so not processed further.

As soon as I pass the value or $instance directly to $instance_data the ported module starts working as expected.

Proposed solution

I'm not sure exactly why it needed to:

Include only defined data in the configuration file.

whereas Drupal 7 is doing just the opposite:

// The serialized 'data' column contains everything from $instance that does // not have its own column and is not automatically populated when the // instance is read.

but I tested many times passing the data with and without array_intersect_key function and had to conclude the inclusive method is not hurting anything while at the same time making it possible to easily port some Drupal 7 modules like comment_alter relying on the same core functions as Drupal 7 did.

Of course, it's always possible to come up with custom solution completely bypassing the core functions, but before digging into alternative ways, I thought to reach out to Backdrop core maintainers/committers if they would consider bringing back the inclusive character of respective code lines in Drupal 7 version.

[argiepiano]

@alanmels I've run into this issue when porting a custom module from D7 to Backdrop. The solution I found: instead of adding new stuff to the root level of the $instance array, I added it into $instance['settings'] as subarray. That gets saved normally.

This implies you'll need to change the functions that retrieve those settings.

I hope this helps.

[klonos]

Thanks @argiepiano 🙏🏼 ...if this has happened to you as well, then it makes it something that is possibly not as uncommon as I originally thought. By the sound of it, it has a relatively simple solution, so it seems that what we need is a change record for that function + some documentation (something in the function docblock + also in https://docs.backdropcms.org/converting-modules-from-drupal).

@docwilmot and/or @bugfolder perhaps this is something that Coder Upgrade can pick up and either fix automatically, or at least point to the change record(?)

[klonos]

@argiepiano could you please provide a before/after set of code from that custom module you ported, to be used as a sample in the change record?

[argiepiano]

@klonos, the $instance['settings'] array has always contained unspecified, field-instance-specific values - there is no change between Drupal and Backdrop in this area, so therefore no record change is needed.

What did change is what @alanmels pointed out: in D7 it was possible to write anything anywhere in $instance, since this was stored in the data column. In Backdrop, only "defined data" is written in the array root of $instance of the json file. That's explained in the doc comment @alanmels included above.

EDIT: I want to add that the change between D7 and Backdrop does make a lot of sense (only allowing "defined data" to be written to the root of the $instance array). It's a much clearer way of doing things. If your module needs to provide additional information for the instance of a field, then it makes sense for that to be stored in the settings subarray of the instance (usually as a subarray using the the name of your module as key, to avoid collisions)

For example, any additional information provided by my_module should be stored in $instance['settings']['my_module'] = array('setting_1' => 'asdf', etc)

[klonos]

Hmm 🤔 ...would the solution then be to add some code in _field_write_instance() that intercepts any "unexpected" field-instance-specific values in the root of $instance, and moves them under $instance['settings']?

[argiepiano]

Hmm 🤔 ...would the solution then be to add some code in _field_write_instance() that intercepts any "unexpected" field-instance-specific values in the root of $instance, and moves them under $instance['settings']?

That will not work, as usually, you will need to retrieve values from the $instance array. If the settings are moved automatically under settings your module has no way of knowing this.

So, perhaps some documentation is needed, and yes, it'd be a good idea for coder_upgrade to catch these and throw a warning

[argiepiano]

A workaround for @alanmels if he prefers not to change the module, would be to add one additional submit handler to the form in comment_alter_form_field_ui_field_edit_form_alter() to manually save the settings to the field instance config json file. But that's really a lot more work for very little gain.

[klonos]

...by the way, the docblock for the _field_write_instance() function says Stores an instance record in the field configuration database., which seems to have been making sense in D7, as that function ended with a drupal_write_record(), but we are storing things in config in Backdrop with a $config->setData()/$config->save().

Same with function field_read_instance() I believe, which says Reads a single instance record from the database..

[argiepiano]

For the record, the correct link to the issue where the changes @alanmels pointed out above is this: https://github.com/backdrop/backdrop-issues/issues/178

The change record for that is found here: https://docs.backdropcms.org/change-records/converted-field-api-to-cmi

There is no mention in that change record about the loss of the ability to write settings at the root level of the instance configuration, nor of the need to write any custom settings inside $instance['settings'].

@klonos, is it typical to edit such an old change record to document this change? Or should this be done somewhere else.

[alanmels]

@alanmels I've run into this issue when porting a custom module from D7 to Backdrop. The solution I found: instead of adding new stuff to the root level of the $instance array, I added it into $instance['settings'] as subarray. That gets saved normally.

This implies you'll need to change the functions that retrieve those settings.

I hope this helps.

Thanks for the pointer. It definitely helped for my use case, so I'll go ahead and close this one. Feel free to re-open and re-purpose it if a change record is needed to prevent others from running into the same problem.

[bugfolder]

and yes, it'd be a good idea for coder_upgrade to catch these and throw a warning

An issue and PR would be welcome.

[argiepiano]

What's the procedure to make a change to the original change record, to add mention of this change? Is there a place where one can create an issue?

[klonos]

@argiepiano opening an issue here and adding the needs - change record label and perhaps the type - documentation label should be fine. Otherwise, I guess an issue in https://github.com/backdrop-ops/docs.backdropcms.org/issues, but why do that when we have this issue here already? 🤷🏼

Reopening...