The various "X as plugins" efforts that are or will be started here and there will for a very large number need to handle the notion of 'settings'. Within core only, this applies to blocksTNG, field widgets, field formatters, field types, image styles, image toolkits, text filters, ... you name it.

Most plugin-like subsystems in core or contrib have their own formulation currently, with widely different or incomplete or similar-but-subtly-varying implementations. I see much value in trying to come up with some a 'standard' core way of handling this, especially given some aspects of it are not so trivial.

I pitched the idea to EclipseGc during his SCOTCH talk at DCMunich, the idea appealed to him.

I see a very common use case on top of the following pattern.

A Plugin (of a given plugin type) has :
- A set of properties that is common to all plugins of this type. They're what the given subsystem expects just to be able to work with a various and "unknown" collection of different implementations. Typically, they're the parameters that the factory used by the plugin type passes to instanciated plugin objects.
- A collection of 'settings', which are specific to each plugin implementation.
'image_crop' has 'width', 'length'; 'image_rotate' has 'rotation'; 'ilmage_grayscale' has none

Handling those 'settings' in a different namespace than the 'fixed properties' is important, because if you don't, you'll find your subsystem cannot add a new 'fixed property' and be sure it won't clash with a setting name used by a random 3rd party plugin implementation (and requiring implementations to prefix by module name is really cumbersome/ugly when it comes to settings names).

The collection of 'settings' is usually exposed in a 'settings' entry in the 'plugin definition format' (whether hook- or annotation-based) defined by the plugin type, and come with metadata :
- Machine name
- Default value
Providing default values lets you populate a 'create' form, and also ensures you can add more settings in a point release without littering your code with isset()s to handle existing definitions where the setting didn't exist yet. But filling in default values upfront on any instanciated object can be costly.
- Translatable flag
This will be needed for "CMI / i18n" - see #1648930: Introduce configuration schema and use for translation
- ... ?

Files: 
CommentFileSizeAuthor
#3 plugin_settings-1764380-3.patch4.15 KByched
PASSED: [[SimpleTest]]: [MySQL] 59,375 pass(es). View
#1 plugin_settings-1764380-1.patch4.16 KByched
PASSED: [[SimpleTest]]: [MySQL] 40,381 pass(es). View

Comments

yched’s picture

FileSize
4.16 KB
PASSED: [[SimpleTest]]: [MySQL] 40,381 pass(es). View

Here's an initial proposal, based on what we currently use in #1742734: [META] Widgets as Plugins.

sun’s picture

Status: Active » Needs work
Issue tags: +Configuration system
+++ b/core/lib/Drupal/Component/Plugin/PluginSettingsBase.php
@@ -0,0 +1,83 @@
+  public function getSettingsMatadata($key) {

Typo in "Matadata".

In general though, this somehow looks very similar to the Config class...? ;) I wonder what the actual plan for "plugin settings in config" is? :)

Tentatively tagging...

yched’s picture

Status: Needs work » Needs review
FileSize
4.15 KB
PASSED: [[SimpleTest]]: [MySQL] 59,375 pass(es). View

Fixed typo.

yched’s picture

Title: Base class for management of 'settings' » Base class for management of Plugin 'settings'

more accurate title

tim.plunkett’s picture

Issue tags: +VDC

Tagging, this sounds helpful.

sun’s picture

+++ b/core/lib/Drupal/Component/Plugin/PluginSettingsBase.php
@@ -0,0 +1,82 @@
+abstract class PluginSettingsBase extends PluginBase implements PluginSettingsInterface {

So... I wonder:

Wouldn't the "proper" class name for this actually be ConfigurablePluginBase...? :)

That is, because I assume there can only be settings for an instance of a plugin?

(You guessed it, I'm trying to figure out whether this concept couldn't be part of Configurables/ConfigEntity... ;))

