Convert field type to typed data plugin for options module

Here's a new patch, we still have to convert the hook_field_validate() but I wanted to get inputs on the attached patch before I continue to work more on that.
The patch coming from #1848570: Upgrade to Doctrine\Common 2.4 is needed, otherwise you'll get a Doctrine\Common\Annotations\AnnotationException.

I just want to note that the allowed values stuff will change very soon in #1758622: Provide the options list of an entity field.

#11: drupal-convert-field-type-to-typed-data-plugin-options-2015689-11.patch queued for re-testing.

This new patch now contains the hook_field_validate() conversion, in order to do that I had to create a new ChoiceConstraint validation class. Hope this one will pass the tests.

Good start:) Some suggestions

1). Extra whitesapce
drupal-convert-field-type-to-typed-data-plugin-options-2015689-16.patch:551: trailing whitespace.
drupal-convert-field-type-to-typed-data-plugin-options-2015689-16.patch:653: trailing whitespace.

2).

+    $settings = $field['settings'];
+
+    if ($field['type'] == 'list_boolean') {

I think if is unnecessary here, since this class is for field type list_boolean.

3). '#element_validate' => array(array($this, 'validateAllowedValues')), should use 'property_constraints' => array() too?

I had to copy the flattenOptions() method from OptionsWidgetBase because the options_array_flatten function has been removed, we should probably put it back, otherwise we'll have to copy it everytime we need it...

The attached patch removes the whitespaces + the field type condition.

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ChoiceConstraint.php
@@ -0,0 +1,38 @@
+ * @todo: Move this below the TypedData core component.

This means we still need to do it in this patch, right?

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ChoiceConstraint.php
@@ -0,0 +1,38 @@
+   * Overrides Range::validatedBy().

{@inheritdoc}

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/field_type/ListBooleanItem.php
@@ -0,0 +1,115 @@
+ *   module = "options",

This wouldn't be needed anymore if we can get #2041423: Rely on 'provider' instead of 'module' for Field plugin types done before :(

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/field_type/ListBooleanItem.php
@@ -0,0 +1,115 @@
+  public static function schema(Field $field) {
+     return parent::schema($field) + array(
+       'columns' => array(
+         'value' => array(
+           'type' => 'int',
+           'not null' => FALSE,
+         ),
+       ),
+     );
+   }

There seems to be some weird indentation going on there and in all the other schema() implementations. This and the item below (accessing settings) needs to be fixed in all places :)

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/field_type/ListBooleanItem.php
@@ -0,0 +1,115 @@
+    $settings = $field['settings'];

You should use $field->getFieldSetting() instead of relying on the ArrayAccess implementation that should be removed soon.

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/field_type/ListItemBase.php
@@ -0,0 +1,273 @@
+ * Plugin base class used by the

by the... ?

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/field_type/ListItemBase.php
@@ -0,0 +1,273 @@
+   * @param $values
...
+   * @return

I know the code where this were copied from did not have them, but we should add type hints everywhere now.

+++ b/core/modules/options/options.module
@@ -382,7 +109,7 @@ function options_field_update_forbid($field, $prior_field) {
-    $field = field_info_field_by_id($field['uuid']);
+    $field = Field::fieldInfo()->getFieldById($field['uuid']);

Out of scope for this issue.

Thanks for working on this! :)

Thanks for your review amateescu, here's a new patch with some of the feedbacks addressed.

Ohh, interdiff, nice :) You forgot the part that $settings = $field->getFieldSettings(); and type hints needs to be done for all new classes..

Is there any typehints left? Because I can't find them.

Good job!

+        'Choice' => array(
+          'choices' => array_keys($allowed_values),
+          'message' => t('%name: illegal value.', array('%name' => $instance['label'])),
+          'multiple' => $instance->getFieldCardinality() == FIELD_CARDINALITY_UNLIMITED,
+        ),

'multiple' will force validated value to be multiple values, i.e. an array. In this case, validator is validating FieldItem's each primitive 'value', an string or number. This line should be deleted to use the default 'multiple' value, i.e., false.

Also how about 'illegal value selected.':)

Yes but I'm only setting multiple to TRUE if the cardinality of the field is unlimited which is wrong, I should do that if the cardinality is != 1.

Hmm, cardinality != 1 will probably fail too:)

You may try an options field with cardinality != 1 and save the field.

Yes you're right, it seems we don't need the multiple flag there, it works fine without.

