Feedback on the Plugins API after converting Field API widgets

yched's picture

As I announced in the WSCCI Plugin Sprint Summary discussion, I started to play with the current state of the Plugins system ('wscci-plugins' branch in the WSCCI sandbox), and ported the "Field Widget" part of Field API. Code is in the 'wscci-plugins-fieldapi' branch, and is fully functionnal. The job was a no-brainer for a large part, which is probably a good sign about the concepts and current state of the code - yay to WSCCI and CTools folks ! Note : only text and option widgets have been ported to the new API for now, so this should be tested on 'page' nodes rather than 'article' (that ships with freetagging and file upload).

Field types, formatters, field storage APIs will most likely be moved to the Plugins system as well eventually, but are currently still unchanged from D7. Formatters and field storage should be as easy as widgets; field types will probably be the meaty part.

Overview :

  • The code exposes a 'field_widget' Plugin type. The dedicated WidgetFactory factory class instanciates the right implementation based on the 'widget_type' found in the $instance structure passed as an option to $mapper::getPluginInstance(). For now, that $instance struct is still the D7-style yummy array, and still comes from the {field_config_instance} table.
  • Implementations (aka 'widget types') implement WidgetInterface, and should subclass the abstract WidgetBase class, that provides a couple default behaviors.
  • As per the WSCCI Plugin Sprint Summary and the code in the 'wscci-plgins' branch, available Widget Implementations, previously exposed using an "info" hook (hook_field_widget_info()), is read from a "Configuration" store as proposed by CMI - currently a mocked up ConfigurationService class handing out Configuration items from an internal hardcoded array.

Below is feedback on issues / questions I found along the way - mostly pertains to exposing and collecting Plugin Implementations.

1. Discovering available Implementations

External code will repeatedly want to list available Implementations for a given plugin type - e.g. field_ui needs to list available widgets to populate the "pick a widget" dropdown select. The Plugin system needs to provide an API for that.

  • For now I added a getPluginImplementations($scope, $plugin_type) method in the mock ConfigurationService class (and pushed that upstream to the wscci-plugins branch), but that class is probably not the right spot.
  • There could be a similar getPluginTypes(), but I'm not sure there's a use case ("give me the list of all the available plugin types" ? what for ?)
  • If (quoted from the sprint summary) "the Mapper is the main point of contact between the outside world and the Plugins system", then should this kind of methods belong to the Mapper ?
  • Then, is Mapper still the right name ? It's not just about mapping to the right Plugin Instance, it's about getting info from the Plugins system. PluginService ? PluginInfo ?

Also, I suspect many consumers for those info will be old-school functional code, that doesn't get any service object through Dep Injection (e.g; form callbacks populating options in a select...). Wherever those methods live, we will need procedural shorthands.

2. Caching

In D7, each plugin-like subsystem based on info hooks needs to take care of caching the collected info. field_info_[field|widget|formatter]_types(), entity_get_info(), image_effect_definitions(), all implement their own static + persistent caching - often having to take care of caching by language because of entries ('label', 'description'...) that typically contain t()'ed strings.

For now, I assumed that the caching logic, if needed, would be handled by the Plugins system "somehow at some point", and in the wscci-plugins-fieldapi branch field_info_widget_types() is just a mere wrapper around a call to a Plugins method. _field_info_collate_types() is (will be) gone.

3. CMI files vs info hooks : altering; tracking provider module; scope of "configuration" land

D7's info hooks (used to expose available field types, widget types, image effects...) usually come with an alter hook.
Example : option widgets (select, checkboxes/radios) are generic, and do not define themselves as tied to any specific field type. Field types that want to use them use hook_field_widget_info_alter() to add themselves in the list of 'relevant field types for the widget' : see options_field_widget_info() and taxonomy_field_widget_info_alter().

In an IRC discussion earlier today, EclipseGc said that basically taxonomy_enable() would do something like :
$config = config('plugin.fields.widget.option_select');  $config->field_types[] = 'taxonomy_term_reference'; $config->save();

Some objections (came only after the discussion ended) :

  • That also means doing the opposite on taxonomy_disable(). Tedious. Plus, 'append a value to an array' is easy to revert (can also be messed up), but what about 'overwrite a value' ? how you put the original value back on disable ? The effects of an alter hook are automatically undone when the module is disabled, I don't think undoing everything that is currently done in alter hooks can be described easily in a series of procedural steps.
  • The case of several modules altering the same pieces is currently solved by module weight commanding hook execution order. With the approcah above, the last module enabled wins. Completely different story.
  • Also, it does seem that Ctools has hook_ctools_plugin_[pre|post]_alter() ?

Info hooks also let us naturally track which module provides which [field_type | widget type | image effect | ...]. It's not clear how we'll collect that info with the CMI based system (short of requiring each module to include a 'module' entry containing its own name in the "config" file that exposes the Plugin Implementation ?).
Not too critical for 'widget type' plugins, but the Field system definitely needs to know which module provides which field type.

