Provide the options list of an entity field

Here's the first patch for that, finally ;)

+++ b/core/lib/Drupal/Core/TypedData/AllowedValuesInterface.phpundefined
@@ -0,0 +1,63 @@
+   * @return array
+   *   The array of available values.

Description a bit terse, is this an indexed array? Then it should probably be a "list of values"?

+++ b/core/lib/Drupal/Core/TypedData/AllowedValuesInterface.phpundefined
@@ -0,0 +1,63 @@
+  public function getAvailableValues(\Drupal\user\Plugin\Core\Entity\User $user = NULL);

You probably want to leave out the type hint yet, we have it in entity access and it's a mess.

No point in adding it before we have a global interface for $user.

If you add it, it should be with use.

+++ b/core/lib/Drupal/Core/TypedData/AllowedValuesInterface.phpundefined
@@ -0,0 +1,63 @@
+   * This is similar to hook_options_list().
+   * @see hook_options_list()

@see should be at the bottom.

+++ b/core/lib/Drupal/Core/TypedData/AllowedValuesInterface.phpundefined
@@ -0,0 +1,63 @@
+   *   should be of the data type (string, number...) expected by the first
+   *   'column' for the field type. Array values are the labels to display

Hm, first column? I would expect that allowed values only works non-complex data, otherwise the value would be an array of somthing?

+++ b/core/lib/Drupal/Core/TypedData/AllowedValuesInterface.phpundefined
@@ -0,0 +1,63 @@
+  public function getAvailableOptions(\Drupal\user\Plugin\Core\Entity\User $user = NULL);

Missing @param for the user.

+++ b/core/lib/Drupal/Core/TypedData/AllowedValuesInterface.phpundefined
@@ -0,0 +1,63 @@
+   * Returns a list of the allowed values in the current context.

This will at least need more documentation to make the difference clear because both have a user context now.

I know you discussed use cases for it, but for the sake of this interface, it might make sense to leave the user context out of the available ones.

+++ b/core/lib/Drupal/Core/TypedData/AllowedValuesInterface.phpundefined
@@ -0,0 +1,63 @@
+  public function getAllowedOptions(\Drupal\user\Plugin\Core\Entity\User $user = NULL);

Can't we derive this based on the allowed values and available options?

A bit weird due to the possible nesting, but...

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraintValidator.phpundefined
@@ -0,0 +1,29 @@
+   * Implements \Symfony\Component\Validator\ConstraintValidatorInterface::validate().

