Updated: Comment #90

Problem/Motivation

Entity field classes should be able to implement methods that declare a list of possible and settable values for the field. This is field specific information and should live directly on the Field class. We want to get rid of the procedural style hook_options_list().

Proposed resolution

Add an AllowedValuesInterface with methods to get all possible options for a field and all settable options for a field. Implementations could be dependent on the current user, so we need to pass that to the methods. Example: the settable options of a workflow state field vary per user, admins can use more options than editors. And we need one method to retrieve the plain values, and one to get a formated options array of values with labels. ==> 4 methods.

Remaining tasks

Provide feedback on the latest provided patch.

User interface changes

none.

API changes

hook_options_list() is removed/deprecated, methods on Field classes are added with AllowedValuesInterface

#1696648: [META] Untie content entity validation from form validation

Original report by fago

As fields have hook_options_list and d7 entity api module had 'options list' callbacks, we want add something similar to property definitions.

We could just follow hook_options_list() what supports opt-groups and generate the flattened allowed values out of it. A requirement that came up in the d7 entity.module was being able to differentiate between 'view' and 'edit' options, e.g. possible already set values and possible values to set. Then, we want to integrate this with validation - so maybe this is better postponed until we know how validation works exactly.

Files: 
CommentFileSizeAuthor
#145 options_list-1758622-145.patch657 bytessmiletrl
PASSED: [[SimpleTest]]: [MySQL] 58,476 pass(es).
[ View ]
#135 field-options-1758622-135.patch41.82 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 58,188 pass(es).
[ View ]
#135 field-options-1758622-135-interdiff.txt3.36 KBklausi
#134 field-options-1758622-134.patch41.77 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 58,506 pass(es).
[ View ]
#134 field-options-1758622-134-interdiff.txt1.92 KBklausi
#131 field-options-1758622-131.patch41.82 KByched
PASSED: [[SimpleTest]]: [MySQL] 57,895 pass(es).
[ View ]
#131 interdiff.txt1.63 KByched
#127 field-options-1758622-127.patch40.95 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 57,894 pass(es).
[ View ]
#127 field-options-1758622-127-interdiff.txt7.76 KBklausi
#124 field-options-1758622-124.patch39 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,153 pass(es).
[ View ]
#124 field-options-1758622-124-pseudointerdiff.txt1.12 KBBerdir
#120 field-options-1758622-120.patch39.61 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 58,193 pass(es).
[ View ]
#116 field-options-1758622-116.patch39.56 KBklausi
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-options-1758622-116.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#116 field-options-1758622-116-interdiff.txt2 KBklausi
#114 field-options-1758622-114.patch38.42 KBklausi
FAILED: [[SimpleTest]]: [MySQL] 56,280 pass(es), 74 fail(s), and 74 exception(s).
[ View ]
#114 field-options-1758622-114-interdiff.txt1.44 KBklausi
#111 field-options-1758622-111.patch37.77 KBklausi
FAILED: [[SimpleTest]]: [MySQL] 56,531 pass(es), 218 fail(s), and 224 exception(s).
[ View ]
#111 field-options-1758622-111-interdiff.txt6.22 KBklausi
#103 field-options-1758622-103.patch37.79 KBPancho
PASSED: [[SimpleTest]]: [MySQL] 56,859 pass(es).
[ View ]
#103 interdiff-97-103.txt6.15 KBPancho
#100 field-options-1758622-99.patch37.78 KBPancho
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Core/TypedData/AllowedValuesInterface.php.
[ View ]
#100 interdiff-97-99.patch6.15 KBPancho
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-97-99.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#99 irc.txt3.46 KBklausi
#97 field-options-1758622-97.patch37.79 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 57,224 pass(es).
[ View ]
#97 field-options-1758622-97-interdiff.txt721 bytesklausi
#93 field-options-1758622-93.patch37.79 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 57,198 pass(es).
[ View ]
#93 field-options-1758622-93-interdiff.txt8.14 KBklausi
#89 field-options-1758622-89.patch37.11 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 57,193 pass(es).
[ View ]
#89 field-options-1758622-89-interdiff.txt1.75 KBklausi
#87 field-options-1758622-87.patch37.11 KBklausi
FAILED: [[SimpleTest]]: [MySQL] 56,556 pass(es), 153 fail(s), and 83 exception(s).
[ View ]
#87 field-options-1758622-87-interdiff.txt838 bytesklausi
#84 field-options-1758622-84.patch37.1 KBklausi
FAILED: [[SimpleTest]]: [MySQL] 56,421 pass(es), 164 fail(s), and 94 exception(s).
[ View ]
#84 field-options-1758622-84-interdiff.txt10.34 KBklausi
#69 field-options-1758622-69.patch36.58 KBPancho
PASSED: [[SimpleTest]]: [MySQL] 56,492 pass(es).
[ View ]
#69 interdiff-66-69.txt1.17 KBPancho
#68 field-options-1758622-68.patch36.49 KBPancho
FAILED: [[SimpleTest]]: [MySQL] 56,813 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
#68 interdiff.txt1.08 KBPancho
#66 field-options-1758622-66.patch36.73 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 56,624 pass(es).
[ View ]
#66 field-options-1758622-66-interdiff.txt2.61 KBklausi
#63 d8_options.interdiff.txt1.83 KBfago
#63 d8_options.patch36.22 KBfago
PASSED: [[SimpleTest]]: [MySQL] 56,342 pass(es).
[ View ]
#60 d8_options.patch35.96 KBfago
PASSED: [[SimpleTest]]: [MySQL] 56,347 pass(es).
[ View ]
#59 d8_options.interdiff.txt4.01 KBfago
#59 d8_options.patch54.55 KBfago
PASSED: [[SimpleTest]]: [MySQL] 56,596 pass(es).
[ View ]
#57 d8_options.patch35.62 KBfago
PASSED: [[SimpleTest]]: [MySQL] 56,334 pass(es).
[ View ]
#57 d8_options.interdiff.txt1.11 KBfago
#55 d8_options.interdiff.txt436 bytesfago
#55 d8_options.patch35.63 KBfago
FAILED: [[SimpleTest]]: [MySQL] 56,318 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#54 d8_options.interdiff-move.txt3.34 KBfago
#54 d8_options.interdiff.txt9.41 KBfago
#54 d8_options.patch36.06 KBfago
FAILED: [[SimpleTest]]: [MySQL] 56,452 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#47 d8_options.patch30.53 KBfago
FAILED: [[SimpleTest]]: [MySQL] 56,618 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#47 d8_options.interdiff.txt13.63 KBfago
#44 d8-allowed-values-interface-1758622-44.patch22.31 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,136 pass(es).
[ View ]
#44 d8-allowed-values-interface-1758622-44-interdiff.txt6.8 KBBerdir
#42 d8-allowed-values-interface-1758622-42.patch21.76 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,045 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#37 d8-allowed-values-interface-1758622-37.patch21.68 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8-allowed-values-interface-1758622-37.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#37 interdiff-1758622-35-37-do-not-test.diff714 bytesdas-peter
#35 d8-allowed-values-interface-1758622-35.patch20.98 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] 57,850 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#35 interdiff-1758622-31-35-do-not-test.diff8.17 KBdas-peter
#31 d8-allowed-values-interface-1758622-31-reroll.patch19.13 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] 57,666 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#20 d8-allowed-values-interface-1758622-20.patch19.14 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8-allowed-values-interface-1758622-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#20 interdiff-1758622-17-20-do-not-test.diff4.53 KBdas-peter
#17 d8-allowed-values-interface-1758622-17.patch18.2 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 55,891 pass(es).
[ View ]
#17 interdiff-1758622-15-17-do-not-test.diff768 bytesdas-peter
#15 d8-allowed-values-interface-1758622-15.patch17.45 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] 55,794 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#15 interdiff-1758622-13-15-do-not-test.diff3.37 KBdas-peter
#13 d8_options.patch15 KBfago
FAILED: [[SimpleTest]]: [MySQL] 55,532 pass(es), 10 fail(s), and 3 exception(s).
[ View ]
#11 d8_options.patch15.01 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_options.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 d8-allowed-values-interface-1758622-9.patch14.54 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] 55,788 pass(es), 8 fail(s), and 3 exception(s).
[ View ]
#9 interdiff-1758622-6-9-do-not-test.diff15.05 KBdas-peter
#6 d8-allowed-values-interface-1758622-6.patch16.31 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] 55,454 pass(es), 9 fail(s), and 14 exception(s).
[ View ]

