Custom OptionWidget have no empty option label

Then if we follow this logic we should remove the switch/case in OptionsWidgetBase::getOptions() and simply just call $this->getEmptyLabel() ?
The code for 'options_select' and 'options_buttons' would move into the respective implementations of the method ?

Thanks @jibran, sorry for letting this slide :-/

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsWidgetBase.php
@@ -130,17 +130,8 @@ protected function getOptions(FieldableEntityInterface $entity) {
-      if ($empty_option = $this->getEmptyOption()) {
...
+      if ($this->getEmptyOption()) {

Looks wrong that we now ditch the result of getEmptyOptions() ?

Also, having two separate methods for "get the empty value" / "get the label for the empty value" feels a bit tedious now.
Maybe we should use the same method to return a "value => label" ?
(no definite opinion here, just a thought)

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsSelectWidget.php
   @@ -74,10 +74,10 @@ protected function getEmptyOption() {
   +        return ['_none' => t('- None -')];
   

   Any reason not to make this a constant?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsWidgetBase.php
   @@ -27,16 +27,6 @@
   -  const OPTIONS_EMPTY_NONE = 'option_none';
   ...
   -  const OPTIONS_EMPTY_SELECT = 'option_select';
   

   We need a change notice for this, and the IS should indicate that its an API change

3. +++ b/core/modules/options/tests/options_test/src/Plugin/Field/FieldWidget/OptionsTestButtonsWidget.php
   @@ -0,0 +1,27 @@
   + * Plugin implementation of the 'options_buttons' widget.
   

   options_test_buttons?

4. +++ b/core/modules/options/tests/options_test/src/Plugin/Field/FieldWidget/OptionsTestSelectWidget.php
   @@ -0,0 +1,26 @@
   + * Plugin implementation of the 'options_select' widget.
   

   options_test_select

Thanks @jibran - looks good, aside from nitpicks in tests :

1. +++ b/core/modules/options/src/Tests/OptionsWidgetsTest.php
   @@ -450,4 +450,45 @@ function testSelectListMultiple() {
   +    // Display form.
   +    $this->drupalGet('entity_test/manage/' . $entity->id());
   +    $this->assertTrue($this->xpath('//div[@id=:id]//div[@class=:class]//input[@value=:value]', array(':id' => 'edit-card-1', ':class' => 'form-item form-type-radio form-item-card-1', ':value' => '_none')), 'A test radio button has a "None" choice.');
   +    $this->assertTrue($this->xpath('//div[@id=:id]//div[@class=:class]//label[@for=:for and text()=:label]', array(':id' => 'edit-card-1', ':class' => 'form-item form-type-radio form-item-card-1', ':for' => 'edit-card-1-none', ':label' => 'N/A')), 'A test radio button has a "N/A" choice.');
   

   English : "Display *the* form" ;-)
   + missing "... and check that xxx" ?

2. +++ b/core/modules/options/src/Tests/OptionsWidgetsTest.php
   @@ -450,4 +450,45 @@ function testSelectListMultiple() {
   +    //
   +    entity_get_form_display('entity_test', 'entity_test', 'default')
   +      ->setComponent($this->card1->getName(), array(
   +        'type' => 'options_test_select',
   +      ))
   +      ->save();
   

   Comment, no comment ? ;-)

3. +++ b/core/modules/options/tests/options_test/src/Plugin/Field/FieldWidget/OptionsTestButtonsWidget.php
   @@ -0,0 +1,27 @@
   +class OptionsTestButtonsWidget extends OptionsButtonsWidget { }
   
   +++ b/core/modules/options/tests/options_test/src/Plugin/Field/FieldWidget/OptionsTestSelectWidget.php
   @@ -0,0 +1,26 @@
   +class OptionsTestSelectWidget extends OptionsSelectWidget { }
   

   Why are those needed ? Why can't we use the regular widgets directly ?

Thanks @jibran

Regarding 3, the thing is that those classes look really pointless as is, and are just plain confusing unless you know/remember that there used to be some hardcoded logic on plugin ids at some point. Testing that something that once was there is now gone is always a bit weird...

I'd vote to remove them, but if they stay they should at least have some explanatory comment ?

"no nitpick review" - I know :-p. I guess some day I will eventually take that hint ;-)

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsWidgetBase.php
   @@ -214,10 +195,11 @@ protected function sanitizeLabel(&$label) {
   +   * @return array
   +   *   Either '_none' as a key and value as a label of the empty option, or an
   +   *   empty array if widget does not support empty option.
       */
      protected function getEmptyOption() { }
   

   Looking at this now, "you need to return '_none' as a key" is weird. If the key is necessarily '_none', why ask implementations to return it, they can just return the label string, or return nothing for "no empty option" ? If you know what the answer *has* to be, then don't ask me ? ;-)

2. +++ b/core/modules/options/src/Tests/OptionsWidgetsTest.php
   @@ -450,4 +450,47 @@ function testSelectListMultiple() {
   +   * Tests the 'options_test_select' and 'options_test_button' widget for empty value.
   

   not true anymore :-)

Sorry for the delay, looks great !

RTBC on my side - will probably need a beta evaluation summary, though.

The patch changes a method in OptionWidgetBase, so theoretically breaks contrib subclasses. *But*, the whole point of the patch is that because of the hardcoded list of plugin ids in OptionWidgetBase (my bad, seemed a good idea 3 years ago :-)), it's currently not really possible to subclass it into new widgets it doesn't know about, so the BC break / disruption is only theoretical.

Thanks @jibran. Adjusted the change notice a bit.

Committed 6a002ab and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

Tiny followup to make the test less coupled to markup/classes: #2474107: Make OptionsWidgetsTest::testEmptyValue() care less about markup