Deprecate remaining options.module functions

Moving the code from options_allowed_values() to ListItemBase::getSettableOptions() does not make any problem.

However, options_allowed_values() is also called from Drupal\field\Plugin\views\argument\FieldList and Drupal\field\Plugin\views\argument\ListString, where I was unable to access to ListItemBase::getSettableOptions().

We could duplicate the code in those view plugins, but this looks weird IMHO. Or maybe create a service which could be accessible both both places. Or there is a hidden way to access to the Field Item from the view plugins I'm unable to find...

Any thoughts?

Regards.

However, options_allowed_values() is also called from Drupal\field\Plugin\views\argument\FieldList and Drupal\field\Plugin\views\argument\ListString, where I was unable to access to ListItemBase::getSettableOptions().

It seems to be that making this method protected was kind of wrong?

So, should we postpone this issue on #2012130: Regression: Views integration for "list" field types is broken or maybe move the function to ListItemBase::getSettableOptions() and manage the allowed values issue in views there?

Huh. Am I missing something?

Oops.

OK, I guess I assumed we'd have made more progress on the Entity Field API DX by now ;-)

Not sure how to resolve these:

Fatal error: Call to undefined method Drupal\field\Entity\FieldConfig::isValueInUse()
  in core/modules/options/options.module on line 51

Fatal error: Call to undefined method Drupal\Core\Field\ConfigFieldItemList::getAllowedValues()
  in core/modules/options/lib/Drupal/options/Plugin/Field/FieldFormatter/OptionsDefaultFormatter.php on line 34

It looks like $field can be a ListItemBase, FieldConfig, or ConfigFieldItemList, depending on what exactly the code in context understands under $field? :P ;)

This function blocks "merge Drupal\options\Plugin\Field\FieldType\ListBooleanItem into Drupal\Core\Field\Plugin\Field\FieldType\BooleanItem"

This fulfills #1577902: [META] Remove all usages of drupal_static() & drupal_static_reset() regarding removal of drupal_static() & Co. from options_allowed_values().

Hello,

I tried to work today on this issue. It seems that finally I got something workable, but it's been more complex than expected. The fact that FieldStorageDefinitionInterface::getOptionsProvider() method requires to pass an entity made it quite hard in the Views situation, where the entity was not directly available.

Also the fact that requiring an instance of ListItemBase to have access to that options that can be directly accessible from the fieldStorage made it a bit tricky.

Not sure if this is the best approach or maybe we could try to use a service or similar that could manage the options allowed values.

Attaching a prrof of concept path. Let's see if testbot agrees.

Regards

Updating issue status

Attaching a new version. Last one had the wrong base path.

Sorry for the noise.

New version trying to solve issues found by testbot.

New try fixing tests.

Another try