Comments

fago’s picture

Title:Provide the allowed values of a property» Provide the options list of a property
fago’s picture

Title:Provide the options list of a property» Provide the options list of an entity field
Project:D8 Entity API improvements» Drupal core
Version:» 8.x-dev
Component:Entity Property API» entity system
Issue tags:+Entity Field API

Moving to core queue.

fago’s picture

Some thoughts on this:
- We should make this general, i.e. apply it to the TypedData API. Such that we can get the allowed-values for a single property (text formats?) as well as for a whole field. Then, we can leverage this for validation also.
- I'd suggest adding a method for that. Not sure whether it should be built-in and be NULL if there are no allowed-values or have its own interface?

das-peter’s picture

Assigned:Unassigned» das-peter
fago’s picture

Issue tags:+Entity validation

We'll need this to have something to validate against.

das-peter’s picture

Status:Active» Needs review
StatusFileSize
new16.31 KB
FAILED: [[SimpleTest]]: [MySQL] 55,454 pass(es), 9 fail(s), and 14 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, d8-allowed-values-interface-1758622-6.patch, failed testing.

Berdir’s picture

+++ 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.

das-peter’s picture

Status:Needs work» Needs review
StatusFileSize
new15.05 KB
new14.54 KB
FAILED: [[SimpleTest]]: [MySQL] 55,788 pass(es), 8 fail(s), and 3 exception(s).
[ View ]

