Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
- #1324618: Implement automated config saving for simple settings forms
Allows modules to use a generic settings form for their configuration values. A settings form may not manage all values within a configuration object though. Also, module A might extend the settings form of module B for a particular setting only. - #1505090: Clean up all (non-default) configuration (files) when a module is uninstalled
At least removes stale/orphan configuration objects (resolving the variables problem we had before).
Comment | File | Size | Author |
---|---|---|---|
#19 | drupal8.configurable-clear.19.patch | 1.48 KB | mtift |
#2 | drupal8.configurable-clear.2.patch | 1.84 KB | sun |
Comments
Comment #1
sunNeed to bump this major.
My system.site.yml file contains this:
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.
Comment #2
sunAt 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.
Comment #3
sun#2: drupal8.configurable-clear.2.patch queued for re-testing.
Comment #4
yched CreditAttribution: yched commentedAgreed 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.
Comment #5
sunActually, 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).
Comment #6
yched CreditAttribution: yched commented@sun : so that would be a a mechanism provided by ConfigEntityBase ? Is there an issue for this already ?
Comment #7
catchThis is the issue: #1783346: Determine how to identify and deploy related sets of configuration.
Comment #8
yched CreditAttribution: yched commentedAlright, added my thoughts on the topic over there, then.
Comment #9
sunPatch in #2 has been merged into #1785560: Remove the sorting of configuration keys
Comment #10
andypostI 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()
Comment #11
sunIrrelevant. $entity != $config. All hooks act on the $entity object, not on $config.
Yes, intentionally. Please read the issue title.
Comment #12
Lars Toomre CreditAttribution: Lars Toomre commented@sun Could you please elaborate? Reading though this issue I am at a loss with your comments in #11. Thanks.
Comment #13
xjm#2: drupal8.configurable-clear.2.patch queued for re-testing.
Comment #14
xjmRelated: #1822048: Introduce a generic fallback plugin mechanism
We have reason in Views to possibly want unavailable keys to be stored in the config.
Comment #16
sunThe 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.
Comment #17
sunComment #18
gddI 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?
Comment #19
mtiftHere is a re-roll
Comment #20
mtiftComment #21
damiankloip CreditAttribution: damiankloip commentedI'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.
Comment #22
mtiftI 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.
Comment #23
xjmDiscussed 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.
Comment #24
mtiftStill needs work for tests
Comment #25
tim.plunkettThis hasn't come up in the past 2 years either. Bumping to 8.1.x, but I think it could be closed.
Comment #26
BerdirIt 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?
Comment #39
larowlanOn the basis of the last two comments we're expecting providers to clean up after themselves now