Filter formats should not save plugin data when the plugin configuration matches defaults

Is this something specific to FilterFormat entities, or is there a generic pattern / issue here ?

It's specific to filters due to them not implementing plugin instances the same way as elsewhere - for filters, the plugin id is also the instance id, so there's no instance create/delete cycle in the API or UI.

Don't know if there's another plugin system / config entity in core that has a similar problem, but it's definitely not the typical case.

I'll let other people more familiar with the filter system RTBC this but :

+++ b/core/modules/system/src/Tests/Entity/ConfigEntityImportTest.php
@@ -125,6 +126,28 @@ protected function doFilterFormatUpdate() {
+  protected function filterFilters(array $filters) {
...
+        // Compare the current settings to the default settings. If there are
+        // any customisations save them to configuration.

This comment cannot be copy/pasted as is from FilterFormat::preSave() :-)

Incomplete review:

+      $default = \Drupal::service('plugin.manager.filter')->getDefinition($filter['id']);
+      if (!$filter['status']) {

We don't need to retrieve $default if the filter is enabled.

#16: lovely!

I could only find nitpicks:

1. +++ b/core/modules/config/src/Tests/ConfigImportAllTest.php
   @@ -69,7 +69,10 @@ public function testInstallUninstall() {
   +    // Purge the data. Need to call this twice to actually remove all the
   +    // fields. First time around will purge the instances. Second time will
   +    // delete the instances and fields.
   

   This scares me.

2. +++ b/core/modules/filter/src/Entity/FilterFormat.php
   @@ -194,8 +195,17 @@ public function preSave(EntityStorageInterface $storage) {
   +        // any customisations save them to configuration.
   
   +++ b/core/modules/filter/src/Tests/FilterAPITest.php
   @@ -335,6 +335,50 @@ function testTypedDataAPI() {
   +   * Tests that FilterFormat::preSave() only saves customised plugins.
   

   s/customise/customize/, I fear… here and elsewhere. I'd prefer UK English also, but alas.

3. +++ b/core/modules/system/src/Tests/Entity/ConfigEntityImportTest.php
   @@ -125,6 +126,26 @@ protected function doFilterFormatUpdate() {
   +   *   An array fo filter plugin configuration keyed by plugin ID.
   

   Language errors.

4. +++ b/core/modules/system/src/Tests/Entity/ConfigEntityImportTest.php
   @@ -125,6 +126,26 @@ protected function doFilterFormatUpdate() {
   +   *   The filtered array of filters.
   +   */
   +  protected function filterFilters(array $filters) {
   

   While I think this is funny, wouldn't it be better if this were named getCustomizedFilters()?

   This could also be a static method.

   Oh… this is a helper in a test class. Let's keep the funkiness then! :P

We do a similar thing for conditions, but completely differently. See ConditionPluginBag::getConfiguration().

    $this->filters = array_filter($this->filters, function($filter) {
      if ($filter['status']) {
        return TRUE;
      else {
        // Compare the current settings to the default settings. If there are
        // any customisations save them to configuration.
        $default = \Drupal::service('plugin.manager.filter')->getDefinition($filter['id']);
        return DiffArray::diffAssocRecursive($filter, $default);
      }
    });

php, loosely typed, readability, yadda yadda :D

I don't think this is RTBC yet, see #19.

+++ b/core/modules/system/src/Tests/Entity/ConfigEntityImportTest.php
@@ -101,13 +102,13 @@ protected function doFilterFormatUpdate() {
-    $this->assertIdentical($filters, $plugin_bag->getConfiguration());
+    $this->assertIdentical($filters, $this->filterFilters($plugin_bag));

I don't know that we need this anymore?

The last patch is green, it might not look like it because of #2310011: Comment render caching breaks comments with patches attached

#2310023: Do not export plugin configuration if it matches the defaults works to move that shared code out of FilterBag and ConditionPluginBag directly into DefaultPluginBag.

What happens here if a module updates the defaults, it'll change the behaviour of existing text formats in the system when they're not customized no?

Once you deployed a config with default values these defaut values won't be part of the active config.
If you as a filter plugin developer want to change default values, this would automatically change the behavior of all these default values
based input formats.
In practice this means that you as a core/contrib developer cannot change default values anymore.
As catch pointed out in IRC this is especially critical in the land of input formats as it could lead to potential security issues.

If you store all configs all the time, changing the default values would have no impact on active behaviour.
If you want on the other hand change the behavior of all the things, you could write an update script
or for custom world simply change all those configurations manually.

Note: This is the normal behavior of CMI.

Storing everything gives you the flexibility to change the default values later, which was always problematic
in contrib world (views for example).

Hmm OK, so should the title be 'should not save plugin data when plugins are disabled' then?

+++ b/core/modules/filter/src/FilterBag.php
@@ -107,4 +107,21 @@ public function sortHelper($aID, $bID) {
+      $default_config = array();

How do we know whether this is an enabled plugin that happens to have configuration matching the defaults, or a disabled plugin that has not been configured at all? Purely because the status is part of that default configuration, so if it's enabled it must be configured? Because then it wouldn't match the defaults? Needs much more explanation in the code at least. Do we have anything that ensures filters are disabled by default?

I agree we do need to be careful when suppressing config output. However, filters are a special case here, and because the default values include 'status' => FALSE, this is safe.

But based on @dawehner and @catch's points, we should probably close #2310023: Do not export plugin configuration if it matches the defaults.

ConditionPluginBag has the same hack, but also because it currently hardcodes the available conditions...

+++ b/core/modules/filter/src/FilterBag.php
@@ -107,4 +107,21 @@ public function sortHelper($aID, $bID) {
+    $configuration = parent::getConfiguration();
+    // Remove configuration if it matches the defaults.
+    foreach ($configuration as $instance_id => $instance_config) {
+      $default_config = array();
+      $default_config['id'] = $instance_id;
+      $default_config += $this->get($instance_id)->defaultConfiguration();
+      if ($default_config === $instance_config) {
+        unset($configuration[$instance_id]);
+      }
+    }
+    return $configuration;
+  }

Can we please document why this is okay, but just okay for filter module?

Yes, good point.

Not sure about other plugins, but Field API explicitly avoids "change of code-defined defaults changes the behavior of fields / widgets / formatters that already exist". Defaults are only used as "initial settings on creation" (or fallback value in case a setting is added to a field type and was unknown / not specified in pre-existing fields). We never change behavior behind site admin's back.

To clarify, what makes FilterFormat a special snowflake is that:

1. It stores configuration of disabled plugin instances. Most of our config entity / plugin bag setups don't do this. E.g., if you remove a field (formatter) from an entity display, it's then gone, it's not retained in the EntityDisplay config object as a disabled instance. Retaining configuration of disabled instances is not unique to filter formats (e.g., blocks can also be configured but not placed in any region), but it's not the common case.
2. Filters are not explicitly instantiated via user action the way that image effects or block instances are. And consequently, there's no UI to delete an instance. Instead, we assume a 1:1 relationship between instance and plugin (I think there's a @todo in HEAD that questions this assumption), and therefore, autoinstantiate an instance for each code-defined plugin whenever a filter format starts interacting with its filter plugin bag.

The combination of the above (autoinstantiate + retain config for disabled) creates the problems we currently have in HEAD, where code changes can leak config updates at unpredictable times, and cause formats to have unwanted dependencies on modules that came about without any user action or benefit to the user, that necessitates the fix in this issue.

Opened #2312379: Bring filter format plugin configuration into line with other systems.

Committed/pushed to 8.x, thanks!

Also posthumously tagging D8 upgrade path since we'd have needed to update existing formats if we were supporting that yet.

I was just working on a new install profile and I found a @todo in standard.install, came to the issue and it got committed an hour ago :)

Anyway, we should now be able to remove this from standard.install

  // @todo Remove in https://www.drupal.org/node/2295129.
  // Resave the plain_text formatter so that default filter plugins and
  // dependencies are calculated correctly. This resolves an issue caused by the
  // fact that filter is installed before editor but the standard profile also
  // enables the file module.
  entity_load('filter_format', 'plain_text')->save();

My mistake, looks like I didn't change to 8.0.x