More generally, I can't say I'm sold on the idea of treating Plugin Implementations as "configuration". The definition of a Plugin Implementations is about describing the code ("what's in there" - exposing a class and some properties specific to the code in its methods). Treating it as config is basically telling site admins "all of this is just some more stuff free for you to configure away, knock yourself out messing with it", and is a dangerous signal IMO. True, you can do as much damage with alter hooks currently, but at least that stays in module code. I also fear that having that info in the "configure" land rather than in core/contrib/custom modules code makes working on bug reports in the queues much more difficult ("Please post a zip containing the plugins.*.xml files in your config folder" ?).

Admittedly, for some part it might just be me being slow to make the mental switch (not a small switch, info hooks have been an established pattern for a while), yet the above concerns are real IMO.

Comments

The definition of a Plugin

yched's picture

The definition of a Plugin Implementations is about describing the code ("what's in there" - exposing a class and some properties specific to the code in its methods)

To be more specific, in the case of field widgets (using array style) :

'text_textarea' => array(
  'class_name' => '\Drupal\Plugin\Core\Field\TextareaWidget',
  'label' => 'Text area (multiple rows)',
  'field_types' => array('text'),    // The list of field types a widget can handle (determined by the set of 'columns'
                                     // they understand in the field values they receive)
  'settings' => array('rows' => 5),

  'multiple_values' => FALSE,        // Whether a widget can hold several values in one single copy of itself (e.g. : multiple select),
                                     // or should be repeated for each of the multiple values in the field (e.g. : textarea)
)

I have a hard time seeing "the list of field types a widget can handle" or "whether a widget can deal with several values" as being configuration just like "site name". Those properties reflect the code in the Implementation's methods. I don't mind a module changing them if it feels it needs to, but not a random site builder.

I guess 'multiple_values' could be turned into a isMultiple() method in the implementation class rather than a property in the definition "array", but that's not the case for 'field_types' (this takes part in the routing logic, we need it before we can instantiate actual objects)

That also means doing the

catch's picture

That also means doing the opposite on taxonomy_disable(). Tedious. Plus, 'append a value to an array' is easy to revert (can also be messed up), but what about 'overwrite a value' ? how you put the original value back on disable ? The effects of an alter hook are automatically undone when the module is disabled, I don't think undoing everything that is currently done in alter hooks can be described easily in a series of procedural steps.

I have been thinking about moving entity info out of an info hook and into configuration (did not look at making entities types or bundles into 'plugins' ye though), and this is one of the main barriers to doing that for me. It can be a mess doing things like this with info + alter (especially things disappearing when modules get disabled and their hooks don't run as we both know), but didn't get my head around extending configuration in a similar way. This feels like as much of a question for heyrocker as the folks working on plugins though?

Off the cuff thoughts

heyrocker's picture

I don't really have a good answer for this at the moment. I just sat here for five minutes reading this over and pondering it and came up with the following, which may or may not suck.

Any module can provide fields.widget.option_select..xml. For instance, taxonomy could provide fields.widget.option_select.taxonomy.xml which would contain

<field_types>
  <field_type>taxonomy_term_reference</field_type>
</field_types>

or something. When a module is enabled, this file is copied to the live config area. When a module is disabled, it is deleted (I don't have this in the spec right now, but it should be added.) When we want to build the list of field types for a widget, we grab fields.widget.option_select.* (which is in the API already) and iterate through the results to build the list, which is cached until next time config is reloaded.

This avoids a couple things I think are important to avoid. First off, we aren't altering anything. The config files remain canonical. Second off, it continues to keep the config files in discrete chunks managed by the modules that provide them while also preventing complicated merges of several config files into one (as you would need if you want to take all these and push them into one plugin.fields.widget.option_select.xml file. Need to revert to defaults? You have the module-provided defaults sitting right there still.

Is this realistic? The more I work with the config system, the more I realize that there is going to end up being a LOT of magic behind the naming conventions, which I have mixed feelings about.

A note on caching and performance

neclimdul's picture

re:2 At this point there are so many questions on how plugins will settle, caching is definitely a solve later problem. There are lots of points at which it makes sense that we /might/ cache right now but until we see how CMI shakes down and where plugins are being used in core(eg boostraping cache backends causes some interesting issues) we don't know how that will work. Its possible the answer is individual factories will be responsible for this and if you want a caching strategy you do it there.

Another more general buzzword filled not on performance, some initial testing with a similar model I was developing prior to the sprint showed just dropping this in for cache backend provided only a slight cost. Since I'm fairly sure that cost was mostly my goofy CMI-ish format loading I'm pretty optimistic that the shared code paths, consistent model, and opportunity for lighter footprint through autloading will offset that cost and hopefully improve performance and more importantly scaling in the long run.

Agreed, "we'll see about

yched's picture

Agreed, "we'll see about caching when the time comes" was what I assumed.

Although, this made me think of another consequence of moving Implementation info to Config files : you don't get to t() 'human readable' properties anymore. I.e no translation of Image effects or widget names in non-english admin UIs.

Field types as plugins

yched's picture

I pushed an initial bunch of work-in-progress (but working !) code for "Field types as plugins" in the wscci_plugins_fieldapi branch.

In short :
- Field and FieldInstance classes are introduced (with corresponding interfaces), to replace the $field and $instance 'definition arrays'.
Those just hold the "properties" (field name, label, cardinality, etc...), methods are mostly accessors (no setters yet).
A large part of the Field API code still relies on the old array-style definitions (notably all the CRUD cycle), so a compatibility layer is in place to move back and forth between the array versions and the object versions.
- I know, "$instance / field instance" is unfortunate wrt "Plugin Instance" - we were first ;-). We'll see about a possible renaming later on.
- The actual business logic is held in classes implementing FieldHandlerInterface. A 'field type' (text, file, date...) is a plugin implementation implementing that interface.
Composition pattern : $field->handler / $instance->handler (protected property, with a public ->handler() accessor)
- The handler is instantiated (through the FieldTypeFactory) when creating a Field object (Field::__construct() method).
- Widgets are another 'Plugin type', they are instanciated (WidgetFactory) on demand, when some code needs to use the widget for a field instance (FieldInstance::widget() method)
- With this kind of lazy instantiation of widgets (and in the future formatters), my goal is to avoid the need for the persistent caching of fully prepared fields and instances definition arrays we had to put in D7. Field and FieldInstance objects should be cheap to create, and widgets and formatters are prepared only if you need them in the page request.

As before, only text fields are ported for now ('page' nodes work, not 'article' nodes) - fields CRUD works ok with those field types.

We can has core patch? :)