Maybe apply this change to demonstrate why 'multiple' line should be deleted:)

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ChoiceConstraint.php
@@ -0,0 +1,38 @@
+  /**
+   * Overrides Range::validatedBy().
+   */
+  public function validatedBy() {

index f9bbfa9..61004ef 100644
--- a/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/LengthConstraint.php

--- a/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/LengthConstraint.php
+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/LengthConstraint.php

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/LengthConstraint.php
+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/LengthConstraint.php
@@ -31,7 +31,7 @@ class LengthConstraint extends Length {

@@ -31,7 +31,7 @@ class LengthConstraint extends Length {
   public $exactMessage = 'This value should have exactly %limit character.|This value should have exactly %limit characters.';
 
   /**
-   * Overrides Range::validatedBy().
+   * @inheritdoc
    */
   public function validatedBy() {

You updated the wrong class/method :) And it's {@inheritdoc}, notice the {}.

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/field_type/ListBooleanItem.php
@@ -0,0 +1,115 @@
+    $element += parent::settingsForm($form, $form_state, $element);

I think we should put the parent:: call at the top, as our class usually needs to add (or replace) something to the element provided by the parent.

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/field_type/ListFloatItem.php
@@ -0,0 +1,66 @@
+  public static function schema(Field $field) {
+     return parent::schema($field) + array(
+       'columns' => array(
+         'value' => array(
+           'type' => 'float',
+           'not null' => FALSE,
+         ),
+       ),
+     );
+   }

Wrong indentation here.

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/field_type/ListIntegerItem.php
@@ -0,0 +1,66 @@
+  public static function schema(Field $field) {
+     return parent::schema($field) + array(
+       'columns' => array(
+         'value' => array(
+           'type' => 'int',
+           'not null' => FALSE,
+         ),
+       ),
+     );
+   }

And here.

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/field_type/ListItemBase.php
@@ -0,0 +1,272 @@
+  public static function schema(Field $field) {
+     return array(
+       'indexes' => array(
+         'value' => array('value'),
+       ),
+     );
+   }

And here :P

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/field_type/ListTextItem.php
@@ -0,0 +1,85 @@
+  public static function schema(Field $field) {
+     return parent::schema($field) + array(
+       'columns' => array(
+         'value' => array(
+           'type' => 'varchar',
+           'length' => 255,
+           'not null' => FALSE,
+         ),
+       ),
+     );
+   }

...

No other complains otherwise :)

Thanks again for your review, I decided to remove the settingsForm implementation from the ListBooleanItem class and I regrouped everything in ListItemBase.

A quick review, this looks pretty good -- looks RTC to me:)

After applying this patch I got errors when trying to add list fields to a node bundle:

Fatal error: Declaration of Drupal\options\Plugin\field\field_type\ListItemBase::settingsForm() must be compatible with that of Drupal\field\Plugin\Type\FieldType\ConfigFieldItemInterface::settingsForm() in /Users/persch/Sites/d8_dev/docroot/core/modules/options/lib/Drupal/options/Plugin/field/field_type/ListItemBase.php on line 16

and

Fatal error: Declaration of Drupal\options\Plugin\field\field_type\ListItemBase::schema() must be compatible with that of Drupal\field\Plugin\Type\FieldType\ConfigFieldItemInterface::schema() in /Users/persch/Sites/d8_dev/docroot/core/modules/options/lib/Drupal/options/Plugin/field/field_type/ListItemBase.php on line 16

Here's a reroll that should fix the errors.

#1758622: Provide the options list of an entity field is going to reformulate a couple things there, let's wait until it's in.

Back to needs work as the linked issue is in :)

Most stuff is associated with the validation. Some possible conversions:

1).
Options fieldItems should imlements interface AllowedValuesInterface
Personally speaking, I don't think it's really necessary for options module to implement this interface. Current options modules doesn't make use of hook_options_list. It provides allowed values via options_allowed_values

But the new OptionsWidgetBase has made use of this new interface, so it forces us to use this interface, if we are going to use this field widget. This also means we should probably move Options_allowed_values into that four methods of the new interface.

2).
Get rid of

+/**
+ * Choice constraint.
+ *
+ * Overrides the symfony constraint to use Drupal-style replacement patterns.
+ *
+ * @todo: Move this below the TypedData core component.
+ *
+ * @Plugin(
+ *   id = "Choice",
+ *   label = @Translation("Choice", context = "Validation"),
+ *   type = { "string" }
+ * )
+ */
+class ChoiceConstraint extends Choice {

We are going to make use this class AllowedValuesConstraint

3).

