Merge ListBooleanItem from options module into BooleanItem

Making the title shorter, I think that's enough and easier to read.

First round...
Let's see what testbot says :)

#2226493: Apply formatters and widgets to Node base fields also adds a very basic widget for the boolean field. Cross-referencing.

Re-rolling moving annotations to static defaultSettings() method.

Fixing tests

Hello,

Here is a new round of the patch... I addressed @andypost comments and moved the OnOffWidget to core. We shouls decide in which issue we are going to address the widget and formatter for Boolean field.

Tests will fail because I didn't change OptionsWidgetTest, so this is just for review.

I'm having also problems with FrontPageTest, trying to dig more on it in the meantime.

Regards

New round fixing ForntPageTest issue, now it should fail only options widget tests.

Regards

One tricky thing is that option.module's ListBooleanItem currently lets you use any two ints for the "on" and "off" values (schema is 'int'), while BooleanItem is strictly about 0 and 1 (and schema is 'tiny int').

In hindsight, that's a useless and absurd feature if you ask today's me, and I can't remember what led to this in CCK :-/. But there are probably existing D6 / D7 sites with data different from 0 / 1 in "boolean" fields... We could normalize the values back into 0 and 1 during migration, but then they might have views based on that...

Re-roll.

Fixing tests. the schema problem was nasty.

Ok, so this almost passed.

Not exactly sure what the allowed values type should be, but I think it should be int, as that's what we're storing, no?

@yched: Ok, so you're suggesting to drop the ability, and then either take care of the migration processing here or do it in a separate issue? Seems like all the allowed values stuff would just go away then, sounds nice... I agree with not seeing why we'd want to support that.

And for the views problem, we have that elsewhere too, for example, we're now shortening vocabulary machine names and other identifiers like that, and if you use those in a view, it's going to be broken too.

Ok, so you're suggesting to drop the ability, and then either take care of the migration processing here or do it in a separate issue?

Yup. Not too sure what the current core practices are regarding whether migration stuff needs to be done in the same patch or in a followup.

Seems like all the allowed values stuff would just go away then, sounds nice

Well, we still need the ability to store custom "on" and "off" labels, but the values themselves would always be 0 and 1, yes.

Discussed with @benjy, he's OK with a follow-up for migrate, will create that.

Ok, I'm pretty confused now, the way the allowed_values settings work seems very weird.. because I don't see how it allows to save a different value, it just seems to mis-use the same settings in a weird way but uses it very differently. What you set as on/off *value* is really the label, it always stores 0/1. If you enter strings or numbers, doesn't matter.

That seems very very confusing to me, As it's now split from the other option field types, it seems much easier to just have a on_label and off_label setting and explicitly use that instead of hardcoding the 0, 1 keys inside allowed_values? Also, the field settings allow for a allowed_values_function callback and the settings form there cares about it, but both widgets hardcode allowed_values and only use that?

So, suggestion:

Drop allowed_values stuff, explicitly use on_label and off_label as simple settings, make it explicit that this is the *label*, not the value.

BooleanWidget seems to be a weird name for the "Check boxes/radio buttons" widget, as is that name, what we really have is one widget that shows a single checkbox and one that shows radios as Yes/No. Naming suggestions for label and class name? It's now or never time...

Also, a multi-value boolean field is probably one of the weirder things I've seen so far, it does then display checkboxes instead of radios so you can have a boolean field set to both on and off and it stores both values as separate field items. options.module, you drunk? :)

Thanks so much for working on this @Berdir :-)

re #26

- Storing on_label and off_label : yes, totally

- widgets : yes, I see no reason to merge "checkbox" & "select" into one widget.
I guess the checkbox one could be Core/Field/Plugin/Field/FieldWidget/CheckboxWidget, plugin id 'checkbox' ?
Then, having a "select" widget would be nice, but I don't think that's the main use case. Maybe just keeping option.module's SelectWidget for this would be good enough ? It's only available if you enable options.module, but do we currently have "boolean" base fields that are output using a select ?