+++ b/core/lib/Drupal/Component/Plugin/PluginSettingsBase.php
@@ -0,0 +1,82 @@
+  public function getSettingsDefinitions() {
+    $definition = $this->getDefinition();
+    return $definition['settings'];
+  }

Please correct me if I'm wrong, but AFAIK, we had formed a consensus in that settings shouldn't actually be part of the plugin annotation but rather moved into a class methods (because the default settings are only used within the class, not needed from the outside).

With this specialized base class, this could be achieved by adding a getDefaultSettings() method to the interface, which isn't implemented by the base class.

+++ b/core/lib/Drupal/Component/Plugin/PluginSettingsBase.php
@@ -0,0 +1,82 @@
+  protected function mergeDefaults() {
+    $this->settings += $this->getSettingsMetadata('default');
+    $this->defaultSettingsMerged = TRUE;
+  }

Regarding the merging, it might be a good idea to follow the latest "overriddenData" design of the Config object here; i.e., retaining the original/custom settings on a separate property, so that it is possible to dissect the custom/specified settings values from the default settings at any time.

This would e.g. possibly allow to identify which settings have been "overridden" with custom values for certain use-cases.

What do you think?

yched’s picture

Please correct me if I'm wrong, but AFAIK, we had formed a consensus in that settings shouldn't actually be part of the plugin annotation but rather moved into a class methods (because the default settings are only used within the class, not needed from the outside).

True - this patch is an almost direct copy/paste of the base helper class I'm using in the "Field API on plugins" sandbox, which is still hook_info()-based. It's only an initial proposal, This could totally move towards a PluginSettingsInterface::getSettingsMetadata() method if that's where we want to go.

Still, another thing to consider is : whatever is in the 'plugin definition', whether hook-based or annotation based, is alterable (not true for annotations right now, but that's what #1705702: Provide a way to allow modules alter plugin definitions is about). Moving stuff out of the definition means it's not alterablme anymore, or has to come up with it's own alteration mechanism.
That's why, for instance, the 'field_types' entry (which is *not* a setting) will probably stay in the 'widget definition', we need those to be alterable (see option widgets, reusable on any field type that sees fit).

Now, stating that 'the collection of settings and their default values is defined in a method and is not alterable' is not a totally unknown model, If I'm not mistaken, that's how Views handlers have worked so far. You alter them by subclassing a new handler.

We don't really know how to support 'alter new settings in', because adding a new setting from the outside means being able to alter the rest of the methods of the plugin to actually make use of the setting, that the original code doesn't know about. Field API, for instance, has limited and inconsistent support for this (we have hook_field_widget_form_alter(), but no hook_field_formatter_alter() for performance reasons; off hand I'm not sure about altering settings forms)

Then again, altering the default value or other metadata (translatable...) on existing settings might have some use cases.

That's typically the kind of reasons why I'd rather have the design discussed at the community level rather than each subsystem on its own.

yched’s picture

- re: retaining the original/unmerged settings on a separate property

Not sure about that. I'm a bit concerned about memory impact, and I'm not sure I see a use case to track how much a given configured plugin deviates from the defaults. It's only runtime stuff.

I think the use of default settings should be :
- fill in missing settings before saving to config, because you want to "freeze" a complete and predictable definition, regardless of possible future changes of 'default values'. (e.g you don't wan't your image style behavior to suddenly change "live" because you uploaded a point release of effect.module that changes the default value for 'crop size')
- fill in missing settings at runtime (which is what the current patch here does), because the config might have been saved before setting 'foobar' was introduced, and you don't want to put isset() checks all over the place.

So in the end, 99% of the time you'll have a 'complete' set of settings with no need to merge defaults. Keeping a duplicate collection of 'original + merged' seems non negligible.

- re: relations to the config system / ConfigEntity

Not sure I see the overlap. Definitions get out of config and are turned into business objects. Those business objects then possibly involve instantiating plugins for some of their work. But in most cases those business objects are not Plugins themselves.

My remarks above about "fill defaults on save" kind of point 'sane practices' that would be cool to enforce in a ConfigEntityBase class, but the thing is we don't have a 1:1 mapping between ConfigEntities and PluginsWithSettings.
In the current design, a Field instance is one config entity that embeds recipes for a widget (plugin) and several formatters (plugins). A View is one config entity that embeds recipes for an arbitrary amount of fields, filters, sorts (plugins)...

sun’s picture

I think I agree with everything you said. Also happy to forget about ConfigEntity for the time being. :)

...with the exception of settings alter hooks. That makes sense for the example you mentioned (applicable field types), but that's not actually a setting, I think that's pure metadata (and valid to be in the annotation, as I can see how that definition is very well needed from the outside).

So how about this then:

1) Add a getDefaultSettings() to the interface (but not base).