+  /**
+   * {@inheritdoc}
+   */
+  public function getConstraints() {
+    $constraint_manager = \Drupal::typedData()->getValidationConstraintManager();
+    $constraints = parent::getConstraints();
+    $instance = $this->getInstance();
+    $entity = $this->getParent()->getParent();
+
+    if (!isset($entity)) {
+      $ids = (object) array('entity_type' => $instance->entity_type, 'bundle' => $instance->bundle, 'entity_id' => NULL);
+      $entity = _field_create_entity_from_ids($ids);
+    }
+
+    $allowed_values = $this->flattenOptions(options_allowed_values($this->getInstance(), $entity));
+
+    $constraints[] = $constraint_manager->create('ComplexData', array(
+      'value' => array(
+        'Choice' => array(
+          'choices' => array_keys($allowed_values),
+          'message' => t('%name: illegal value selected.', array('%name' => $instance['label'])),
+        ),
+      ),
+    ));
+

This part could be updated. Change 'Choice' to 'AllowedValues'.
Add allowed values at getSettableValues(), see class AllowedValuesConstraintValidator
So, we also need to remove this line + 'choices' => array_keys($allowed_values),, becuase it will be overridden at the validator to make use of getSettableValues().

Most changes come from #39.

One Notable thing regarding TypedDataManager::getConstraints()

  // If the definition does not provide a class use the class from the type
  // definition for performing interface checks.
  $class = isset($definition['class']) ? $definition['class'] : $type_definition['class'];
  // Check if the class provides allowed values.
  if (array_key_exists('Drupal\Core\TypedData\AllowedValuesInterface', class_implements($class))) {
    $constraints[] = $validation_manager->create('AllowedValues', array());
  }

I think this part of code is wrong. This constraint added here will validate the whole object/typedData as value against an array of 'AllowedValues'. But in fact, we need to validate a property of the object, e.g., value of ListTextItem object here, to against an array of allowed values, not to validate the ListTextItem object itself against the allowed value array.

This part of code is deleted in this patch. Maybe a cleanup for #1758622: Provide the options list of an entity field, and it needs to add , or check existing test to show why this part of code should be deleted.

There're still bugs, but post it here, in case someone else wants to play with it too.

This time it should be fine.

interdiff-36-44.txt indicates the change since #36.

It includes a bug fix for #1758622: Provide the options list of an entity field.

-    if (array_key_exists('Drupal\Core\TypedData\AllowedValuesInterface', class_implements($class))) {
+    if (array_key_exists('Drupal\Core\TypedData\AllowedValuesInterface', class_implements($class)) && array_key_exists('Drupal\Core\TypedData\PrimitiveInterface', class_implements($class))) {
       $constraints[] = $validation_manager->create('AllowedValues', array());

The default constraints for object/class which implements AllowedValuesInterface isn't always as primitive value to validate against the allowed value array. If this object is an ComplexData, e.g., ListTextItem, then it will result in problem that tries to validate the whole object againt an array.

So, we need to make sure the default contraints at TypedDataManager::getConstraints only works for Prmitive value. The ComplexData's property constraints shoud be added at its own ::getConstraints function, like this:

+    $constraints[] = $constraint_manager->create('ComplexData', array(
+      'value' => array(
+        'AllowedValues' => array(
+          'message' => t('%name: illegal value selected.', array('%name' => $instance['label'])),
+        ),
+      ),
+    ));

oops:)

#36: drupal-convert-field-type-to-typed-data-plugin-options-2015689-36.patch queued for re-testing.

#44: options_field_type-2015689-44.patch queued for re-testing.

Need more work since #2074547: Remove ConfigFieldItemInterface::getInstance() is in.

Remove $this->getInstance() for fieldItem class.

Remove module entry.

We als need to figure out the validation stuff in #2015687: Convert field type to FieldType plugin for taxonomy module .

Marked #2014671: [META] Convert all field types to plugins as duplicate now we're down to the last two.

Attempt at a re-roll of the patch. Setting to needs review until test bot has a go at it (probably going to fail).

+++ b/core/modules/options/lib/Drupal/options/Plugin/Field/field_type/ListBooleanItem.php
@@ -0,0 +1,66 @@
+namespace Drupal\options\Plugin\field\field_type;

Namespace should be Drupal\options\Plugin\Field\FieldType

for all field types defined in this patch.

+++ b/core/modules/options/lib/Drupal/options/Plugin/Field/field_type/ListItemBase.php
@@ -0,0 +1,345 @@
+use Drupal\field\Plugin\Type\FieldType\ConfigFieldItemBase;

Class changed to

Drupal\Core\Field\ConfigFieldItemBase;

+++ b/core/modules/options/lib/Drupal/options/Plugin/Field/field_type/ListItemBase.php
@@ -0,0 +1,345 @@
+    if (in_array($field['type'], array('list_integer', 'list_float', 'list_text'))) {

$field is an object, not an array.

Additionally there is a section in ListItemBase that requires the widget to change the allowed values description based on the widget. This is not possible in D8 because of form display modes.

Here's a patch for review.

- I did not include the rename commit in the interdiff as it would make it as large as the patch itself.
- With regard to my previous comment, I made the description text contain all of the instructions. I have attached an image to show the output at a mobile width. More help text will make users' eyes glaze over and not read it. I think this is better than removing it altogether though.

Forgot interdiff that I uploaded and then deleted twice in the previous comment due to slow ajax response.

One line fix to fix all those test fails on my local install.

One line cosmetic fix.

Tagging for reroll.

Re-rolled.

- fetched origin
- rebase origin/8.x
- diff origin/8.x

Edit: I hid some older patches.

Changes form_error() usage to \Drupal::formBuilder()->setError() with additional $form_state parameter.

68: 2015689-drupal-options-field-type-68.patch queued for re-testing.

Simple re-roll with git rebase origin/8.x

Fixed patch to work with changes in TypedDataManager and ComplexDataInterface

• #2015687: Convert field type to FieldType plugin for taxonomy module
• #2112239: Convert base field and property definitions

Patch failed.

- Fixed API change in #2143263: Remove "Field" prefix from FieldDefinitionInterface methods.

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
@@ -362,7 +362,7 @@ public function getConstraints(DataDefinitionInterface $definition) {
-    if (is_subclass_of($class,'Drupal\Core\TypedData\AllowedValuesInterface')) {
+    if (is_subclass_of($class,'Drupal\Core\TypedData\AllowedValuesInterface') && is_subclass_of($class, 'Drupal\Core\TypedData\PrimitiveInterface')) {
       $constraints[] = $validation_manager->create('AllowedValues', array());

Instead of removing it, this should embrace allowed values validation now - analogously to term references.

@mradcliffe thanks for your work!

Here's a patch to clean the code a bit. Also, the main property of list field is 'value', so the default main property of FieldItemBase should be ok to do with AllowedValuesInterface.

Just to clarify, will the flattenOptions method on formBuilder make a tighter dependency on field validation/constraints and the Form API? I'm not familiar with the new \Drupal static methods and how loose or tight they are.

\Drupal::formBuilder() is a core required method, i.e. form api is a required component. So, from my eyes, the dependency could be ignored and it's ok to use these methods in other parts, not just inside form api.

There're many other methods created by \Drupal container, and we have used them everywhere, e.g., \Drupal::typedDataManager().

Yes, I know we like to depend on the container a lot already. I think it's probably the easiest way to get the patch in. Being able to solve this issue would be best in Drupal 9. It's always good to start thinking about it now. For instance, field validation is not necessarily form validation.

I think this needs to be reviewed by @fago, I'm not up to date enough with the allowed values discussion that happened in the taxonomy issue to RTBC this.

80: drupal_options_field_type-2015689-80.patch queued for re-testing.

This should fix that problem.

87: drupal_options_field_type-2015689-87.patch queued for re-testing.

This is just a code cleanup with no intended functional changes.

Opened #2169983: Move type-specific logic from ListItemBase to the appropriate subclass as a normal priority, non-beta-blocking followup, and added @todos linking to it.

+++ b/core/modules/options/options.module
@@ -25,184 +25,6 @@ function options_help($path, $arg) {
-  // Alter the description for allowed values depending on the widget type.
-  if ($field_type == 'list_boolean') {
-    $form['allowed_values']['#description'] .= '<p>' . t("For a 'single on/off checkbox' widget, define the 'off' value first, then the 'on' value in the <strong>Allowed values</strong> section. Note that the checkbox will be labeled with the label of the 'on' value.") . '</p>';
-  }
+++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListItemBase.php
@@ -0,0 +1,306 @@
+    // It is no longer possible to alter the description of allowed values
+    // depending on widget type because of form displays. The help text must
+    // instead be verbose.
+    $element['allowed_values']['#description'] .= '<p>' . t("For a 'single on/off checkbox' widget, define the 'off' value first, then the 'on' value in the <strong>Allowed values</strong> section. Note that the checkbox will be labeled with the label of the 'on' value.") . '</p>';
+    $element['allowed_values']['#description'] .= '<p>' . t("The 'checkboxes/radio buttons' widget will display checkboxes if the <em>Number of values</em> option is greater than 1 for this field, otherwise radios will be displayed.") . '</p>';

This UI change seems out of scope for this issue, so this patch reverts it, but attempts to improve the code comment that I think caused the confusion that led to the initial change.

I reviewed #87 quite thoroughly prior to my round of minor tweaks, and from my perspective, this patch is now good to go, including the way that AllowedValuesInterface is used (per #84). But since this is assigned to fago, and I've now authored some of it, I'll defer to him to RTBC. I agree with #83 that some of the dependencies on services in Drupal:: can be improved but that none of that is a regression from HEAD or release blocking. Anyone who wants to can still work on improving that in a follow up.

This should fix the problem.

97: drupal_options_field_type-2015689-97.patch queued for re-testing.

I agree with #83 that some of the dependencies on services in Drupal:: can be improved but that none of that is a regression from HEAD or release blocking.

Definitely!

Patch looks pretty good to me. I just found some small issues (see below), else this looks RTBC to me.

1. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListItemBase.php
   @@ -0,0 +1,308 @@
   + * Plugin base class inherited by the options field types.
   ...
   +    // Flatten options firstly, because Settable Options may contain group
   ...
   +   *   Boolean value indicating whether to generate keys based on the position of
   

   Comment exceeds 80chars.

2. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListItemBase.php
   @@ -0,0 +1,308 @@
   +   * @see options_allowed_values_string()
   

   This needs to be updated to point to the method.

3. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListTextItem.php
   @@ -0,0 +1,82 @@
   +          'max' => 255,
   ...
   +    $constraints[] = $constraint_manager->create('ComplexData', array(
   +      'value' => array(
   

   This should be in the data definition of the 'value' property as there is no reason to define it dynamically here.

No other remarks than @fago's #100.

Not strictly related, but while reviewing, created the following issues:
#2171393: Remove implementation of removed hook_options_list()
#2171395: OptionsWidgetBase should use WidgetInterface::massageFormValues()

Indeed, constraints should be moved.

1. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListItemBase.php
   @@ -228,16 +228,17 @@ public function validateAllowedValues($element, &$form_state) {
       *   The list of allowed values in string format described in
   -   *   options_allowed_values_string().
   +   *   allowedValuesString().
   

   This needs to be ListItemBase::allowedValuesString(), otherwise it's a function.

2. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListTextItem.php
   @@ -55,28 +55,11 @@ public static function schema(FieldDefinitionInterface $field_definition) {
      public function getPropertyDefinitions() {
        if (!isset(static::$propertyDefinitions)) {
   +      $constraints = array('Length' => array('max' => 255));
          static::$propertyDefinitions['value'] = DataDefinition::create('string')
   -        ->setLabel(t('Text value'));
   +        ->setLabel(t('Text value'))
   +        ->setConstraints($constraints);
       }
   

   This seems short enough to put it on a single line, but fine if we do it like this elsewhere too.

oops, missed that~

Changes look good, @fago, @effulentsia and @yched confirmed everything else, that should be enough. RTBC :)

Sorry, I thought I posted that, but apparently closed the tab before doing so :-/

+++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListItemBase.php
@@ -238,7 +238,7 @@ public function validateAllowedValues($element, &$form_state) {
+   * @see ListItemBase::allowedValuesString()

I think @see needs to have the full namespaced class so that it gets correclty parsed by api.module

(just reacting to the last interdiff, didn't check the rest of the patch for other ocurrences)

Thanks, fixed. No other occurences.

Thanks @smilerl. RTBC+1.

Great work folks!

Committed and pushed to 8.x.

I believe this now un-postpones #2087393: Remove LegacyField once all field types converted.

We may need to add to/extend an existing change notice about this, so tagging.

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release

I think we're good, this is part of the meta that is already referenced in https://drupal.org/node/2064123 and this was simply the last one to be converted.