@inheritdoc

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterAPITest.phpundefined
@@ -106,4 +106,89 @@ function testFilterFormatAPI() {
+      // @todo Is there really no nicer way to do this?
+      $global_user = $GLOBALS['user'];
+      $GLOBALS['user'] = drupal_anonymous_user();

No, but you don't need to worry about reverting it, TestBase takes care of this.

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterAPITest.phpundefined
@@ -106,4 +106,89 @@ function testFilterFormatAPI() {
+      $this->assertEqual(
+        $allowed_options,
+        array(
+          'plain_text' => 'Plain text'
+        ),
+        'Expected allowed format options for anoymous found'

Long function calls are always tricky and we don't have strict coding standards but we usually try to write them on a single line. U usually use variables to shorten them.

And if you're doing assertEqual/assertText, I think the assertion message can be left out in most cases.

+++ b/core/modules/filter/lib/Drupal/filter/Type/FilterFormat.phpundefined
@@ -0,0 +1,56 @@
+    // @todo Evil call to procedural code, how can we get rid of it?
+    foreach (filter_formats() as $format) {

Inject the filter format storage controller or just the filter formats. But that's unfortunate if you don't actually need it.

+++ b/core/modules/text/lib/Drupal/text/Tests/Formatter/TextFormattedUnitTest.phpundefined
@@ -0,0 +1,110 @@
+class TextFormattedUnitTest extends TextPlainUnitTest {

Extending from an actual test is not something we usually do, expect you actually want to re-run those test functions in here, but then a common base clase would make more sense. Why not EntityUnitTestBase or whatever that's called?

+++ b/core/modules/text/lib/Drupal/text/Tests/Formatter/TextFormattedUnitTest.phpundefined
@@ -0,0 +1,110 @@
+    // @todo Add helper methods for all of the following.

Was confused about these todo's, but you copied it from TextPlainUnitTest.

+++ b/core/modules/text/lib/Drupal/text/Tests/Formatter/TextFormattedUnitTest.phpundefined
@@ -0,0 +1,110 @@
+    $this->view_mode = 'default';
+    $this->display = entity_get_display($this->entity_type, $this->bundle, $this->view_mode)
+      ->setComponent($this->field_name, array(
+        'type' => $this->formatter_type,
+        'settings' => $this->formatter_settings,
+      ));
+    $this->display->save();

You don't seem to be using display stuff.

+++ b/core/modules/text/lib/Drupal/text/Tests/Formatter/TextFormattedUnitTest.phpundefined
@@ -0,0 +1,110 @@
+    $this->setFieldItem($entity, $this->field_name, array(
+      'value' => $value,
+      'format' => 'x',

I think this class does too much abstraction, there's no need for it, the API is actually considerably more complicated than simply using it directly. I simplified it in my clean-up TestEntity issue.

@berdir Thanks for the review!

This will at least need more documentation to make the difference clear because both have a user context now.

I know you discussed use cases for it, but for the sake of this interface, it might make sense to leave the user context out of the available ones.

Yes, that's a valid point. I'm not sure how to document this, either. We probable should outline an use case and try to break it down into an understandable documentation.

Inject the filter format storage controller or just the filter formats. But that's unfortunate if you don't actually need it.

As discussed I've removed the @todo now - we have to refactor such code globally anyway.

Extending from an actual test is not something we usually do, expect you actually want to re-run those test functions in here, but then a common base clase would make more sense. Why not EntityUnitTestBase or whatever that's called?

Oh, that was not supposed to be in the patch - removed it.

The tests were mainly failing because the text module now depends on the filter module and thus unit tests have to declare that explicitly.

Another bunch of fixes.

Damn, I checked the wrong FieldAccessTest - note to myself "We've namespaces now!"

+++ b/core/lib/Drupal/Core/TypedData/AllowedValuesInterface.phpundefined
@@ -0,0 +1,73 @@
+ * For example, you cannot set the "authenticated" role on the user role field,
+ * but still it is one of the available values for the field. Thus allowed
+ * values would be used during any editing context, while available values
+ * would be used when e.g. filtering for existing values.

As discussed, let's use a different example.

+++ b/core/lib/Drupal/Core/TypedData/Validation/Metadata.phpundefined
@@ -91,4 +91,14 @@ public function getPropertyName() {
+   * @return TypedDataInterface
+   *   The typed data object.
+   */
+  public function getTypedData() {

Interface is not namespaces. How will that change when TypedDataInterface goes away?

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraint.phpundefined
@@ -0,0 +1,28 @@
+ *   label = @Translation("AllowedValues", context = "Validation")

This shouldn't be CamelCase.

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraint.phpundefined
@@ -0,0 +1,28 @@
+  public $minMessage = 'You must select at least %limit choice.|You must select at least %limit choices.';
+  public $maxMessage = 'You must select at most %limit choice.|You must select at most %limit choices.';

Not sure if the message makes sense, as it's not really about the number of choices but being valid or not?

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterAPITest.phpundefined
@@ -106,4 +106,66 @@ function testFilterFormatAPI() {
+      $violations = $data->validate();
+      $this->assertEqual(count($violations), 1, "Validation violation about for format 'foo' found");

Let's also add at least one case here where we check the returned constraint (property path, correct one, ...)

+++ b/core/modules/filter/lib/Drupal/filter/Type/FilterFormat.phpundefined
@@ -0,0 +1,55 @@
+  public function getAvailableOptions($account = NULL) {
+    $values = array();
+    foreach (filter_formats() as $format) {
...
+  public function getAllowedOptions($user = NULL) {
...
+    foreach (filter_formats($user) as $format) {

As mentioned, not sure about the $account argument to available options, according to the current documentation and example implementation, it seems useless. As we don't need it yet, I'd suggest to leave it out and re-add when we do have a use case/discuss in a separate issue.

Here's an updated patch. I hope got all necessary changes.

#20: d8-allowed-values-interface-1758622-20.patch queued for re-testing.

The entity validation issue is in, so let's integrate it with that and also add basic test coverage. Can we already use the text field that's attached to entity_test and it's format for this?

Two nitpicks:

interface AllowedValuesInterface {
  public function getAllowedValues($account = NULL);
  public function getAllowedOptions($account = NULL);
  public function getAvailableValues($account = NULL);
  public function getAvailableOptions($account = NULL);
}

The naming of the interface and is a bit irritating, since it defines both allowed and available values/options methods. Even more, allowed is the subset of available, so maybe find something more generic as interface name?

  public $minMessage = 'You must select at least %limit choice.|You must select at least %limit choices.';
  public $maxMessage = 'You must select at most %limit choice.|You must select at most %limit choices.';

We probably don't want to use format_plural() here, because it's calling t(), and building plurals without taking the locale into consideration doesn't make much sense, but does this piped string notation have any precedent in Core?

Looking at the interface a bit, it appears to me that this interface is much more about options than it is values. The only idea I can see here that's unique is that it has methods to return a constrained set of options, so that should be reflected in the name.

I'd also rework the methods a bit like so:

interface ConstrainableOptionsInterface {

  public function getOptions($account = NULL);

  public function getOptionValues($account = NULL);

  public function getAvailableOptions($account = NULL);

  public function getAvailableOptionValues($account = NULL);
}

It was very confusing to me at first differentiating between allowed and available. When I read getAvailableOptions, I think that means that I'm getting a set of currently available options, and that availability could be influenced by the current user or some type of state.

I also like the idea of treating the constrained version as a special case, which makes it easier to understand. getOptions simply returns all the options whereas getAvailableOptions tells me that it is a subset.

It's a little odd that they both have the Values methods as well. If the FilterFormats example is typical, I'd say we should remove the Values methods and always return an array of objects. If the caller needs the label, they can call the ->label method on the object.

It's a little odd that they both have the Values methods as well.

I think the reason to have getValues() is that getOptions() could return a structured (nested) array to be used in a <select> html field. And this isn't very usable if you just want a "pure" list of values to work with.

Here's a simple re-roll from which I'll continue.

Here we go:

• Added text format test / assertion to EntityValidationTest. No need for further integration it worked out of the box like a charm :)
• Fixed & enhanced test in FilterAPITest
• Renamed getAllowedValues() / getAllowedOptions() to getValues() / getOptions()
• Fixed some doc blocks in AllowedValuesInterface

As soon as we've a final agreement on the naming I'll update the patch.

Added dependency to the filter module in the failing test.

#37: d8-allowed-values-interface-1758622-37.patch queued for re-testing.

Re-roll.

That filter api test became a DUBT test overnight :)

Updated the test, the interdiff is a bit big because I also removed a conditional if, no need to do that in a test.

The methods on the interface look fine to me, but we still need some documentation improvements there.

#44: d8-allowed-values-interface-1758622-44.patch queued for re-testing.

Doc improvements look good.

Is the options hook just for backwards compatibility? is there nothing in the options callback documentation that we still need?

+++ b/core/lib/Drupal/Core/TypedData/AllowedValuesInterface.phpundefined
@@ -0,0 +1,75 @@
+   * @param object $account
...
+  public function getValues($account = NULL);
...
+   * @param object $account
...
+  public function getOptions($account = NULL);

Let's make this an AccountInterface now that we have it.

+++ b/core/lib/Drupal/Core/TypedData/AllowedValuesInterface.php
@@ -0,0 +1,77 @@
+   * @return array
+   *   An array allowed values.

should be "An array of allowed values."

The rest looks pretty good, as far as I can tell.

Some more nitpicks:

+++ b/core/lib/Drupal/Core/TypedData/AllowedValuesInterface.phpundefined
@@ -0,0 +1,77 @@
+/**
+ * Interface for retrieving allowed values.

add: "and possible values."

+ * While allowed values define the values that are allowed to be set by a user,
+ * possible values specify which values existing data might have.

The essence of this interface (reflected in the naming) are the allowed values. Therefore the explanation should be the other way around:
"While possible values specify which values existing data might have, allowed values define the values that are allowed to be set by a user."

Also, we're now switching forth and back between "values" to "options". At this point we shouldn't:

+ * For example, in an workflow scenario the allowed options for a state field
+ * might depend on the currently set state, while possible options are all

"values" not "options". Also comma (,) after "scenario".

+ * states. Thus allowed values would be used during any editing context, while

Better: "in an editing context"

+ * possible values would be used for presenting filtering options in a search.

Now that we explained the difference between "allowed" and "possible", we need to say that both are provided by this interface. Now we also want to explain the difference between "values" and "options", for example:
"For convenience, lists of both allowed and possible values are also provided as structured options arrays that can be used in an Options widget such as a select box or checkboxes.

@see \Drupal\options\Plugin\field\widget\OptionsWidgetBase"

+++ b/core/lib/Drupal/Core/TypedData/AllowedValuesInterface.php
@@ -0,0 +1,83 @@
+<?php
+/**

Missing space here.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigEntityReferenceItemBase.php
@@ -147,6 +147,22 @@ public function instanceSettingsForm(array $form, array &$form_state) {
+      $entity = $this->getParent()->getParent();

This requires you to think a bit about what it's doing (traversing typed data objects is my guess) so it might use a comment..

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.php
@@ -157,13 +166,14 @@ protected function getOptions() {
    * @param array $items
    *   The field values.
-   *
+   * @param int $delta
+   *   (optional) The delta of the item to get options for. Defaults to 0.
    * @return array
    *   The array of corresponding selected options.

Spacing looks wrong.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityValidationTest.php
@@ -133,6 +133,19 @@ protected function checkValidation($entity_type) {
     $this->assertEqual($violation->getInvalidValue(), $test_entity->name->value, 'Violation contains invalid value.');
+
+
+    $test_entity = clone $entity;

Extra empty line.

+++ b/core/modules/text/lib/Drupal/text/Plugin/field/widget/TextareaWidget.php
@@ -89,4 +90,15 @@ public function formElement(array $items, $delta, array $element, $langcode, arr
+  /**
+   * {@inheritdoc}
+   */
+  public function errorElement(array $element, ConstraintViolationInterface $violation, array $form, array &$form_state) {
+    if ($violation->arrayPropertyPath == array('format') && isset($element['format']['#access']) && !$element['format']['#access']) {
+      // Ignore validation errors for formats if formats may not be changed,
+      // i.e. when existing formats become invalid. See filter_process_format().
+      return FALSE;
+    }
+    return $element;

This is duplicated 3 times, I think it warrants a TextWidgetBase abstract, but not necessarily in this patch :)

Other than that, the patch and functionality looks awesome!

This looks great to me, apart from the few parts that I don't fully understand, related to that filter format validation stuff :)

I tested this with a use case that I have for this interface and it works great.

Setting to needs work to address @amateescu's feedback and it needs a re-roll anyway.

Fixed the white space issues.

Did not introduce TextWidgetBase, that can be a follow-up.

Code is in the field-options-1758622 branch in fago's entity sandbox.

Thanks :)

+ public function errorElement(array $element, ConstraintViolationInterface $violation, array $form, array &$form_state) {
This is duplicated 3 times, I think it warrants a TextWidgetBase abstract, but not necessarily in this patch :)

At least shouldn't be duplicated three times. TextareaWithSummaryWidget::errorElement() can inherit from the parent TextareaWidget::errorElement().

Bulls***. Forget patch #68.
This one here should work, but it might not be worth it.

So in any case #66 remains RTBC.

In the meantime I created #2032745: Create a TextWidgetBase as a followup.

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

tag did not stick. trying again.

Patch removes hook_options_list().
If we can just deprecate it instead, this could be tagged 'Backwards compatible API change' and would be still acceptable.

Oh, we don't completely remove the hook, it's still invoked by BC methods in ConfigEntityReferenceItemBase (and LegacyConfigFieldItem). Would that be enough for backwards compatibility, or do we need to continue invoking it from OptionsWidgetBase and keep documentation in options.api.php?

Actually, it's not completely clear to me what exactly belongs to the frozen API and what doesn't. Are remainders of the D7 API always frozen if not explicitely marked deprecated, even in unusual in-between states mixing old and new API features? And what happens to API functions that become obsolete while the remaining conversions are completed?
Some enlightment would be nice!

[edit:] Or wouldn't the API freeze only come into effect after beta1 has been rolled out? Without a beta1, module developers won't start porting their modules anyway. In that case another "deprecate what will soon be removed" week would be awesome.

@Pancho, we don't really care, this was rtbc on July 1st :)

If that's enough, then fine... :)

Updated issue summary.

issue summary template

Update the issue summary to make committer feedback easier. Please improve it as you see fit!

This interface is rather awkward. I don't understand why we return two sets of options/values. Furthermore, I'm confused about which of the two functions returns the superset; getValues() or getPossibleValues().

Ok, so let's try to make it more obvious. I agree that getValues() is pretty bad and does not tell me anything.

Proposal:
getAllPossibleValues(): Returns an array of all possible values which existing and future data might have.
getAllPossibleOptions(): Returns an array of all possible values with labels for display.
getAllowedValues(): Returns an array of allowed values that can be used in the current context (might depend on the user, the current field value etc.). Subset of getAllPossibleValues().
getAllowedOptions(): Returns an array of allowed values for display. Subset of getAllPossibleOptions().

Let me know if you like that, then I'll roll a patch. Or make a counter proposal :-)

Discussed this with fago in IRC, turns out I got it all wrong.

There is no method to retrieve a super set of all possible values that ever exist. Is that a problem for us? The possible values are also dependent on the user.

We discussed:
getAllowedEditValues() + getAllAllowedValues()
getViewableValues() + getEditableValues() + getAllValues()
getAllValues() with $op as we had it in entity.module d7 (options list callback)
getAllValues(), getSetableValues() with optional $account ?
CRUD as $op parameter?

Thinking more about it, new proposal:

getAllPossibleValues(): Returns an array of all possible values which existing and future data might have (superset of all values). If the optional $account parameter is passed, then the array is filtered to values viewable by the user.
getAllPossibleOptions(): same as getAllPossibleValues(), except that it returns labels and/or groups for the values.
getSetableValues(): Returns an array of values that are allowed to be set in the current context (might depend on the user, the current field value etc.).
getSetableOptions(): same as getSetableValues(), except that it returns labels and/or groups for the values.

The most important change is to use "Setable" instead of "Allowed", which is a bit more clear I think.

I don't think we need CRUD since only create and update is relevant here. The implementation of getSetableValues() can determine from its own field context which operation is meant (create if the field value is not set, update if there is already a field value).

Yes, "Settable" would be both more descriptive and more correct than "Allowed".
Finally, preexisting values also remain allowed for existing data.
Note though that it is "Settable", not "Setable".

Secondly, if the other list of options is supposed to be the superset of "Settable" and "Preexisting" values, then it might be both clearer and more flexible to have methods for all of "Settable" values, "Preexisting" values, and (possibly) their superset.
Finally, for search filters we don't need values that might be settable, yet are currently not used. We usually only want the preexisting ones.

Thirdly, what would then be the use case of a superset?
"Conceivable" might be worth a thought, but a concrete use case in mind would probably lead us to a better name for the superset.

Ok, implemented that accordingly.

Note that the filter API test case fails locally for me, but I have no idea why.

Not a blocker (and no better proposal for now), but the "ignore errors" part in texteare widget smells wrong. It shouldn't be up to the widget to decide that a reported error should be discarded ? The validation level should not report the error in the first place ?

Also, not fully convinced by the signature changes in OptionWidgetBase (added $delta defaulting to 0), seems half baked. Maybe pass a FieldItem directly, rather than a Field that gets by default silently resolved to the first item ?

Again shouldn't block commit IMO, sounds fixable post commit without api change.

Some test fails seem unrelated, fixed one method call.

the filter API test case will still fail.

Tracked down the filter API test fail and fixed one remaining old getOptions() name.

added comment updated number

Updated the issue summary to reflect the new "settable" terminology.

Anyone up for RTBC or other feedback?

Fixed remarks from #91. Not touching $delta vs. full $item in OptionWidgetBase.

Much better now!
However, there still seem to be some inconsistencies:

+  /**
+   * Returns an array of all possible values.
+   *
+   * If the optional $account parameter is passed, then the array is filtered to
+   * values viewable by the account.
+   *
+   * @param \Drupal\Core\Session\AccountInterface $account
+   *   (optional) The user account for which to filter the possible viewable
+   *   values. If omitted the default behavior is to return all possible values.
+   *
+   * @return array
+   *   An array of all possible values.
+   */
+  public function getAllPossibleValues(AccountInterface $account = NULL);

[...]

+  /**
+   * Returns an array of settable values.
+   *
+   * If the optional $account parameter is passed, then the array is filtered to
+   * values settable by the account.
+   *
+   * @param \Drupal\Core\Session\AccountInterface $account
+   *   (optional) The user account for which to filter the settable values. If
+   *   omitted the default behavior is to return all settable values.
+   *
+   * @return array
+   *   An array of settable values.
+   */
+  public function getSettableValues(AccountInterface $account = NULL);

1)
"values settable by the account" is clear, but what are "values viewable by the account" meant to be?
It's not used in any implementation, and in all case I can think of, it wouldn't be usable, so we might want to leave the parameter out. But maybe I simply didn't get the right idea.

2)
What exactly does "all" refer to?
In getAllPossibleValues() (and -Options) it is part of the function name, while in getSettableValues() (and -Options) it is not.
However, according to the docs "all" refers to not being filtered by $account. The inconsistency is confusing.
Resolution would depend on the decision on 1.

3)
+ * Returns an array of settable values with lables for display.
Typo: "lables"

Fixed the typo.

getAllPossibleValues() can be used for example in a Views exposed filter selectbox to filter entries in some table. Depending on the acting user you might want to display a different set of selectable values in the selectbox that the user can see for filtering. So those values are "viewable" by the user. I think we want to keep the user parameter to keep the interface flexible enough for future use and contrib.

The name getValues() is bad because it is too broad and does not tell us anything about its meaning. getAllValues() is also bad because you don't really get all values as soon as you pass a user account object as parameter. getPossibleValues() is harder to differentiate from getSettableValues(). So getAllPossibleValues() is the closest that we could come up with to reflect the meaning. Feel free to suggest a better name.

New suggestion that came up in IRC discussing with fago (IRC log attached):
getOptions() + getOptionValues() for the settable values, because we want to use this as default, so that people don't accidentally use the possible values in places where they actually mean settable values.
getPossibleOptions() + getPossibleOptionValues() for the viewable/possible values. We are avoiding "all" here, because it is a filter operation if you pass a user object.

Now we did enough bike shedding so that we are back to the patch in #69, except that getValues() is now getOptionValues(). We can improve the docs though with the discussion we had in between.

Before I roll any more patches I need consensus on the names. Please +1 this suggestion or propose concrete alternatives.

So I renamed getAllPossibleValues() to getPossibleValues(), and getAllPossibleOptions() to getPossibleOptions().

Finally, I improved the docs quite a bit, explaining what the options arrays can be used for.
'Multi-dimensional' should be 'two-dimensional' because cascading optgroups aren't allowed. Maybe 'two-level' would be even better, in any case it would be easier to understand, and finally, depending on how you see an array, a flat array is already two-dimensional.
Also removed abbreviations such as 'e.g.' and 'i.e.'.

IMHO, there's not much missing to be RTBC again.

Sorry for misnaming the interdiff. Maybe someone can stop it being tested?

Oops. Erroneously killed a brace.
This one should do what the one in #100 was advertised to do.

Note also that I crossed #99, so this patch only does what the consensus was before #99.
Further renames could go on top of this one here.

[edit:] Re #99:
As @klausi said, what you're proposing from your IRC chat is close to what we already had in #66, which @Dries didn't like too much as stated in #78. I'd be fine with that, though, and I'm not sure we can do any better.

getNewValues() / getNewValueOptions()
- vs. -
getPossibleValues() / getPossibleValueOptions()

would be yet another alternative, which might be both easier to differentiate and remember, while still giving a clue what kind of options these are. getNewValueOptions() also looks more like the default options set for entering new data.

However, I agree we should settle for either of these proposals now. For a final decision we should maybe assign to @Dries who had expressed his doubts in #78.

Thanks for the work since #78 on clarifying and documenting the difference between possible and settable. I especially like the Workflow example used in the docs for the interface.

However, do we have any use cases in core for possible? We don't appear to in the patch. The filter format example in the patch doesn't really serve as the use case, because in core, we *don't* let the user view which formats are possible. We only ever let them see the ones they can set.

If core has no use case for possible, why include it in this interface? Perhaps it should be a separate interface? Also, I'm a bit confused on why getPossibleValues() is an instance-level method rather than a static method. It seems like it shouldn't depend on the current data in the item, or should it? However, it would need to depend on the field definition, so it can't really be a static method without also receiving the definition as a parameter. This confusion is partially what's causing me to think that maybe it should be a separate interface worked out in contrib rather than being added to core without more clarity around that.

Thanks. #106 makes sense. Given that, I like getSettable*() and getPossible*(). Another option I can think of is to unify into a single getAllowed*(), but add a parameter for something like $ignore_current_data. The advantage of that would be that for many simple implementors, there's no difference between the two and they could ignore that parameter instead of needing to implement multiple methods. But then we'd need to bikeshed that parameter name. Assigning to Dries for feedback in general, now that we have more info on use cases identified in the patch and in #106, but feel free to keep discussing in the meanwhile if you want to.

I'm comfortable with the new interface. It's better than what we had before but I'm still not 100% about it, yet I don't have good suggestions to make it better. I also don't want to hold this up further as we have a lot of other things to work on. We can always change it later if we come up with a better approach. So, green light from me! :)

I don't think we should change this later, because it is an interface and that should rarely change.

The $ignore_current_data parameter proposed in #107 does not seem to be a good idea IMHO. On method invocation that is just an anonymous boolean flag which makes the code harder to read and less explicit:

// Obscure boolean flag.
$options = $field->getAllowedOptions($account, TRUE);
// Explicit method name.
$options = $field->getSettableOptions($account);

I'm also ok with the current getSettable*() and getPossible*(), so are effulgentsia, Pancho, fago and Dries. I think we can move on now, so setting to RTBC. Feel free to change the status back if you disagree.

Committer feedback obtained in #108...

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraintValidator.phpundefined
@@ -0,0 +1,31 @@
+      // @todo Refactor the global user out of here.
+      $account = \Drupal::request()->attributes->get('account');

Looks like this refactoring has taken place :) ... but now it's _account

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterAPITest.phpundefined
@@ -184,4 +188,87 @@ function testFilterFormatAPI() {
+    \Drupal::request()->attributes->set('account', $user);
...
+    \Drupal::request()->attributes->set('account', $filtered_html_user);

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.phpundefined
@@ -106,17 +113,20 @@ public static function validateElement(array $element, array &$form_state) {
+      $options = $items[$delta]->getSettableOptions(\Drupal::request()->attributes->get('account'));

Should be _account

Also needs a reroll...

git ac https://drupal.org/files/field-options-1758622-103.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 38692  100 38692    0     0  15558      0  0:00:02  0:00:02 --:--:-- 24914
error: patch failed: core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.php:157
error: core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.php: patch does not apply
error: patch failed: core/modules/text/lib/Drupal/text/Plugin/field/widget/TextareaWidget.php:10
error: core/modules/text/lib/Drupal/text/Plugin/field/widget/TextareaWidget.php: patch does not apply
error: patch failed: core/modules/text/lib/Drupal/text/Plugin/field/widget/TextfieldWidget.php:10
error: core/modules/text/lib/Drupal/text/Plugin/field/widget/TextfieldWidget.php: patch does not apply

Rerolled + fixed _account in the request object.

Fixing tags, hopefully.

This should fix at least the fatal errors, but OptionsFieldTest for example still fails. It looks like a change to the field settings is not reflected in the entity form that is retrieved later.

Fixed the "provider" vs. "module" settings change in the entity field API. Also fixed the OptionsFieldTest by reinitializing the outdated field object.

#116: field-options-1758622-116.patch queued for re-testing.

Rerolled, not other changes.

Tagging per Dries in #108.

Needs a reroll

git ac https://drupal.org/files/field-options-1758622-120.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 40565  100 40565    0     0  13247      0  0:00:03  0:00:03 --:--:-- 34819
error: patch failed: core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.php:11
error: core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.php: patch does not apply

Re-roll, one small change already got in in the getType() issue, left with a stupid use conflict because this unecessarly used the Sring class, see interdiff :)

Sorry, can we hold this for a sec ?
I now I'm terribly late to this issue, but I'm reviewing now and have a couple remarks

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/DataType/FilterFormat.phpundefined
@@ -0,0 +1,62 @@
+  public function getPossibleValues(AccountInterface $account = NULL) {
+    return array_keys($this->getPossibleOptions());
+  }

Won't this be the same implementation everywhere ?

Providing those in a base class is probably useless since most classes that might want to use it have more interesting classes to extend.
It makes me wonder if we really need those methods in the interface,though. We could just provide the "getPossibleValues()" with labels, and let the callers do array_keys if that's all they're interested in ?
No biggie though, feel free to ignore me if this has been discussed already.

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.phpundefined
@@ -13,6 +13,13 @@
+ * Field types willing to enable one or several of the widgets defined in
+ * options.module (select, radios/checkboxes, on/off checkbox) need to
+ * implement the AllowedValuesInterface to specify the list of options to
+ * display in the widgets.

Doesn't seem to be true, since FilterFormat is the only class that extends AllowedValuesInterface right now ?
OptionsWidgetBase simply assumes the existence of a getSettableOptions() method. For field types currently using option widgets, getSettableOptions() is implemented by ConfigEntityReferenceItemBase / LegacyConfigFieldItem, which both provide only this method but not the full AllowedValuesInterface - maybe they should ?

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.phpundefined
@@ -107,17 +114,20 @@ public static function validateElement(array $element, array &$form_state) {
       // Get the list of options from the field type module, and sanitize them.
-      $field_type_info = \Drupal::service('plugin.manager.entity.field.field_type')->getDefinition($this->fieldDefinition->getFieldType());
-      $module = $field_type_info['provider'];
-      $options = (array) $module_handler->invoke($module, 'options_list', array($this->fieldDefinition, $this->entity));
+      $items = $this->entity->getNGEntity()->get($this->fieldDefinition->getFieldName());
+      // Limit the settable options for the current user account.
+      $options = $items[$delta]->getSettableOptions(\Drupal::request()->attributes->get('_account'));

The callers of getOptions() now directly receive $items as a Field object, so no need for this dance.
They should pass that $items object to the getoptions() method.

Also, I don't think the $delta param is needed. A FieldItem object doesn't know its own delta, so the getSettableOptions() method will not return a different list of options whether it's called on the first item or the second - and this would be sick if they did IMO. Just call getSettableOptions() on $items[0].
Same for getSelectedOptions() - plus, getSelectedOption*s*($items, $delta) really makes no sense :-)

Actually, probably getOptions() should rather directly receive the FieldItem object, let the caller figure out which. Makes more sense for the method IMO.

Then $this->entity can be removed in WidgetBase, it's only used in getOptions() to pass to hook_options_list_alter(), but passing $items->getparent() or $item->getParent()->getParent() is good enough.

Shameless plug to #2061331: Add getEntity() helper in FieldInterface / FieldItemInterface, BTW...(getEntity() as shorhand for getParent()->getParent())

+++ b/core/modules/options/options.api.phpundefined
@@ -6,66 +6,6 @@
-function hook_options_list(\Drupal\Core\Entity\Field\FieldDefinitionInterface $field_definition, \Drupal\Core\Entity\EntityInterface $entity) {

The phpdoc for hook_options_list_alter() still refers to hook_options_list()
+ code still calls hook_options_list(), with a "@todo: Convert all legacy callback implementations to methods".
Do we have a followup for this ?

+ Minor stuff while we're here:

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/DataType/FilterFormat.phpundefined
@@ -0,0 +1,62 @@
+    foreach (filter_formats() as $format) {
+      $values[$format->id()] = $format->label();
+    }
+    return $values;

filter_formats() already returns formats keyed by id(), so this could be a one liner with array_map() & a closure.
Same for getSettableOptions()

- Double whiteline in EntityValidationTest
- TextareaWidget: missing empty line after last method

No, getPossibleValues() and getPossibleOptions() will not be the same implementation everywhere, because getPossibleOptions() can also provide option groups (think about a selectbox offering grouped options). In this example there are no groups, so the values are simple the keys of the options. See the @return doc block on AllowedValuesInterface.

OptionsWidgetBase is not a field type, so this does not necessarily have to implement to AllowedValuesInterface. ConfigEntityReferenceItemBase / LegacyConfigFieldItem can be handled in a follow-up I think.

Removed $delta, good idea about passing the individual item directly!

Not removing $this->entity, since that is still used in geOptions().

I don't think we have a follow-up yet for completely removing hook_options_list().

Thanks @klausi.

Ack for getPossibleOptions() / optgroups.

OptionsWidgetBase is not a field type, so this does not necessarily have to implement to AllowedValuesInterface. ConfigEntityReferenceItemBase / LegacyConfigFieldItem can be handled in a follow-up I think

Sure, I was not advocating for OptionsWidgetBase to implement AllowedValuesInterface.
Followup for ConfigEntityReferenceItemBase + hook_options_list() : why not, but we should open those then.

Not removing $this->entity, since that is still used in geOptions().

geOptions() doesn't need it anymore, the entity is in the $item:
$entity = $item->getParent()->getParent(); (can be reduced to $item->getEntity() after #2061331: Add getEntity() helper in FieldInterface / FieldItemInterface is in)
Much cleaner than fetching it from the outside and hoping it matches.

Minor:

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.phpundefined
@@ -114,20 +115,18 @@ public static function validateElement(array $element, array &$form_state) {
+   * @param FieldItemInterface $item

Lacks full namespace

[side note: I can make the above changes myself in the sandbox if that's fine with you - don't wan't to do it without permission though :-) ]

yched: permission granted, of course!

You can pull from the field-options-1758622 branch in https://drupal.org/sandbox/fago/1497344

Done
(didn't push yet, don't seem to have commit rights ATM) [edit: thks @klausi :-)]

Ok, so yched seems to be happy now, the testbot is happy and the rest of us have been happy before + the minor changes look good ==> back to RTBC.

And here is the follow-up issue: #2069739: AllowedValuesInterface for ConfigEntityReferenceItemBase / LegacyConfigFieldItem.

Marked #2015689: Convert field type to typed data plugin for options module as postponed on this.

Patch does not apply anymore, rerolled.

alexpott pointed out that we should use the current_user service. Also moved a variable around to be more coherent in the code.

Since #2064181: Filter format access bypass on POST/PATCH is postponed on this (which is a critical security issue) this is critical as well.

Committed 5277135 and pushed to 8.x. Thanks!

Fixed some stuff during the commit... "rightaway" is not a word and is unnecessary anyway.

diff --git a/core/lib/Drupal/Core/TypedData/AllowedValuesInterface.php b/core/lib/Drupal/Core/TypedData/AllowedValuesInterface.php
index e8955d8..bc58989 100644
--- a/core/lib/Drupal/Core/TypedData/AllowedValuesInterface.php
+++ b/core/lib/Drupal/Core/TypedData/AllowedValuesInterface.php
@@ -54,9 +54,9 @@ public function getPossibleValues(AccountInterface $account = NULL);
    *   If omitted, all possible options are returned.
    *
    * @return array
-   *   An array of possible options for the object that may rightaway be used in
-   *   an Options widget, for example when existing data should be filtered.
-   *   It may either be a flat array of option labels keyed by values, or a
+   *   An array of possible options for the object that may be used in an
+   *   Options widget, for example when existing data should be filtered. It may
+   *   either be a flat array of option labels keyed by values, or a
    *   two-dimensional array of option groups (array of flat option arrays,
    *   keyed by option group label). Note that labels should NOT be sanitized.
    */
@@ -88,11 +88,11 @@ public function getSettableValues(AccountInterface $account = NULL);
    *   omitted, all settable options are returned.
    *
    * @return array
-   *   An array of settable options for the object that may rightaway be used in
-   *   an Options widget, usually when new data should be entered.
-   *   It may either be a flat array of option labels keyed by values, or a
-   *   two-dimensional array of option groups (array of flat option arrays,
-   *   keyed by option group label). Note that labels should NOT be sanitized.
+   *   An array of settable options for the object that may be used in an
+   *   Options widget, usually when new data should be entered. It may either be
+   *   a flat array of option labels keyed by values, or a two-dimensional array
+   *   of option groups (array of flat option arrays, keyed by option group
+   *   label). Note that labels should NOT be sanitized.
    */
   public function getSettableOptions(AccountInterface $account = NULL);
 }
diff --git a/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetInterface.php b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetInterface.php
index de588cb..e27484e 100644
--- a/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetInterface.php
+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetInterface.php
@@ -134,7 +134,7 @@ public function formElement(FieldInterface $items, $delta, array $element, $lang
    * @param array $form_state
    *   An associative array containing the current state of the form.
    *
-   * @return array|false
+   * @return array|bool
    *   The element on which the error should be flagged, or FALSE to completely
    *   ignore the violation (use with care!).
    */

-   * @return array|false
+   * @return array|bool
    *   The element on which the error should be flagged, or FALSE to completely
    *   ignore the violation (use with care!).

I think actually the previous way was correct. This way, it seems as though the function could return TRUE as well.

false is not a valid typehint. bool is.

"false" is a valid type hint according to our coding standards and should be used if TRUE is never returned: https://drupal.org/coding-standards/docs#types

As discussed in IRC already, according to our standard, false is valid and would have been correct there: https://drupal.org/coding-standards/docs#types

But it doesn't matter that much :)

Follow-up: #2075827: Fix $this->container->set('current_user', $user); in FilterAPITest

Change notice draft created: https://drupal.org/node/2075873

Looks ok to me, removed an unecessary isset() arround $account. Looking at the example, wouldn't it be supposed to fall back to the global user is non given and not deny access? Wondering if we shouldn't make it required, that's much more in line with how $account->hasPermission() works, as opposed to user_access(), which has a built-in default.

Would be a API change but none implements this yet, so...

I think this patch missed something:)

Indeed...

@webchick, it's more of a convention to add this parameter $account to avoid the possible parameter confusing imho, because all the four methods depend on the paramter $account, at least literally. So, it's quite wierd that one call to one of these four functions suddenly doesn't need the paramter anymore.

And yes, getPossibleOptions($account) in this context doesn't actually use the parameter $account in its function body. So, it won't have any real bad effect to simply call $this->getPossibleOptions() directly.

Here's another fix for this patch at #2015689-44: Convert field type to typed data plugin for options module .

Agreed with #148. This doesn't need tests, because the change is to a specific implementation that doesn't depend on $account, but passing the parameter through is cleaner coding style.

Oh, cool. Thanks for the clarification.

Committed and pushed to 8.x. Thanks!

Restoring title for posterity

Restoring priority and tags for posterity.