2) Make the merge method merge the getDefaultSettings() + actual settings. (skip metadata, getDefinition() is available any time anyway)

3) Push the button. ;)

[4) Adjust later as we move forward.]

dawehner’s picture

Did you ever considered to support subelement arrays, so not just one level of settings/

    $settings['alter'] = array(
      'contains' => array(
        'alter_text' => array('default' => FALSE, 'bool' => TRUE),
        'text' => array('default' => '', 'translatable' => TRUE),
        'make_link' => array('default' => FALSE, 'bool' => TRUE),
      ),
    );
    return $settings;
xjm’s picture

+++ b/core/lib/Drupal/Component/Plugin/PluginSettingsInterface.phpundefined
@@ -0,0 +1,68 @@
+   * @param $ŝettings

Pourquoi y a-t-il un circonflexe? :)

yched’s picture

@xjm: LOL - parce que c'est plus joli bien sur.

@dawehner: no, I haven't really considered providing defaults for nested structures, all the cases I met in Field API (or image effects, or text filters...) could be expressed with a flat array of keys (whose values can be arrays themselves, but rather as in "a list of values" rather than "a structured array" for which you'd want to provide defaults on internal keys). Not sure what the implications would be.

effulgentsia’s picture

In #1833716-148: WYSIWYG: Introduce "Text editors" as part of filter format configuration, quicksketch was feeling the pain of not having this issue all figured out already. That issue will give us another use case to make sure whatever we put into the base class isn't too use case specific.

quicksketch’s picture

I haven't really considered providing defaults for nested structures, all the cases I met in Field API (or image effects, or text filters...) could be expressed with a flat array of keys (whose values can be arrays themselves, but rather as in "a list of values" rather than "a structured array" for which you'd want to provide defaults on internal keys).

I think flat arrays may be a fine limitation of this system, assuming that your settings are split out properly. For example in Fields, there are settings for widgets, settings for the instance, and settings for the field itself. As long as all three of those are separate, flat arrays, managing these settings shouldn't be difficult. Ideally, each module will manage its own sub-array of settings if needed. FileField Sources for example might make a widget setting group all under "filefield_sources", and worry about setting its own defaults for subkeys, which will work out fine as long as modules that extend FileField sources also declare all their defaults to FileField Sources (and then if module extend those modules, they declare their defaults also). Basically I think this would mean that a module could essentially "own" the top level declarations of a settings array, and be responsible for merging any defaults underneath it.

quicksketch’s picture

Status: Needs review » Needs work

Hmm, upon reviewing this actual patch, I found that it's codifying the current approach used by Field module into an abstract class.

+  public function getSettingsDefinitions() {
+    $definition = $this->getDefinition();
+    return $definition['settings'];
+  }

This basically is recommending that other modules put giant "arrays of doom" into our plugin Annotations (like Field module is doing already). I admit this is a convenient solution because it has the AlterDecorator available as part of the class discovery, which allows other modules to easily change the defaults using hook_whatever_info_alter(). However I don't think this is a clean solution.

