Problem

  • Orphan/obsolete keys and sub-keys within configuration objects are retained forever.

Goal

  • Only store current configuration values in configuration objects.

Details

  • Various implementations of config()->save() are not able to use ->clean() or ->setData() to completely erase and replace all existing values in a configuration object. They can only use ->set() for particular keys.
  • In turn, this means that you can add a key 'foo' today, and it will be retained forever, unless cleaned up manually.
  • This is much worse than the problem of orphan/obsolete variables we had before, since those variables could at least be identified relatively easily. It's close to impossible to identify stale/orphan keys or sub-keys within thousands of configuration objects.

Related issues

Files: 
CommentFileSizeAuthor
#19 drupal8.configurable-clear.19.patch1.48 KBmtift
PASSED: [[SimpleTest]]: [MySQL] 58,059 pass(es). View
#2 drupal8.configurable-clear.2.patch1.84 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.configurable-clear.2.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

sun’s picture

Priority: Normal » Major

Need to bump this major.

My system.site.yml file contains this:

page:
    0: ''
    front: node
    1: ''
    2: ''
    3: ''
    403: ''
    404: ''

i.e., at some point, some weird state, code, or module in between led to the addition of the 0, 1, 2, 3 keys.

Since then, these keys exist in all storages. Re-saving the site information settings form only updates the known/valid keys, but leaves everything else alone.

sun’s picture

Status: Active » Needs review
Issue tags: +Needs tests, +Configurables
FileSize
1.84 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.configurable-clear.2.patch. Unable to apply patch. See the log in the details link for more information. View

At least for Configurables, I'd like to suggest to change ConfigStorageController to clear out the entire existing config object before writing the new data.

That is, because the data contained within a config object for a Configurable should be 100% known and explicitly defined.

Something along the lines of attached patch.

sun’s picture

#2: drupal8.configurable-clear.2.patch queued for re-testing.

yched’s picture

Agreed with #2.
I'll leave it to someone from the CMI team to RTBC thoigh :-)

IMO, that's also one of the reasons why we can't sipport module B's settings about module A's ConfigEntities to be stored within the CMI files for those ConfigEntities (i.e comment settings for 'article' to be stored within node.types.article.yml). Too many chances of having the external data wiped out.

sun’s picture

that's also one of the reasons why we can't sipport module B's settings about module A's ConfigEntities to be stored within the CMI files for those ConfigEntities (i.e comment settings for 'article' to be stored within node.types.article.yml)

Actually, the architectural idea for nested config would be that a config entity would use a specialized ConfigStorageController that is able to "attach" other config objects/entities onto a primary config entity (similar to fields).

yched’s picture

@sun : so that would be a a mechanism provided by ConfigEntityBase ? Is there an issue for this already ?

catch’s picture

yched’s picture

Alright, added my thoughts on the topic over there, then.

sun’s picture

andypost’s picture

Status: Needs review » Needs work
+++ b/core/modules/config/lib/Drupal/config/ConfigStorageController.php
@@ -270,6 +270,10 @@ public function save(StorableInterface $entity) {
     $this->preSave($entity);
     $this->invokeHook('presave', $entity);
 
+    // Clear out any possibly existing keys in an existing configuration object,
+    // so any potentially stale keys are removed.
+    $config->clear();

I strongly disagree with this change! This should be done before presave hooks execution

This could kill all ability to inject other data into ConfigEntity - we have the same with drupal_write_record()

sun’s picture

Status: Needs work » Needs review

This should be done before presave hooks execution

Irrelevant. $entity != $config. All hooks act on the $entity object, not on $config.

This could kill all ability to inject other data into ConfigEntity

Yes, intentionally. Please read the issue title.

Lars Toomre’s picture

@sun Could you please elaborate? Reading though this issue I am at a loss with your comments in #11. Thanks.

xjm’s picture

#2: drupal8.configurable-clear.2.patch queued for re-testing.

xjm’s picture

Related: #1822048: Introduce a generic fallback plugin mechanism

We have reason in Views to possibly want unavailable keys to be stored in the config.

Status: Needs review » Needs work

The last submitted patch, drupal8.configurable-clear.2.patch, failed testing.

sun’s picture

The patch in #2 is still necessary.

For static (non-entity) config objects, we probably want to "don't care" and rely on update.php + proper maintenance by modules.

sun’s picture

Component: configuration system » configuration entity system
heyrocker’s picture

I agree about disregarding static config objects and the patch looks good, but I'm not clear on xjm's comment about why they would want this data for missing handler management. Can you give an example of how this would present a problem?

mtift’s picture

FileSize
1.48 KB
PASSED: [[SimpleTest]]: [MySQL] 58,059 pass(es). View

Here is a re-roll

mtift’s picture

Status: Needs work » Needs review
damiankloip’s picture

I'm not sure how this affects us with broken handlers, this is data we want to store, so we'll store it.

This patch will essentially clear configuration before saving so you only have relevant data that was on the entity? That's totally fine for us. Its just a part of handler data , same as any other option we have in there. Missing handlers are stored no differently.

mtift’s picture

I thought it would be helpful to note that this patch was discussed quite a bit in #1785560: Remove the sorting of configuration keys and that the code from this issue was included in a few of the patches from that issue, such as in comment #36 from that issue. That code was removed after webchick described it as killing kittens and others suggested this patch might be solving a "theoretical" problem.

One of the most relevant comments from the other issue was #45 in which @alexpott describes a conversation he had with @sun.

To me, the idea of clearing out config objects before writing new data into them makes a lot of sense and would reduce the likelihood of retaining unwanted config data.

xjm’s picture

Priority: Major » Normal

Discussed this issue at MWDS with @alexpott and @mtift. We agreed that, while this is not ideal, the responsibility is on the configuration provider code to clean up their stale data. It's not nearly as bad as in D7 and prior because the files are namespaced and so forth. It also hasn't really come up as an issue during the course of the D8 cycle.

mtift’s picture

Status: Needs review » Needs work

Still needs work for tests

tim.plunkett’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue summary: View changes

This hasn't come up in the past 2 years either. Bumping to 8.1.x, but I think it could be closed.

Berdir’s picture

It kind of has now in #2648956: Editing a view leaves old keys hanging, results in invalid schema. But maybe that issue is the final argument for saying that this is something that the specific config entity has to care about and we'd wont fix this?

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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.