@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.

Status:Needs review» Needs work

The last submitted patch, d8-allowed-values-interface-1758622-9.patch, failed testing.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new15.01 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_options.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Updated patch. Sry, I forgot doing an interdiff.

Status:Needs review» Needs work

The last submitted patch, d8_options.patch, failed testing.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new15 KB
FAILED: [[SimpleTest]]: [MySQL] 55,532 pass(es), 10 fail(s), and 3 exception(s).
[ View ]

re-rolled

Status:Needs review» Needs work

The last submitted patch, d8_options.patch, failed testing.

das-peter’s picture

Status:Needs work» Needs review
StatusFileSize
new3.37 KB
new17.45 KB
FAILED: [[SimpleTest]]: [MySQL] 55,794 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Another bunch of fixes.

Status:Needs review» Needs work

The last submitted patch, d8-allowed-values-interface-1758622-15.patch, failed testing.

das-peter’s picture

Status:Needs work» Needs review
StatusFileSize
new768 bytes
new18.2 KB
PASSED: [[SimpleTest]]: [MySQL] 55,891 pass(es).
[ View ]

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

Berdir’s picture

Status:Needs review» Needs work
+++ 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.

fago’s picture

+ * 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.

Yes, as discussed I think a workflow state field is a better example. Allowed options probably depend on the existing state, while available options are all states.

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

Yep, I think re-working validation will be the most trickiest part. I think we still need to differentiate between Typed objects and the plain value then, difference being that in stead of having a TypedDataInterface we just say it must be a classed object. So the type-hint goes to object then I guess.

@min-max:
The constraint has options for the number of items you may select which we inherit from the parent. So that's already there, we just have to fix the messages to use drupal style replacement characters.

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

I agree checking the property path should be fine. There is no way to get the constraint which caused the violation in symfony validator though.

das-peter’s picture

Status:Needs work» Needs review
StatusFileSize
new4.53 KB
new19.14 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8-allowed-values-interface-1758622-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

fago’s picture

oh, I forgot to answer this one:

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.

It's not useless, I can give you a use-case you'll need it even in core: Give me the options list for an entity reference. Obviously, this options list would have to be access filtered, for which you'll need the user. I guess it works already that way by just using the global user.

@patch: Looks good to me!

fago’s picture

fago’s picture

Issue tags:+sprint
Berdir’s picture

Berdir’s picture

Status:Needs review» Needs work

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?

Pancho’s picture

Two nitpicks:

<?php
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?

<?php
 
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?

fago’s picture

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?

