In the code we have the following comment

  public function preSave(EntityStorageInterface $storage) {
    // Ensure the filters have been sorted before saving.
    $this->filters()->sort();

    parent::preSave($storage);

    $this->name = trim($this->label());

    // @todo Do not save disabled filters whose properties are identical to
    //   all default properties.
  }

This causing problems for #2144263: Decouple entity field storage from configurable fields since its changes allow ConfigImportAllTest uninstall the filter module which then causes a discrepancy between staging and active after import.

Files: 
CommentFileSizeAuthor
#41 interdiff.txt1010 bytestim.plunkett
#41 filter-2295129-41.patch6.9 KBtim.plunkett
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,321 pass(es). View
#23 18-23-interdiff.txt4.43 KBalexpott
#18 2295129.18.patch8.69 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,337 pass(es). View
#15 13-15-interdiff.txt839 bytesalexpott
#13 2295129.13.patch8.17 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,804 pass(es). View
#13 11-13-interdiff.txt724 bytesalexpott
#11 2295129.11.patch8.3 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,791 pass(es). View
#11 8-11-interdiff.txt3.23 KBalexpott
#8 2295129.8.test-only.patch2.2 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,740 pass(es), 3 fail(s), and 0 exception(s). View
#8 2295129.8.patch4.77 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,921 pass(es), 2 fail(s), and 6 exception(s). View
#8 4-8-interdiff.txt3.1 KBalexpott
#5 2295129.4.patch2.31 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,904 pass(es), 2 fail(s), and 93,510 exception(s). View
#1 2295129.1.patch801 bytesalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,911 pass(es), 1 fail(s), and 0 exception(s). View

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
801 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,911 pass(es), 1 fail(s), and 0 exception(s). View

Patch to show how to cause the problem

yched’s picture

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

Status: Needs review » Needs work

The last submitted patch, 1: 2295129.1.patch, failed testing.

effulgentsia’s picture

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.

alexpott’s picture

FileSize
2.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,904 pass(es), 2 fail(s), and 93,510 exception(s). View

Here's a patch that does exactly what the @todo wants and fixes the bug.

alexpott’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: 2295129.4.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.1 KB
4.77 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,921 pass(es), 2 fail(s), and 6 exception(s). View
2.2 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,740 pass(es), 3 fail(s), and 0 exception(s). View

Added tests and fixed the exceptions.

Status: Needs review » Needs work

The last submitted patch, 8: 2295129.8.test-only.patch, failed testing.

The last submitted patch, 8: 2295129.8.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.23 KB
8.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,791 pass(es). View

Fix up the tests.

yched’s picture

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

alexpott’s picture

FileSize
724 bytes
8.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,804 pass(es). View

Yep that comment is not necessary considering the function's docblock.

sun’s picture

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.

alexpott’s picture

FileSize
839 bytes
8.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,806 pass(es). View

Good point.

alexpott’s picture

FileSize
659 bytes
8.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,940 pass(es). View

We can remove the hack in standard.install now :)

Wim Leers’s picture

#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

alexpott’s picture

FileSize
2.53 KB
8.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,337 pass(es). View
  1. Well this was the case in HEAD and D7 until #2144263: Decouple entity field storage from configurable fields we can safely remove this now :)
  2. Fixed
  3. Fixed
  4. :)
tim.plunkett’s picture

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

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
chx’s picture

Status: Reviewed & tested by the community » Needs work
    $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

tim.plunkett’s picture

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

alexpott’s picture

Status: Needs work » Needs review
Related issues: +#2309961: Filter plugins should use defaultConfiguration() properly
FileSize
4.43 KB
8.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,287 pass(es). View

Discussed with @tim.plunkett on IRC - FilterBase::defaultConfiguration currently is a bit weird. This patch makes minimal changes to fixes this issue - #2309961: Filter plugins should use defaultConfiguration() properly will make further changes to brings filters into line with other plugins.

tim.plunkett’s picture

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

alexpott’s picture

FileSize
4.2 KB
6.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,127 pass(es). View

Reverted all changes to ConfigEntityImportTest and moved the removal into FilterBag::getConfiguration

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

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

tim.plunkett’s picture

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

catch’s picture

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?

dawehner’s picture

Status: Reviewed & tested by the community » Needs work

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

beejeebus’s picture

#28 and #29 +10000000000

alexpott’s picture

Hmmm... this is not that simple. Look at the current hack in standard.install

If I install a module that provides a filter plugin should it resave every existing filter format to add it's disabled plugin configuration?

alexpott’s picture

Also if changing the default values of a disabled filter plugin represents a security issue - I'm scratching my head. I do think that we should ensure that filter plugins are disabled by default.

alexpott’s picture

For clarity - the status of the plugin is part of the "default configuration" - no filter plugin in enabled by default. The moment you enable the plugin it is saved in config and changing the programatic defaults has no effect.

catch’s picture

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?

alexpott’s picture

+++ b/core/modules/filter/src/Plugin/FilterBase.php
@@ -107,7 +107,12 @@ public function getConfiguration() {
   public function defaultConfiguration() {
-    return array();
+    return array(
+      'provider' => $this->pluginDefinition['provider'],
+      'status' => FALSE,
+      'weight' => $this->pluginDefinition['weight'] ?: 0,
+      'settings' => $this->pluginDefinition['settings'],
+    );
   }

This ensures that they are disabled by default.

I'll add a comment there that this is important. And I'll also add a comment about how saving disabled plugin configuration when it has been customised is a good thing too.

beejeebus’s picture

"If I install a module that provides a filter plugin should it resave every existing filter format to add it's disabled plugin configuration?"

i think this means - every new filter plugin, disabled or not, forces changes in every existing filter format? did we take a wrong turn somewhere?

i guess i'm missing the big picture on how that makes any sense at all.

alexpott’s picture

#36 we will only be taking this wrong turn if we don't commit this patch. This patch prevents disabled filter plugins from newly installed modules affecting existing filter format config entities.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community

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

beejeebus’s picture

discussed this with timplunkett. he explained to me that what this patch does is stop the crazy runtime filter stuff from bleeding through to CMI writes.

and code changes shouldn't change behaviour, because the magic instances that are added are all disabled.

so. kinda crazy, but i think i'm satisfied with this.

dawehner’s picture

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

tim.plunkett’s picture

FileSize
6.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,321 pass(es). View
1010 bytes

Yes, good point.

yched’s picture

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.

effulgentsia’s picture

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.

  • catch committed 2ecdf8c on 8.0.x
    Issue #2295129 by alexpott, tim.plunkett: Fixed Filter formats should...
catch’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +D8 upgrade path

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.

benjy’s picture

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();
benjy’s picture

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

Status: Fixed » Closed (fixed)

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