In #1671198-11: Merge $conf overrides only once per instantiated Config object, and move initial setName() into Config constructor it is said "The methods on the Config object operate on the defined current data and state of the Config object itself only, and that's it." That smells like bad design to me. The Config object should be responsible, well, for handling config. It should reflect an in-memory state of a named config object in a given storage. I find explicit load and even explicit save a violation of this. The caller should have no care that the config is even stored in any ways, if you get what I mean. It can be argued that for the saving to always work correctly #1670370: Enforce writing of config changes during import, even if the module callback handled it already needs to be resolved. Right now I have not removed any save methods because of this.

CommentFileSizeAuthor
config_encapsulated.patch4.83 KBchx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, config_encapsulated.patch, failed testing.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Needs review

Note: this still worths a review -- it should be trivial to fix those notices. Funny that the configuration tests passed.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Needs work
Issue tags: +Configuration system 
+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -271,6 +288,13 @@ class Config {
+  function __destruct() {
+    if ($this->tainted) {
+      $this->save();
+    }
+  }

Sorry, but a strong -1 on this.

That's needless and totally weird magic, not encapsulation.

This particular change seems to be the whole point of this issue, and that doesn't fly at all for me, so I'd recommend to won't fix this idea/issue.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Closed (won't fix)

Whatever you say. I am sick of fighting you.