Convert option widgets to Plugin system

Note : the porting is basically done already in a sandbox of mine.
Don't start this without pinging me.

These are the current widgets:

• options_select
• options_buttons
• options_onoff

blocked on #1705702: Provide a way to allow modules alter plugin definitions

Yup, that's because taxonomy module currently relies on hoo_field_widget_info_alter() to use option widgets (select / checkboxes) on taxo terms.
But without #1705702: Provide a way to allow modules alter plugin definitions, the hook runs only within HookDiscovery, and thus cannot affect widgets exposed in annotations. So, this works while option widgets stay in the old API.

Extracted and adapted the code from my old sandbox.

Also, added a workaround in LegacyDiscoveryDecorator to allow taxonomy fields to use option widgets - meaning #1705702: Provide a way to allow modules alter plugin definitions is not a blocker anymore.

(note : code lives in a new field-plugins-widgets-options-1751234 branch in the D8 Field API sandbox)

Fixed tests - the UI for "on/off checkbox" does not allow '0' as a 'on' value anymore, so the widget code does not support it either, but that was still the case being tested.

#1705702: Provide a way to allow modules alter plugin definitions is in 8.x.

@kotnik: right, but it was actually not a blocker anymore :-)

Rebased so it can be applied.

Also, hook_field_widget_info_alter() is never called, so taxonomy module fields have only autocomplete option. I think we should fix it here, just not sure how.

Also, hook_field_widget_info_alter() is never called, so taxonomy module fields have only autocomplete option

Yes, see #1785256-46: Widgets as Plugins. Should be fixed in a separate patch over there.

Yes, see #1785256-46: Widgets as Plugins. Should be fixed in a separate patch over there.

It's actually fixed in #1817180: Tests: hook_widget_info_alter() is not called anymore.

Rerolled since #1705702: Provide a way to allow modules alter plugin definitions is fixed.

Looks good. But i see taxonomy_field_widget_info_alter from #1751244: Convert taxonomy module widgets to Plugin system is also in this patch.

Small docs remark in OptionsButtonsWidget. It still says: Plugin implementation of the 'options_select' widget.

Also an extra remark. Maybe we should recheck the widget class names.

OptionsOnOffWidget can be OnOffWidget, or even CheckboxWidget. OptionsSelectWidget can be SelectWidget. OptionsButtonsWidget can be ButtonsWidget.

Fixed #14, let's not mix patches.

I'm not sure it we should rename the widgets themselves as text and number are using this naming pattern (Module+Scope+Widget), what I renamed is the "Base" thing out of the option widget as we're not using base for anything else (number or text)

#16: 1751234-16_options_widgets_plugins.patch queued for re-testing.

#16: 1751234-16_options_widgets_plugins.patch queued for re-testing.

Actually, agreed with zuuperman that the 'Options' prefix is useless. But the base class is an abstract base class, not an actual widget implementation, so It should have 'Base' as a suffix.

- Renamed classed to SelectWidget, ButtonsWidget, OnOffWidget and (back to) OptionsWidgetBase
- Adjusted phpdocs to the recent code conventions
- Moved options_field_widget_validate() (#element_validate callback) to a method in OptionsWidgetBase, now that it's possible. Done as a static method, so that $this (the widget object, including the $field and $instance structs) does not get serialized with the form.

No easy interdiff, due to the file renames, sorry.

Reroll + bump ?

Rerolled after #1862656: Move field type modules out of Field API module

Hmm, looks like I forgot to actually upload the patch last time :-/.

Anyway. Reroll after entity_reference.
& bump :-)

We need this for #1875992: Add EntityFormDisplay objects for entity forms - new patch with two minor changes - should be still green, so will RTBC when it comes back green.

Afaik, these are the last remaining uses of the LegacyWidget plugin, shouldn't we remove it in this patch?

Hmm good idea, I'll look at it this evening (unless someone beats me to it).

#25: options_widgets_plugins-1751234-25.patch queued for re-testing.

Note, we don't have to kill this off yet, #1796316: Convert Link/URL widgets / formatters to plugin system is still open

This one should be better.

Thanks for unbreaking this folks :-)

-    $info['options_select']['field_types'][] = 'taxonomy_term_reference';
+    $info['options_select']['field_types'][] = 'entity_reference';

So soon ? 'taxonomy_term_reference' field type still exists for now, right ?

Look where that change was made ;)

Er, doh, right...

You thought I was going mad from that taxonomy field deprecation issue, right? Well.. not yet! :P

Argh..

Ah, right. Which means there probably is a "use DiscoveryInterface" statement that can be dropped in OptionsWidgetBase...

