Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This is one of the sub-tasks of #1742734: [META] Widgets as Plugins to convert all core widgets. This is currently explicitly postponed.
Comment | File | Size | Author |
---|---|---|---|
#58 | options_widgets_plugins-1751234-58.patch | 30.68 KB | yched |
#58 | interdiff.txt | 3.41 KB | yched |
#55 | options_widgets_plugins-1751234-55.patch | 30.06 KB | yched |
#53 | Screen Shot 2013-04-15 at 21.41.22.png | 31.74 KB | swentel |
#53 | options_widgets_plugins-1751234-53.patch | 31.3 KB | swentel |
Comments
Comment #1
yched CreditAttribution: yched commentedNote : the porting is basically done already in a sandbox of mine.
Don't start this without pinging me.
Comment #2
pcambraThese are the current widgets:
Comment #3
swentel CreditAttribution: swentel commentedblocked on #1705702: Provide a way to allow modules alter plugin definitions
Comment #4
yched CreditAttribution: yched commentedYup, 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.
Comment #5
yched CreditAttribution: yched commentedExtracted 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)
Comment #7
yched CreditAttribution: yched commentedFixed 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.
Comment #8
kotnik CreditAttribution: kotnik commented#1705702: Provide a way to allow modules alter plugin definitions is in 8.x.
Comment #9
yched CreditAttribution: yched commented@kotnik: right, but it was actually not a blocker anymore :-)
Comment #10
kotnik CreditAttribution: kotnik commentedRebased 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.
Comment #11
yched CreditAttribution: yched commentedYes, see #1785256-46: Widgets as Plugins. Should be fixed in a separate patch over there.
Comment #12
kotnik CreditAttribution: kotnik commentedIt's actually fixed in #1817180: Tests: hook_widget_info_alter() is not called anymore.
Comment #13
webflo CreditAttribution: webflo commentedRerolled since #1705702: Provide a way to allow modules alter plugin definitions is fixed.
Comment #14
nils.destoop CreditAttribution: nils.destoop commentedLooks 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.
Comment #15
nils.destoop CreditAttribution: nils.destoop commentedAlso 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.
Comment #16
pcambraFixed #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)
Comment #18
webflo CreditAttribution: webflo commented#16: 1751234-16_options_widgets_plugins.patch queued for re-testing.
Comment #20
nils.destoop CreditAttribution: nils.destoop commented#16: 1751234-16_options_widgets_plugins.patch queued for re-testing.
Comment #21
yched CreditAttribution: yched commentedActually, 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.
Comment #22
yched CreditAttribution: yched commentedReroll + bump ?
Comment #23
yched CreditAttribution: yched commentedRerolled after #1862656: Move field type modules out of Field API module
Comment #24
yched CreditAttribution: yched commentedHmm, looks like I forgot to actually upload the patch last time :-/.
Anyway. Reroll after entity_reference.
& bump :-)
Comment #25
swentel CreditAttribution: swentel commentedWe 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.
Comment #27
amateescu CreditAttribution: amateescu commentedAfaik, these are the last remaining uses of the LegacyWidget plugin, shouldn't we remove it in this patch?
Comment #28
swentel CreditAttribution: swentel commentedHmm good idea, I'll look at it this evening (unless someone beats me to it).
Comment #29
swentel CreditAttribution: swentel commented#25: options_widgets_plugins-1751234-25.patch queued for re-testing.
Comment #31
swentel CreditAttribution: swentel commentedNote, we don't have to kill this off yet, #1796316: Convert Link/URL widgets / formatters to plugin system is still open
Comment #32
amateescu CreditAttribution: amateescu commentedThis one should be better.
Comment #33
yched CreditAttribution: yched commentedThanks for unbreaking this folks :-)
So soon ? 'taxonomy_term_reference' field type still exists for now, right ?
Comment #34
amateescu CreditAttribution: amateescu commentedLook where that change was made ;)
Comment #35
yched CreditAttribution: yched commentedEr, doh, right...
Comment #36
amateescu CreditAttribution: amateescu commentedYou thought I was going mad from that taxonomy field deprecation issue, right? Well.. not yet! :P
Comment #38
amateescu CreditAttribution: amateescu commentedArgh..
Comment #39
yched CreditAttribution: yched commentedAh, right. Which means there probably is a "use DiscoveryInterface" statement that can be dropped in OptionsWidgetBase...
Comment #40
amateescu CreditAttribution: amateescu commentedThat's right :)
Comment #41
swentel CreditAttribution: swentel commentedLooks fine to me.
Comment #42
yched CreditAttribution: yched commentedRerolled after #1801356: Entity reference autocomplete using routes, entity_reference_menu() is gone - unrelated, but just broke hunk context.
Comment #43
xjm#42: options_widgets_plugins-1751234-42.patch queued for re-testing.
Comment #44
swentel CreditAttribution: swentel commentedWe're going to postpone on #1735118: Convert Field API to CMI, that one is close, bear with us folks!
Comment #45
yched CreditAttribution: yched commentedrerolled after #1735118: Convert Field API to CMI.
Comment #46
swentel CreditAttribution: swentel commentedLive from the party, we need to check for the 'inheritdoc' comments - see comment on field API patch. Also applies to the other patches.
Comment #48
yched CreditAttribution: yched commented--> {@inheritdoc}
Comment #50
yched CreditAttribution: yched commentedDamn, how did I miss that one ?
Comment #52
yched CreditAttribution: yched commentedReroll, 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 :
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.
Comment #53
swentel CreditAttribution: swentel commentedI think they both failed thanks to a nice newline in options module ;)
Comment #54
yched CreditAttribution: yched commentedLOL. Headdesk time. There goes one hour of my life.
Thanks @swentel :-)
Comment #55
yched CreditAttribution: yched commentedReverting the trim() change in Drupal\taxonomy\Tests\TermTest, then (was interdiff #52)
Comment #56
swentel CreditAttribution: swentel commentedOk, let's do this.
Comment #57
alexpottWe should be using late static binding so "if you subclass, you should be able to override the constant" (@timplunkett)
We should be using late static binding...
We should be using late static binding...
Also in the complete theme_options_none function now looks like:
$output = ($option == 'option_none' ? t('- None -') : t('- Select a value -'));
should use the constant from the class...And then the function docblock needs to be updated to say...
Comment #58
yched CreditAttribution: yched commentedRight, fixed.
Comment #59
yched CreditAttribution: yched commentedBumping priority, this is now a blocker for #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets.
Comment #60
alexpottCommitted 6fa5b69 and pushed to 8.x. Thanks!
Comment #61
alexpott