- Actually, the widgets provided by options.module are in fact supposed to be agnostic and work with any field type that implements AllowedValuesInterface.
So they would be better off in Core, now that Core can provide plugins (wasn't the case back when we converted options.module to plugins). No need to depend on options.module if you just want the widgets.
That's kind of what we had in D7 - options.module was just providing agnostic widgets, and list.module was providing the field types. Then the two were merged because for some reason "list" was problematic for a module name in D8 (reserved keyword or something). But now we have Core for that :-)
Probably for a separate issue though. Opened #2302021: Move options.module widgets in Core.

- multiple-valued booleans : yeah, those are a bit awkward :-/. IIRC, the current checkbox widget actually only lets you edit one value at delta 0.
Strictly speaking, no reason a "multiple value boolean field" wouldn't make sense, even though in practice that's really rare.
It's more a question of how to present the checkbox widget - a series of separate checkboxes, all with the same label, in a tabledrag ? (well, why not I guess :-/)

- I didn't remember that the "radios / checkboxes" widget (ButtonsWidget) was available for boolean fields, but apparently that's the case in D7 too :-/. Yeah, I guess its behavior is a bit weird, the "single = checkboxes / multiple = radios" logic in ButtonsWidget is totally not a good match here.
A "radio" widget would make sense for bools, but it probably needs to be a separate widget strictly for bools - probably for a followup ?

Putting it all together, we'd have :
- Core/SelectWidget : applies to whatever field types want to (and are able to) use them, and that includes bools
- Core/[the other widgets currently provided by options.module] : same as above, but not used for bools, not relevant
- Core/BooleanCheckboxWidget : the current OnOffWidget, only applies to bool items
- (possibly) Core/BooleanRadioWidget : a new widget, only applies to bool items

Ok, did most of that I think.

Kept the buttons/radio widget available for the boolean field type, works fine apart from the weird multiple behavior and that was like that before.

Fixed the new tests (they were still PSR-0) and extend them, removed the boolean/checkbox tests from OptionsWidgetsTests.

Moving the widgets sounds like a good idea, I guess we could move the formatters too? For some reason that I don't understand, they still use options_allowed_values(), while they could just use the AllowedValuesInterface methods? That part I'm not sure about, because now we can't use those formatters anymore, as the allowed_values setting keys are gone.

Fixed tests.

Biggest left-over is formatters, as mentioned above. We can't use the options.module formatter right now, but it would be possible if they'd use the interface. Being able to output 0/1 with the number formatter doesn't seem very useful, if we can't fix the options formatters here, maybe we should add a simple formatter that displays the on/off label instead?

Yes, agreed that options.module formatter should use AllowedValuesInterface instead of options_allowed_values(), stumbled on it in #2293773-68: Field allowed values use dots in key names - not allowed in config too.

Should we clean that up the other issue or should we fix it here instead of introducing a new formatter for the boolean field?

I think at this point it would make sense to postpone #2293773: Field allowed values use dots in key names - not allowed in config on this one here.
Removing Bool as a List field type will significantly simplify whatever approach gets retained over there.

[edit: fixed issue link. Pasting issues URLs on a phone is hell]

Added the formatter.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/BooleanItem.php
   @@ -18,10 +20,21 @@
   +      'on_label' => '1',
   +      'off_label' => '0',
   

   These default labels seem kind of bizarre? "On" and "Off" or "Enabled" and "Disabled" seem more logical?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/BooleanItem.php
   @@ -48,4 +61,56 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
   +      '#title' => t('On label'),
   ...
   +      '#title' => t('Off label'),
   

   What about '"On" label' and '"Off" label'? Kind of difficult to understand otherwise, no? :)

   Also: $this->t()? Perhaps you'll need to use StringTranslationTrait though.

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/BooleanItem.php
   @@ -48,4 +61,56 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getPossibleValues(AccountInterface $account = NULL) {
   +    return array(0, 1);
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getPossibleOptions(AccountInterface $account = NULL) {
   +    return array(
   +      0 => $this->getSetting('off_label'),
   +      1 => $this->getSetting('on_label'),
   +    );
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getSettableValues(AccountInterface $account = NULL) {
   +    return array(0, 1);
   +  }
   

   Why zero and one and not… boolean values for this? It's called the BooleanItem after all?

   If it gets stored as a zero and a one, that's fine, but that implementation detail shouldn't matter here, right?

Thanks for the review.

1. They are, but that's what we had before (well, before it was empty with a description that it will default to 1/0).

2. Makes sense to me, wasn't sure what to use there.

3. Both the storage and the form API uses 0/1, the boolean field value was always 0/1, I'm just making it a bit more explict than it was before (which had array($off, $on), resulting in the same).

Ok, switched to On and Off by default, using $this->t() (can't use it in defaultSettings(), that's static).

Updated the issue summary.

Thanks for the clarification and reroll. I still think it's bizarre that we use 0/1 rather than false/true, but that's here for historical reasons and it's out of scope to change that.

Other than that, the patch looks good to me, besides the silliest of silly nitpicks:

+++ b/core/modules/field/config/schema/field.schema.yml
@@ -97,6 +97,34 @@ entity_form_display.field.hidden:
+  label: 'List (boolean) settings'
...
+  label: 'List (boolean)'

The "List" part is now obsolete?

Yep.

This is just lovely :-)

Minor remarks :

1. +++ b/core/modules/node/src/Tests/NodeAccessLanguageAwareCombinationTest.php
   @@ -53,7 +53,7 @@ public function setUp() {
   -      'type' => 'list_boolean',
   +      'type' => 'boolean',
   ...
          'settings' => array(
   

   not visible in patch context, but the 'settings' is still 'allowed_values' - which shows that it's irrelevant now and could be ditched altogether.
   Same in NodeAccessLanguageAwareTest

2. +++ b/core/modules/options/src/Tests/OptionsWidgetsTest.php
   @@ -97,7 +97,7 @@ function setUp() {
        $this->bool = entity_create('field_config', array(
          'name' => 'bool',
          'entity_type' => 'entity_test',
   -      'type' => 'list_boolean',
   +      'type' => 'boolean',
          'cardinality' => 1,
   

   $this->bool is actually never used in the test now, the whole definition could go away ?

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/CheckboxWidget.php
   @@ -62,22 +61,17 @@ public function settingsSummary() {
   +      '#default_value' => !empty($items[0]->value),
   

   ok, options.module fun ahead :-)

   The previous OnOffWidget was 'multiple_value' = TRUE.
   Meaning one single copy of its formElement() was handling the edition of multiple values. This formElement() actually returned one single checkbox, meaning, the widget actually ever only let you edit one single value. Thus, it was only using $items[0]->value, since that's the only value it ever affected.

   Now, the new CheckboxWidget doesn't have 'multiple_value' = TRUE, so, like most widgets, it gets repeaded several times in a tabledrag when used on a multiple-value field.
   Then, the code above should become !empty($items[$delta]->value)

   BUT : WidgetBase automatically adds one extraneous empty element. And a checkbox, the moment it's there, submits either 0 or 1 - you can not leave the widget "empty".
   Meaning, on an unlimited field, you save one new value each time you submit the form :-)

   As stated above, "multiple boolean" fields are an edge case in practice. I'd suggest leaving CheckboxWidget to 'multiple_value' = TRUE, and we can add a BooleanRadios widget later, that will allow multiple values. Started working on one, but would clutter this patch.

   In prevision for this, I'd also suggest renaming CheckboxWidget / checkbox to BooleanCheckboxWidget / boolean_checkbox. We'll have BooleanRadiosWidget / boolean_radios next to that.

4. +++ b/core/modules/field/src/Tests/Boolean/BooleanFieldTest.php
   @@ -0,0 +1,177 @@
   +  public static $modules = array('entity_test', 'field_ui', 'options');
   

   'options' is not needed ?

Also, I'd hate to bikeshed, but I'd think "Yes / No" would be more suitable for default labels. I can't think of a case where I wouldn't want to change the default "On / Off".

You don't have to delete my patch just because it's not perfect :p (Yes, I know it's a bug ;))

1. Updated it to us on_label, off_label, the took the extra step before, doesn't hurt to keep that.
2. Nice catch, removed.
3. Ok, convinced ;) Back to multiple, renamed. This might break some tests due to the form element change, but I'm too lazy right now to look for that.
4. I need options module because I'm also testing the radio widget provided by options.module. We can remove that when we move those widgets :)

Messing with the types just reminded me that this will conflict with #597236: Add entity caching to core but one more re-roll isn't going to make a difference there.

Just that test, then :)

Oops - yeah, I do remember a crosspost when posting #41, didn't see it had such radical effects :-)
[heh, almost did that again just right now]

+++ b/core/modules/field/config/schema/field.schema.yml
@@ -291,3 +319,15 @@ entity_form_display.field.number:
+entity_form_display.field.checkbox:

Needs to be adjusted for the new plugin id ?

Yeah :)

Yay !

RTBC++ :) yched beat me to it :D

Working on a draft notice

Draft notice: https://www.drupal.org/node/2304127

+ Updated the "API changes" section a bit in the summary.

This is at a minimum major given that it would help us move forward with the beta blocker at #2293773: Field allowed values use dots in key names - not allowed in config.

Yay!

Reminder: If this is committed, make sure that #597236: Add entity caching to core is set back to needs work, as they conflict (not sure if the patch conflicts, but that issue adds a new list_boolean field)

Oh, ignore that, I actually used an integer field type there because boolean had no widgets :)

Looks fantastic. Committed/pushed to 8.x, thanks!

Awesome ! Congrats !

Yay, rerolled all the approaches in #2293773: Field allowed values use dots in key names - not allowed in config following this one.