Problem/Motivation

#1898434: Remove theme_options_none, use an alter hook instead for changing empty option label added this switch case in OptionsWidgetBase::getOptions()

// Add an empty option if the widget needs one.
      if ($empty_option = $this->getEmptyOption()) {
        switch ($this->getPluginId()) {
          case 'options_buttons':
            $label = t('N/A');
            break;

          case 'options_select':
            $label = ($empty_option == static::OPTIONS_EMPTY_NONE ? t('- None -') : t('- Select a value -'));
            break;
        }

        $options = array('_none' => $label) + $options;
      }

See there is no default option so if one has to create a new OptionWidget one will receive

Message Group Filename Line Function Status
Undefined variable: label Notice OptionsWidgetBase.php 144 Drupal\Core\Field\Plugin\Field\FieldWidget\OptionsWidgetBase->getOptions()
Undefined variable: label Notice OptionsWidgetBase.php 144 Drupal\Core\Field\Plugin\Field\FieldWidget\OptionsWidgetBase->getOptions()
Undefined variable: label Notice OptionsWidgetBase.php 144 Drupal\Core\Field\Plugin\Field\FieldWidget\OptionsWidgetBase->getOptions()
Undefined variable: label Notice OptionsWidgetBase.php 144 Drupal\Core\Field\Plugin\Field\FieldWidget\OptionsWidgetBase->getOptions()

which can be seen here https://qa.drupal.org/pifr/test/960293 #2413641-6: Add OptionWidgets for single value target type DER fields
One can re-write the complete function in custom OptionWidget but OptionsWidgetBase should have default case.

Proposed resolution

Add getEmptyLabel() and remove getEmptyOption().

Remaining tasks

Commit it.

User interface changes

Custom OptionWidget have empty option label.

API changes

It adds new method OptionsWidgetBase::getEmptyLabel() and removes getEmptyOption()
It removes OptionsWidgetBase::OPTIONS_EMPTY_NONE and OptionsWidgetBase::OPTIONS_EMPTY_SELECT as well.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because custom OptionWidget can't have empty option label
Issue priority Major because contrib is affected by this.
Prioritized changes This is a bug fix
Disruption Disruptive for contributed and custom modules because it will removes the existing method and adds new one. See #23
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jibran’s picture

And patch

yched’s picture

Status: Needs review » Needs work

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 ?

jibran’s picture

Status: Needs work » Needs review
FileSize
2.24 KB
2.45 KB

Nice suggestion @yched. How about this?

yched’s picture

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)

jibran’s picture

How about now?

Status: Needs review » Needs work

The last submitted patch, 5: custom_optionwidget-2426781-5.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
7.2 KB
4.23 KB
7.83 KB

Thank you for the review. Better late then never :)
Here is a test-only patch showing the bug in head. Fixed the fails in #5 because of stupid mistake.

The last submitted patch, 7: custom_optionwidget-2426781-7-test-only.patch, failed testing.

larowlan’s picture

  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

jibran’s picture

Thanks for the review.

  1. Let's fix that in #2448545: Convert '_none' option to a constant, deprecate form_select_options() and form_get_options() and move them to a new class.
  2. Can I get a +1 for the current approach first then I'll create the change notice?
  3. Fixed.
  4. Fixed.

Status: Needs review » Needs work

The last submitted patch, 10: custom_optionwidget-2426781-10.patch, failed testing.

Status: Needs work » Needs review
yched’s picture

Status: Needs review » Needs work

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 ?

jibran’s picture

Status: Needs work » Needs review
FileSize
2.01 KB
8.04 KB

Thanks for the review. Now I know why @amateescu was so happy about no nitpick review :D

  1. Updated both the comments.
  2. Added comments.
  3. This was the exact issue I want to fix here that custom OptionWidgets don't have empty option label so these are needed to show the fails.
yched’s picture

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 ;-)

jibran’s picture

FileSize
2.91 KB
6.03 KB

Yeah you are write no need to test removed code. So updated the patch.

jibran’s picture

FileSize
1.19 KB
6.22 KB
+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsWidgetBase.php
@@ -131,17 +121,7 @@ protected function getOptions(FieldableEntityInterface $entity) {
+        $options = $empty_option + $options;

Can't do NULL + array() so updated the patch.

jibran’s picture

FileSize
1.22 KB
6.34 KB
+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsWidgetBase.php
@@ -120,7 +120,8 @@ protected function getOptions(FieldableEntityInterface $entity) {
+      if (!empty($empty_option)) {

Why check this?

Status: Needs review » Needs work

The last submitted patch, 18: custom_optionwidget-2426781-18.patch, failed testing.

jibran’s picture

Ignore #18.

yched’s picture

  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 :-)

jibran’s picture

Status: Needs work » Needs review
FileSize
3.92 KB
6.28 KB

Oh man another awesome review. Overall patch looks much cleaner now. Thanks for the incredible review.
Fixed both issues.

yched’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs beta evaluation

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.

jibran’s picture

Issue summary: View changes
Issue tags: -Needs beta evaluation

Awesome.
Added beta evaluation and updated purpose resolution to the recent approach in the patch. Also added draft change notice https://www.drupal.org/node/2468669 as per #9.

yched’s picture

Thanks @jibran. Adjusted the change notice a bit.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 6a002ab on 8.0.x
    Issue #2426781 by jibran: Custom OptionWidget have no empty option label
    
star-szr’s picture

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.