Nope, but that's something which symfony validator already does and is supported in Drupal as well - as we implement the translator interface there. That is, this runs correctly through format_plural() - which takes locale into account?

fago’s picture

Even more, allowed is the subset of available, so maybe find something more generic as interface name?

Can you recommend better names then? Afaik, allowed values is the term commonly used for that, so I think it's a reasonable choice.

msonnabaum’s picture

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:

<?php
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.

das-peter’s picture

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.

das-peter’s picture

Status:Needs work» Needs review
StatusFileSize
new19.13 KB
FAILED: [[SimpleTest]]: [MySQL] 57,666 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

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

fago’s picture

Status:Needs review» Needs work

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.

Sort of. I think getAllowedValues can always be influenced by the current user or some internal state. But getAvailableOptions is about a list of options that might be in existing objects. E.g. consider a view which displays the options in an exposed filter select list. It would have to show all options that exist in all of the objects, whereas the allowed options tell you what options you can actually set.

Actually, the "all options that exist in all of the objects" would fit more into a definition object - but we do not have that separation by now...

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

True, however we currently do no support this as of the docs of getOptions(). I've been thinking this as well, but turns out field API does not support this by now. Still, I thought that having a separate getAllowedValues() makes it more explicit that this is used for constraint purposes also.

So maybe we should add support for nested $option structures right now, so the differentiation makes more sense from the beginning?

fago’s picture

Status:Needs work» Needs review

cross-post

fago’s picture

@ConstrainableOptionsInterface

So what I dislike a bit about going with "options" primary is that
- it might get confused with "options" in terms of "settings", "preferences" in general. E.g. symfony uses it that way: http://symfony.com/doc/current/components/options_resolver.html (and we do for validation constraint options).
- options does not tell me it's validated. ConstrainableOptions makes that up, but "AllowedValues" is more straight-forward to me here and partly already used by field API.

@label(): We also have situations where there is no object we could get the label from, e.g. field API types like list_integer allow you to configure a list like
key1|label1
key2|label2

das-peter’s picture

StatusFileSize
new8.17 KB
new20.98 KB
FAILED: [[SimpleTest]]: [MySQL] 57,850 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, d8-allowed-values-interface-1758622-35.patch, failed testing.

das-peter’s picture

Status:Needs work» Needs review
StatusFileSize
new714 bytes
new21.68 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8-allowed-values-interface-1758622-37.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Added dependency to the filter module in the failing test.

Status:Needs review» Needs work

The last submitted patch, d8-allowed-values-interface-1758622-37.patch, failed testing.

das-peter’s picture

Status:Needs work» Needs review
Berdir’s picture

Status:Needs review» Needs work
Issue tags:+sprint, +Entity Field API, +Entity validation

The last submitted patch, d8-allowed-values-interface-1758622-37.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new21.76 KB
FAILED: [[SimpleTest]]: [MySQL] 58,045 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Re-roll.

Status:Needs review» Needs work

The last submitted patch, d8-allowed-values-interface-1758622-42.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new6.8 KB
new22.31 KB
PASSED: [[SimpleTest]]: [MySQL] 58,136 pass(es).
[ View ]

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.

Status:Needs review» Needs work
Issue tags:-sprint, -Entity Field API, -Entity validation

The last submitted patch, d8-allowed-values-interface-1758622-44.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
Issue tags:+sprint, +Entity Field API, +Entity validation
fago’s picture

StatusFileSize
new13.63 KB
new30.53 KB
FAILED: [[SimpleTest]]: [MySQL] 56,618 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

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

Indeed, AvailableValues isn't very descriptive. I think "PossibleValues" describes it better, so I updates docs+patch to go with that. Please check the interface whether that works for you.

Also, I updated the patch to work with 8.x HEAD again. Then, field-types as data types went in so I converted options.module to rely on the interface instead of the fake-hook-callback + made it work via the legacy-field support.

Status:Needs review» Needs work
Issue tags:-sprint, -Entity Field API, -Entity validation

The last submitted patch, d8_options.patch, failed testing.

fago’s picture

Status:Needs work» Needs review

#47: d8_options.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+sprint, +Entity Field API, +Entity validation

The last submitted patch, d8_options.patch, failed testing.