That's right :)

Looks fine to me.

Rerolled after #1801356: Entity reference autocomplete using routes, entity_reference_menu() is gone - unrelated, but just broke hunk context.

#42: options_widgets_plugins-1751234-42.patch queued for re-testing.

We're going to postpone on #1735118: Convert Field API to CMI, that one is close, bear with us folks!

rerolled after #1735118: Convert Field API to CMI.

Live from the party, we need to check for the 'inheritdoc' comments - see comment on field API patch. Also applies to the other patches.

--> {@inheritdoc}

Damn, how did I miss that one ?

Reroll, but the test fails are beyond me.

- Can't reproduce the "Cannot modify header information" warning in Drupal\system\Tests\Common\AddFeedTest
- *Can* reproduce the fail in Drupal\taxonomy\Tests\TermTest, happens on those lines :

  $json = $this->drupalGet('taxonomy/autocomplete/taxonomy_' . $this->vocabulary->id(), array('query' => array('q' => $input)));
  $this->assertEqual($json, '{"\u0022' . $term_objects['term3']->name . '\u0022":"' . $term_objects['term3']->name . '"}'

The assertEqual fails because with patch, the $json string starts by a carriage return, which is not the case in HEAD.
I have absolutely no clue why the patch would have such an effect, though.

For now, made the test pass again by adding a trim($json), but something is weird here.

I think they both failed thanks to a nice newline in options module ;)

LOL. Headdesk time. There goes one hour of my life.
Thanks @swentel :-)

Reverting the trim() change in Drupal\taxonomy\Tests\TermTest, then (was interdiff #52)

Ok, let's do this.

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/widget/ButtonsWidget.phpundefined
@@ -0,0 +1,76 @@
+  protected function getEmptyOption() {
+    if (!$this->required && !$this->multiple) {
+      return self::OPTIONS_EMPTY_NONE;
+    }
+  }

We should be using late static binding so "if you subclass, you should be able to override the constant" (@timplunkett)

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.phpundefined
@@ -0,0 +1,206 @@
+  /**
+   * Returns the empty option to add to the list of options, if any.
+   *
+   * @return string|null
+   *   Either self::OPTIONS_EMPTY_NONE, self::OPTIONS_EMPTY_SELECT, or NULL.
+   */

We should be using late static binding...

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/widget/SelectWidget.phpundefined
@@ -0,0 +1,85 @@
+  protected function getEmptyOption() {
+    if ($this->multiple) {
+      // Multiple select: add a 'none' option for non-required fields.
+      if (!$this->required) {
+        return self::OPTIONS_EMPTY_NONE;
+      }
+    }
+    else {
+      // Single select: add a 'none' option for non-required fields,
+      // and a 'select a value' option for required fields that do not come
+      // with a value selected.
+      if (!$this->required) {
+        return self::OPTIONS_EMPTY_NONE;
+      }
+      if (!$this->has_value) {
+        return self::OPTIONS_EMPTY_SELECT;
+      }
+    }
+  }

We should be using late static binding...

Also in the complete theme_options_none function now looks like:

/**
 * Returns HTML for the label for the empty value for non-required options.
 *
 * The default theme will display 'N/A' for a radio list and '- None -' or
 * 'Select a value' for a select.
 *
 * @param $variables
 *   An associative array containing:
 *   - option: A string representing the option that should be displayed.
 *     Either 'option_none' or 'option_select'.
 *   - widget: The widget object requesting the option.
 *   - instance: The instance definition.
 *
 * @ingroup themeable
 */
function theme_options_none($variables) {
  $option = $variables['option'];
  $widget = $variables['widget'];

  $output = '';
  switch ($widget->getPluginId()) {
    case 'options_buttons':
      $output = t('N/A');
      break;

    case 'options_select':
      $output = ($option == 'option_none' ? t('- None -') : t('- Select a value -'));
      break;
  }

  return $output;
}

$output = ($option == 'option_none' ? t('- None -') : t('- Select a value -')); should use the constant from the class...

use Drupal\options\Plugin\field\widget\OptionsWidgetBase;
$output = ($option == OptionsWidgetBase::OPTIONS_EMPTY_NONE ? t('- None -') : t('- Select a value -'));

And then the function docblock needs to be updated to say...

 *   - option: A string representing the option that should be displayed.
 *     Either OptionsWidgetBase::OPTIONS_EMPTY_NONE or OptionsWidgetBase::OPTIONS_EMPTY_SELECT.

Right, fixed.

Bumping priority, this is now a blocker for #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets.

Committed 6fa5b69 and pushed to 8.x. Thanks!