+++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
@@ -58,15 +92,14 @@ public function getSettableValues(AccountInterface $account = NULL) {
   public static function generateSampleValue(FieldDefinitionInterface $field_definition) {
-    $allowed_options = options_allowed_values($field_definition->getFieldStorageDefinition());
+    $allowed_options = $field_definition->getSetting('allowed_values');
     $values['value'] = array_rand($allowed_options);
     return $values;

this does mean we lose the ability to use this function in combination with a allowed values callback?

Also, looks like the cacheable-feature was removed.

And I don't think the approach you have with views is feasible, the APi was defined for entity to be optional.

Maybe a static method or a service as you suggested would be better.

Hello,

Thank you for your review. As mentioned in my last comment, this approach is not the best and needs some improvements in order to get the final solution.

Regarding your comments:

this does mean we lose the ability to use this function in combination with a allowed values callback?

We did not use the allowed values callback in that situation because the method is defined at the ListItemBase level and in that method I don't have access to it, so the less bad solution was to use the allowed values property.

Also, looks like the cacheable-feature was removed.

Given that we store the allowed values as a property at the ListItemBase level, I believe it's not necessary to add the cache layer.

And I don't think the approach you have with views is feasible, the APi was defined for entity to be optional.

It is optional in the current APi, but if we rely on the FieldStorageDefinitionInterface::getOptionsProvider(), it is now required. In the issue where it was introduced, it was commented that it could be optional in the future. However, it has not happened yet.

Maybe a static method or a service as you suggested would be better.

Agree, otherwise I think we would have to break so many things to have this in place.

Do you have any idea? Some discussion or guidance would be much appreciated.

Hi,

Here is a new patch following a simpler approach as a proof of concept. Just replace options_allowed_values with a service that behaves in the same way.

Of course, it would require some cleanup, but just throwing the idea to start the discussion.

Sorry, forgot to add the new file to the patch.

The related issue #1909744: Allow adding predefined lists to the options fields via plugins implements a plugin manager for predefined list that provides the same DX as the current options_allowed_values()

Updating deprecating message drupal:8.8.0 and will be removed from drupal:9.0.0 to drupal:9.1.0 and will be removed from drupal:10.0.0

Linking #3161345: options_allowed_values() cache pollution, which would effect what we're working on here.

1. +++ b/core/modules/options/src/Plugin/views/argument/NumberListField.php
   @@ -35,7 +35,8 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
    
        $field_storage = $this->getFieldStorageDefinition();
   -    $this->allowedValues = options_allowed_values($field_storage);
   +    $this->allowedValues = $allowed_options = \Drupal::service('options.allowed_values')->allowedValues($field_storage);
   +
   

   the local variable doesn't do anything it seems, the method is over after this.

2. +++ b/core/modules/options/src/Plugin/views/argument/StringListField.php
   @@ -34,7 +34,7 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
        parent::init($view, $display, $options);
    
        $field_storage = $this->getFieldStorageDefinition();
   -    $this->allowedValues = options_allowed_values($field_storage);
   +    $this->allowedValues = $allowed_options = \Drupal::service('options.allowed_values')->allowedValues($field_storage);
   

   same.

Also the usual deprecation version updates.

I have re-rolled it against 9.3.x and fix from #44.
Kindly review it

See coding standard issues linked on "Custom Commands Failed".

Tried fixing custom command failure errors, thanks!

Looks good overall, but:

+++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
@@ -55,7 +55,7 @@ public function getSettableValues(AccountInterface $account = NULL) {
+    $allowed_options = \Drupal::service('options.allowed_values')->allowedValues($this->getFieldDefinition()->getFieldStorageDefinition(), $this->getEntity());

All of these calls to the new service should have it injected into the plugin.

@plopesc #33:

The current patch essentially moves code from a function to a static method, which is IMHO not the idea of the original IS.

but if we rely on the FieldStorageDefinitionInterface::getOptionsProvider(), it is now required. In the issue where it was introduced, it was commented that it could be optional in the future. However, it has not happened yet.

I think that this (FieldStorageDefinitionInterface::getOptionsProvider()) is the way to go, rather than last patch. Or du i miss something?

Fixed the "custom command fail issue", I am not sure that link is correct, Please have a look and advise.

+++ b/core/modules/options/options.module
@@ -71,7 +71,7 @@
- * @see callback_allowed_values_function()
+ * @see https://www.drupal.org/project/drupal/issues/2171397

so the drupalci check expects the next @see after @deprecated to be a link to the change record. We could still keep the old reference as long as it's below the URL.

The URL would need to go the change record though, and not this issue.

\Drupal\Core\Field\FieldStorageDefinitionInterface::getOptionsProvider() has a required $entity argument, while this service does not. I don't think that it is possible to online the implementation directly into \Drupal\options\Plugin\Field\FieldType\ListItemBase::getSettableOptions() because getting a list item object without an entity is very tricky and would require to create a dummy content entity at runtime for e.g. views filters. All the calls in the options.module views filter/argument plugins do not pass an entity and can't do so.

I don't see a problem with having this as a separate service, seems like the only feasible option?

Re #48: typed data plugins do not support DI. But views plugins do, see below.

1. +++ b/core/modules/options/src/OptionsAllowedValues.php
   @@ -0,0 +1,61 @@
   +  /**
   +   * Array containing the cached allowed values.
   +   *
   +   * @var array
   +   */
   +  protected $allowedValues;
   

   "Array containing" seems a but superfluous, suggestion: "Cached allowed values, keyed by field target type, field name and whether or not an entity was provided as context"

2. +++ b/core/modules/options/src/OptionsAllowedValuesInterface.php
   @@ -0,0 +1,42 @@
   +   *
   +   * The strings are not safe for output. Keys and values of the array should be
   +   * sanitized through \Drupal\Core\Field\AllowedTagsXssTrait::fieldFilterXss()
   ...
   +   *
   

   this description needs to be synced with what's on options_allowed_values(), this trait doesn't exist anymore.

3. +++ b/core/modules/options/src/OptionsAllowedValuesInterface.php
   @@ -0,0 +1,42 @@
   +   *
   +   * @see callback_allowed_values_function()
   +   */
   +  public function allowedValues(FieldStorageDefinitionInterface $definition, FieldableEntityInterface $entity = NULL);
   

   I'd suggest to use getAllowedValues(), I think we're going away from methods that have that shortened way. Helps to make it clear that it returns that value and doesn't set or something.

4. +++ b/core/modules/options/src/Plugin/views/argument/NumberListField.php
   @@ -35,7 +35,8 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
    
        $field_storage = $this->getFieldStorageDefinition();
   -    $this->allowedValues = options_allowed_values($field_storage);
   +    $this->allowedValues = \Drupal::service('options.allowed_values')->allowedValues($field_storage);
   

   the views filter plugins could use DI.

Worked on points 1 and 4 (DI) from #52.
Other points still need work...
Hopefully it helps somewhere...

• Working on this
• Converted #53 to a MR (no changes, verbatim)
• Hide patches
• Adding tags

It'd expand this to both remaining functions probably

Ready for review.

Sorry this one appears to need a rebase please.

Created #3583435: PP-1 Deprecate/remove ListItemBase::submitFieldStorageUpdate() as followup which is blocked on this.

Ready for a new review

Looks good to me but will let nicxvan mark it as he’s been more involved in the review already

I think this is ready now!

This one is surprisingly complex and there is one follow up.

Deprecations are the correct versions and consistent.

Only thing that might be worth thinking about is OptionsAllowedValuesInterface, but it might make sense to allow that to be swapped out.

New files have declare(strict_types=1);

We had a broader plan for how to tackle this in #2329937: Allow definition objects to provide options as part of the D8 entity API lifecycle.

We just never got time to finish it - but the API over there still sounds very solid.

In this issue we're moving the legacy function and introducing OptionsAllowedValuesInterface which is another API we will have to support/deprecate if we ever finish that work.

Was any consideration given to moving the logic of OptionsAllowedValues internal to ListItemBase::getSettableOptions and making that the new permanent API we replace `options_allowed_values` with?

Then the views plugins could use `$this->getFieldStorageDefinition()->getOptionsProvider()->getSettableOptions()` instead of injecting the new service.

I'll ping @fago for his thoughts.

Yes, in fact that was the original approach, looks like it was pivoted in 34 due to 32 and 33.

Ran the manual 'test-only' test. Output:

https://git.drupalcode.org/issue/drupal-2171397/-/jobs/9261107

LGTM.

Thank you @oily, however, running test only is only really helpful for Bugs. This is a Task the test only fails because the new service that was created no longer exists, not because there is a specific test tracking something.

We don't need to run test only for Tasks.

@nicxvan Okay, thank you for the reminder. I misinterpreted the output, too.

No problem!

I went through 32-34 in more detail and I think it's talking about the second parameter in options_allowed_values(FieldStorageDefinitionInterface $definition, ?FieldableEntityInterface $entity = NULL)

Looking in contrib about half or more of the calls seem to be without the optional parameter, putting it on ListItemBase essentially makes this required.
https://search.tresbien.tech/search?q=options_allowed_values

The views comment also seems accurate too, but I'm less sure I understood that correctly.

Re #78 thoughts then on emitting a deprecation error from the new interface if $entity is not passed?
That gives us a chance to move to the desired API in a subsequent major.

Should this be NW for the latest feedback?

Yes