Berdir’s picture

Doc improvements look good.

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

fago’s picture

+++ b/core/modules/options/options.api.php
@@ -6,66 +6,6 @@
- *   widget. The HTML tags defined in _field_filter_xss_allowed_tags() are
- *   allowed, other tags will be filtered.

This is the only part of the docs I was not sure about. Referencing to this function at the interface does not make much sense and filtering probably depends on the caller?

The rest of the docs are reflected, i.e. I moved parts to the AllowedValuesInterface.

fago’s picture

Assigned:das-peter» fago

Re-rolling this one.

fago’s picture

Assigned:fago» Unassigned
Status:Needs work» Needs review
StatusFileSize
new36.06 KB
FAILED: [[SimpleTest]]: [MySQL] 56,452 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
new9.41 KB
new3.34 KB

Updated patch attached.

It contains:
- move to annotations
- docu improvements in regard of invoking the legacy-hook as requested by berdir
- fixes for the FilterFormatAccess test - this has been a bit tricky as they way this works if a filter format value becomes invalid (format gets deleted) is that users cannot edit it any more, but values are kept. However, with this patch this issues validation errors - so I had to enhance field API's errorElement() a bit to allow ignoring this error. This also shows we should do #2027059: Pass validation violation property paths on to widgets.

fago’s picture

StatusFileSize
new35.63 KB
FAILED: [[SimpleTest]]: [MySQL] 56,318 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
new436 bytes

Noticed I forgot to remove left-over use statement from previously.

Status:Needs review» Needs work

The last submitted patch, d8_options.patch, failed testing.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new1.11 KB
new35.62 KB
PASSED: [[SimpleTest]]: [MySQL] 56,334 pass(es).
[ View ]

This reveals a left-over from #2002102: Move TypedData primitive types to interfaces - attached patch fixes the @todo.

Berdir’s picture

+++ 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.

fago’s picture

StatusFileSize
new54.55 KB
PASSED: [[SimpleTest]]: [MySQL] 56,596 pass(es).
[ View ]
new4.01 KB

oh, yes.

fago’s picture

StatusFileSize
new35.96 KB
PASSED: [[SimpleTest]]: [MySQL] 56,347 pass(es).
[ View ]

Forgot to merge in latest 8.x changes before diffing.

klausi’s picture

+++ 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.

Pancho’s picture

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"

fago’s picture

StatusFileSize
new36.22 KB
PASSED: [[SimpleTest]]: [MySQL] 56,342 pass(es).
[ View ]
new1.83 KB

thanks, I updated the patch based on the feedback.

amateescu’s picture

+++ 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!

Berdir’s picture

Status:Needs review» Needs work

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.

klausi’s picture

Status:Needs work» Needs review
StatusFileSize
new2.61 KB
new36.73 KB
PASSED: [[SimpleTest]]: [MySQL] 56,624 pass(es).
[ View ]

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.

amateescu’s picture

Status:Needs review» Reviewed & tested by the community

Thanks :)

Pancho’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new1.08 KB
new36.49 KB
FAILED: [[SimpleTest]]: [MySQL] 56,813 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

+  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().

Pancho’s picture

StatusFileSize
new1.17 KB
new36.58 KB
PASSED: [[SimpleTest]]: [MySQL] 56,492 pass(es).
[ View ]

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

Pancho’s picture

Status:Needs review» Reviewed & tested by the community

So in any case #66 remains RTBC.

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

YesCT’s picture

Issue tags:+RTBC July 1

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

YesCT’s picture

tag did not stick. trying again.

Pancho’s picture

Issue tags:+API change

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.

Pancho’s picture

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.

amateescu’s picture

Issue tags:-API change

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

Pancho’s picture

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

Pancho’s picture

Issue summary:View changes

Updated issue summary.

klausi’s picture

Issue summary:View changes

issue summary template

klausi’s picture

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

Dries’s picture

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().

klausi’s picture

Status:Reviewed & tested by the community» Needs review

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 :-)

klausi’s picture

Status:Needs review» Needs work

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?

klausi’s picture

Status:Needs work» Needs review

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).

Pancho’s picture

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.

fago’s picture

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

