Merge PluginSettingsInterface into ConfigurableInterface

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

Tagging, this sounds helpful.

+++ 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?

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

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;

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

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

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.

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.

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.

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.

Crossposting #1957330: Make possible for parsers and fetchers to expose configuration through plugins

I guess this is some topic for drupal 9 now?

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

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.

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

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

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

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

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

is this issue still valid?

Yes, still valid.

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

Should this be moved to 9.0.x?

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

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

See #2502967: Mark PluginSettingsInterface as deprecated for removal before Drupal 9.0.x.

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.

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.

#2579743: Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed would help this resolve where onDependencyRemoval would go.

I think instead of the base class here better to use trait from #2852463: Create a trait and base class to implement \Drupal\Component\Plugin\ConfigurableInterface

Being blissfully unaware of this issue (managed to skip right over an @todo pointing here), I opened #3166481: Give field widget + formatter plugins a version of defaultSettings() that has the FieldDefinition as context to address a related pain point that this issue would also solve. It seems there's no reason for PluginSettingsInterface::defaultSettings() to be public static, and it would greatly benefit from having the context you could figure out if you had an instantiated object to work with.

Haven't read the full history yet, but adding #3166481 as related for now.

Thanks,
-Derek