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
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 |
Comment | File | Size | Author |
---|---|---|---|
#22 | custom_optionwidget-2426781-22.patch | 6.28 KB | jibran |
#22 | interdiff.txt | 3.92 KB | jibran |
Comments
Comment #1
jibranAnd patch
Comment #2
yched CreditAttribution: yched commentedThen 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 ?
Comment #3
jibranNice suggestion @yched. How about this?
Comment #4
yched CreditAttribution: yched commentedThanks @jibran, sorry for letting this slide :-/
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)
Comment #5
jibranHow about now?
Comment #7
jibranThank 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.
Comment #9
larowlanAny reason not to make this a constant?
We need a change notice for this, and the IS should indicate that its an API change
options_test_buttons?
options_test_select
Comment #10
jibranThanks for the review.
Comment #13
yched CreditAttribution: yched commentedThanks @jibran - looks good, aside from nitpicks in tests :
English : "Display *the* form" ;-)
+ missing "... and check that xxx" ?
Comment, no comment ? ;-)
Why are those needed ? Why can't we use the regular widgets directly ?
Comment #14
jibranThanks for the review. Now I know why @amateescu was so happy about no nitpick review :D
Comment #15
yched CreditAttribution: yched commentedThanks @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 ;-)
Comment #16
jibranYeah you are write no need to test removed code. So updated the patch.
Comment #17
jibranCan't do
NULL + array()
so updated the patch.Comment #18
jibranWhy check this?
Comment #20
jibranIgnore #18.
Comment #21
yched CreditAttribution: yched commentedLooking 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 ? ;-)
not true anymore :-)
Comment #22
jibranOh man another awesome review. Overall patch looks much cleaner now. Thanks for the incredible review.
Fixed both issues.
Comment #23
yched CreditAttribution: yched commentedSorry 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.
Comment #24
jibranAwesome.
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.
Comment #25
yched CreditAttribution: yched commentedThanks @jibran. Adjusted the change notice a bit.
Comment #26
alexpottCommitted 6a002ab and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
Comment #28
star-szrTiny followup to make the test less coupled to markup/classes: #2474107: Make OptionsWidgetsTest::testEmptyValue() care less about markup