File-based configuration API

subscribe.

Various clean-ups and fixes. Mainly to ease reviews.

+++ b/core/includes/config.incundefined
@@ -0,0 +1,593 @@
+    $files = glob($module_config_dir . '/' . '*.xml');
...
+  // @todo What about 32-bit CPUs?

Lets try to delete that todo...

According to http://eprint.iacr.org/2010/548.pdf sha-512 is always faster than sha-256. So I don't see any problems with 32 bit CPU's.

Pushed some code style cleanup. :) (Greg also merged in sun's cleanups from above and further.)

Pushed a fix for the config test

EDIT:

Shouldn't

readFromFile()

and

deleteFromActive()

be part of the DrupalConfigVerifiedStorageInterface

- I agree on that "default" styles should not be kept in code. Keep config at 1 place, not in a config store and scattered in hook_default_... functions. Thus save it to config on install. The flag of being overridden or not could be kept though, but probably needs a "restore" hook besides an install hook.
- New code should follow the new documentation standards: document all param and return types.
- Don't repeat functionality of drupal_array_get_nested_value() (premature micro optimization?):

+++ b/core/includes/config.incundefined
@@ -0,0 +1,593 @@
+  public function get($key) {
+    $parts = explode('.', $key);
+    if (count($parts) == 1) {
+      return isset($this->data[$key]) ? $this->data[$key] : NULL;
+    }
+    else {
+      return drupal_array_get_nested_value($this->data, $parts);
+    }
+  }

Simpler:

+  public function get($key) {
+    $parts = explode('.', $key);
+    return drupal_array_get_nested_value($this->data, $parts);
+  }

(occurs multiple times in the code.)

I also believe that the 'Overridden' flag might still be useful. It's nice that we can get rid of 'in code' and 'in database', but it would be easier for a developer that 'inherits' a Drupal site to see what to expect from it (overridden image styles, default views, etc.) :)

Agree with #12 and #13. Thus yes, keep that information available in the admin screens, but don't make it part of this patch. Comparing stored config with the hooks that defined them shouldn't be too difficult.

[ - Moved description/anatomy/callgraph into summary -]

Initial code review issues

Critical:

1. The impact on DX is pretty negative here. Instead of one wonky internal variable name that you usually need to look up somewhere else in the code-base, there are two names now, whereas one of them is additionally namespaced, which leaves the impression that you, as a developer, are actually dealing with more than two keys/names now...:
   
   $value = variable_get('cache_lifetime', 0);
   
   becomes
   
   $config = config('system.performance');
$value = $config->get('cache_lifetime');
   
   To remedy that, I wonder whether we could get back to one key somehow:
   
   $value = config()->get('system.performance.cache_lifetime');
   
   (mayhaps using a different delimiter (:).
2. The default value handling is unclear to me. I'm aware that variable_get()'s concept of default values is obsolete and default values are supposed to live in config files shipping with each module. However, there is no guarantee that a default config value actually exists, since Drupal is modular and modules can support other modules optionally.

   If I'm not mistaken, it looks like there's a chance for code getting way more complex:
   

   // We don't depend on someothermodule, but if it's there and 'thing'
// was enabled, account for it.
if (variable_get('someothermodule_thing', FALSE) == 'special_condition') {
  mymodule_thingie();
}
   
   becomes a bit more:
   
   // Diff 1: isset() obviously doesn't work on the expression.
// Diff 2: No (potentially custom) fall-back default value.
$enabled = config('someothermodule')->get('thing');
if (isset($enabled) && $enabled == 'special_condition') {
  mymodule_thingie();
}
3. In light of previous example ^^, it looks like the config file namespacing enforces a sub-key where no sub-key was previously needed? I.e., variable_get('mymodule_check'); needs to be "categorized" somehow now? Otherwise, you'd have config('mymodule')->get('check') and in turn a config file with name mymodule.xml...?

Major:

1. I expect the config() factory return and all public $config methods (except ->get()) to be chainable; i.e.:
   
   $config = config('foo.bar');
$config->set('foo', 'bar');
$config->save();
   
   should allow for
   
   config('foo.bar')->set('foo', 'bar')->save();
2. config_get_signed_file_storage_names_with_prefix() states:
   
   * Given the following configuration files:
* - core.entity.node_type.article.xml
* - core.entity.node_type.page.xml
*
* You can pass a prefix 'core.entity.node_type' and get back an array of the
* filenames that match. This allows you to iterate through all files in a
* branch. Note that this will only work on the level above the tips, so
* a prefix of 'core.entity' would return an empty array.
   
   I'd expect to get the full tree in any case though. Since the comment does not clarify why that limitation exists, I'm challenging it, and don't see why it shouldn't be possible.
3. Various config file lookup functions are using glob() and other filesystem functions, while glob() is known for having various edge/special cases. Given what is actually done these functions and that we can use PHP5, it might make sense to use DirectoryIterator and RecursiveDirectoryIterator instead. (usage example, simple)
4. I believe the config_encode(), _decode(), _xml_to_array(), _array_to_xml() functions should be part of the DrupalConfig or SignedFileStorage class. Which one mainly depends on whether we think that format processing is the responsibility of the (individual) storage or can be generalized for all storages. I'd personally try the latter first.
5. Various class methods could use either shorter or clearer names. Some methods (like verify($contentOnSuccess = FALSE)) are doing too many different things. Such methods are normally split out into multiple, whereas each one sets and accesses data properties on the class.

Minor:

1. The global $drupal_config_directory_name, $drupal_config_key variables i) shouldn't start with 'drupal_' and ii) are not really clearly named. I wouldn't expect a directory basename in the first (but a full path). And the "key" doesn't mean anything to me. Rename to: $config_directory_basename and $config_signature_key.
2. DrupalVerifiedStorageSQL should be renamed to DrupalConfigVerifiedStorageSQL, and likewise,
   DrupalConfigVerifiedStorageInterface should be renamed to ConfigVerifiedStorageInterface.
3. That said, is there any particular reason for having the "verified" part in the class and interface names? Is there a "unverified" storage in the first place?

Congrats on the recent code rush, @heyrocker !

For people interested in CMI, and as @hejrocker pointed in #2, I'd like to stress the fact that "reload config files from disk and react to changes" is the one last glaring hole in the API. E.g. for the scope of the current patch (image styles) : the computed image files for a style should be wiped when the definition of the style changes. The current state of the CMI API has no way to detect that.

AFAIK there are currently not two people with the same vision for this, and discussion on the topic has stalled for the last two months. Sorting this out is critical before the CMI code and concepts can be proof-tested against more complex subsystems (like Field API, which will be non-trivial...)

See http://groups.drupal.org/node/190729 - more precisely:
- the section 'CMI : the “from files to active store” flow' in the OP, for the proposal that came out in BAcamp
- my 'Reload flow" comment (http://groups.drupal.org/node/190729#comment-632198) for why I have doubts it will float, and an alternate proposal.

crosspost

and resetting tag as well.

Subscribe. Also, I don't care much about performance on 32-bit CPUs, even if SHA-512 does turn out to be slower. The signature check is not a critical-path operation.

From irc, as a response to sun:

aspilicious> sun, about your dx issue isn't that solved if we can chain?
<sun> not really
<aspilicious> $value = config()->get('system.performance.cache_lifetime');  or $value = config('system.performance)->get('cache_lifetime');
<aspilicious> is the same dx wise
<aspilicious> its the same amount of parts to remember
<aspilicious> just structured in a different way
<bojanz> If I'm fetching multiple values, won't it get kinda repetitive to include the prefix on each get() call?
<aspilicious> bojanz, yes!
<aspilicious> thats why I agree with chaining but not with the other proposal
<bojanz> exactly

Deleted because duplicate.

Updated issue summary.

Updated issue summary.

I agree with sun about config()->get('system.performance.cache_lifetime');.

<bojanz> If I'm fetching multiple values, won't it get kinda repetitive to include the prefix on each get() call?

Is not really revelant; in both case you have to do multiple calls, so in term of code typed it's more or less the same.

<aspilicious> $value = config()->get('system.performance.cache_lifetime');  or $value = config('system.performance)->get('cache_lifetime');

I disagree too, one expose the configuration as one single configuration tree, which is more natural; The second is confusing.

Choosing one of the other doesn't impact the storage backend, nor the internal representation of the data: it's a public facade. But if we choose a public facade that exposes the configuration as one single tree, we may end up having a nice API such as, for example, being able to replace the active layer by a configuration backend such as GConf on Linux or the Windows registry: it sounds really fun (and may be performance wize in some case)!

Plus having a public facade that considers one single tree would be a step more to decorellate the original split from the day to day usage, and would allow to use non biased (incremental?) caching mecanism based on usage and not on internal representation for reading optimization.

Note: I fully understand that the Configuration system has to differentiate between the configuration object and the configuration key within that object. I am not questioning nor criticizing that. (Although the granularity of configuration objects remains to be an open question, in light of Drupal being a modular/extensible system.)

In a sense, it is comparable to currently doing:

variable_set('mymodule_thing', array('foo' => 'bar'));
$myconfig = variable_get('mymodule_thing', array());
var_dump($myconfig);  // array('foo' => 'bar')

Contrary to above claims, this current code allows to get multiple values from a single configuration object:

$config = config('mymodule');
$foo = $config->get('foo');
$bar = $config->get('bar');

And with some additional tweaking, I can easily see it providing access to the full object, too; as in:

$config = config('mymodule')->getAll();
$foo = $config['foo'];
$bar = $config['bar'];

However, it enforces this code logic also on simple/scalar configuration values, which - as even the initial conversion in this patch nicely demonstrate - is the most common use-case for configuration lookups.

Technically, we could plant a tweak into the config() factory or into a separate config_get() helper function, which automatically splits the passed in argument into $file/$name and $key; e.g.:

function config_get($id) {
  // Name and key are delimited by a semicolon.
  // Note that both name and key may contain dots/periods indicating the namespace and sub-keys.
  list($name, $key) = explode(':', $id, 2);
  return config($name)->get($key);
}

I've committed some class/interface naming clean-ups yesterday. After sleeping over this, I have many more considerations on the code/implementation to discuss. Will consult @heyrocker for how and where to put them.

Updated issue summary.

@heyrocker How does the code behaves if some modules gives you:a.b.var in file a.xml and another one gives you a.b.var in a.b.xml? Will I have two config files in common dir a.xml and a.b.xml? If so, how does the API differenciate them?

@hejrocker and with a.xml in module X and a.xml in module Y. Those files get collected and grouped in one directory.

@heyrocker That's what I guessed right after writing my comment, thanks for the confirmation.

EDIT: Did you made a mistake, shouldn't it be:

$config = config('a');
print config->get('b.var');

$config = config('a.b');
print config->get('var');

?

yeah pounard you're right, and thnx hejrocker for your answer :)

re. #15 - last time i argued we should kill the "you must know that this is about files to use the API" requirement, it was pointed out to me that this is a feature, not a bug.

I don't see the benefit of having files as part of the API either. To me, files are a storage engine, but not a necessity so there shouldn't be any "file" in any interface.

For people interested in CMI, and as @hejrocker pointed in #2, I'd like to stress the fact that "reload config files from disk and react to changes" is the one last glaring hole in the API. E.g. for the scope of the current patch (image styles) : the computed image files for a style should be wiped when the definition of the style changes. The current state of the CMI API has no way to detect that.

Yep, having hooks for reactions is a really critical feature.

+++ b/core/includes/config.inc
@@ -0,0 +1,718 @@
+function config_get_verified_storage_names_with_prefix($prefix = '') {
+  return DrupalVerifiedStorageSQL::getNamesWithPrefix($prefix);

I find it confusing that parts of the code are in the Drupal namespace and parts are in the config namespace. Any reasons for that?
I'd have expected a single namespace.

+++ b/core/includes/config.inc
@@ -0,0 +1,718 @@
+/**
+ * Represents the default configuration storage object.
+ */
+class DrupalConfig {

Again, it's strange to have config() and DrupalConfig.

+++ b/core/includes/config.inc
@@ -0,0 +1,718 @@
+  /**
+   * Saves the configuration object to disk as XML.
+   */
+  public function save() {
+    $this->_verifiedStorage->write(config_encode($this->data));
+  }

Why encode the data before passing it on? I'd assume it's up to the storage mechanism whether the data should be encoded in XML or not?

Also, that pattern of config-objects with an associated storage system is very similar to entities + their storage mechanism. At least, we should compare solutions to learn from each other.

+++ b/core/modules/image/image.module
@@ -331,45 +322,6 @@ function image_file_predelete($file) {
@@ -493,41 +445,19 @@ function image_styles() {

@@ -493,41 +445,19 @@ function image_styles() {
     else {
       $styles = array();
 

Caching (static + permanent), as well all the associated CRUD hooks is something what the entity api already takes care of. Not sure if there are ways to benefit from each other, but maybe exposing config-objects as entities via a DrupalConfig entity storage backend would make sense? At least, we should streamline all the CRUD function names and hooks with those of entities for DX I guess.
Howsoever, I'd like to see the config system to take care about caching for me.

+++ b/core/includes/config.inc
@@ -0,0 +1,718 @@
+function config_get_verified_storage_names_with_prefix($prefix = '') {
+  return DrupalVerifiedStorageSQL::getNamesWithPrefix($prefix);

I find it confusing that parts of the code are in the Drupal namespace and parts are in the config namespace. Any reasons for that?
I'd have expected a single namespace.

That said, the term "DrupalVerifiedStorage" confuses me too. What makes the storage verified?

+++ b/core/includes/config.inc
@@ -0,0 +1,718 @@
+/**
+ * Represents the default configuration storage object.
+ */
+class DrupalConfig {

Again, it's strange to have config() and DrupalConfig.

+++ b/core/includes/config.inc
@@ -0,0 +1,718 @@
+  /**
+   * Saves the configuration object to disk as XML.
+   */
+  public function save() {
+    $this->_verifiedStorage->write(config_encode($this->data));
+  }

Why encode the data before passing it on? I'd assume it's up to the storage mechanism whether the data should be encoded in XML or not?
That said, is it planned to support dedicated storage mechanisms per config object, e.g. to put node-types or any other exportable in their own db-table? That's important to don't have to serialize big-chunks of xml to load a single config object, e.g. a view.

Then another random question: Assuming nodes types become config objects too, how would a module expose an additional property to the config object?

diff --git a/.htaccess b/.htaccess
index a69bdd4..398f960 100644
--- a/.htaccess
+++ b/.htaccess
@@ -3,7 +3,7 @@
 #
 
 # Protect files and directories from prying eyes.
-<FilesMatch "\.(engine|inc|info|install|make|module|profile|test|po|sh|.*sql|theme|tpl(\.php)?|xtmpl)$|^(\..*|Entries.*|Repository|Root|Tag|Template)$">
+<FilesMatch "\.(sig|xml|engine|inc|info|install|make|module|profile|test|po|sh|.*sql|theme|tpl(\.php)?|xtmpl)$|^(\..*|Entries.*|Repository|Root|Tag|Template)$">
   Order allow,deny
 </FilesMatch>

I'm not sure, but if this is the root folder, I guess that I cannot share .info, .xml, .sig, .po, .sh or .sql files anymore using my Drupal blog: this is not something we can allow to be released! Hiding PHP or VCS files is legit, but disallowing all of theses seems radical to me.

In fact anything that is not PHP or . folders should be kept public: the real problem is Drupal not being outside of webroot, not the opposite.

If I'm not mistaken, please move the file match disallow rules into a specific .htaccess into the config folder, but not everywhere else.

This initiative seems to have developed so far with primary reference to site staging and deployment use cases.

A second set of use cases relevant to configuration storage relate to bundling and distributing sets of configuration-based functionality, as is done in Drupal distributions. This second set of use cases raises a number of distinct questions and issues. Perhaps these have already been explored, but I haven't found where. It seems important to make sure we're explicitly capturing and addressing configuration needs relevant to distributing configuration.

To begin to scope these, I've posted a wiki page at http://groups.drupal.org/node/205673. It could use attention both from Configuration Management Initiative contributors and distribution and Features maintainers. From there, potentially, considerations can be moved back to this issue or additional issues can be posted.

Assigning to myself for a closer look. This shouldn't stop other reviews from happening.

So I looked at this and I'm already very happy with this. Great job.

First and foremost, I think the design and architecture are sound -- it is really not complicated.

I think that (for now), it is OK to require an explicit reload when configuration files are changes on disk. This means that there should be a button in the administration page that explicitly reloads configuration from disk into the active store. I don't think we should support dynamic reloading of configuration files at this stage. Similarly, in the other direction, I'd just write configuration changes in the UI immediately to disk -- at least for the time being.

Let's go with the minimal viable even and than we can worry about more fine-grained caching strategies in follow-up patches when we can do some actual benchmarking and measurements. Different caching strategies could exist; and I can see us exposing a couple of alternatives through, guess what, some configuration settings.

I have a crazy proposition:

Do we need to keep the idea of an installation profile after this patch lands in core?

I mean, most of what we use install profiles for is configuration of a Drupal site in some predefined way. With the file based configuration api, it seems like the kinds of applications that we build as installation profiles currently would best be distributed as a bundle of configuration files.

+++ b/core/includes/config.incundefined
@@ -0,0 +1,718 @@
+      $verified_storage = new DrupalVerifiedStorageSQL($config_name);

Do we need 'Verified' in the name? It seems redundant.

Do we need to prefix classnames with Drupal? It may be noise, especially as we add more classes?

Why not call this 'ConfigSQLStorage' or so?

Looks pretty clean to me. Nice work!

I raised an eyebrow at the format gymnastics here. Do we really need to json_encode and decode right afterwards?

function config_decode($data) {
  if (empty($data)) {
    return array();
  }
  $xml = new SimpleXMLElement($data);
  $json = json_encode($xml);
  return json_decode($json, TRUE);
}

Similarly, we I guess we can't get pretty XML with just simpleXML extension? I do agree that pretty is important.

function config_encode($data) {
  // creating object of SimpleXMLElement
  $xml_object = new SimpleXMLElement("<?xml version=\"1.0\"><config></config>");

  // function call to convert array to xml
  config_array_to_xml($data, $xml_object);

  // Pretty print the result
  $dom = new DOMDocument('1.0');
  $dom->preserveWhiteSpace = false;
  $dom->formatOutput = true;
  $dom->loadXML($xml_object->asXML());
  return $dom->saveXML();
}

I noticed that copyToFile() method below is actually reading $data from DB even though it knows we know the value of $data in this method. Perhaps optimize this.

public function write($data) {
    $this->writeToActive($data);
    $this->copyToFile();
  }

Some more comments -- more to come.

#1.

+++ b/core/includes/config.incundefined
@@ -0,0 +1,718 @@
+/**
+ * Moves the default config supplied by a module to the live config directory.
+ *
+ * @param
+ *   The name of the module we are installing.
+ */
+function config_install_default_config($module) {

I think it would be good to explain _why_ we want to duplicate configuration files. This is somewhat unusual but I can see why it could matter for security purposes.

#2.

+++ b/core/includes/config.incundefined
@@ -0,0 +1,718 @@
+ * Represents the signed file storage interface.
+ *
+ * Classes implementing this interface allow reading and writing configuration
+ * data to and from disk, while automatically managing and verifying
+ * cryptographic signatures.
+ */
+class SignedFileStorage {

I think it would be good to explain why we want to cryptographically sign this file.

#3.

+++ b/core/includes/config.incundefined
@@ -0,0 +1,718 @@
+/**
+ * Defines an interface for verified storage manipulation.
+ *
+ * This class allows reading and writing configuration data from/to the
+ * verified storage and copying to/from the signed file storing the same data.
+ */

Again, don't really understand what it means for the storage to be 'verified' -- and whether it is important enough to expose in the class name.

#4.

+++ b/core/includes/config.incundefined
@@ -0,0 +1,718 @@
+  /**
+   * Implements DrupalConfigVerifiedStorageInterface::write().
+   */
+  public function write($data) {
+    $this->writeToActive($data);
+    $this->copyToFile();

I thought it was a bit unbalanced that we 'write' to the active store, but that we 'copy' to file.

Two possible alternatives:

  $this->writeToActive();
  $this->writeToFile();

  $this->writeToCache();
  $this->writeToFile();

Do we need to keep the idea of an installation profile after this patch lands in core?

Short answer is no... at the risk of bikeshedding, the reason I say no is that install profiles (especially for distribution on drupal.org) are a lot more than a few configuration directives; they're packages of modules, patches, themes, etc. that are wrapped up and often modified by the install profile itself to make site setup (and setting of configuration options) an easier task.

I'm not at all sure about writing straight back to files either, especially since it seems that's potentially going to make things harder rather than easier. Is there an issue where this is being discussed in more detail?

While I hate to reopen this topic, I honestly don't think many people are going to cry if we get rid of it and move to JSON at this point. I am a bit reluctant to commit this first patch without resolving this issue, as we would have to go back and convert a lot of default XML to JSON. Any comments here greatly appreciated.

I think some people might cry with joy ;) No complaints from me at all.

"In IRC, webchick mentioned that one of the things she wanted to test first was to push new configuration to a site, reload it, and see it take effect. That functionality doesn't exist right now and she said it makes it harder to test and grok. Do we consider this a requirement for the first patch?"

i have some basic code for this in the 8.x-file-config-reload branch. i don't like it, because the write-to-file-on-all-changes decision just makes this scary.

i don't agree with #43 - i don't think this design is good. we have no answer to the basic reload use case that webchick asked for. we have no answer to 'we have no way to stop your pushed changes from being overwritten'. i've said that before, and i'll shut up if i'm the only one who thinks so.

Thoughts... I apologize in advance if what I write has been discussed before somewhere else and I'm not about to pretend I know what the proposed code is exactly doing.

- "Pushing new configuration to a site" and "write-to-file-on-all-changes"
I normally take this to mean copying configuration files from an authoritative master (i.e., staging) and using something like rsync to put the files on a production instance. Not something Drupal should handle because it can't assume the underlying architecture of the production operation system (I imagine someone creating a module that would push/pull configuration from a master to production using a web-based service as transport.). I think what you may mean by "pushing new configuration" is writing out a new set of configuration files based on whats in an "active store" (which did webchick mean?). I don't know what the write-to-file-on-all-changes decision was about, but as a change manager I'd want to always be in control of when new configuration files are written out because each new configuration set represents a baseline and I'd want to determine when baselines are established (like maybe you don't want to automatically commit to your VCS and tag it as the next version after saving a file). Equally, I'd always want to be in control of when new configuration is loaded into an "active store" as it is a potentially service impacting event. My push process would probably send a reload command after files are copied into place. I would have little need to write new configuration files on a production instance because I've designated my authoritative master to be where such changes are managed (unless production _is_ my authoritative master and I'm writing to files in order to back them up or something). When I push (rsync) new files to production, I always intend to overwrite whatever is in production's "active store". If there's a service impacting issue related with configuration (maybe dblog got left on and the server suddenly got real busy), I'd want to make changes on the fly on the production instance. I might want to do this by editing a config file on production and reloading it or I might want to do this by directly affecting the "active store". During this time, I'd want to disable anything that automatically overwrites the production instance's config files or "active store" temporarily, depending upon however I've architected the production operation system.

- Reload it and see it take effect
Absolutely this is the core functionality that I think most people would expect to see from the CMI initiative but I think it useful to divide "making configuration changes" from "reading and storing configuration". Different types of changes necessitate different implementation methods as some changes are simple assignments while others require a little bit of scripting intelligence/conditional checking/contextual awareness (cf., a variable change vs. adding a new field to a content type vs. adding a new view which queries a new content type). It seems beyond the scope of a subsystem that "reads and stores configuration" in files to handle actual change implementation. Who knows, there might be use cases when an operator won't even use files to read and store configuration in. Maybe reading configuration is just a series of JSON responses from a master server (scary thought?). As for "making configuration changes", change management always involves making changes from a baseline, which is theoretically always known ahead of time. I can't help but always think of hook_install, hook_update_n, etc., when this subject comes up. These functions assume a specific baseline (how many thousands of errors have been thrown when someone's database schema didn't match the baseline hook_update_n assumed? :) ).

@heyrocker: JSON as a config format would have the advantage of being more common in PHP projects from what I've seen than XML these days, and would be consistent with using Composer to replace info files should we go that route (cf, #1398772: Replace .info.yml with composer.json for extensions). On the other hand, I am increasingly convinced that we are going to need an XML canonical serialized entity format (cf, http://groups.drupal.org/node/197583), even if we also have a JSON version. So really, we're damned if we do, damned if we don't. I can live either way. Based on what you've found with the difficult of working with XML, I'm thinking you're probably right that JSON is less-bad than XML at this point.

@Sborsody: The basic idea is that we shift from a "database is always right for config, and sometimes we hack in a way to push it to code so it can be checked into git" model (as now) to a "config lives natively on disk where it is accessible to version control, but we also have an in-DB (or similar) copy of it for speed and to allow for diffing after doing a git pull on production". That DB copy is the "active store". Of course, keeping those two copies in sync creates all sorts of fun and exciting questions that are still being worked out in this thread. The canonical baseline for a configuration state in this model becomes a tag in your VCS.

@Crell

Yes, that's the goal, to have the configuration live in a file so it can be checked into a VCS and tagged in order to establish baselines. The secondary purpose of using files is to package configuration (in a standardized format) into an easily transportable form between different environments. That makes it great for those of us building and maintaining/upgrading/expanding sites or managing a staging/prod system. It may not be so great for some operator who sees no need to write configuration to a file. Maybe the operator is just someone running a blog and they don't use any staging environment thus have no need to transport files between environments and they don't use any VCS to manage changes. Or maybe the operator likes to use Services to transport configuration. I think the decision of when to write from active store to files needs to be left up to the operator. Any kind of "write-to-file-on-all-changes" function should be ...wait for it... configurable.

OK, great. I apologize if I bring some use case up that has been discussed elsewhere before. Just trying to think through possibilities and how I'd use this system.

Has the idea of having separate locations for reading and writing files come up? Like, someone is changing the active store and the change gets written out to an "export" file automatically, it won't overwrite someone trying to make changes via the "import" file.

I'm going to (temporary) un-assign this from myself as I've signed off on the approach above and given my initial set of feedback. I want to do a second, more detailed review, but I'll give heyrocker and the others time to incorporate the community's outstanding feedback first. Until then, this doesn't need to be assigned to me. Feel free to re-assign it to me when you want my opinion.

I've added #1431292: Migrate date format configuration to CMI (postponed for now) which would be the first to tinker with once we are in a state to work with migrating configuration to CMI, because that is the prime example of translatable config in core (and is also small enough to be doable in a short timeframe while demonstrating how it would work).

Also marking with D8MI tracking tags because this is a required pre-requisite (not expecting/advocating we add language support right away in this first patch!).

So, I'm actively using this at this point and it'd be really nice if we could get all the values in a config object simultaneously. Currently working to implement this in a plugin solution for blocks and there are definitely instances here where we push the block's configuration down to the theme layer. Passing a stdClass down from a certainly point would be really helpful. I was thinking adding to DrupalConfig something like this:

  public function getData() {
    return (object) $this->data;
  }

I think that would make people's lives a lot easier in many places, especially if we're in a situation where we're not 100% sure of all the keys that might exist and would like to offload decisions about that sort of thing.

I reviewed the patch and the discussion since its posting. Here are my thoughts.

We switched to XML for numerous reasons, including better UTF-8 support (versus inconsistent standards about escaping characters in JSON), support for comments (rather than a Drupalism around JSON keys), consistent IDE editing support, and extensible attributes on tags (potentially useful for i18n and other context). If we get all of that at the cost of some silly-looking code around loading and saving, I think that's a net gain. The only other decent alternative is Yaml, which is prettier (than even JSON), gaining ground as a configuration format, and is less limited than JSON. Unfortunately, there's no native PHP support for the format.

I'd be okay with switching out "verified" for some other moniker like "active" or "validated" in our class names. But, as heyrocker explains, it's an important distinction to maintain in some way.

I'd like to take on-disk formats to a higher level of canonical-ness by sorting the keys. Example files should also get updated to be in canonical format. This will minimize the diff in version control, a design goal we have.

The SQL store should probably just use serialized PHP. It's faster than JSON, and the copy in the database isn't supposed to be human-friendly.

The rest of the patch looks good. Other than XML and putting signatures in separate files, I don't see many functional differences versus what we designed in Denver. I'm glad to see some integration with core.

Fix tag deletion.

Fixed SimpleTest integration.

We talked about this patch at length at the WSCCI sprint over the weekend. We spent 4+ hours discussing the current patch. Specifically, we talked about the architecture and suggested some changes to the storage mechanism (i.e. move to a re-usable key-value store). We also discussed some of the known issues such as (1) internationalization support, (2) re-loading of configuration, (3) the various security aspects, (4) upgradability, (5) reacting to configuration changes, etc. (Greg and others are going to share more details on what we discussed.)

After a lot of discussion, we decided that none of the known issues are show stoppers for a first patch to go in -- other than writing tests and making all the core tests pass. Fixing the known issues can happen in follow-up patches as we don't really expect these to change the API in a big way. In other words, the plan and consensus is to commit this patch once the tests are in place and all pass. It will help other work on Drupal 8 make progress.

+++ b/.htaccessundefined
@@ -3,7 +3,7 @@
+<FilesMatch "\.(sig|xml|engine|inc|info|install|make|module|profile|test|po|sh|.*sql|theme|tpl(\.php)?|xtmpl)$|^(\..*|Entries.*|Repository|Root|Tag|Template)$">

Adding xml to the deny list is going to prevent modules like XML sitemap from working at all. I have seen this has been discussed previously but I don't see this is actually been addressed? Seems like these config file type .htaccess restrictions should live in the actual config directories?

16 days to next Drupal core point release.

The .htaccess bug on .xml files was fixed long ago in my to-be-merged branch already...

#75 did not contain all of @heyrocker's changes in #73. Please make sure you merge the latest 8.x-file-config main branch before rolling patches ;)

Attached patch fixes some more things, and adds a first chunk of basic CRUD tests. All individual changes can be found in my sun-config branch. The interdiff is against cmi's main branch, not against the previous patch.

Attached patch resolves almost all test failures. (but not all)

However, I had to add an insane amount of critical @todos, which I wasn't aware of before:

- Config values are not casted to strings (despite promised).
- Config keys are not casted to strings (although promised, too).
- XML can be invalid and not parse-able for many reasons.
- Config keys are not validated/sanitized.
- config()->clear() should really be ->unset().
- Configuration must not be additionally cached by a module in any way (static cache / database cache).
- Some modules invoke drupal_alter() on configuration (e.g., image styles).
- Need a way to list config object names/suffixes _after_ a specified prefix.
- Need a way to determine whether a config object exists.
- Some modules might have a valid use-case for retrieving/listing config objects using a wildcard within the name (instead of only searching by prefix).
- The key of a retrieved value is unknown; since you only get the value. Configuration values (or sub-values) may be passed forward to another function/callback, and thus, that function no longer knows about the key of the value. (unless the key is contained in the value, which is a very very wonky duplication)
- Config keys must not contain periods (within a specific key).

For the generic key/value store there is #1202336: Add a key/value store API open. I'd love to see work on that (or find time to do it), but definitely not a pre-requisite.

+++ b/core/includes/bootstrap.incundefined
@@ -239,6 +239,9 @@ const REGISTRY_WRITE_LOOKUP_CACHE = 2;
+// Will go away when we've converted to PSR-0
+require_once DRUPAL_ROOT . '/core/includes/config.inc';

Hasn't that been done?

Or does this mean that the procedural functions in includes/config.inc will be converted to a PSR-0 class?

+++ b/core/includes/bootstrap.incundefined
@@ -1342,8 +1345,10 @@ function drupal_page_header() {
 function drupal_serve_page_from_cache(stdClass $cache) {
+  $config = config('system.performance');
+
   // Negotiate whether to use compression.
-  $page_compression = variable_get('page_compression', TRUE) && extension_loaded('zlib');

I'd like to benchmark/profile this (ideally both) to see how this impacts page caching performance before we commit. I'm OK with committing a regression to get this in - but let's know if there is one and how much and have an issue open to resolve it if so.

+++ b/core/includes/config.incundefined
@@ -0,0 +1,256 @@
+  $dom->loadXML($xml_object->asXML());
+  // @todo The loaded XML can be invalid; throwing plenty of PHP warnings but no

Oh it is catchable if we do this :)
#1247666: Replace @function calls with try/catch ErrorException blocks

I ran out of time, I'll try to look more soon.

dreditor status change.

Attached patch fixes the ImageDimensionsUnitTest once for all.

Interdiff is against #79.

Alrighty, @heyrocker merged my branch into the mainline. Did some minor follow-ups on that to reduce the patch size.
Interdiff is against the 8.x-file-config branch.

@beejeebus wants to look into the remaining Image module test failure tonight (which I can reproduce locally, but fail to explain).

I'll keep on working on completing basic CRUD tests tomorrow.

Terribly sorry, ->set() was not chainable yet. Fixed those new test failures.

Rerolled around #1050746-70: HTTPS sessions not working in all cases

Re-rolled, taking all changes into account since #87.

@Dries

"Specifically, we talked about the architecture and suggested some changes to the storage mechanism (i.e. move to a re-usable key-value store)."

This sounds like it ought to be created as a persistent derivative of the cache API. (Alternatively, the cache API could be a non-persistent derivative of the key/value store API.) This would also be a great place for session data, form tokens, etc.

"We also discussed some of the known issues such as (1) internationalization support, (2) re-loading of configuration, (3) the various security aspects, (4) upgradability, (5) reacting to configuration changes, etc. (Greg and others are going to share more details on what we discussed.)"

I would definitely like to see those comments. ETA?

This just changes image_style_form_submit(), so that in a rename, the new name is saved first, then the old deleted, so that the new name can be passed to the delete function for updating image field formatter settings. Locally, that makes the remaining 2 tests pass for me. If bot comes back green, I'll post just that change to the CMI sandbox issue queue.

[Edit: looks like @heyrocker already committed my changes to the sandbox, so no follow-up issue there needed.]

@David Strauss: While I agree with your thinking and initially had the same considerations, cache backends actually use a schema that is more than a simple key/value store. A cache backend at the very least needs to record and track when a cache item was created and when it expires (and be able to query/filter all items based on that), and the backend also needs to support advanced methods like flushing all items or deleting all items by prefix. While that could be an extension of a mere key/value store, the concept already sounds vastly different merely based on that. But I'd agree we totally should investigate that direction.

Let's talk about that in #1202336: Add a key/value store API.

re: #103:

1. Custom caching of configuration

   Can only lead to bugs, really. The config system does not provide a module hook to allow modules to react when configuration is updated. Thus, if a module uses an additional static/db caching layer around the configuration value(s), you can be 99% sure to work with potentially stale and outdated configuration data.

   You'd never ever cache the results of a variable_get() either. Wrapping config lookups into a cache is fundamentally wrong.

   This inherently means that the config system needs to be as performant as variable_get(). I don't see why that shouldn't be the case already, or at least, as we've already discussed, we most likely want to switch the DB store to use serialized PHP values instead of XML (which needs rather performance-heavy encoding and decoding).

2. Knowledge/discovery of the key of a nested value

   The problem is not related to the file format in any way; it would be identical with JSON or whatnot. The issue is that when retrieving a nested key of a configuration object, and passing that key's value forward to some other function, then the other function no longer knows about the original nested key within the configuration object.

   The recent fixes for Image effect tests demonstrated the issue. Without adding the nested key's keyname to the value that is passed forward or saved, functions being called down the line are not able to regain the nested key's keyname from where the value originates from.

   However. After further discussing that issue with @beejeebus in IRC, I strongly believe that code like the Image Effect API is something we do not want support in the first place.

   That is, because with the changes of this patch, the Image Effect API is now a relatively stupid wrapper around the Image Style API. The effects are contained in a style. This means that the Image Effect API tries to perform CRUD operations and manage a deeper section within a configuration object.

   To put this into relation, consider the following scenario:

   • You have two APIs.
   • Both APIs manage configuration variables.
   • The first API sets and gets variables as usual.
   • The second API retrieves a value in an array key via variable_get() belonging to the first API, and updates that value (and the entire variable) with variable_set().

   You'd never design an API to manage value within another variable in the current system. ;) And so I believe the idea of doing so is bogus with the config system, too.

   Effectively, we want to remove pretty much all parts of the Image Effect API functions, and adjust the entire Image module code to work on image styles only. Since we fixed all test failures already, that can be a follow-up issue.

3. drupal_alter() on configuration

   I actually think that this is totally bogus, too. Again, would you ever do the following?

     $value = variable_get('mymodule_something');
     drupal_alter('mymodule_something', $value);
   

   I don't think so. Altering configuration on the fly via drupal_alter(), whenever it is loaded, looks completely nonsensical to me.

   AFAICS, this (nicely) forces modules to actually update configuration when they need to add or alter something. This will definitely have a lot of implications on all of the existing APIs involving configuration currently, but I strongly believe it's a unforeseen, implicit step into the right direction. We actually had a lot of discussions on DrupalCons and in IRC around this topic in the past, and if I'm not mistaken, pretty much everyone agrees that it's what we always wanted and needed to do, but never came around to implement. :)

   In other words, we want to remove that drupal_alter(). As that is a precursor for a relatively huge architectural change, I could see the potential requirement to figure out the replacement code/behavior for such drupal_alter()s in this first patch. Personally, I'd also be happy with a follow-up issue though.

Re #106.3: field_get_display() is one use-case where an alter step is absolutely necessary, because run-time information may influence what's desired above and beyond what's saved as the configuration. But I think it makes sense for now for such alters to be called from the client code, and not have the alter embedded in the config system. Especially since such use-cases will almost always require additional context beyond what the config system is aware of.

I'd say an alter() hook on stuff that is loaded (from the db in D7-, from db or config files in D8) and can be saved back is nonsensical to begin with - on next save, the altered stuff gets merged in and becomes the "official" version, that gets re-altered on next load... Larsen effect :-)

I think that the only things that currently have alter() hooks are :
- stuff defined in code : forms, render arrays, menu router paths, views data mapping, *_info() hooks...
- run-time stuff that do not get saved back : e.g. the field_get_display() example @effulgentsia mentioned (adjust the display settings used for a field just before it gets rendered - but do not touch the $instance definition)

That's the very reason why I always resisted a hook_field_alter($field), btw. We don't know how to handle that pattern.

In fact, I'd say this distinction is part of the heuristics about "what goes in config files / what goes in code". The content of config files cannot be altered through hooks in code.

(FWIW, It's also why I'm really unconvinced by the WSCCI / Plugins API proposal to move "list of available plugin implementations" from info hooks to config files - although I'm not sure whether that is still on the table in @EclipseGc's latest code)

Unintended tag change

re: 106.3: variable_get somewhat has an alter hook that modules are using: hook_init (or any hook really), because $conf was/is a global.

Modules using it: Spaces, Domain config, Strongarm.

I've also used it in custom module, for example, to change the maintenance theme to the currently active theme so batch operations don't suddenly switch themes (d6).

It's ugly hack but it's been needed, specially for per context configuration.

Feels a bit odd to have #1444620: Remove file signing from configuration system as a followup (and not a part of this issue), since we are then adding code that will get removed right away.

I see the alter discussion starting up in other places as well (for example, by nedjo the Distributions groups page he linked to).
Note that right now complex configuration such as Views and Rules (which I envision using CMI one day) is alterable.
This allows a Commerce submodule to alter a default rule to add its own condition, or at least disable the rule so that another one can take over.
The solution depends on general "overriding" ideas and functionality that gets built over time. A great option would be having the ability to say "module X made this change. The user made these other changes". If not, at least having a simple alter when the configuration gets moved from the module to the central place would make it behave close like the current implementation (module gets uninstalled, the configuration can get reverted). Can't say it's very clean or nice, though.

It is a complex problem that doesn't need to be solved by the first few patches, but it is definitely something that needs to be discussed (from here to Denver).

@heyrocker: newbie question here:

When CMI is in full effect should we be pulling all values of all variables from the $config object?

For example, in _drupal_bootstrap_page_cache() we have

$config = config('system.performance');
$cache_enabled = $config->get('cache');

...but later we don't use the $cache object to do ...

      if (variable_get('page_cache_invoke_hooks', TRUE)) {
        bootstrap_invoke_all('boot');
      }

See? We're doing a variable-get here instead of pulling the value from $config (which I assume is because the value of 'page_cache_invoke_hooks' isn't stored there.)

Why were the assertion messages ('Expected img tag was found.', etc.) removed in #77 by sun (I suppose)? Aren't those useful?

#117 @cosmicdreams: All variable_get()s and variable_set()s will be converted to this configuration system after this patch lands.

At this point, however, we didn't figure out yet how to deal with "internal" variables like the one you mentioned. That is, because it exists via $conf in settings.php only (which is early-early-bootstrap, not to say, pre-Drupal bootstrap), cannot be configured through the UI in any way, and thus rather maps to a global variable (instead of a $conf value). We'll figure out those edge-cases along the way.

All other $conf values are regular variables (variable overrides, to be exact). This hack to override variables won't be supported anymore, AFAICS.

#118 @Tor Arne Thune: Test assertion messages like that are hiding essential debug information that is important in case a test fails, since you do not see why the test fails. I'm actually trying hard to educate people that custom assertion messages should only be used when the default assertion message is bogus. (the raw, asserted values are helpful, not bogus)

I had to do this particular tweak for this exact test twice (for another patch) in the meantime. In both cases, the custom message prevented the developer from seeing what's actually wrong.

Okay, fair enough. Good to know for the future.

Dang, I went through the latest patch with Dreditor, but about halfway through, Chrome decided that using my trackpad to scroll down was the same thing as the "back" gesture, so I lost everything.

I don't have time to go through it all again, so here's only a few notes:

+++ b/core/includes/config.incundefined
@@ -0,0 +1,253 @@
+/**
+ * @todo
+ *
+ * @param $prefix
+ *   @todo
+ *
+ * @return
+ *   @todo
+ */
+function config_get_names_with_prefix($prefix) {
+  return config_get_verified_storage_names_with_prefix($prefix);

Why do we need a wrapper function here? Why not just call config_get_verified_storage_names_with_prefix() directly?

+++ b/core/includes/config.incundefined
@@ -0,0 +1,253 @@
+/**
+ * Decodes configuration data from its native format to an associative array.
+ *
+ * @param $data
+ *   Configuration data.
+ *
+ * @return
+ *   An associative array representation of the data.
+ */
+function config_decode($data) {
+  if (empty($data)) {
+    return array();
+  }
+  $xml = new SimpleXMLElement($data);
+  $json = json_encode($xml);
+  return json_decode($json, TRUE);

People that have been following the CMI conversations know why this function behaves the way it does, but I'd like to see a bit of documentation that says why we need to create a SimpleXMLElement, then json_encode and decode it.

+++ b/core/includes/config.incundefined
@@ -0,0 +1,253 @@
+  // creating object of SimpleXMLElement
+  $xml_object = new SimpleXMLElement("<?xml version=\"1.0\"?><config></config>");
+
+  // function call to convert array to xml

Coding standards: Should start with a capital, and end with a full stop. There's also trailing space in a few locations throughout the patch. This patch should probably not be held up on coding standards: we can capture that in followup patches, but if somebody has to reroll it, maybe try to fix those (there's only a handful of them).

+++ b/core/lib/Drupal/Core/Config/DrupalConfig.phpundefined
@@ -0,0 +1,198 @@
+    // @todo Reverse this and throw an exception when encountering a key with
+    //   invalid name. The identical validation also needs to happen in get().
+    //   Furthermore, the dot/period is a reserved character; it may appear

There's a few places throughout the patch where there's @todo items inside functions (rather than in the function docblock). For some reason, I thought that having @todo comments inside functions wasn't allowed by the coding standards or by the phpdoc spec, but I could be wrong.

Out of the issues I mentioned above, I think only the wrapper function issue is a blocker for this patch (unless there's a good reason for it, that is).

with #116 passing the test suite, what does this patch need? User testing? If so I can give this a go tonight.

+++ b/core/includes/config.incundefined
@@ -0,0 +1,253 @@
+ */
+function config($name, $class = 'Drupal\Core\Config\DrupalConfig') {
+  // @todo Replace this with an appropriate factory.
+  return new $class(new DrupalVerifiedStorageSQL($name));
+}

There's a couple of things that bother me about this. These don't block a commit, but I'd like us to have a follow-up issue (if one doesn't already exist) so we can resolve it earlier rather than later.

First, the storage object is instantiated for each $name. Normally with storage stuff we have a single instance (database, cache) then it's up to that to fetch data as needed. If we changed this, it'd mean the various crud methods on the storage class would need a $name method. So that'd be an API change for storage engines, but not for anyone else.

This would allow a storage class to do similar stuff to DrupalCacheArray (i.e. cumulatively cache requested config objects over time). Or just for static caching of configuration that could be handled by the storage class. Without having to do it static class properties or similar. Also if we move to dependency injection, I'd expect a single class to be passed around (although that class could be a factory that individually instantiates the storage backend each time but that feels a bit odd).

I'm also not sure why we're allowing the caller to specify which config class gets returned or what the use case for that is. I can think of use-cases for the storage engine to swap out the config class (i.e. one that actually calls back to storage rather than holding any data at all or something), but not so much the caller. There's also no explanation of why you might want to specify a different class in the phpdoc so that doesn't help to figure it out.

6 days to next Drupal core point release.

I was thinking more like:

<?php
function config($name, $class = 'Drupal\Core\Config\DrupalConfig') {
// @todo factory.
static $storage_class = new DrupalVerifiedStorageSQL(();
return $storage_class->get($name, $class);
}

So the end-user API is exactly the same, but the storage API changes.

Agreed with @catch in #127. I don't think the public API needs to change for this. We merely change config() to act more like a factory with regard to the storage backend.

This means that only the internals of each individual storage backend will change -- collecting the requested keys on an internal property and operating on a key of that property, instead of instantiating an entirely new storage backend class instance for every config object that is operated on. Most of that logic can likely be forked from the existing CacheArray class.

So is the changes requested in #127-129 all that is needed for this?

Sounds like catch's main concern was around how the storage API is figured out. This was responded to in #129 as something that could be a follow-up with other class re-architecture, since it shouldn't impact the public-facing API. This would still allow us to get something committed that other D8 work could build from. I asked heyrocker where the best place for that discussion was, and he said it was at http://drupal.org/node/1202336#comment-5625320. (#1202336: Add a key/value store API)

So given we have a follow-up for that discussion, given that we have to example implementations, given that this system has been reviewed at various points by dozens of very smart people, and given that we have an entire gaggle of tests for CRUD operations and various data types, I'm going to be bold and mark this patch RTBC.

Not that it matters much, but I second the RTBC. The patch is good enough right now and, as other have noted, committing it will unblock other work on D8.

I'm a little confused of what's the latest position in regard of caching an reactions.

From #106:

Custom caching of configuration

Can only lead to bugs, really. The config system does not provide a module hook to allow modules to react when configuration is updated. Thus, if a module uses an additional static/db caching layer around the configuration value(s), you can be 99% sure to work with potentially stale and outdated configuration data.

You'd never ever cache the results of a variable_get() either. Wrapping config lookups into a cache is fundamentally wrong.

Sure, that's fine for the variable_get() use-case. But for the configuration system to be usable for storing configuration objects we need custom caching functionality on top of it. The same for reactions. Is it still the plan to work on that as follow-ups?

Related, please also make sure there are hooks that allow modules to act when a bunch of configuration objects is read into the active store. I recently ran into the need for this when working with the exportable entities of the entity api module, which kind of already implements the active-store idea by syncing all defaults into the database. More details at #1458568: efficiently rebuild caches when rebuilding defaults.

I think the storage back end should be responsible for caching if at all possible, that's part of the reason for #125/#126/127. If there's a need for custom caching over and above that then we need a proper follow-up issue to discuss it before this goes in.

If #1202336: Add a key/value store API is the issue to deal with caching in storage back ends this then we need to bump that to critical task since the absence of either static or persistent caching is going to regress performance once we start converting more things to the new API. I'm not sure that issue is really the right place to track this, but if heyrocker is happy with CMI relying on that issue then it's probably OK.

I'd like to profile this myself before it goes in - Greg's profiling flagged up the db_table_exists() issue, would like to see how a couple of other things look like before commit. We're still way over thresholds and I'd prefer if this didn't go in until we're under. Will probably leave the actual commit to Dries either way.

I think the storage back end should be responsible for caching if at all possible, that's part of the reason for #125/#126/127. If there's a need for custom caching over and above that then we need a proper follow-up issue to discuss it before this goes in.

I don't think that flies, as there are cases where the cache is special and does not correspond 1:1 to the configuration object storage. In Rules, configurations are processed and prepared for fast execution before they are cached. Also, there are custom aggregations like all rules that react on a certain event, which are aggregated to single event-set and cached for efficient execution when the event occurs. (Just getting all X rules would not be the same).
Not sure whether there are more modules that do something like that and have such needs, but at least Rules does.

I don't think supporting such "custom" caching layers is a big deal though, as all we need is hooks for reacting upon changes. And we are going to need that anyway if we want to be able to store bundle-objects as config objects as the field API has to be notified when bundles change, so it can care about the necessary db-changes.

Had a quick Skype call with @fago to bring him up to speed on the config system and also discuss recent comments.

Braindump:

• config CRUD hooks - module hooks - module config API functions - Config module API functions

  @fago was mainly concerned about "entity-ish" configuration objects (such as node types, image styles, rules). Modules currently provide API functions for loading, saving, and deleting configuration. Especially the save and delete API functions invoke alter, insert, update, and delete hooks in all modules. Additionally, Entity API module's exportables approach naturally and natively provides the old/previous/original configuration when invoking the update hook. The hooks and original config have many use-cases and are important for managing configuration in a modular system.

  We get behind the idea that the low-level config system should be able to work as independent as possible. Thus, the low-level config system should ideally have no dependency on the module system. We also discussed the idea of two implementations, an independent low-level Config class that would be extended into a DrupalConfig, which would then depend on the module system in order to additionally invoke module hooks - but in the end, the idea of invoking hooks for every single config read/save/delete sounded nonsensical. Rather:

  We can get behind the idea that module-specific configuration objects should be managed and maintained through a module's dedicated API functions. No real difference to now; if there's a module API function to read/save/delete a variable, then you better use that function and don't fiddle with the variable directly.

  When looking at the patch, it's relatively clear that most probably all module API functions for "entity-ish" configuration, such as the converted image style functions, will all look pretty much identical. We therefore see a potential for adding generic configuration CRUD functions to the Configuration module (config.module) at a later point, which would be wrappers around the low-level config system, but additionally have a dependency on and account for the module system, in order to harmonize those module API hooks for config CRUD operations. That would eliminate a lot of duplicate code (similar to the other entification changes currently happening for D8). Custom functions like image_styles(), image_style_save(), and image_style_delete() would entirely go.

  Can totally be a follow-up issue.

• reload hooks - module system

  The above shouldn't be confused with #1447686: Allow importing and synchronizing configuration when files are updated. The reload/synchronization operation will also need module hooks. That said, it's possible that above mentioned configuration CRUD API functions and hooks will be helpful/re-usable for the reload/synchronization operation.

• no static/db caching in user-space - no drupal_alter - only CRUD - not modular

  We agreed that any additional static/db caching layer on top of the config system can only lead to problems. Not to be mistaken with configuration performance optimizations -- the configuration system itself may, similar to some current subsystems, implement some advanced techniques for prefetching known-to-be-used configuration objects for a particular request, or any other means that improve its performance.

  We also agreed that drupal_alter() on configuration is nonsensical. @yched nicely clarified in #110 why.

  However, no drupal_alter() and whatsoever also means that configuration is effectively no longer "modular". Variables weren't either. But anything non-modular in a modular system always causes problems down the line. Above mentioned config CRUD module API functions might help to remedy that.

  Would be a good idea to remove the commented out code and also the left-in drupal_alter() in image.module (and image.api.php) as part of this patch.

• default config - install profiles - maintenance updates

  It wasn't very clear to @fago whether default module config would be retained within a module's config directory (and read from there). That part could probably use some better documentation. Speaking of, a @file or @defgroup config explaining the high-level functionality and architectural design of the configuration system would be a good idea.

  We weren't really sure how "default" (?) configuration for installation profiles would be handled. The current patch only cares for module-provided default config. Specifically, we brainstormed about how an installation profile would override a module's default configuration.

  With regard to all config, both default and custom/active, it's also not clear how maintenance updates would work. Both from a module perspective, but also with regard to installation profiles (as well as Feature modules, which, like installation profiles, will mainly consist of configuration + updates).

• explicit write to disk operation - versioning - locked config - locked config objects

  We discussed that the automated write to disk on every config()->save() doesn't fully make sense from a staging perspective. The automated write is suitable for the development site (on which you want to update the possibly versioned file config), but that same automated and unconditional write can hi-jack and break the architectural design idea of the envisioned reload operation, if it happens on the staging or production site, because that would potentially overwrite new but not yet reloaded config in the filesystem.

  At the same time, you may not want to completely lock your staging/production configuration, so you can still perform config changes there. In turn, the write to disk should be at least an opt-in/opt-out; in any case, not unconditionally be part of the config()->save() operation.

  That would get us much closer to the design idea of original (inactive) and overridden (active) configuration, and we'd have properly and cleanly encapsulated "reload" (synchronize) and "dump" (write-to-disk) operations in the configuration system.

  (Speaking of, @sun is still tempted to rename this issue into "Synchronized configuration system", which is the actual abstracted architectural design idea and goal.)

 /**
+ * Adds {config} table for new Configuration system.
+ */
+function system_update_8003() {
+  // @todo Temporary.
+  if (db_table_exists('config')) {
+    db_drop_table('config');
+  }
+  db_create_table('config', array(
+    'description' => 'Default active store for the configuration system.',
+    'fields' => array(
+      'name' => array(
+        'description' => 'The identifier for the configuration entry, such as module.example (the name of the file, minus .json.php).',
+        'type' => 'varchar',
+        'length' => 255,
+        'not null' => TRUE,
+        'default' => '',
+      ),
+      'data' => array(
+        'description' => 'The raw JSON data for this configuration entry.',
+        'type' => 'blob',
+        'not null' => TRUE,
+        'size' => 'big',
+        'translatable' => TRUE,
+      ),
+    ),
+    'primary key' => array('name'),
+  ));
+}

This mentions JSON (and even json.php) but the API is currently using xml.

Also there is no upgrade path for the converted variables and I don't see a follow-up issue linked (and that would have to be a critical bug since we'd be immediately breaking the upgrade path on committing this).

Ok, testbot came back with a pass at http://qa.drupal.org/pifr/test/231588 (not sure why this doesn't show up on the site yet).

catch's review was around comments and such, which were subsequently fixed in #140. Confirmed the patch applies to 8.x. Therefore, marking back to RTBC.

heyrocker's requested that this commit be done as a merge, rather than via a patch, in order to provide proper credit to everyone who worked on it. The commands for that (apparently) are:

git remote add heyrocker http://git.drupal.org/sandbox/heyrocker/1145636.git
git fetch heyrocker
git merge heyrocker/8.x-file-config

Assigning to Dries to take a look at.

And actually doing what I said I did now. :)

My review was not just around comments, we need a critical bug opened to add an upgrade path for the converted variables if it's not done as part of this patch. We already have plenty of critical upgrade path bugs open and are just about under thresholds, so there's space for one more, but let's not forget to open that.

The following looks like it would require a critical bug to be opened also:

+  // @todo This is actually causing a bug right now, because you can install
+  // without hitting install_settings_form_submit() if your settings.php
+  // already has the db stuff in it, and right now in that case your
+  // config directory never gets created. So this needs to be moved elsewhere.

This might need a little thought on how to solve, but I guess since both $config_directory_name and $config_signature_key are random strings, they could be dealt with the same way as $drupal_hash_salt is (in the case where it's not provided in settings.php)?

Alternatively, we could require that people who want to install Drupal without ever making settings.php writable by the web server must specify a $config_directory_name by hand (in addition to the database credentials which they must specify now).

Read up on all the new comments, looked at the code once more, and decided to commit this to 8.x. Epic win.

Great work everyone. See you in the follow-up issues.

Setting priority/category.

This can come off Dries's plate now.

What's the change notice that's needed? We have not yet removed variable_get().

That we added a whole file-based configuration system, with all its new parts? :)

Looks good :) The change notice does reference Drush, but that's not really a problem.

Hooray!

Moving the title back to ease future reference.

And, untagging. :)

Cross-posting #1508872: Image effects duplicate on edit which is probably related.