When running list_field_update_forbid() the list module uses the static value of $field['settings']['allowed_values'] to determine whether any keys have been lost in the update. This seems to ignore the allowed_values_function if one is set. As such, it seems like it would be better to call list_allowed_values() to determine the allowed values because it checks for an allowed_values_function before falling back to $field['settings']['allowed_values'].

The only thing I'm unsure of is exactly the role caching plays in this, which could be the reason $field['settings']['allowed_values'] was chosen.
It'd be great if someone with more familiarity with the list module could weigh in on this piece.

The use case this affects is when allowed values are set, then subsequently removed and replaced with an allowed_values_function.

To reproduce:

  1. Add List (text) field with at least one allowed value to a node type.
  2. Create an instance of the node type with that field set to ensure there is data in the database for the field.
  3. Call field_update_field() on the field and pass $field['settings'] => array('allowed_values_function' => 'some_function', 'allowed_values' => array()) to reset the allowed values and provide a function that will be called in place of supplying them statically.

I would expect:

To have no problem updating the field if the keys returned by my allowed_values_function match the ones that were previously statically defined via allowed_values.

What happened instead:

FieldUpdateForbiddenException: A list field (@field_name) with existing data cannot have its keys changed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

caxy4’s picture

Attaching patch referenced in description.

Status: Needs review » Needs work
caxy4’s picture

I did some more thinking about this and I think it'd be better to not modify calls to generate the allowed values list.

Instead, we should just skip validation if an allowed values function is set because the function negates the need for validation.

Attaching another patch that does this.

caxy4’s picture

Status: Needs work » Needs review