Problem/Motivation

When creating list fields it is possible to use functions to define the allowed values for the list. This can either be done using a fixed list configured while creating the field in the UI or by setting the property allowed_values_function in the field storage configuration.

The property not only allows setting a procedural function but allows developers to use callbacks defined in classes (see #2982949):

This allows using something like

<?php
  /**
   * Implements hook_entity_field_storage_info_alter().
   */
  #[Hook('entity_field_storage_info_alter')]
  public function entityFieldStorageInfoAlter(&$fields, EntityTypeInterface $entity_type): void {
    if (($entity_type->id() === 'node') && isset($fields['field_test_alllowed_values'])) {
      $fields['field_test_allowed_values']->setSetting('allowed_values_function', [MyOptions::class, 'allowedValues']);
    }
}

Unfortunately this results in an error when trying to edit the field in the UI:

TypeError: Drupal\Component\Utility\Html::escape(): Argument #1 ($text) must be of type string, array given, called in /var/www/html/docroot/core/lib/Drupal/Component/Render/FormattableMarkup.php on line 238 in Drupal\Component\Utility\Html::escape() (line 433 of core/lib/Drupal/Component/Utility/Html.php).

When printing information about the allowed_values_function in ListItemBase::storageSettingsForm(), it is always assumed that the value for “allowed_values_function” is a string. Since it is allowed to pass the callable as array this assumption is wrong.

Steps to reproduce

* add a field of type "List (text)" to a content type
* alter the field storage info of this field in a custom module and set the allowed_values_function as shown above
* open the field settings in the admin UI

Proposed resolution

* update ListItemBase::storageSettingsForm() to handle cases where allowed_values_function is an array

Issue fork drupal-3579349

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

stborchert created an issue. See original summary.

stborchert’s picture

Issue summary: View changes

stborchert’s picture

Status: Active » Needs review

I've added a test that sets the allowed_values_function to [OptionsAllowedValues::class, 'simpleValues']. Without the second commit (the one, that fixes the issue) the test fails as expected.

idebr’s picture

Would this be a good case for the callable resolver? https://www.drupal.org/node/3368504

stborchert’s picture

@idebr The callable resolver already works for allowed_values_function. The problem here is the output in the field UI where the text always expects a string.

smustgrave’s picture

Status: Needs review » Needs work

Left a comment on the MR thanks!

stborchert’s picture

That should work, too. I'll try and update the MR.

smustgrave’s picture

ping me and I'll happily review quickly

stborchert’s picture

Hm, I have some troubles accessing the field config page in OptionsWidgetsTest. In OptionsFieldUITest we've got $this->adminPath but this does not work here ...
Question: how do I access the field configuration page (something like admin/structure/[...]/fields/[...]) in OptionsWidgetsTest for the entity_test type?

stborchert’s picture

Status: Needs work » Needs review

Finally managed to find the correct route for the field config. Phew :)

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Bug Smash Initiative, +Needs Review Queue Initiative

I couldn't find a better test to expand so believe it's fine as it's own. Manually testing using the test module and appears to be working. Believe all feedback has been addressed

godotislate’s picture

Status: Reviewed & tested by the community » Needs work

Needs a rebase for merge conflict. Also, seems like there are a lot of unrelated changes and files in the MR diff. Maybe that will be resolved by the rebase.

smustgrave’s picture

Yea seems like a bad rebase maybe? But 100% remember not seeing that when I looked after I was pinged in slack. Weird

stborchert’s picture

Status: Needs work » Needs review

Seems I managed to mess the commits with the last rebase :/
Opened a new MR and closed https://git.drupalcode.org/project/drupal/-/merge_requests/15098.

smustgrave’s picture

Status: Needs review » Needs work

Super close! Just some small comments on the test. Rest looks good!

stborchert’s picture

Status: Needs work » Needs review

Hope I could address the last comments ... :)

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Made 1 change to avoid using code blocks inline, almost positive eventually that will be a phpcs rule to prevent them.

Rest of the feedback appears to be addressed.

stborchert’s picture

I checked earlier, and /** */ isn't allowed. But your version works too :)

amateescu’s picture

Status: Reviewed & tested by the community » Needs work

Posted a few comments on the MR.

stborchert’s picture

Status: Needs work » Needs review
godotislate’s picture

The config schema for the allowed_values_function in options.schema.yml is this:

field.storage_settings.list_integer:
  type: mapping
  label: 'List (integer) settings'
  mapping:
    #...
    allowed_values_function:
      type: string
      label: 'Allowed values function'

We'll likely need to update that somehow to allow both strings and arrays. But going further, do we need to prevent Closures from being set as the allowed values function in PHP?

smustgrave’s picture

Status: Needs review » Needs work

Seems like there was additional feedback to be looked at in the previous comment

Thanks