Allow adding predefined lists to the options fields

Removing tags for now (just to get focus on our personal issue list scraper)

Add module http://drupal.org/project/list_predefined_options

Needs a summary update, as the Proposed resolution shouldn't be a hook :)

It should be fairly simple to add the D8 code I wrote to Options core module, IIRC it's mostly a plugin and a little bit of UI.

Yup, much! Thanks!

One question: should this be a separate modoule, or should the functionality be added to Options module?

I had another look at the the D8 port of list_predefined_options (at #2635932: Drupal 8?), and I see now that there are some changes that would be needed or desirable to bring it into core:

- rather than store the plugin ID in third party settings, we should instead add a new property ('predefined_options_plugin') to the schemas for the storage settings, eg field.storage_settings.list_integer
- options_allowed_values() should use the predefined_options_plugin setting if present to get the options. (basically, do the work of list_predefined_options_allowed_values() in my module code)
- the allowed_values_function system should be deprecated (the allowed_values_function property, and callback_allowed_values_function()). This is because the same thing can be accomplished with a plugin.
- list_predefined_options_form_field_storage_config_edit_form_builder() isn't necessary, since there's no longer a need to set the 'allowed_values_function' property.
- additionally, the 'US states' plugin should be removed IMO, as it's country-specific, and Drupal has an international audience.

Also, it might be an idea to consider the UI, since adding the elements in the actual field storage settings form rather than using hook_form_alter() means we could do something different, if it improved the usability of the form.

Hi, I just found my self using the list_predefined_options port quite a lot and felt eager to see it in core. The attached patch ports the feature with the design notes from #12 also added some tests but still some tasks remaining. I put it on Needs review so once most of the patch get reviewed I will continue working on the remaining tasks.

Took the lease of updating the issue summary.

Attached new patch with more tests, also fixed CS from bot and removed the @trigger_error that was making some tests to fail. Still needs work seems some tests keep failing. If is okay for maintainers this can be assigned to me.

Seems that removing allowed_values_function from options.schema.yml is breaking some integration tests on other core's components. I just add it back and restore the field storage settings form element (the warning message still be shown) and the tests. I think I'll need some assistance here on how to proceed.

Added the predefined_options_plugin key to Umami profile configuration files to avoid configuration tests failures.

Well seems this is working fine now putting it to Needs review.

I've queue for another test round to check everything is working with latest codebase. @joachim if someone could review this since seems complete by your guidelines.

Hi,

I don't understand what the problem is with the current 'allowed_values_function' and what this issue adds that could not be done using the current 'allowed_values_function' functionality. Can you develop please ?

There's none, this feature request is about replacing it with a new plugin type. The #1909744-21: Allow adding predefined lists to the options fields refers about BC, the allowed_values_function is used on tests outside the options module also other contrib modules may be using it to define their own options selectors. Please read the whole thread specially #1909744-12: Allow adding predefined lists to the options fields, @joachim describes the proposed solution. There is also a previous issue seeking this exact same feature #2305385: Convert "allowed_values_function" to a plugin.

Re-roll patch created for 9.3.x.

Fixed fail tests, Please have a look.

Rerolled for 9.2.x

This is looking good! And the new hook has documentation -- great!

Mostly typos, but I do wonder whether we should use a proper metadata object instead of the old D7-era $cacheable boolean.

1. +++ b/core/modules/options/options.api.php
   @@ -36,6 +36,23 @@ function hook_options_list_alter(array &$options, array $context) {
   +function hook_options_predefined_options_info_alter(&$definitions) {
   

   (Yay! Documentation!)

2. +++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
   @@ -80,22 +103,37 @@ public function isEmpty() {
   -      '#access' => empty($allowed_values_function),
          '#element_validate' => [[static::class, 'validateAllowedValues']],
          '#field_has_data' => $has_data,
          '#field_name' => $this->getFieldDefinition()->getName(),
          '#entity_type' => $this->getEntity()->getEntityTypeId(),
          '#allowed_values' => $allowed_values,
   +      '#access' => empty($allowed_values_function),
   

   This line is moving for no reason. Since the patch needs work anyway, would be good to remove this change.

3. +++ b/core/modules/options/src/Plugin/PredefinedOptionsPluginBase.php
   @@ -0,0 +1,14 @@
   +  // Add common methods and abstract methods for your plugin type here.
   

   Stray scaffolding code.

   In fact, we don't need this class since it's empty.

4. +++ b/core/modules/options/src/Plugin/PredefinedOptionsPluginInterface.php
   @@ -0,0 +1,37 @@
   +   * Returns the of allowed values.
   

   Typo.

5. +++ b/core/modules/options/src/Plugin/PredefinedOptionsPluginInterface.php
   @@ -0,0 +1,37 @@
   +   * @param bool &$cacheable
   

   I wonder if we should take this opportunity of a new system to change from this boolean $cacheable parameter to returning a proper cache metadata object.

6. +++ b/core/modules/options/src/Plugin/PredefinedOptionsPluginInterface.php
   @@ -0,0 +1,37 @@
   +   *   returned was specifically adjusted for that entity and cannot not be
   

   'cannot not'

Reroll for 9.3.x.

Removed the lines where typing was added in some variables. In Drupal 9.3.0 they are already added. File: core/modules/options/tests/src/Functional/OptionsFieldUITest.php

Re-rolled patch for 9.5.x

#36 Still needs to be addressed.

1 (nothing to do), 2, 4 and 6 are fixed.

For 3, not really sure what's going on, as the interface is defining getAllowedValues. I guess we could add a stub instead of removing it?
For 5, Still a to-do, currently too late for me to start on that.

Sounds like a good improvement over callbacks!

Some observations:

1. +++ b/core/modules/options/config/schema/options.schema.yml
   @@ -17,6 +17,9 @@ field.storage_settings.list_integer:
   +    predefined_options_plugin:
   +      type: string
   
   @@ -50,6 +53,9 @@ field.storage_settings.list_float:
   +    predefined_options_plugin:
   +      type: string
   
   @@ -83,6 +89,9 @@ field.storage_settings.list_string:
   +    predefined_options_plugin:
   +      type: string
   

   This needs an upgrade path to set the empty value

2. +++ b/core/modules/options/options.api.php
   @@ -36,6 +36,23 @@ function hook_options_list_alter(array &$options, array $context) {
   +function hook_options_predefined_options_info_alter(&$definitions) {
   

   I don't think this currently has test coverage?

3. +++ b/core/modules/options/options.api.php
   @@ -78,6 +95,11 @@ function hook_options_list_alter(array &$options, array $context) {
   + * @deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. Once the
   + *   predefined options plugin feature is completed.
   + *
   + * @see https://www.drupal.org/project/drupal/issues/1909744
   

   Versions need updating and needs to point to a Change record and not this issue

4. +++ b/core/modules/options/options.module
   @@ -80,12 +81,30 @@ function options_allowed_values(FieldStorageDefinitionInterface $definition, Fie
   +      catch (\Exception $e) {
   +        // Fail silently if the plugin does not exists or cannot be instantiated.
   +        watchdog_exception('options', $e);
   +        return [];
   +      }
   

   Do we usually fail silently on these? I think in other instances we just let it fail hard, something is seriously wrong if a plugin disappears.

5. +++ b/core/modules/options/options.module
   @@ -80,12 +81,30 @@ function options_allowed_values(FieldStorageDefinitionInterface $definition, Fie
   +      watchdog_exception('options', new \RuntimeException(t("The use of 'allowed_values_function' is deprecated, use :class plugins instead", [
   +        ':class' => PredefinedOptions::class,
   +      ])));
   

   An indication of which field is using this might help people track down what they need to update? And why the watchdog_exception and not just a trigger_error("bla bla", E_USER_DEPRECATED);?

6. +++ b/core/modules/options/src/Annotation/PredefinedOptions.php
   @@ -0,0 +1,33 @@
   +  public $id;
   

   Add a type hint

7. +++ b/core/modules/options/src/Annotation/PredefinedOptions.php
   @@ -0,0 +1,33 @@
   +  public $label;
   

   add a type hint

8. +++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
   @@ -106,6 +144,36 @@ public function storageSettingsForm(array &$form, FormStateInterface $form_state
   +    elseif (!empty($allowed_values_function = $this->getSetting('allowed_values_function'))) {
   

   Setting vars in ifs I think we try to avoid (it's pretty clumsy to read)

9. +++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
   @@ -329,4 +397,24 @@ protected static function castAllowedValue($value) {
   +      $this->messenger()
   +        ->addWarning($this->t('The use of callback functions is deprecated. Please replace %function function with a plugin. :link.', [
   

   Why not just trigger_error("bla bla", E_USER_DEPRECATED);

10. +++ b/core/modules/options/src/Plugin/Options/Timezones.php
    @@ -0,0 +1,26 @@
    +namespace Drupal\options\Plugin\Options;
    ...
    +class Timezones extends PredefinedOptionsPluginBase {
    

    Are we sure we want to supply this for all installs of Drupal, it feels like a good test case to use, but I'd say it should probably be in a test module. I don't think the need for this is in the '80% of all sites' category.

11. +++ b/core/modules/options/src/Plugin/PredefinedOptionsPluginInterface.php
    @@ -0,0 +1,37 @@
    +   * @param bool &$cacheable
    +   *   (optional) If an $entity is provided, the $cacheable parameter should be
    +   *   modified by reference and set to FALSE if the set of allowed values
    +   *   returned was specifically adjusted for that entity and cannot not be
    +   *   reused for other entities. Defaults to TRUE.
    

    Do we need test coverage for this?

12. +++ b/core/modules/options/src/Plugin/PredefinedOptionsPluginInterface.php
    @@ -0,0 +1,37 @@
    +   * @return array
    +   *   The array of allowed values. Keys of the array are the raw stored values
    +   *   (number or text), values of the array are the display labels. If $entity
    +   *   is NULL, you should return the list of all the possible allowed values
    +   *   in any context so that other code (e.g. Views filters) can support the
    +   *   allowed values for all possible entities and bundles.
    

    We need a test for this I feel, so use the options list in the Views UI and make sure it works

13. +++ b/core/modules/options/tests/options_test/src/Plugin/Options/PredefinedOptionsTestPlugin.php
    @@ -0,0 +1,37 @@
    +    if ($entity === NULL) {
    +      return [];
    +    }
    ...
    +    $values = [
    +      $entity->label(),
    +      $entity->toUrl()->toString(),
    +      $entity->uuid(),
    +      $entity->bundle(),
    +    ];
    

    This does exactly the opposite of what the method docblock says you should do when $entity is NULL, that sounds like a bad example to set

14. +++ b/core/modules/options/tests/src/Functional/OptionsFieldUITest.php
    @@ -333,6 +352,35 @@ public function assertAllowedValuesInput(string $input_string, $result, string $
    +  public function assertPredefinedOptionsInput($plugin_id, $result, $message) {
    

    We tend not to set messages for assertions anymore so we can probably leave that out

15. +++ b/core/modules/options/tests/src/Functional/OptionsFieldUITest.php
    @@ -333,6 +352,35 @@ public function assertAllowedValuesInput(string $input_string, $result, string $
    +      self::assertEquals($plugin->getAllowedValues($field_storage), $result, $message);
    

    We use $this and not self:: for assertions

16. +++ b/core/modules/options/tests/src/Functional/OptionsPredefinedOptionsPluginTest.php
    @@ -0,0 +1,34 @@
    +namespace Drupal\Tests\options\Functional;
    ...
    +class OptionsPredefinedOptionsPluginTest extends OptionsPredefinedOptionsTestBase {
    

    This is not using the browser I think? so does this need to be a functional test or can it be a kernel test?

17. +++ b/core/modules/options/tests/src/Functional/OptionsPredefinedOptionsPluginTest.php
    @@ -0,0 +1,34 @@
    +    $this->assertEquals($values, []);
    ...
    +    $this->assertEquals($values, $expected_values);
    

    Expected values go first

18. +++ b/core/modules/options/tests/src/Functional/OptionsPredefinedOptionsValidationTest.php
    @@ -0,0 +1,44 @@
    +    $allowed_values = $this->plugin->getAllowedValues($this->fieldStorage, $this->entity);
    +    foreach ($allowed_values as $key => $value) {
    

    Add a check that $allowed_values is not empty, currently if it is empty, the test passes

19. +++ b/core/modules/options/tests/src/Functional/OptionsSelectPredefinedOptionsTest.php
    @@ -0,0 +1,41 @@
    +        self::assertNotEmpty(array_search($value, $values, TRUE));
    

    We use $this not self:: for assertions

Oh forgot to refresh and missed the MR, so some of that might have been addressed already!

Actually not 😉 Think all your comments still apply. Although the deprecated watchdog_exceptions are replaced with the logger API.

We have another issue for using backed enums as a form of predefined list: #3249599: Add support for PHP 8.1 Backed Enums in Select, Checkboxes, Radio elements

Can we update/refocus this issue so it specifically outlines this is a plugin based solution?

Bonus for why plugin based solution is worthwhile alongside the enum solution (presumably dynamism?)

I understand allowed_values_function is due to be deprecated by this issue, it would be ideal to have a solution compatible with both plugin and enum solutions simultaneously.

I've made a D9/10 release of https://www.drupal.org/project/list_predefined_options.

#3249599: Add support for PHP 8.1 Backed Enums in Select, Checkboxes, Radio elements is not the same as this issue -- it's operating at the FormAPI level. This issue is at the FieldAPI level.

> Bonus for why plugin based solution is worthwhile alongside the enum solution (presumably dynamism?)

Dynamism is one. That would include things like being able to depend on other Drupal components, e.g. #1412946: Allow Views to be used for predefined lists. Reusability is another.

> it would be ideal to have a solution compatible with both plugin and enum solutions simultaneously.

Define a plugin which returns an enum.

Patch for Drupal 10.2.x