This issue is the 1st step towards migrating Field API towards Plugins.
Attached patch starts with widgets. Next in line, in separate patches:
- Formatters - basically ready in #1785748: Field formatters as plugins (build on the widgets patch), but needs more performance testing
- Fields types
- Field storage backends - or not, depending on how #1497374: Switch from Field-based storage to Entity-based storage goes
Work happens in the D8 Field API sandbox, in the field-plugins-widgets-1785256 branch.
Work leading to a core patch was coordinated in #1742734: [META] Widgets as Plugins (now marked as dupe of this one)
"Field widget" Plugin type
The patch introduces the "Field widget" plugin type, managed by WidgetPluginManager. It uses annotations discovery, and a custom WidgetFactory factory (as advised by EclipseGc rather than the ReflectionFactory, for performance reasons).
The WidgetPluginManager object is accessed through field_get_plugin_manager('widget'), which acts as a static factory for now. This will move to the DIC at some point (after #1708692: Bundle services aren't available in the request that enables the module is solved).
WidgetInterface / WidgetBase
Actual widget classes implement WidgetInterface, and will most probably want to subclass WidgetBase.
WidgetBase methods hold the code that currently exists in field_default_*():
Old | New |
---|---|
field_default_form() | Widgetinterface::form() |
field_default_form_errors() | Widgetinterface::flagErrors() |
field_default_extract_form_values(), field_default_submit() | Widgetinterface::submit() |
WidgetInterface also includes methods replacing the old hook_field_widget_*() hooks:
Old | New |
---|---|
hook_field_widget_settings_form() | Widgetinterface::settingsForm() |
hook_field_widget_form() | Widgetinterface::formElement() |
hook_field_widget_error() | Widgetinterface::errorElement() |
(complex tricks with FAPI #callbacks) | (new) Widgetinterface::massageFormValues() |
Those methods are usually not called directly, but rather called by the methods above.
To make it clearer for widget developpers which methods need to be implemented vs. which are probably better off inherhited from WidgetBase,
the interface has actually been split between WidgetBaseInterface (wrapper methods) and WidgetInterface (methods you actually want to implement - extends WidgetBaseInterface). Credits @EclipseGc for the suggestion.
Object instanciation
Widget plugins are created from an $instance structure (since the $instance holds the definition of the widget). They are instanciated on demand (when a widget for that $instance is needed on the page) and are persisted through the request within the $instance itself. There is no real place other than the $instance where the Widget could be persisted, since a widget is "per instance".
This implies turning the $instance structure into a classed object.
- This is something we want to do long-term (move away from dumb-array-of-hell $field and $instance structs)
- This is something that we'll *need* to do anyway as part of #1735118: Convert Field API to CMI ($field and $instance as ConfigEntities)
- This very much makes sense for our case here.
Yet, to limitate the impact on the external facing API (switching $instance['property'] to $instance->property or $instance->getProperty() across all of core would be an unreviewable 500k+ patch), we're just turning $instance to a FieldInstance implements ArrayAccess
class here. Credits @sun for the idea and implementation.
FieldInstance::getWidget() calls the plugin system to instantiate the Widget plugin, then persists it for the rest of the request.
Similarly, there will be a FieldInstance::getFormatter($view_mode) when we do "formatters as plugins"
This also means that we cannot support hook_field_widget_properties_alter() changing the widget type on the fly on a per-entity basis. The hook still works, but does not receive an $entity in its $context parameter. We discussed this with @EclipseGc and @chx (who filed the original use case for "entity by entity" in D7), and this was deemed a reasonable regression - see #1762828-10: Figure out hook_field_widget_properties_alter() for details.
Additionally, _field_info_prepare_instance_widget() is gone. Widget properties are not pre-massaged on all existing instances as part of _field_info_collate_field() anymore, but only when actually creating a widget opbject. Same will happen to _field_info_prepare_instance_display() when we do "formatters as plugins".
BC layer
In order to:
- keep the patch size reasonable
- focus reviews on a limited amount of implementations
- crowdsource the rest of the conversions as potential "novice" followups, getting more people familiar with the API
this patch only converts a couple core widgets to the new API : text widgets, number widget, test widgets.
The other widgets (option widgets, image & file upload, taxo autocomplete, email) are kept fully functionnal by a LegacyWidget implementation, that takes care of proxying method calls to the old-style hooks. WidgetPluginManager discovers legacy widgets through a custom LegacyDiscoveryDecorator, that uses HookDiscovery to append the widget definitions still exposed through hook_field_widget_info(). Credits @EclipseGc for the idea.
API changes:
- hook_field_widget_info() is replaced by annotations. Some keys are renamed (white spaces replaced by underscores).
Note : we currently have no standard location to document the expected format for the annotations - i.e. the documentation currently contained in the phpdoc for hook_field_widget_info().
- hook_field_widget_settings_form(), hook_field_widget_form(), hook_field_widget_error() : replaced by WidgetInterface methods (see above)
- field_default_form(), field_default_form_errors(), field_default_extract_form_values(), field_default_submit() : replaced by WidgetInterface methods (see above)
- hook_field_widget_properties_alter(), hook_field_widget_properties_ENTITY_TYPE_alter() : the 'entity' and 'default' entries in $context are gone.
Related patches
Patch depends on:
#1751100: Bump minimum version of php required to 5.3.5 - our use of ArrayAccess requires PHP 5.3.4.
Testbots run 5.3.4 now, but the actual core requirement bump hasn't been committed yet
Patch currently includes:
#1778942: Discovery::getDefinition() / getDefinitions() : inconsistent return values for "no result" - to get rid of notices in tests
#1764380: Merge PluginSettingsInterface into ConfigurableInterface (a self contained version, that will be used within Field API plugins even if that issue goes nowhere)
Patch includes a workaround for:
#1764232: CacheDecorator provides no way to clear cached definitions - denoted by a @todo
The rest of the widget conversions will need:
#1705702: Provide a way to allow modules alter plugin definitions - option widgets need hook_field_widget_info_alter()
Benchmarks
Benchmarks will be tracked at #1781250: Run performance tests.
Credits
Code-wise: @yched, @Stalski, @zuuperman, @sun, @tstoeckler - with xtra special thanks to @Stalski for awesome help on various subtasks.
Many thanks @EclipseGc for numerous feedback & suggestions on Plugin API.
Comment | File | Size | Author |
---|---|---|---|
#32 | field-plugins-widgets-1785256-32.patch | 132.35 KB | yched |
#32 | interdiff.txt | 1.6 KB | yched |
#31 | field-plugins-widgets-1785256-31.patch | 132.36 KB | yched |
#31 | interdiff.txt | 2.75 KB | yched |
#29 | 1785256-interdiff-29.interdiff.txt | 639 bytes | larowlan |
Comments
Comment #1
yched CreditAttribution: yched commentedAnd the patch.
Comment #2
sunAwesome! :)
I just pushed a field-plugins-widgets-1785256-sun that's based on yours and includes some trivial/minor fixes.
Aside from that, I only have two questions:
Minor: This looks a bit weird -- submit is invoked before validate?
This is the only change I dislike in the new code. We're trying hard to remove all of the state information from the $form arrays by moving them into $form_state... and these changes here are in the opposite direction :( ;)
Comment #2.0
sunadd "field_default_submit() is gone too"
Comment #3
yched CreditAttribution: yched commentedThanks for the many phpdoc fixes, @sun !
The fact that we now hint string / int @params had escaped me.
I merged your commits in the main branch, and deleted field-plugins-widgets-1785256-sun.
The only change I left out was :
It seems the rest of core currently does "use ArrayAccess".
Will comment on your other points in a followup.
Comment #4
yched CreditAttribution: yched commentedAlso, since we moved the work to this new issue, I created field-plugins-widgets-1785256 to be the new main branch.
The old field-plugins-widgets-1742734 is gone.
Updated patch with sun's changes. Mostly phpdoc fixes - attaching the only code change as interdiff.
Comment #5
aspilicious CreditAttribution: aspilicious commentedWell we are changing the standard for stuff like ArrayAccess. It should be \ArrayAccess now. :p
Comment #6
yched CreditAttribution: yched commentedHah, ok then, switched back to "implemets \ArrayAccess".
Comment #7
yched CreditAttribution: yched commentedRe @sun #2 :
- field_test_entity_nested_form_validate() / "This looks a bit weird -- submit is invoked before validate?":
Yes, that's the current behavior of EntityFormController:
validate() calls field_attach_submit() (through buildEntity()) before field_attach_form_validate() (on that topic, shameless plug : #1768526: NodeFormController::validate() calls buildEntity() twice)
I therefore adjusted and simplified the flow of operations within field_attach_submit() and field_attach_form_validate() and the associated WidgetInterface::submit() method accordingly. More specifically, field_default_extract_form_values() and field_default_submit() have been merged into a single WidgetBase::submit().
But then the field_test_entity_nested_form() flow needs to be adjusted to a similar flow as EntityFormController's.
I guess field_test_entity_nested_form() (two entities in the edit same form) should move over to actually using EntityFormController, but I'm not sure whether EFC would support this specific case. I'd rather not do this within this patch though. field_test_entity_nested_form() currently implements its own flow, that's for another issue to change.
- field_ui_field_edit_form_submit() reading from $form['#custom_property']
That's how field_ui_field_edit_form(), and actually most field_ui's forms work currently. field_ui_field_edit_form() currently sets $form['#field'] and $form['#instance'] already. The patch just uses what is there in a different way
I agree that this should move to $form_state instead, but that's a more general cleanup that's outside the scope of this patch.
Comment #7.0
yched CreditAttribution: yched commentedAdded benchmark issue
Comment #7.1
yched CreditAttribution: yched commentedfix typo
Comment #7.2
yched CreditAttribution: yched commentedFixed branch name
Comment #8
sunThanks for the clarifications!
With the nitpicks out of the way, I reviewed this a second time in-depth, and I actually think it's ready to land.
There might be room for improving some details; e.g., renaming the WidgetInterface method names, and possibly also reducing the arguments to the formSingleElement() & other methods by providing the contextual data like $form and $form_state on the class instance, but those are really minor and can be happily investigated in follow-up issues.
All existing tests pass, and this is architecturally definitely much better than the current procedural callback mess.
Comment #9
yched CreditAttribution: yched commentedFixed incorrect namespace in a PHPdoc. No code impact.
Comment #10
swentel CreditAttribution: swentel commentedCan't be longer than 80 chars - please don't kill me :)
Comment #11
swentel CreditAttribution: swentel commentedI'll leave it RTBC though.
Comment #12
aspilicious CreditAttribution: aspilicious commentedReroll for that, don't give me credits thnx :)
Comment #13
yched CreditAttribution: yched commentedThks @aspilicious :-)
Which reminds me (it might be a bit lost in the longish OP) :
@core committers, see the "credits" section at the end for commit message credits.
Comment #13.0
yched CreditAttribution: yched commentedCredit sun about ArrayAccess
Comment #14
yched CreditAttribution: yched commentedUpdated patch :
- Doesn't include the upstream fixes to the Plugin API anymore - that are pending on 'needs tests' in #1778942: Discovery::getDefinition() / getDefinitions() : inconsistent return values for "no result"
- Instead, adds a self-included workaround by overriding PluginManagerBase::getDefinition() in WidgetPluginManager, with a @todo to remove that when upstream is fixed.
Comment #15
yched CreditAttribution: yched commentedUpdated patch, fixes phpdocs remarks made by @tim.plunkett in #1785748: Field formatters as plugins, but that in fact apply here.
Comment #16
Lars Toomre CreditAttribution: Lars Toomre commentedThere is an indention issue here. I believe the '-' associated with each option should line up under the 'An' of the first line and then any continuation lines should be indented a further two spaces. Also each line should be as close to 80 characters before wrapping. More details on list documentation are at http://drupal.org/node/1354#lists.
Also, I would ask that this patch add type hinting to all @param declarations it touches. Several are missing. Thanks.
Comment #17
webchickLars: It's helpful when submitting a review with only minor formatting changes to instead just make the changes yourself, rather than put an important patch through another Needs work / Needs review / RTBC cycle.
Comment #18
Lars Toomre CreditAttribution: Lars Toomre commentedSorry @webchick did not mean to change the status... However, given my problems getting a patch to be accepted, I am unable to generate a core patch at this time. I thought that I was trying to be helpful in actually reading a proposed patch and offering helpful comments. But whatever, ....
Comment #19
yched CreditAttribution: yched commentedFixed #16, and did another check on phpdocs.
Switching back to RTBC.
Comment #20
tim.plunkettWonderful, RTBC +1.
Sorry @Lars Toomre, I understand that you had good intentions.
Comment #21
webchickSorry; didn't mean to imply you were not being helpful! :) Just it would be even more helpful to fix minor things as you notice them. But if you're unable to do that, that's fine.
Comment #22
yched CreditAttribution: yched commentedMissing newline.
Comment #23
yched CreditAttribution: yched commentedCatch asked about performance impact, so copy pasting from #1781250: Run performance tests :
So, I ran my own benchmarks on node/N/edit between :
- 8.x
- only transforming $instance to ArrayAccess, no other code change
- widgets as plugins
With ab:
8.x - Time per request: 149.320 ms
ArrayAccess - Time per request: 151.462 ms, + 1.4 %
Widgets as plugins - Time per request: 156.840 ms, + 3.5 %
When turning to xhprof to check CPU diffs between 'ArrayAccess only' and 'widgets as plugins':
- field_attach_form takes 5.010ms longer to run (inclusive wall time)
This matches the difference above, which makes sense since all the changes happen below field_attach_form()
- ClassLoader::loadClass takes 5.711ms longer to run, with 7 more calls
So it seems:
- The ArrayAccess BC trick adds some overhead, but we know we want to get rif of it at some point
- After that, the overhead is mostly incurred by the autoloader.
Loading more classes is inherent to an "X as plugins" patch, and we know we have a slow autoloader already.
Comment #24
Lars Toomre CreditAttribution: Lars Toomre commentedThanks all. I did not mean to cause any problems. Maybe someday I will get a patch to (re)roll correctly and eventually accepted.
Minor nit in the interim... If this patch gets rerolled, the term 'id' in comments should be 'ID'. Other than that, I did not see any other issues in #19.
Thanks @yched for the reroll.
Comment #25
yched CreditAttribution: yched commentedFixed #24
@Lars Toomre: no worries, thanks for the reviews, those were definitely helpful.
Then, as webchick and tim said, pushing a large patch back from RTBC on code style fixes will likely raise a couple irritated reactions :-)
Best to reroll yourself if you can, or leave at RTBC and just point the errors for a next reroll, like you just did in #24.
Other than those "etiquette" points, please don't restrain yourself :-)
Comment #26
yched CreditAttribution: yched commentedAnother couple doc style fixes - and I'm done for tonight :-)
Comment #27
aspilicious CreditAttribution: aspilicious commentedWhite space
Comment #28
aspilicious CreditAttribution: aspilicious commenteddamnit!
Comment #29
larowlanFixes whitespace issues at 27
Comment #30
catchSo we need PHP version bumped to 5.3.5 for this. I just said I'd do that 1st October. However since this will only break Field UI on certain environments, not including the test bots, and the full requirements check will stop you upgrading Drupal at all, I'm happy to commit this before then (assuming it survives at RTBC for 24-48 hours), and if someone's running 5.3.3 and notices in the next couple of weeks that'll be extra motivation to update their local environment :p
Comment #30.0
catchAdded credits.
Comment #31
yched CreditAttribution: yched commentedAdded a couple missing docblocks on object properties.
Comment #32
yched CreditAttribution: yched commentedFixes typo / language.
Comment #33
xjmGave this another read this morning. Thank you for the excellent documentation on the interfaces. I noticed a few super minor issues with code style and docs grammar, and one method name that made me arch an eyebrow (
massageFormValues()
is kind of weird), but they're by no means worth kicking this back from RTBC. (If I don't get around to fixing them myself I'll file a followup issue.)Tagging to avoid commit conflicts. This patch is a big deal. :)
Comment #34
webchick#32: field-plugins-widgets-1785256-32.patch queued for re-testing.
Comment #35
webchickThis patch is really, really great. It gets rid of all kinds of messy pseudo-hook crap in favour of standardized plugin stuff, and starts introducing much-needed OO (rather than arrays of arrays of arrays) into Field API. +100.
I found a few small things ("informations" vs. "information" in one comment or another—sorry, I lost the Dreditor session), but nothing worth holding up the patch.
Committed and pushed to 8.x. YEAH!
We'll definitely need a change notice for this. If people are able to turn that around quickly, that'd be greatly appreciated, because we're bumping right up against the critical task limit with all of the outstanding change notices from recently-committed features.
Comment #36
webchickRemoving avoid commit conflicts tag.
Comment #37
swentel CreditAttribution: swentel commentedStarted change notice at http://drupal.org/node/1796000 - reviewing now, will set to needs review in a few moments.
Comment #38
podarokFirst try for Change Notice
http://drupal.org/node/1795998
Due to huge changes into API, Change record can`t describe all examples in API changing D7->D8
As for me - my vote for create follow-up issue in http://dgo.to/examples for this and #1785748: Field formatters as plugins issues with a complicated example
Comment #39
podarok#37 looks good as example only change ;)
Do wee need merge this change records?
Comment #40
swentel CreditAttribution: swentel commentedI'm not sure whether we need all the background information in the change notice - other change notices seem much smaller too and only list API changes. I'd stick with that and a basic example.
It would be great to get an example in the examples project indeed so we can link to that from the change notice.
Comment #41
yched CreditAttribution: yched commentedYay !
Hard for me to help on the change notice right now (web access mostly from my phone).
1796000 looks good, would just need a word about the changes in hook_field_widget_properties_alter().
Comment #42
swentel CreditAttribution: swentel commentedAdded 'hook_field_widget_properties_alter() and hook_field_widget_properties_ENTITY_TYPE_alter() still exist but the 'entity' and 'default' entries in $context are gone.' to the change notice.
Comment #43
Anonymous (not verified) CreditAttribution: Anonymous commentedI added an issue to the Examples for Developers module, #1796606: D8 Port: field_example.
Comment #44
podarok#42 makes http://drupal.org/node/1796000 good
and with follow-up #1796606: D8 Port: field_example looks like this issue RTBC
Comment #45
andypost1796000 is enough, much better then we have mostly
Comment #46
yched CreditAttribution: yched commentedNote: #1705702: Provide a way to allow modules alter plugin definitions got in, which means HookDiscovery no longer runs the hook_field_widget_info_alter().
AlterDecorator needs to be added to the discovery used in WidgetPluginManager.
I'm not able to roll patches right now - anyone up for it ?
Comment #48
tstoecklerOpened #1817180: Tests: hook_widget_info_alter() is not called anymore.
Comment #49
tstoecklerNote that that was actually a cross-post, but leaving open for potential reviewers to notice the new issue. Feel free to close, if you deem that appropriate.
Comment #51
amateescu CreditAttribution: amateescu commentedSorry for spamming an old thread, but core finally realized that we need an interface for configurable plugins, so I'll just link this issue where a part of
Drupal\field\Plugin\PluginSettingsInterface
was introduced asDrupal/Component/Plugin/ConfigurablePluginInterface
: #2033383: Provide a default plugin bagComment #51.0
amateescu CreditAttribution: amateescu commentedAdds a word about "formatters as plugins"