[regression] options_allowed_values() signature doesn't allow for Views filter configuration

What's needed here that isn't in #2238085: [regression] options_allowed_values() signature doesn't allow for Views filter configuration?

Sorry I meant #2116363: Unified repository of field definitions (cache + API).

I think we should improve allowed values workflow to work along what I've proposed for default values here: #2226267: Improve default value handling of fields to be consistent.

In short, the main way to get allowed values should be via the (field) definition. Field types should be able to provide a suiting default implementation.

Marking as beta deadline as this will change how you define allowed values for field types and how you retrieve them.

Beta deadline means we'd bump it to 8.1.x or 9.0.x if it doesn't get in, which we can't if it's supposed to block the 8.x release.

This feels like it's more likely to be an API addition than a change, but moving to beta blocker since fago's comment suggests not.

@fago, so #2226267: Improve default value handling of fields to be consistent would potentially solve this and (with a regression test and fix to the Views handler) this one could then be marked as a duplicate?

(The patch in #2226267: Improve default value handling of fields to be consistent so far only includes API additions but it also looks like it was mainly a proof-of-concept.)

Not sure if that issue will solve this? This is about having to pass a field instance in to get option values.

Please include steps to reproduce this.

This is not really something to reproduce. This is just something that is not currently possible.

ok, I started looking at this and came up with a possible solution:

• We do not have per field type definition classes so we have to use a separate class for providing that logic if it should not live all static the field item. As it's not the 99% use case I think a separate class is the better and cleaner solution here + allows one to easily override it.
• Moved AllowedValuesInterface to an AllowedValuesProviderInterface and allow field types to specify a provider class. Then allow field definitions to provide per instance classes such that we can keep supporting per instance allowed values such as field instances support already.
• On FieldDefinition we can do a setAllowedValues() which internally keeps the value and provides a suiting allowed values provider.
• Problem with the new interface is that it now needs to get the existing value passed on for generating settable options or values. For DX having just a generic typed data interface would suck here as then you just get a $data parameter, instead of FieldItem. Therefore, and in the light of #2268049: Decouple field definitions from typed data definitions imo we should provide dedicated interface but keep them aligned, such that we can easily bridge from the field implementation to the typed data implementation once decoupled.
• For typed data, we do not have another use case of specifying of allowed values in core yet, but Rules we'll going to use the same for specifying allowed context values.

Attached is a first implementation start, unfortunately I was not able to finish it.

1. +++ b/core/lib/Drupal/Core/Field/DefaultAllowedValuesProvider.php
   @@ -0,0 +1,72 @@
   +class DefaultAllowedValuesProvider implements AllowedValuesProviderInterface {
   

   It's not really a "default" (as in "basic implementation that works in most cases"), it's an implementation based on an 'allowed_values' field setting.

   IMO it would make sense to split this into an abstract Base class containing the flattenOptions(), getPossibleValues(), getSettableValues(), getSettableOptions() methods, and leaving the getPossibleOptions() method for actual implementations.

   Which points out that
   - it's slightly inconsistent to have the interface be Allowed*Values*providerInterface, with the actual business case logic residing on getPossible*Options*()
   - the 'allowed_values' setting is in fact about 'options', not 'values' :-)

2. +++ b/core/lib/Drupal/Core/Field/DefaultAllowedValuesProvider.php
   @@ -0,0 +1,72 @@
   \ No newline at end of file
   

   :-)

3. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
   @@ -575,4 +575,22 @@ public function getUniqueStorageIdentifier() {
   +  public function setAllowedValues($values) {
   +    return $this->setting('allowed_values', $values);
   

   Not a fan of this. 'settings' are the land of field types, we shouldn't have methods around this in the generic FieldDefinition. The split between 'generic logic of the Field API' and 'business logic specific to a (couple) field type(s)' is a very structuring one, I'd be wary of blurring that line.

   It's a fact that the FieldDefinition API provides no generic, built-in functionality for "allowed values", because, unlike "default values", it's a notion that makes sense for only a couple field types. So it's not really a bug if there's no "generic way to get the list of possible values for a field", it never was the case in D7.

If *anyone* else wants to work on this, please just ping me and I'll hand it back :)
But, for now, I'm going to look at it.

In d7 you could get allowed values based on a field instance. Now you need to pass in an actual entity. That's the main issue here.

Here is a fixed version of the patch. This now doesn't blow up at least :)

Let's see how it gets on anyway.

However, after getting it working and having a dig around. This still does not fix our immediate issue of what to do about options_allowed_values() and allowed_values_function and having to pass an entity as a parameter. We need to only support list fields that do not want to use an entity, or drop the parameter I guess...

Or, we just do something like this :p

I do think having a provider is better though.

EDIT: yched, I haven't ignored your feedback, getting on to that.

I was pondering #22. I think that might be enough if we fix all the places expecting an actual entity?

Well, IIRC, in D7 the ability to make the "allowed values callback" depend on the host entity got added as a side feature request, with the explicit warning that
- there would not always be a known entity (specifically for cross-bundle cases like Views),
- and that the callbacks needed to account for that fact : e.g the result with a NULL entity should be a superset of the responses with an actual $entity.

The problem with #22 is that options_allowed_values() calls methods on $entity to build the static cache key :

$cache_id = implode(':', array($entity->getEntityTypeId(), $entity->bundle(), $field_definition->getName()));

We need to replace that with $field_definition->getTargetEntityTypeId(), $field_definition->getBundle()
(the latter doesn't exist currently, it's being added in #2144263: Decouple entity field storage from configurable fields)

Yes, that would break for sure, was just the general idea :)

That is one easy way to tack it though....

Would it be possible to get the allowed values similar to how the AllowedValuesConstraintValidator works?

// $this->context is of type ExecutionContextInterface
$allowed_values = $this->context->getMetadata()->getTypedData()->getSettableValues($account);

Re. #26/27. From that issue:

+++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
@@ -471,6 +470,26 @@ public function setTargetEntityTypeId($entity_type_id) {
   /**
    * {@inheritdoc}
    */
+  public function getBundle() {
+    return isset($this->definition['bundle']) ? $this->definition['bundle'] : NULL;
+  }
+
+  /**
+   * Sets the bundle this field is defined for.
+   *
+   * @param string|null $bundle
+   *   The bundle, or NULL if the field is not bundle-specific.
+   *
+   * @return $this
+   */
+  public function setBundle($bundle) {
+    $this->definition['bundle'] = $bundle;
+    return $this;
+  }

+++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
@@ -55,6 +55,15 @@
+   * Gets the bundle the field is defined for.
+   *
+   * @return string|null
+   *   The bundle the field is defined for, or NULL if it is base field; i.e.,
+   *   it is not bundle-specific.
+   */
+  public function getBundle();

None of that looks too specific to what's being done in the rest of the issue; maybe we could pull it out into its own separate patch? Edit: unless the bundle is not yet part of the definition and I just missed that elsewhere. :)

Re: #26 — so should this be postponed/blocked on #2144263: Decouple entity field storage from configurable fields?

@geerlingguy : well, #2144263: Decouple entity field storage from configurable fields should be close now, but worst case, as @xjm said, the patch here can always stel the parts that add getBundle(), it's fairly straightforward code IIRC.

Yeah, I'd suggest just trying this patch with those bits added and maybe a -do-not-test.patch that excludes them; we can easily remove them once #2144263: Decouple entity field storage from configurable fields goes in. If it turns out to be more onerous than that we can postpone it.

Here is an update as per #32

Here is a reroll of #21. I still think that if we want to make this better, D8 style, we need this. And not the new simplistic approach suggested in #22. We would still be left with the procedural function that we would need to call. which is meh.

Thoughts on that? If we want the simple approach, we can. It is just a quick fix though really. This could be a good chance to clean this area up a little. Do we also want to consider whether we are going to make the entity dependency optional for allowed values or remove it entirely. I think this is fine, views options will not work correctly if an entity is needed. So any field that did this would be useless there anyway. Or if we suck it up and make it optional, and accept this will not work with views. Basing options on the current entity is really not a major use case? Seems like very few people would want this.

Is there are way someone could get the entity if needed? So we could just have it available but not a part of the api to call this?

Do we also want to consider whether we are going to make the entity dependency optional for allowed values or remove it entirely. I think this is fine, views options will not work correctly if an entity is needed. So any field that did this would be useless there anyway. Or if we suck it up and make it optional, and accept this will not work with views. Basing options on the current entity is really not a major use case? Seems like very few people would want this.

Looking back at #1541792: Enable dynamic allowed list values function with additional context, that introduced this "allowed values by $entity" feature 2 years ago (thus quite late post 7.0 - was added to D8 with a backport to D7 - see commit at #1541792-45: Enable dynamic allowed list values function with additional context)

- Originally posted by @amitaibu, @dww and @chx picked up with lots of interest, notably for project.module (https://www.drupal.org/project/dereference_list). @tim.plunkett and @sun seemed to want this too

- Views and the impact on Views is not mentioned once in the issue :-)
The behavior this introduced is : the allowed values for the field "in general", and for the field "in a specific $entity" can be different.
The only way this can be consistent is : the $entity-specific allowed values have to be a subset of the "general" allowed values. Then how it translates in Views can make sense.
But this constraint is :
a) entirely up to the implementations of the custom "allowed values callbacks",
b) not explicitely mentioned anywhere in the docs

(As an interesting side note, it was that issue that added '#entity' => $entity in each $form[$field_name], just so that the options widget can grab it. Widgets have other ways to access the parent $entity now, so we can unburden the $form structure. Separate issue though, opened #2293723: Generate lighter $form[$field] structures)

I'm really not a fan of this feature either (and it seems I didn't participate in that issue at all back then, I guess i was afk). But the uses cases seem real :-/
Also, D7 + Views D7 have been working with it for the past 2 years (kind of - you can get weird behavior if your "allowed values" callbacks don't respect the implicit "subset" constraint). It's only the current shape of the D8 API that provides no way to get the allowed values without an $entity, and thus blocks Views, the D7 API allows that.

So I'd say it's more a question of fixing the D8 API than removing the feature, as kludgy / fragile as it is.

Or : if we removed that feature, which I'd be happy with :-), how can similar behavior be attained ? By form alters and adding custom constraints ?

Part of me thinks we should just remove the regression by returning to the optional-ness of $entity.

I don't see how #38 gets us any closer to solving the Views use case. If we want to make $entity optional, we can do so without passing in $entity_type_id, since that can be retrieved internally within options_allowed_values() via $field_definition->getFieldStorageDefinition()->getTargetEntityType().

However, the more important problem for the Views use case is that the first parameter is a FieldDefinitionInterface, and Views only has a FieldStorageDefinitionInterface to work with. Which makes figuring out a good signature for the allowed_values_function difficult.

I have an idea I want to try.

You can't always get the allowed values in D7 views either, but at least it doesn't fatal.

I was shooting for that level of fix. But I completely missed the FieldDefinitionInterface vs FieldStorageDefinitionInterface difference :(

Looking forward to your idea.

Here's what I think is the minimum needed to solve this issue. I'll open some follow ups though in a bit.

Filed all the followups. Will work on #42's test failures next.

I don't understand why flattenOptions() strips labels, but apparently that's by design, so adjusted for that.

Nothing in #45 looks beta-bound to me, so removing that tag. Instead, I added a "beta deadline" tag to #2305397-1: Field type (item) classes implementing AllowedValuesInterface::getPossible(Values|Options)() is incompatible with Views and possibly other use cases. Also, retitling for the limited fix of #45.

Regarding the "needs tests" tag, do we need to add tests here, or can we do that in #2012130: Regression: Views integration for "list" field types is broken instead? I don't think additional unit tests would be helpful for this. Integration tests with Views would be, but need that issue.

Here's a plain re roll after #2287727: Rename FieldConfig to FieldStorageConfig

2) In #46, @effulgentsia suggested that we add tests as part of #2012130: Regression: Views integration for "list" field types is broken.

Sounds like nothing is blocking this one. I'll request a retest .

While the proposed patch solves the problem for Views by bypassing our new API for options lists, it does not solve the underlying problem. However, as effulgentsia summarized nicely at #2305397: Field type (item) classes implementing AllowedValuesInterface::getPossible(Values|Options)() is incompatible with Views and possibly other use cases the current API could be used by creating dumb, empty entity objects. Still, that's sub-optimal and I'd love to see a decent solution that solves the problems outlined here and covers our other use cases on context definitions as well. Therefore, I opened #2329937: Allow definition objects to provide options for the proposed solution.

Code needs work based on #52, we need to eat our own food or order something else, not sneak in the kitchen to pick some sweets

updated issue summary to note this is blocking #2012130: Regression: Views integration for "list" field types is broken (so when something happens here, like a fix, we remember to activate the other issue)

Reroll of #47

adding blocker tag, since this blocks #2012130: Regression: Views integration for "list" field types is broken, so that the d8 blockers is accurate. Remove the blocker tag when this issue is fixed, and unpostpone 2012130.

Can't find anything that annoys me, but this could probably get a +1 from fago and/or yched for RTBC.

I've been discussing #2329937: Allow definition objects to provide options with yched yesterday and it will require some more work, thus I'm fine with moving on with this stop-gap fix first. Patch generally looks good, but here a remark:

+++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsDefaultFormatter.php
@@ -34,16 +34,30 @@ class OptionsDefaultFormatter extends FormatterBase {
+      // We assume here that the possible options do not vary by item within
+      // a multivalued list field, and optimize for performance by only
+      // invoking it once. This assumption is not guaranteed by
+      // AllowedValuesInterface, but is assumed to be true for list fields.
+      // @see \Drupal\options\Plugin\Field\FieldType\ListItemBase::getPossibleOptions().
+      if (!isset($possible_options)) {

It's not allowed values interface any more. Howsoever, possible options should include all possible options which cannot vary by value as well, it should include all possible options. Thus, I'd say it's ok to just do $items->getFieldDefinition()->getOptionsProvider->getPossibleOptions() what
a) looks less weird
b) it's the main API one should use for fetching options, as it would cover any other options that might be set on the field definition

Also, strictly speaking the change of the function signature is an "API" change, but I don't think it's an issue because as said the right API to fetch options is the field definition getOptionsProvider() method.

1. +++ b/core/modules/options/options.module
   @@ -54,30 +54,43 @@ function options_field_storage_config_delete(FieldStorageConfigInterface $field_
   -function options_allowed_values(FieldDefinitionInterface $field_definition, EntityInterface $entity) {
   +function options_allowed_values(FieldStorageDefinitionInterface $definition, FieldItemListInterface $items = NULL) {
   

   Receiving a FSDI rather than a FDI is fine (nitpick : I'd still go for accuracy and call the param $field_storage_definition)

   But why pass a FieldItemListInterface instead of an Entity ?

2. +++ b/core/modules/options/options.module
   @@ -54,30 +54,43 @@ function options_field_storage_config_delete(FieldStorageConfigInterface $field_
   -  $cache_id = implode(':', array($entity->getEntityTypeId(), $entity->bundle(), $field_definition->getName()));
   +  $cache_keys = array($definition->getTargetEntityTypeId(), $definition->getName());
   +  if (isset($items)) {
   +    $cache_keys[] = $items->getEntity()->bundle();
   +  }
   

   I dug into the git history of this line, and I think we shouldn't bother with adding the bundle here. It was added by #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets (d9f7b3a), which was the root of the issue here : it's the issue that changed options_allowed_values() from
   "receive a $field + maybe an $instance and an $entity for context"
   to
   "always receive a FDI and an $entity".

   Now that we revert to a saner state ("receive an FSDI + maybe an entity for context"), we should also revert to "cache values if they depend only on the FSDI (= entity_type + field_name), and let callbacks opt out of cacheability if the additionally depend on the $entity context, through the $cacheable parameter)

+++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsDefaultFormatter.php
@@ -34,16 +34,30 @@ class OptionsDefaultFormatter extends FormatterBase {
+      // If there's a single string label for the value, output that.
+      // Otherwise, if the value is no longer an allowed value, or if it is
+      // within or the top-level of an option group, output the value itself.
+      // @todo \Drupal\Core\Form\OptGroup::flattenOptions() would be useful
+      //   here if it retained labels.
+      if (isset($possible_options[$value]) && is_string($possible_options[$value])) {

options groups : right, OptionsProviderInterface provides no way to get "a *flat* list of values with labels".

The efficient way, for cases with huge lists of allowed values, would be to have a getLabels(array $values) method in OptionsProviderInterface, instead of fetching the full list of values.

Adding a new method to OptionsProviderInterface is a BC break for existing implementations. We're considering maybe breaking it in #2329937: Allow definition objects to provide options, but if we do we should try to break it only once.

So for now, we need to flatten the list ourselves here :-/. And yeah, it's sad that OptGroup::flattenOptions() doesn't retain labels. I opened #2392301: OptGroups::flattenOptions() should preserve labels.

Meanwhile, it looks like we need to copy the helper code from OptionWidgetBase::flattenOptions() :-(

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsWidgetBase.php
   @@ -200,7 +200,7 @@ protected function getSelectedOptions(FieldItemListInterface $items, $delta = 0)
   -  protected function flattenOptions(array $array) {
   +  public static function flattenOptions(array $array) {
   
   +++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsDefaultFormatter.php
   @@ -34,16 +35,31 @@ class OptionsDefaultFormatter extends FormatterBase {
   +      $flattened_possible_options = OptionsWidgetBase::flattenOptions($possible_options);
   

   Hmm, not too fond of this. A Widget base class is not a great place to put generic helpers :-)

   I'd be fine with it being temporary until we have a better solution like #2392301: OptGroups::flattenOptions() should preserve labels, but if we commit this now, removing it later would be an API change.

2. +++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsDefaultFormatter.php
   @@ -34,16 +35,31 @@ class OptionsDefaultFormatter extends FormatterBase {
   +      // We assume here that the possible options do not vary by item within
   +      // a multivalued list field, and optimize for performance by only
   +      // invoking it once. This assumption is not guaranteed by
   +      // OptionsProviderInterface, but is assumed to be true for list fields.
   +      // @see \Drupal\options\Plugin\Field\FieldType\ListItemBase::getPossibleOptions().
   +      if (!isset($possible_options)) {
   +        /* @var \Drupal\Core\TypedData\OptionsProviderInterface $item */
   +        $possible_options = $items->getFieldDefinition()->getOptionsProvider()->getPossibleOptions();
   +      }
   

   Oops, I thought I posted that earlier, but apparently d.o ate it (or I never pushed "Submit").

   Now that this works on $items, this has no reason to be in the loop anymore.

   if ($items->count()) {
    $options = ... (obtained from $items) ...
     foreach ($items as $item) {
       if (isset($options[$item->value]) {
         ...
       }
       else {
         ...
       }
     }
   }
   

   looks way simpler :-)

Sorry :-/

1. +++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsDefaultFormatter.php
   @@ -34,16 +35,33 @@ class OptionsDefaultFormatter extends FormatterBase {
   +    // We assume here that the possible options do not vary by item within
   +    // a multivalued list field, and optimize for performance by only
   +    // invoking it once. This assumption is not guaranteed by
   +    // OptionsProviderInterface, but is assumed to be true for list fields.
   +    // @see \Drupal\options\Plugin\Field\FieldType\ListItemBase::getPossibleOptions().
   +    $possible_options = NULL;
   +    if (!isset($possible_options)) {
   +      $provider = $items->getFieldDefinition()
   +        ->getFieldStorageDefinition()
   +        ->getOptionsProvider('value', $items->getEntity());
   +      $possible_options = $provider->getPossibleOptions();
   +    }
   

   $possible_options = NULL;
   if (!$possible_options) {
     $possible_options = ...;
   }
   

   Er, we can further simplify that, can't we ? ;-)

   Also, that longish comment is not needed IMO. Yes, allowed values are the same for all deltas, that's not a new thing, that's just how they work. OptionsWidget::getOptions() does the same already, and doesn't bother with this long explanation.

   So in the end it's as simple as :

   $provider = (...);
   $options = $this->flattenOptions($provider->getPossibleOptions()):
   

2. +++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsDefaultFormatter.php
   @@ -34,16 +35,33 @@ class OptionsDefaultFormatter extends FormatterBase {
   +      // Otherwise, if the value is no longer an allowed value, or if it is
   +      // within or the top-level of an option group, output the value itself.
   

   The "option group" part is irrelevant now

Streamlined the formatter code a bit more
+ as mentioned in #66, we need an if ($items->count()) so that we don't trigger a possibly costly computation of allowed values if there is nothing to display.

Oops, same as #72 without the trailing space :-/

1. +++ b/core/modules/options/options.module
   @@ -54,30 +56,41 @@ function options_field_storage_config_delete(FieldStorageConfigInterface $field_
   + *   configuring Views filters, this is not passed, so it is recommended that
   

   minor: I'd even say it should be that way.

2. +++ b/core/modules/options/options.module
   @@ -54,30 +56,41 @@ function options_field_storage_config_delete(FieldStorageConfigInterface $field_
   + *   such functions are written such that the  array keys returned when this
   

   one space to much before "array keys".

3. +++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsDefaultFormatter.php
   @@ -34,21 +35,41 @@ class OptionsDefaultFormatter extends FormatterBase {
   +  ¶
   

   still white space here.

4. +++ b/core/modules/options/tests/options_test/options_test.module
   @@ -5,13 +5,16 @@
   -function options_test_allowed_values_callback(FieldDefinitionInterface $field_definition, EntityInterface $entity) {
   +function options_test_allowed_values_callback(FieldStorageDefinitionInterface $definition, FieldableEntityInterface $entity) {
   

   Isn't that an API change for allowed values callbacks?

Well,

-function options_test_allowed_values_callback(FieldDefinitionInterface $field_definition
+function options_test_allowed_values_callback(FieldStorageDefinitionInterface $definition

is indeed an API change - that this exactly the broken concept that is being fixed here, but yes, it is an API change.

Other than that, @fago pointed that the signature of options_test_allowed_values_callback() should have the $entity param default to NULL, as otherwise PHP will fatal when the function is called with a NULL $entity (the patch already changes the other test callback, options_test_dynamic_values_callback() that way).

I am going to work on a bit of... unclear documentation in the patch. :)

So one of the reasons the documentation for options_allowed_values() is confusing is that inside the parameter documentation, it's attempting to explain how to write a specific kind of callback that happens to get registered within that function. We have a standard way of documenting callbacks since #1250500: [policy adopted; patch done] Find a (standard) way to document callback signatures, so I'll try adding that to make things more clear for developers providing field option callbacks.

How's this?

Now with the missing predicate.

Way much better!

Nice to see this being sorted out - I think we also need to do something for default_value_callback since this requires an entity too - although this might not be so critical. I might have created an issue for this already but I can not find it.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 4de09b6 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/options/options.api.php b/core/modules/options/options.api.php
index 4aa6105..67d67a7 100644
--- a/core/modules/options/options.api.php
+++ b/core/modules/options/options.api.php
@@ -5,6 +5,9 @@
  * Hooks provided by the Options module.
  */
 
+use Drupal\Core\Entity\FieldableEntityInterface;
+use Drupal\Core\Field\FieldStorageDefinitionInterface;
+
 /**
  * Alters the list of options to be displayed for a field.
  *
diff --git a/core/modules/options/tests/options_test/options_test.module b/core/modules/options/tests/options_test/options_test.module
index 5bc0055..2d719d3 100644
--- a/core/modules/options/tests/options_test/options_test.module
+++ b/core/modules/options/tests/options_test/options_test.module
@@ -6,7 +6,6 @@
  */
 
 use Drupal\Core\Entity\FieldableEntityInterface;
-use Drupal\Core\Field\FieldItemListInterface;
 use Drupal\Core\Field\FieldStorageDefinitionInterface;
 
 /**

Did the above (very minor) fixes on commit.

Posted a followup patch to refine the callback docs : #2393061: Adjust phpdoc for callback_allowed_values_function()