I don't think we need to separate the superset from "pre-existing", as for search filters etc. you don't want only real pre-existing values but all possible values, thus you already want the super-set (possibly filtered for the current user) there. Searching only for real-pre-existing values is faceted search, which needs to be directly database/search server driven to be efficient anyway.

This I think we should go with

  • getSettableValues()
  • getAllPossibleValues()

- both having an optional user $account argument, which can be used to limit the values/options only to the one the user has access to (if passed only).

klausi’s picture

StatusFileSize
new10.34 KB
new37.1 KB
FAILED: [[SimpleTest]]: [MySQL] 56,421 pass(es), 164 fail(s), and 94 exception(s).
[ View ]

Ok, implemented that accordingly.

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

Status:Needs review» Needs work

The last submitted patch, field-options-1758622-84.patch, failed testing.

yched’s picture

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.

klausi’s picture

Status:Needs work» Needs review
StatusFileSize
new838 bytes
new37.11 KB
FAILED: [[SimpleTest]]: [MySQL] 56,556 pass(es), 153 fail(s), and 83 exception(s).
[ View ]

Some test fails seem unrelated, fixed one method call.

the filter API test case will still fail.

Status:Needs review» Needs work

The last submitted patch, field-options-1758622-87.patch, failed testing.

klausi’s picture

Status:Needs work» Needs review
StatusFileSize
new1.75 KB
new37.11 KB
PASSED: [[SimpleTest]]: [MySQL] 57,193 pass(es).
[ View ]

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

klausi’s picture

Issue summary:View changes

added comment updated number

klausi’s picture

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

Anyone up for RTBC or other feedback?

fago’s picture

+++ b/core/lib/Drupal/Core/TypedData/AllowedValuesInterface.php
@@ -0,0 +1,92 @@
+   * @param \Drupal\Core\Session\AccountInterface $account
+   *   (optional) The user account for which to filter the possible values.

