Move type-specific logic from ListItemBase to the appropriate subclass

Side note: ListItemBase::getSchema() defining just indexes feels a bit weird IMO.

Maybe remove it and repeat the indexes in the actual child classes ?

Working on this as I talk with @yched on IRC.

Attaching patch which moves code from ListItemBase to each subclass. Maybe it could be split into some more methods to avoid some code duplication in validateAllowedValues() or extractAllowedValues(), but I believe this is fine now.

Let's see testbot.

Regards.

needs reroll, it seems

Just a reroll, not reviewed.

1. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListBooleanItem.php
   @@ -60,4 +63,68 @@ public function getPropertyDefinitions() {
   +      '#value_callback' => 'options_field_settings_form_value_boolean_allowed_values',
   

   ideally this would be moved to a static method on the class

2. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListFloatItem.php
   @@ -60,4 +63,133 @@ public function getPropertyDefinitions() {
   +      '#element_validate' => array(array($this, 'validateAllowedValues')),
   

   validateAllowedValues() would also be best as a static method, set with
   '#element_validate' => array(get_class($this), 'validateAllowedValues'))

   (#field_name and #entity_type will need to be placed on the $element, so that the static method doesn't need to use $this)

3. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListFloatItem.php
   @@ -60,4 +63,133 @@ public function getPropertyDefinitions() {
   +  protected function extractAllowedValues($element) {
   

   - That method would also be a static

   - It could just receive the $element['#value'] as a string, would make the "job" of the method clearer.
   (would need to receive $element['#field_has_data'] as a $generate_keys bool param as well)

   - Not sure whether that one can be smartly split betwwen a common implementation in the base class + smart & limited local overrides. Let's keep that for a later step.

4. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListFloatItem.php
   @@ -60,4 +63,133 @@ public function getPropertyDefinitions() {
   +  public function settingsForm(array $form, array &$form_state, $has_data) {
   

   It looks like most of this implementation could be kept in the parent ListItemBase class, just delegating to a protected method in each child class for the $description text.

   Only ListBooleanItem::settingsForm() is different enough that it would override ListItemBase::settingsForm() completely.

5. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListFloatItem.php
   @@ -60,4 +63,133 @@ public function getPropertyDefinitions() {
   +  public function validateAllowedValues($element, &$form_state) {
   

   Similarly, that method could be kept in the base class ?

Attaching patch. I think it addresses all the points you suggested in #5.

Added some empty protected static methods in ListItemBase. Also proposed a split approach for extractAllowedValues() method.

This patch removes almost 100 lines of code compared to patch in #4 :) 6 files changed, 233 insertions(+), 328 deletions(-)

Any thoughts?

Nice !

1. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListItemBase.php
   @@ -55,95 +54,24 @@ public function getSettableOptions(AccountInterface $account = NULL) {
   +      '#field_has_data' => $has_data,
   +      '#access' => empty($allowed_values_function),
   +      '#field_name' => $this->getFieldDefinition()->getName(),
   +      '#entity_type' => $this->getEntity()->getEntityTypeId(),
   +      '#allowed_values' => $this->getFieldSetting('allowed_values'),
   

   Nitpick: let's move #access above #element_validate, instead of in the missdle of ad-hoc # properties that are there for validateAllowedValues (#field_has_data, #field_name, ...)

2. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListItemBase.php
   @@ -156,40 +84,31 @@ public function settingsForm(array $form, array &$form_state, $has_data) {
   +  protected function settingsFormDescription() {
   +    return '';
   +  }
   

   misses a PHPdoc - also, could be declared as abstract with no method body ?

3. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListItemBase.php
   @@ -156,40 +84,31 @@ public function settingsForm(array $form, array &$form_state, $has_data) {
   +  public function isEmpty() {
   +    $value = $this->get('value')->getValue();
   +    return empty($value) && (string) $value !== '0';
   

   While we're moving this around, we could get rid of the ->get()->getValue() syntax, which is especially confusing here.
   return empty($this->value) && (string) $this->value !== 0;
   should work.

   Also, the method would probably be better off moved up before settingsForm() rather than in the middle of the helpers for "allowed values".

4. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListItemBase.php
   @@ -197,57 +116,41 @@ public function validateAllowedValues($element, &$form_state) {
   +        if ($error = static::invalidOption($key)) {
   

   name of the method is not ideal. validateOption() ?

5. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListItemBase.php
   @@ -263,16 +166,14 @@ protected function extractAllowedValues($string, $generate_keys) {
   +      // Otherwise see if we can use the value as the key. Detecting true
   +      // integer strings takes a little trick.
   +      elseif (static::validateOptionValue($text)) {
   

   The comment about the "little trick" should be moved to the subclass that does the "trick" :-).

6. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListItemBase.php
   @@ -299,11 +196,62 @@ protected function extractAllowedValues($string, $generate_keys) {
   +  protected static function validateOptionValue($value) {
   +    return FALSE;
   +  }
   

   Hm - keep as an abstract ?

7. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListItemBase.php
   @@ -299,11 +196,62 @@ protected function extractAllowedValues($string, $generate_keys) {
   +  protected static function invalidOption($option) {
   +    return NULL;
   +  }
   

   Just leave an empty function body then ?

8. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListItemBase.php
   @@ -299,11 +196,62 @@ protected function extractAllowedValues($string, $generate_keys) {
   +   * Indicates if an option value is invalid or not.
   

   "Checks whether a candidate allowed value is valid" ?

   More generally :
   Latest patch manages to mutualize the overall extractAllowedValues() logic at the base class level, delegating specific processing to three helper methods:
   validateOptionValue()
   processOptionKey()
   invalidOption()

   Great, but IMO there is still room for improvement on the method names:
   - to clarify the exact role of each method as opposed to the other two, it's a bit confusing right now.
   - to make it clear that those methods pertain to the processing of the "allowed values" setting (as opposed to handling actual runtime field values in the FieldItem)

   With the current method names, it's a nice code achievement, but arguably a loss in understandability - it's a fine line :-)

9. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListItemBase.php
   @@ -197,57 +116,41 @@ public function validateAllowedValues($element, &$form_state) {
   -  protected function extractAllowedValues($string, $generate_keys) {
   +  protected static function extractAllowedValues($element_value, $has_data) {
   

   The old $string name was better IMO - the fact that it comes from an $element['#value'] is irrelevant to the method.

Hi, thanks for reviewing @yched ;)

Addressing most of your points, commenting some of them here:

2.- It can't be abstract because currently ListOptionBoolean does not display a description. We could add a '#markup' item to settingsForm(), but I think it is not necessary.

6.- This function can't be static because it is not necessary in ListOptionBoolean

8.- Methods renamed:

• validateOptionValue() -> validateOptionValue() (Yes, that's the same ;))
• processOptionKey() -> allowedValueAlter()
• invalidOption() -> validateAllowedValue()

While working on this patch I realized that those all new functions are related only for that field types which provide multiple options. All those methods are not necessary for the boolean field type. So I believe we could create a new level of abstraction, creating a new abstract class, where we place the settingsForm() and all its related methods. ListFloatItem, ListIntegerItem and ListTextItem would inherit from that class, keeping ListItemBase cleaner and containing only the logic common to all the options field types. It would improve readability, however, it makes the architecture a bit complex.

Attaching file options_logic-2169983-9-abstract.patch where I implement the proposed concept. I tried to make some of its methods abstract (abstract protected static function foo()), but my PHP throws some errors, maybe because of the version,

Regards

Fixing bug in explicit type comparison...

Actually, the "bool" type is different enough that IMO it would be simpler to disconnect it completely from a List**Base class.

Even the reusing the getPossibleValues() / getSettableValues() from ListItemBase is not a real gain since the "flatten" part is irrelevant for the bool field type.

Keeping a simple class hierarchy IMO wins over duplicating 10 lines of simple code that only make partial sense to reuse.

+ I think in the end we want to merge ListBooleanItem up into the 'boolean' "base / non-configurable" field type (Drupal\Core\Entity\Plugin\Field\FieldType\BooleanItem). that will be easier if it doesn't depend on a much richer base class it doesn't really need.

Decoupling ListBooleanItem from ListItemBase.

Re-rolling

Re-rolling after #2152825: rename FieldItemBase::getFieldSetting[s]() to getSetting[s]() #2200821: Rename Fieldinterface and FieldInstanceInterface and #2095303: Rename 'field_entity' to 'field_config' and 'field_instance' to 'field_instance_config'

Re-rolling

Thanks @plopesc, sorry for letting that one slip by.

I think we can still improve / simplify the helper submethods.

1. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListFloatItem.php
   @@ -31,25 +31,72 @@ class ListFloatItem extends ListItemBase {
   +  protected static function allowedValueAlter($value) {
   +    // Float keys are represented as strings and need to be disambiguated
   +    // ('.5' is '0.5').
   +    if (is_numeric($value)) {
   +      $value = (string) (float) $value;
   +    }
   +    return $value;
   

   allowedValueAlter() is only needed by ListFloatItem, and the task could probably be done in an override of extractAllowedValues() building on $values = parent::extractAllowedValues(); and then massaging the keys.

2. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListFloatItem.php
   @@ -31,25 +31,72 @@ class ListFloatItem extends ListItemBase {
   +  protected static function validateAllowedValue($option) {
   +    if (!is_numeric($option)) {
   +      return t('Allowed values list: each key must be a valid integer or decimal.');
   +    }
   +    else {
   +      return NULL;
   +    }
   +  }
   

   The else clause can be omitted - same in other implementations of the method.

   Also, in practice, validateAllowedValue() & validateOptionValue() look like they are duplicate - they check the same condition, and are named confisingly similar :-). They could probably be merged in a single validateAllowedValue() method ?

   Then we're left with only validateAllowedValue(), which in fact could probably have a single generic implementation in ListItemBase, that validates the value against the TypedData definition of the 'value' property defined in propertyDefinitions(), which already specifies all the constraints we need and is able to check them.
   That last part is probably better left for a followup though :-)

Also, not directly touched by the patch, but according to PHPstorm the "$value = $key = FALSE;" line in extractAllowedValues() is useless.

Addressing your comments:

• Removed allowedValueAlter()
• Removed useless return NULL sentences
• Merged validateAllowedValue() & validateOptionValue() into validateAllowedValue()
• Removed useless declaration line in extractAllowedValues()

This iteration looks good :) 4 files changed, 22 insertions(+), 63 deletions(-)

About the validation against typedData, I made some tests wit no luck. Some of them because we were calling it from a static context. I'm not familiar with it, and probably I'm missing something.

Regards

Nice - let's see what the testbot says.

Side note : Arguably, a more elegant way for ListFloatItem::extractAllowedValues() could be:

$keys = array_keys($values);
$labels = array_values($values);
$keys = array_map(function ($a) {
  return is_numeric($key) ? (string) (float) $key : $key;
}, $keys);
$values = array_combine($keys, $labels);

hah, I had almost that code, but finally I decided to change it...
I thought that the use of array_keys() + array_values() + array_combine() could hit the performance (although this function is not critical at all), so I replaced it with just a foreach loop instantiating a new array.

I'm totally open to change it if you consider ti better. ;)

Yeah, perf is not too much of an issue here. Well, no biggie either way :-)

Uploading patch with more elegant implementation, saving more lines of code!
1 file changed, 6 insertions(+), 13 deletions(-)

Needs serious re-roll after #2192259: Remove ConfigField.. Item and List classes + interfaces

Re-rolling.

It was easier than I expected :)

Let's see what testbot says...

+++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListBooleanItem.php
@@ -52,4 +70,92 @@ public static function propertyDefinitions(FieldDefinitionInterface $field_defin
+      'indexes' => array(
+        'value' => array('value'),

+++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListFloatItem.php
@@ -31,25 +31,66 @@ class ListFloatItem extends ListItemBase {
+      'indexes' => array(
+        'value' => array('value'),

+++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListIntegerItem.php
@@ -31,25 +31,50 @@ class ListIntegerItem extends ListItemBase {
+      'indexes' => array(
+        'value' => array('value'),

+++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListTextItem.php
@@ -31,28 +31,52 @@ class ListTextItem extends ListItemBase {
+      'indexes' => array(
+        'value' => array('value'),

Any reason for index to add?

@yched List fields are only ones that defines indexes - any reason?

1. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListBooleanItem.php
   @@ -26,20 +30,34 @@
   +class ListBooleanItem extends FieldItemBase implements AllowedValuesInterface {
   

   Suppose better to merge this with core's BooleanItem as we do for other types in #2169983: Move type-specific logic from ListItemBase to the appropriate subclass

2. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListItemBase.php
   @@ -55,95 +54,31 @@ public function getSettableOptions(AccountInterface $account = NULL) {
   +    $element['allowed_values']['#description'] = $this->settingsFormDescription();
   
   @@ -157,39 +92,26 @@ public function settingsForm(array $form, array &$form_state, $has_data) {
   +   * Provides the field type specific settings form element #description.
   ...
   +   *   The field type settings form specific description.
   ...
   +  abstract protected function settingsFormDescription();
   
   +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListFloatItem.php
   @@ -31,25 +31,66 @@ class ListFloatItem extends ListItemBase {
   +  protected function settingsFormDescription() {
   +    $description = '<p>' . t('The possible values this field can contain. Enter one value per line, in the format key|label.');
   +    $description .= '<br/>' . t('The key is the stored value, and must be numeric. The label will be used in displayed values and edit forms.');
   +    $description .= '<br/>' . t('The label is optional: if a line contains a single number, it will be used as key and label.');
   +    $description .= '<br/>' . t('Lists of labels are also accepted (one label per line), only if the field does not hold any values yet. Numeric keys will be automatically generated from the positions in the list.');
   +    $description .= '</p>';
   +    $description .= '<p>' . t('Allowed HTML tags in labels: @tags', array('@tags' => _field_filter_xss_display_allowed_tags())) . '</p>';
   +    return $description;
   +  }
   
   +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListIntegerItem.php
   @@ -31,25 +31,50 @@ class ListIntegerItem extends ListItemBase {
   +  protected function settingsFormDescription() {
   +    $description = '<p>' . t('The possible values this field can contain. Enter one value per line, in the format key|label.');
   +    $description .= '<br/>' . t('The key is the stored value, and must be numeric. The label will be used in displayed values and edit forms.');
   +    $description .= '<br/>' . t('The label is optional: if a line contains a single number, it will be used as key and label.');
   +    $description .= '<br/>' . t('Lists of labels are also accepted (one label per line), only if the field does not hold any values yet. Numeric keys will be automatically generated from the positions in the list.');
   +    $description .= '</p>';
   +    $description .= '<p>' . t('Allowed HTML tags in labels: @tags', array('@tags' => _field_filter_xss_display_allowed_tags())) . '</p>';
   +    return $description;
   +  }
   
   +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListTextItem.php
   @@ -31,28 +31,52 @@ class ListTextItem extends ListItemBase {
   +  protected function settingsFormDescription() {
   +    $description = '<p>' . t('The possible values this field can contain. Enter one value per line, in the format key|label.');
   +    $description .= '<br/>' . t('The key is the stored value. The label will be used in displayed values and edit forms.');
   +    $description .= '<br/>' . t('The label is optional: if a line contains a single string, it will be used as key and label.');
   +    $description .= '</p>';
   +    $description .= '<p>' . t('Allowed HTML tags in labels: @tags', array('@tags' => _field_filter_xss_display_allowed_tags())) . '</p>';
   +    return $description;
   +  }
   

   This part is messy, "field type settings form element"...
   But actually used as description for allowed values!

@yched List fields are only ones that defines indexes - any reason?

This does make sense to me. Checkboxes and Selects or something that you then often create views with filters, e.g. display all nodes which have selected option C. You don't do that for a textfield or a number (or much less frequently)

Patch trying to address #29

Renamed settingsFormDescription() to allowedValuesDescription()

Removed list_boolean into boolean field type. Not sure if this is the best approach, or maybe could be better extend Drupal\Core\Entity\Plugin\Field\FieldType\BooleanItem in options module, like entity_reference does.

This is just a proof of concept... Let's see what testbot says

1. +++ b/core/modules/options/options.module
   @@ -34,12 +33,16 @@ function options_help($path, $arg) {
   +  // Make the entity reference field configurable.
   

   typo: Make the boolean field configurable

2. +++ b/core/modules/options/options.module
   @@ -34,12 +33,16 @@ function options_help($path, $arg) {
   +  $info['boolean']['settings']['allowed_values'] = array();
   +  $info['boolean']['settings']['allowed_values_function'] = '';
   

   Should be moved to annotation

3. +++ b/core/modules/options/options.module
   @@ -34,12 +33,16 @@ function options_help($path, $arg) {
   +  $info['boolean']['provider'] = 'options';
   

   not sure is needed

@andypost:
- indexes: what @Berdir said. Plus list types are not the only ones, text fields have an index on the 'format' property, and e_r fields have an index on the target_id

- merging bool field types: was mentioned earlier in this thread, totally +1, but I wanted to keep that for a followup, to not bikeshed this patch more than it already has been. It's kind of outside the scope here (internal code refactoring without any functional change)
If latest patch is fine, then good, otherwise i'd suggest working on "merge bool types" in a separate issue.

Alzo, @andypost #29.2 : yes, what is the issue ? The allowed values are field-level settings, and thus the input ui is in the "field settings form".
Also, this patch just moves code around but doesnt change ui or help texts from current HEAD, so discussing this, if needed, is not for this issue.

Yes, we really should limit scope to #26
Moving to Core needs more changes... then schema...
@plopesc Please re-roll it with rename of the description.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/BooleanItem.php
@@ -21,7 +24,35 @@
+  public function getSettableOptions(AccountInterface $account = NULL) {
+    return options_allowed_values($this->getFieldDefinition(), $this->getEntity());

When no 'options' module enabled this does not works

Re-rolling...

Removing unnecessary "module" keys in @FieldType annotation.

Looks rtbc!

+++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListFloatItem.php
@@ -31,25 +30,66 @@ class ListFloatItem extends ListItemBase {
+      $keys = array_map(function ($key) {
+        return is_numeric($key) ? (string) (float) $key : $key;
+      }, $keys);

We've lost the comment as to why we're doing the (string) (float) casting

Adding the removed comment line.
Putting it back to RTBC given that the interdiff only adds the comment.
Regards.

Committed 5909b03 and pushed to 8.x. Thanks!

We need to check existing change notices.

Anyone up for a "merge Drupal\options\Plugin\Field\FieldType\ListBooleanItem into Drupal\Core\Field\Plugin\Field\FieldType\BooleanItem" followup ? :-)

If we wanna move it, then at least first issue should be solved to remove dependency from ListBooleanItem
1) #2171397: Move options_allowed_values() to a method somewhere ?
2) #2037217: Should we need dynamic options in options_allowed_values()?

@andypost: hm, is that for sure ? ListBooleanItem is sufficiently trivial regarding allowed values (always two allowed values, 0 and 1), and sufficiently disconnected from the other List* field types (as this patch here made clear) that I'm not sure that it's really held back by those issue.

So filed #2226063: Merge ListBooleanItem from options module into BooleanItem
Just not sure about formatters and widgets

Thanks @andypost !

And thanks @plopesc for sticking with this issue here :-)

Also, opened #2226071: List field types: replace implementations of validateAllowedValue() with Typeddata validation for #19 :

Then we're left with only validateAllowedValue(), which in fact could probably have a single generic implementation in ListItemBase, that validates the value against the TypedData definition of the 'value' property defined in propertyDefinitions(), which already specifies all the constraints we need and is able to check them.
That last part is probably better left for a followup though :-)

In #20, @plopesc said he had troubles making this work, but maybe it might help with @fago & @berdir around in Szeged ?