The Annotations on classes were intended to be used for discovery, not for storing large arrays of settings which aren't needed until the class is actually instantiated for some reason. Sun included a litany of worries about the use of Annotations in the original issue, comment #73:

I really like the idea of annotations, and I was and still am fine with the example that is contained in the actual patch. But I really dislike the amount of endless flexibility and thus possible abuse, which allows coders to inject settings and whatnot non-metadata into annotations. I wish we could enforce limitations.

@merlineofchaos followed up in the next comment:

Metadata that is used only after object instantiation should be moved to methods and/or protected variables

However none of this seemed to have come to fruition, and instead we've ended up with the exact situation @sun feared, where massive arrays of annotations are being used to define everything possible about the class usage, from node view modes to views base tables to field widget defaults. All of this information is irrelevant to the "discovery" of the class and could easily be deferred until the class is actually used for some purpose.

There are a couple of places in #1683644: Use Annotations for plugin discovery where performance is addressed, but for me this is a big DX issue. The syntax for Annotations is terrible (as also mentioned in that issue). It's not JSON and it's not PHP. It's an arbitrary syntax with bizzarre rules defined by Doctrine: you can't specify an empty list, you have to put each entry of a list on it's own line, single quotes aren't supported, quotes are optional on the first level but not the second, etc.

On top of the DX issue, I'm sure that storing massive arrays of configuration defaults and settings into the class discovery cache is not going to be optimal. Even if Reflection is fast, I doubt that the parsing of all those options is going to be nearly as efficient as a native method that returns this information directly.

So in summary, I think this initiative is a good idea. A common system for providing code-based defaults + CMI based configuration would be great, but this patch takes us into territory that Annotations were not intended to be used for. On the other hand, everyone is already doing this anyway... maybe we should just codify it anyway if the abuse is unstoppable.

yched’s picture

Regarding the place where settings are defined (annotations vs somewhere else) :

So, yeah, the current patch is based on the actual class that's currently used in HEAD by Field API for its own plugin types (widgets and formatters). And right, for now, they use "plugin definitions" as a direct port of D7 info hooks, i.e as "all the metadata about the plugin", not just what's needed for discovery & instantiation. That was totally meant as an intermediate step, pending community agreement on how we want to handle this.
The goal of this issue was precisely to make that discussion happen, and provide code patterns & tools to make sure that the resulting decision gets applied consistently in core and contrib.

Also note that "where the settings are read from" is only like 2 lines in one method in the PluginSettingsBase class. Granted, it's an important architectural point, but has a minor impact on the actual proposed code & methods for the class.

Now, more precisely on that issue :
The collection of "plugin definitions" has baked-in alter and cache mechanisms. If we move "the metadata about settings" somewhere else (a static method ?), it means we need to add our own layers for alter, and thus probably for static & persistent cache too - and the nice cache clearing logic issues that go with it.
That's precisely what we're seeing in #1822458: Move dynamic parts (view modes, bundles) out of entity_info(), BTW - that issue moves the list of bundles and view modes out of the entity type definition, and therefore has to add separate alter and cache on top of the separately collected info. Bundles and view modes *need* to get out of the entity type definition, because of dependencies and infinite recursion issues. In our case with settings, we don't have that structural obligation though.

Whenever we have to instantiate a plugin, we need to load both the "plugin definitions" metadata and the "settings" metadata in memory anyway.

So I'd question whether we really expect "several chunks of metadata, each collected separately with separate alter step and static + persistent caches that we need to figure out when to clean" will be more performant and manageable than "one single, all-inclusive list of metadata". Doesn't seem obvious to me, and at least is clearly more painful code wise.

quicksketch’s picture

Doesn't seem obvious to me, and at least is clearly more painful code wise.

My experience with annotations makes me want to move as much code as possible out of them. Large arrays in phpDoc have been a poor experience for me:
- Each list item has to go on its own line.
- You can't specify an empty list i.e. some_setting = {}.
- Bracket matching doesn't work in my IDE.
- Code indent/dedent doesn't work inside phpDoc.
- Syntax errors are easy to make, and the errors provided by Drupal upon such errors give you "character position" instead of line number, making errors difficult to actually locate. If you have a long annotation, finding "position 614" is not an easy task.