I think this should mention what's the default behaviour also. It's a bit repetitive as it is already noted generally above, but as often the $account = NULL default is used differently I think this is worth making clear.

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
@@ -382,6 +382,14 @@ public function getConstraints($definition) {
+    // If the definition provides a class check for further validation criteria.

The comment is wrong - there is always a class we use to check validation criteria.

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraintValidator.php
@@ -0,0 +1,29 @@
+      $allowed_values = $this->context->getMetadata()->getTypedData()->getSettableValues();

Should this check allowed values for the currently logged in user or do not filter based on the user account? I think not including magic access checks to the global users is a sane default, but how could I pass a user here?

I feel like the whole validation process could need an optional user we validate the data for - so that goes beyond this issue. Should we open a follow-up for that?

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/DataType/FilterFormat.php
@@ -0,0 +1,63 @@
+  public function getSettableOptions(AccountInterface $user = NULL) {
+    $user = empty($user) ? $GLOBALS['user'] : $user;

This defaults to the current user but that's not what the interface specified.

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 ?

Well, I think the validation level is right in that the data is not valid any more. What's strange here is that we allow an entity having out-dated, invalid data to pass validation and continue saving it - but that's existing behaviour.
So the existing behaviour is that this invalid parts cannot be edited, but pass validation. It's the widget who decides that this invalid value cannot be edited but is kept, so I think it's also ok to have the widget decide to ignore this validation violation. Also in the code there, field API already by-passes validation for widgets with #access FALSE (generally), so I think this is just continuing with that behaviour on a more fine-granular level as it allows to ignore validation fails from parts of the widget having #access FALSE. And it must be done by the widget itself, because when an item property fails validation we cannot know whether this property is edited as part of a combined widget or not.

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 ?

Well, if you have a multi-value select widget you can go with only one item and you'll have to assume options for each field item are the same. For that case, I think having the field item be optional and just using any should be fine, as it should not matter anyway.
But, if you use a widget per field item we can go for an individual item's field options - it might matter here. But yes, I agree we should better pass the whole $item instead of just the delta in this case.

fago’s picture

Status:Needs review» Needs work
klausi’s picture

Status:Needs work» Needs review
StatusFileSize
new8.14 KB
new37.79 KB
PASSED: [[SimpleTest]]: [MySQL] 57,198 pass(es).
[ View ]

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

fago’s picture

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

fago’s picture

Status:Needs review» Reviewed & tested by the community

Thanks, the improvements are good + I think the updated terminology should be clearer, so setting to RTBC again.

Pancho’s picture

Status:Reviewed & tested by the community» Needs work

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

<?php
/**
+   * 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"

klausi’s picture

Status:Needs work» Needs review
StatusFileSize
new721 bytes
new37.79 KB
PASSED: [[SimpleTest]]: [MySQL] 57,224 pass(es).
[ View ]

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.

fago’s picture

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.

Agreed - there are definitely cases where users are not even allowed to see an option, think of an ER & entity view access.

getPossibleValues() is harder to differentiate from getSettableValues(). So getAllPossibleValues() is the closest that we could come up with to reflect the meaning.

I'm not sure getAllPossibleValues() is really that much better/easier to differentiate from getSettableValues() and Pancho has a valid point here with "all" not being "all" once you filter based on user access. getPossibleValues() + getSettableValues() would work for me.

klausi’s picture

Status:Needs review» Needs work
StatusFileSize
new3.46 KB

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.

Pancho’s picture

Status:Needs work» Needs review
StatusFileSize
new6.15 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-97-99.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new37.78 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Core/TypedData/AllowedValuesInterface.php.
[ View ]

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.

Pancho’s picture

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

Status:Needs review» Needs work

The last submitted patch, interdiff-97-99.patch, failed testing.

Pancho’s picture

Status:Needs work» Needs review
StatusFileSize
new6.15 KB
new37.79 KB
PASSED: [[SimpleTest]]: [MySQL] 56,859 pass(es).
[ View ]

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.

Pancho’s picture

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.

effulgentsia’s picture

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.

fago’s picture

Assigned:Dries» Unassigned
Issue tags:-Needs committer feedback

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.

Problem is, when core does not define the interface for possible values, there won't be any "officially" shared interfaces modules could rely on. I think what would happening is that people implement the Settable-variant of core and are done for it, meaning the cases where such a differentiation is needed won't be handled properly - e.g. pluggable widgets available for views-exposed-filters or a condition plugin which needs to configure the value possibly set won't be possible without such an established part of the interface.

Then, I think we also have one example in core: user roles. Initially we used as the documentation example, but we replaced it with workflow states as it makes a better, simpler example. So when you edit a user, the settable roles won't include the anonymous user role, but all possible options should include it (not so fancy for the exposed views form until we have more anonymous accounts, but still needed for the condition case).

Another example we've in core is the views-based ER-selection widget, which could probably need a flag to differentiate between possible and settable values. Without the differentiation in core we won't ever be able to handle that.

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?

Yes, I've been thinking about that as well. getPossibleValues() would probably fit better on a non-existing definition class, but we don't want people to have to provide multiple classes. Making getPossibleValues() static is indeed an interesting idea. When we started with this a while ago, the plan to handle additional metadata context was to still instantiate empty objects and have fields checking that context of empty objects for getting additional metadat (e.g. a field inside an empty entity of type X) - for what we still needed instances, however meanwhile our approach shifted to using class based definition objects with interfaces, so you can have entity-type-aware definitions (field instances) open the door to make this static. Still, we don't have the class based definitions in typed data - but I think we should still tackle that for a better integration of fields+typed data. (yes, this needs more discussion). update: see #2047229: Make use of classes for entity field and data definitions

I still like the idea of just having getOptionValues() instead of getSettableValues(), but I'm not keen on bikesheding this further.

effulgentsia’s picture

Assigned:Unassigned» Dries
Issue tags:+Needs committer feedback

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.

Dries’s picture

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! :)

klausi’s picture

Status:Needs review» Reviewed & tested by the community

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:

<?php
// 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.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:-Needs committer feedback+Needs reroll

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
klausi’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll+Needs committer feedback
StatusFileSize
new6.22 KB
new37.77 KB
FAILED: [[SimpleTest]]: [MySQL] 56,531 pass(es), 218 fail(s), and 224 exception(s).
[ View ]

Rerolled + fixed _account in the request object.

klausi’s picture

Fixing tags, hopefully.

Status:Needs review» Needs work

The last submitted patch, field-options-1758622-111.patch, failed testing.

klausi’s picture

Status:Needs work» Needs review
StatusFileSize
new1.44 KB
new38.42 KB
FAILED: [[SimpleTest]]: [MySQL] 56,280 pass(es), 74 fail(s), and 74 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, field-options-1758622-114.patch, failed testing.

klausi’s picture

Status:Needs work» Needs review
StatusFileSize
new2 KB
new39.56 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-options-1758622-116.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Needs review» Needs work
Issue tags:-sprint, -Entity Field API, -Entity validation, -RTBC July 1

The last submitted patch, field-options-1758622-116.patch, failed testing.

klausi’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work
Issue tags:+sprint, +Entity Field API, +Entity validation, +RTBC July 1

The last submitted patch, field-options-1758622-116.patch, failed testing.

klausi’s picture

Status:Needs work» Needs review
StatusFileSize
new39.61 KB
PASSED: [[SimpleTest]]: [MySQL] 58,193 pass(es).
[ View ]

Rerolled, not other changes.

fago’s picture

Status:Needs review» Reviewed & tested by the community

Thanks klausi for keeping up with HEAD and re-rolling - I had a look at the changes and the whole patch again. -> This looks good, so let's move on.

webchick’s picture

Tagging per Dries in #108.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

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
Berdir’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.12 KB
new39 KB
PASSED: [[SimpleTest]]: [MySQL] 58,153 pass(es).
[ View ]

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 :)

yched’s picture

Status:Reviewed & tested by the community» Needs review

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

yched’s picture

+++ 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

klausi’s picture

StatusFileSize
new7.76 KB
new40.95 KB
PASSED: [[SimpleTest]]: [MySQL] 57,894 pass(es).
[ View ]

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().

yched’s picture

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

yched’s picture

[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 :-) ]

klausi’s picture

yched: permission granted, of course!

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

yched’s picture

StatusFileSize
new1.63 KB
new41.82 KB
PASSED: [[SimpleTest]]: [MySQL] 57,895 pass(es).
[ View ]

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

klausi’s picture

Status:Needs review» Reviewed & tested by the community

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.

yched’s picture

klausi’s picture

StatusFileSize
new1.92 KB
new41.77 KB
PASSED: [[SimpleTest]]: [MySQL] 58,506 pass(es).
[ View ]

Patch does not apply anymore, rerolled.

klausi’s picture

StatusFileSize
new3.36 KB
new41.82 KB
PASSED: [[SimpleTest]]: [MySQL] 58,188 pass(es).
[ View ]

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

klausi’s picture

Priority:Normal» Critical

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

alexpott’s picture

Title:Provide the options list of an entity field» Change notice: Provide the options list of an entity field
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

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!).
    */
tstoeckler’s picture

-   * @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.

tim.plunkett’s picture

false is not a valid typehint. bool is.

klausi’s picture

"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

Berdir’s picture

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 :)

klausi’s picture

klausi’s picture

Status:Active» Needs review

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

Berdir’s picture

Title:Change notice: Provide the options list of an entity field» Provide the options list of an entity field
Status:Needs review» Fixed
Issue tags:-Needs change record

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...

smiletrl’s picture

Status:Fixed» Needs review
StatusFileSize
new657 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,476 pass(es).
[ View ]

I think this patch missed something:)

yched’s picture

Status:Needs review» Reviewed & tested by the community

Indeed...

webchick’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests
smiletrl’s picture

Status:Needs work» Needs review

@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.

smiletrl’s picture

effulgentsia’s picture

Title:Provide the options list of an entity field» [Small followup for] Provide the options list of an entity field
Priority:Critical» Normal
Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs tests, -API change, -RTBC July 1, -Approved API change

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.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Oh, cool. Thanks for the clarification.

Committed and pushed to 8.x. Thanks!

yched’s picture

Title:[Small followup for] Provide the options list of an entity field» Provide the options list of an entity field

Restoring title for posterity

effulgentsia’s picture

Priority:Normal» Critical
Issue tags:+API change, +RTBC July 1, +Approved API change

Restoring priority and tags for posterity.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary:View changes

updated "allowed" with "settable"