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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Status: Active » Needs review
FileSize
132.41 KB

And the patch.

sun’s picture

Issue tags: +Field system, +Plugin system

Awesome! :)

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:

+++ b/core/modules/field/tests/modules/field_test/field_test.entity.inc
@@ -337,9 +337,11 @@ function field_test_entity_nested_form($form, &$form_state, $entity_1, $entity_2
 function field_test_entity_nested_form_validate($form, &$form_state) {
   $entity_1 = entity_create('test_entity', $form_state['values']);
+  field_attach_submit('test_entity', $entity_1, $form, $form_state);
   field_attach_form_validate('test_entity', $entity_1, $form, $form_state);
 
   $entity_2 = entity_create('test_entity', $form_state['values']['entity_2']);
+  field_attach_submit('test_entity', $entity_2, $form['entity_2'], $form_state);
   field_attach_form_validate('test_entity', $entity_2, $form['entity_2'], $form_state);
 }

Minor: This looks a bit weird -- submit is invoked before validate?

+++ b/core/modules/field_ui/field_ui.admin.inc
@@ -2085,12 +2099,12 @@ function field_ui_field_edit_form_validate($form, &$form_state) {
 function field_ui_field_edit_form_submit($form, &$form_state) {
-  $instance = $form_state['values']['instance'];
-  $field = $form_state['values']['field'];
+  $instance = $form['#instance'];
+  $field = $form['#field'];
+  $entity = $form['#entity'];

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

sun’s picture

Issue summary: View changes

add "field_default_submit() is gone too"

yched’s picture

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

- use ArrayAccess;

- class FieldInstance extends ArrayAccess {
+ class FieldInstance extends \ArrayAccess {

It seems the rest of core currently does "use ArrayAccess".

Will comment on your other points in a followup.

yched’s picture

Also, 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.

aspilicious’s picture

Well we are changing the standard for stuff like ArrayAccess. It should be \ArrayAccess now. :p

yched’s picture

Hah, ok then, switched back to "implemets \ArrayAccess".

yched’s picture

Re @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.

yched’s picture

Issue summary: View changes

Added benchmark issue

yched’s picture

Issue summary: View changes

fix typo

yched’s picture

Issue summary: View changes

Fixed branch name

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks 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.

yched’s picture

Fixed incorrect namespace in a PHPdoc. No code impact.

swentel’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/field/field.api.phpundefined
@@ -692,7 +692,7 @@
+ * Drupal\field\Plugin\Type\Widget\WidgetPluginManager class. A widget is implemented

Can't be longer than 80 chars - please don't kill me :)

swentel’s picture

Status: Needs work » Reviewed & tested by the community

I'll leave it RTBC though.

aspilicious’s picture

Reroll for that, don't give me credits thnx :)

yched’s picture

Thks @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.

yched’s picture

Issue summary: View changes

Credit sun about ArrayAccess

yched’s picture

Updated 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.

yched’s picture

Updated patch, fixes phpdocs remarks made by @tim.plunkett in #1785748: Field formatters as plugins, but that in fact apply here.

Lars Toomre’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/field/field.attach.incundefined
@@ -108,6 +108,95 @@ const FIELD_STORAGE_INSERT = 'insert';
+ * @param array $options
+ *   An associative array of additional options, with the following keys:
+ *    - field_name: The name of the field whose operation should be invoked. By
+ *    default, the operation is invoked on all the fields in the entity's
+ *    bundle. NOTE: This option is not compatible with the 'deleted' option;
+ *    the 'field_id' option should be used instead.
+ *    - field_id: The id of the field whose operation should be invoked. By
+ *    default, the operation is invoked on all the fields in the entity's'
+ *    bundles.

There 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.

webchick’s picture

Lars: 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.

Lars Toomre’s picture

Status: Needs work » Reviewed & tested by the community

Sorry @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, ....

yched’s picture

Fixed #16, and did another check on phpdocs.
Switching back to RTBC.

tim.plunkett’s picture

Wonderful, RTBC +1.

Sorry @Lars Toomre, I understand that you had good intentions.

webchick’s picture

Sorry; 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.

yched’s picture

Missing newline.

yched’s picture

Catch 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.

Lars Toomre’s picture

Thanks 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.

yched’s picture

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

yched’s picture

Another couple doc style fixes - and I'm done for tonight :-)

aspilicious’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/field/lib/Drupal/field/FieldInstance.phpundefined
@@ -15,6 +15,8 @@
+   * ¶

White space

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

damnit!

larowlan’s picture

Fixes whitespace issues at 27

catch’s picture

So 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

catch’s picture

Issue summary: View changes

Added credits.

yched’s picture

Added a couple missing docblocks on object properties.

yched’s picture

Fixes typo / language.

xjm’s picture

Issue tags: +Avoid commit conflicts

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

webchick’s picture

webchick’s picture

Title: Widgets as Plugins » Change notice: Widgets as Plugins
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

This 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.

webchick’s picture

Issue tags: -Avoid commit conflicts

Removing avoid commit conflicts tag.

swentel’s picture

Started change notice at http://drupal.org/node/1796000 - reviewing now, will set to needs review in a few moments.

podarok’s picture

Status: Active » Needs review

First 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

podarok’s picture

#37 looks good as example only change ;)
Do wee need merge this change records?

swentel’s picture

I'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.

yched’s picture

Yay !

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().

swentel’s picture

Added '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.

Anonymous’s picture

I added an issue to the Examples for Developers module, #1796606: D8 Port: field_example.

podarok’s picture

Status: Needs review » Reviewed & tested by the community

#42 makes http://drupal.org/node/1796000 good
and with follow-up #1796606: D8 Port: field_example looks like this issue RTBC

andypost’s picture

Title: Change notice: Widgets as Plugins » Widgets as Plugins
Priority: Critical » Normal
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record

1796000 is enough, much better then we have mostly

yched’s picture

Note: #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 ?

Status: Fixed » Closed (fixed)

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

tstoeckler’s picture

Status: Closed (fixed) » Fixed
tstoeckler’s picture

Note 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.

Status: Fixed » Closed (fixed)

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

amateescu’s picture

Sorry 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 as Drupal/Component/Plugin/ConfigurablePluginInterface: #2033383: Provide a default plugin bag

amateescu’s picture

Issue summary: View changes

Adds a word about "formatters as plugins"