So while making a separate system for default settings/altering/caching may seem like a hassle, I feel like it's the right thing to do. Even if it's not much, parsing less annotations has to be faster than parsing more.

ParisLiakos’s picture

dawehner’s picture

I guess this is some topic for drupal 9 now?

yched’s picture

Probably :-(.

This stalled on "where do we put the definitions / default values of the settings so that they can be alterable", and it took us quite some time to realize that they in fact shouldn't be alterable (#2005434: Let 3rd party modules store extra configuration in EntityDisplay)...

yched’s picture

Version: 8.x-dev » 9.x-dev
Category: bug » task
Issue tags: -Configuration system +Field API
EclipseGc’s picture

I sort of feel like tim's plugin bag work has taken care of a lot of this. See if #2033383: Provide a default plugin bag doesn't fulfill most of this and let's see if we can just close this.

Eclipse

amateescu’s picture

Yep, I signaled the same thing here #2033383-49: Provide a default plugin bag. This issue could be just about the second paragraph of that comment.

tim.plunkett’s picture

Title: Base class for management of Plugin 'settings' » Merge PluginSettingsInterface into ConfigurationPluginInterface
Version: 9.x-dev » 8.x-dev
Component: base system » plugin system

I'm not sure we need all those extra methods, but it's probably a good idea.

yched’s picture

Title: Merge PluginSettingsInterface into ConfigurationPluginInterface » Merge PluginSettingsInterface into ConfigurablePluginInterface

The thing is that the APIs in "Field" land are now consistently settled on calling things "settings", which works pretty well over there.
Widgets / formatters do $this->getSetting($name) / getSettings() for their own settings, or $this->getFieldSetting($name) for field settings
Similarly in field type classes, or for everything that deals with field definitions.

I'm not really (read: "really not" :-p) willing to move to different naming patterns - especially if based on "Configurable / Configuration", things are messy enough with Field / ConfigurableField / FieldDefinition...

tim.plunkett’s picture

Okay, then are we fine with them being separate?

\Drupal\Component\Plugin\PluginBase has a comment at the bottom:

  // Note: Plugin configuration is optional so its left to the plugin type to
  // require a getter as part of its interface.

And the variable is $this->configuration. So we're pretty happy with ConfigurablePluginInterface as is...

yched’s picture

Also, the point of this issue was to distinguish between "the configuration keys that are the same whatever the plugin" & "the configuration keys that vary plugin by plugin", and to handle the specific challenges of the latter. This is what Drupal\field\Plugin\PluginSettingsInterface is doing, and I'm not sure this is what ConfigurablePluginInterface and it's implementations do currently.

EclipseGc’s picture

Currently, the approaches (outside of fields) don't care. We put all plugin configuration into the entity's "settings" parameter and wash our hands of any further need to muck with it. We already know what elements are common across all plugins because we have custom annotation classes that establish that. I'm unsure what the benefit would be to doing anything more in depth then that at this point. I don't think anyone's suggesting fields should change at this late date, but perhaps we should keep pressing on this issue a bit so that we can determine of there's some benefit beyond what we've identified, or if fields should attempt to emulate the rest of core for D9.

Eclipse

jibran’s picture

Status: Needs work » Needs review
Issue tags: -Field API, -VDC, -Plugin system

#3: plugin_settings-1764380-3.patch queued for re-testing.

jibran’s picture

3: plugin_settings-1764380-3.patch queued for re-testing.

andypost’s picture

is this issue still valid?

tim.plunkett’s picture

Status: Needs review » Needs work
Issue tags: +Plugin system, +Field API

Yes, still valid.

donquixote’s picture

It seems this issue is actually two-fold (or three-fold):

  1. Improvements around ConfigurablePluginInterface.
  2. Stop Field plugins from sticking out as a black sheep. (that's what the issue title says)
  3. The Field plugins have some features that are "more powerful" (or more robust) than the other plugins based on ConfigurablePluginInterface. E.g. the distinction between "settings" and "3rd party settings".
    Merging PluginSettingsInterface into ConfigurablePluginInterface would mean that some of this is lost, unless we improve ConfigurablePluginInterface first.
    On the other hand, some of the features of Field plugins might be unnecessary bloat for some of the other plugin systems.

1.
ConfigurablePluginInterface has changed since this issue started, but the following is still bothering me:

  • What about an abstract base class ConfigurablePluginBase?
    Currently the plugins all need to do this by themselves, which looks quite redundant.
  • Why does PluginBase::$configuration exist in a class that does not implement ConfigurablePluginInterface?
    Why is this not in a subclass?
  • What about interface and base class for getLabel() and getDescription() ?
  • What about interface and base class for settingsForm()?

2.
I have been confused finding that we have separate interfaces for PluginSettingsInterface and ConfigurablePluginInterface. I imagine that others will be just as confused seeing this the first time.
Also it is odd to find PluginSettingsInterface and PluginSettingsBase in the Field namespace, when its code really has nothing that makes it specific to or dependent on the Field system.
And then, PluginSettingsBase inherits the $configuration property, but then it introduces its own $settings property instead.
These things will keep people assuming there must be some really sophisticated reason behind this, when in fact it is just a historical leftover.
I also found that the difference between "settings" and "third party settings" is not sufficiently explained in PluginSettingsInterface.

3.
Yes, we might lose some of the capabilities of PluginSettingsInterface, if we merge it into ConfigurablePluginInterface.
But then why not go with a 3rd option, and introduce sth like a ConfigurablePluginWithThirdPartySettingsInterface, which would satisfy the need to separate different kinds of settings?

Handling those 'settings' in a different namespace than the 'fixed properties' is important

Currently, the approaches (outside of fields) don't care. We put all plugin configuration into the entity's "settings" parameter and wash our hands of any further need to muck with it.

The suggestion above would allow to decide separately for each plugin type whether we want to have all settings mushed together or distinguish them.

But now I don't even know if @sun was talking about the same thing when he said "fixed properties".

Xano’s picture

Should this be moved to 9.0.x?

Xano’s picture

As far as I can see, this won't make it into Drupal 8 anymore. Would deprecating the interface be a viable alternative?

andypost’s picture

Suppose deprecated is a way to go, so removal could be planed for 9.x

Xano’s picture

Wim Leers’s picture

I just filed #2546590: Main content block shows up on page, even if hidden, in which I saw a related problem:

  /**
   * Sets a particular value in the block settings.
   *
   * @param string $key
   *   The key of PluginBase::$configuration to set.
   * @param mixed $value
   *   The value to set for the provided key.
   *
   * @todo This doesn't belong here. Move this into a new base class in
   *   https://www.drupal.org/node/1764380.
   * @todo This does not set a value in \Drupal::config(), so the name is confusing.
   *
   * @see \Drupal\Component\Plugin\PluginBase::$configuration
   */
  public function setConfigurationValue($key, $value);

It made very little sense for my block to implement that method, and it points to this issue. Would be great to simplify BlockPluginInterface tremendously. But, alas, feels like 9.x stuff at this point.

tim.plunkett’s picture

Version: 8.0.x-dev » 9.x-dev
catch’s picture

Version: 9.x-dev » 8.3.x-dev
Issue tags: +Needs issue summary update

Moving this back down to 8.x, although this issue has taken multiple turns, so it's not at all clear to me what's still in scope. We have modules for adding methods to interfaces where there's a corresponding base class and similar, and deprecating interface methods etc. so seems worth exploring what if anything could be done in 8.x.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.