webchick's picture

We can has core patch? :)

Dependency chain

yched's picture

The right timing for a core patch is one of the things I'm wondering about.
I could definitely use the testbot, + start discussions and nitpicking about the OO design (classes layout, which method lives where, etc...) and overall API impact sooner rather than later.

OTOH, the current code in ''wscci-plugins-fieldapi' relies on the API provided by the 'wscci-plugins' branch, which itself :
- is based on a mocked implementation of the CMI store, that serves info about available plugin types and plugin implementations
- is branched off the 'wscci' branch, which hosts the now deprecated approach for the Add unified context system to core patch. The current Plugins API therefore currently relies on $context / drupal_get_context() stuff (even though the "Field API as plugins" work itself doesn't really use that context layer currently)

So a core patch would carry some baggage right now. I guess the mock CMI store could be acceptable temporarily in a core patch, but the bringing the $context stuff seems over the top.

If the "$context" approach in the wscci branch is officially abandoned, maybe we should :
- rename / delete the branch (and possibly some of the numerous other branches in the sandbox as well ?)
- remove the $context stuff from the 'wscci-plugins' branch. I don't really use it in the Field API branch, but I don't know about EclipseGc's own 'wscci-plugins-blocks' branch. I also don't want to make a hold up on the 'wscci-plugins' branch, which is not my work :-)

@Crell, @EclipseGc, @neclimdul : thoughts ?

So, yeah I think the better

EclipseGc's picture

So, yeah I think the better answer right now is "play with the branch". This was a first pass at plugins in general and is totally based on Configuration stubbing, and the context approach that has been abandoned. I'm in the process of trying to allot the time to do a second pass at plugins utilizing symfony components and pimple... which should hopefully change individual plugins very minimally but will be a pretty significant change the closer to the foundation you get. This will STILL be based on configuration stubbing though I think since, as I understand it, the existing CMI patches are very limited in their scope, and probably not ready for us to use even in testing.

Scopewise, the only thing the

heyrocker's picture

Scopewise, the only thing the current CMI patches are missing is internationalization and the process to notify modules about updates. Other than that I consider it feature complete from an MVP perspective - its not everything I'd like it to be but if we shipped with just these things I wouldn't be unhappy. So if there is scope that you need that isn't in the system or the items above, speak now.

Talked with heyrocker in IRC,

EclipseGc's picture

Talked with heyrocker in IRC, and apparently I am under a false impression here. I'll try to include CMI in the branch I'm attempting to build for showing how this should all work together (and hopefully nailing down plugins in general).

Eclipse

Refactoring wscci-plugins is

yched's picture

Refactoring wscci-plugins is fine by me. I'll adapt the field API code downstream, I don't have that much of a surface contact (probably less than you do with blocks)

+1 on principle about symfony components and pimple. As a side note, I'm personally lurking at symfony's ParameterBag class for "array of settings" stuff, which Field API uses quite heavily.

Also on the "timing for OO field API core patch" question, I guess one point is when you guys feel like the Plugin API can handle being in the spotlight - and what is the most appropriate first patch to do so. An "OO field API" patch will have a massive amount of code besides just the Plugins API.

Yeah actually my current plan

neclimdul's picture

Yeah actually my current plan is to build a plugin patch/branch relying on nothing in CMI or other parts of WSCCI. Context and CMI can be bolted in to specific places as they mature.

Web Services and Context Core Initiative

Group organizers

Group notifications

This group offers an RSS feed. Or subscribe to these personalized, sitewide feeds: