Allow adding predefined lists to the options fields

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: ✨ Add support for PHP 8.1 Backed Enums in Select, Checkboxes, Radio elements Needs work

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 this plugin and enum solutions.

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

✨ Add support for PHP 8.1 Backed Enums in Select, Checkboxes, Radio elements Needs work 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. ✨ Allow Views to be used for predefined lists Active . 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