This is a tracking issue for the file-based configuration work going on as part of the configuration management initiative. The actual work on this is going on in the initiative sandbox in the branch 8.x-file-config, but this issue is for the purpose of providing periodic updates and occasional patches to demonstrate progress and help keep the issue on the radar of members of the core team. The goal is to provide a standard api through which configuration information can be stored, exported, and imported in a standard format (currently XML.) Prior architecture discussions on this issue can be found at http://groups.drupal.org/deployment-build-systems-change-management.

The goal for the initial patch is

- A full API for interfacing with configuration information
- At least one core implementation (ideally more though)
- Whatever work needs to be done at install-time to make this work

Anatomy

  • This is a configuration subsystem that intends to read and write configuration objects.
  • Configuration objects are basic key/value pairs. However, the value may be in a structured format, such as XML or JSON (or in more usual terminology, the value may use a Content-Type other than 'text/plain'). [« sun has an idea...]
  • Configuration objects can be stored in different storages. The subsystem supports multiple storage engines. Each storage engine has to implement methods defined on the ConfigStorageInterface.
  • One of the main purposes of the subsystem is to allow configuration objects to be stored in multiple storages (e.g., files and database) and be synchronized between them. This is primarily for performance reasons, as filesystem access and value parsing can be a bottleneck. It also allows for an explicit "reload" operation, which protects the system from things like syntax errors in the config files. The subsystem therefore differs between "active" storage and "non-active" storage.
  • Every configuration object is additionally verified through a signature based on its content.
  • The subsystem comes with a bare SignedFile handler that uses native PHP functions to read and write a file including its signature.
  • For Drupal, there is a base (abstract) DrupalConfigStorage storage engine, which partially implements the storage engine interface and provides additional methods that use the SignedFile handler to read and write files to disk. Its main purpose is to implement the synchronization between the file storage and the "active" storage. Most notably, it implements the write() and delete() methods, which save or delete a configuration object from both storages in one go.
  • There is a DrupalConfigStorageSQL storage engine, which Drupal uses by default as active storage. It reads and writes configuration objects to the SQL database. Very similar to variable_get(), variable_set(), and variable_del() in the past.
  • Lastly, there is a DrupalConfig class, which is the main application programming interface for Drupal modules and all code. It is instantiated and returned by the config() factory function.

In its current design, an instantiated DrupalConfig class only handles one configuration object. To access a different one, a new DrupalConfig has to be instantiated.

The internals can be pseudo-depicted like this:

$config = config('file.key');
           |
           ----------- new DrupalConfigStorageSQL('file.key')
                                    |
                                    v
   new DrupalConfig(DrupalConfigStorageSQL)
           |
           v
       ->read() <------------------------------------ DATABASE
           |
           v
     config_decode()
           |
           v
 ->set([property], [value])
           |
           -----------|= RESULT:  $config (DrupalConfig)
                                    ->storage = DrupalConfigStorageSQL
                                    ->data (array)
                                      [property1] => [value1]
                                      [property2] => [value2]
                                      ...


$value = $config->get('name');
                   |
                   v
        drupal_array_get_nested_value($config->data, array('name'))


$config->set('name', 'value');
           |
           v
   drupal_array_set_nested_value($config->data, array('name'), 'value')


$config->save();
           |
           v
     config_encode($config->data)
           |
           v
  DrupalConfigStorage->write($data)
           |
           -----------> DrupalConfigStorageSQL->writeToActive($data) ------> DATABASE
           |
           v
     ->copyToFile()
           |
           -----------> new SignedFile('file.key')
                                   |
                                   |     DrupalConfigStorageSQL->read() <--- DATABASE
                                   |             |
                                   v             v
                             SignedFile->write($data)

Discussion issues

Files: 
CommentFileSizeAuthor
#145 cmi.patch117.98 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 34,830 pass(es). View
#140 cmi.patch117.98 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 34,819 pass(es). View
#126 cmi.patch118.01 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 34,613 pass(es). View
#116 cmi.patch118.14 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 34,612 pass(es). View
#105 cmi_103.patch129.79 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 34,397 pass(es). View
#100 cmi.100.patch111.26 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 34,369 pass(es). View
#97 cmi.97.patch111.03 KBsun
FAILED: [[SimpleTest]]: [MySQL] 34,385 pass(es), 2 fail(s), and 0 exception(s). View
#97 interdiff.txt16.73 KBsun
#95 cmi.patch113.62 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cmi_5.patch. Unable to apply patch. See the log in the details link for more information. View
#94 drupal-1270608-94.patch113.69 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 34,386 pass(es), 2 fail(s), and 0 exception(s). View
#90 cmi.patch114.08 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cmi_4.patch. Unable to apply patch. See the log in the details link for more information. View
#87 cmi.87.patch110.99 KBsun
FAILED: [[SimpleTest]]: [MySQL] 34,333 pass(es), 5 fail(s), and 1 exception(s). View
#87 interdiff.txt1.2 KBsun
#85 cmi.85.patch110.9 KBsun
FAILED: [[SimpleTest]]: [MySQL] 33,832 pass(es), 13 fail(s), and 1 exception(s). View
#85 interdiff.txt17.65 KBsun
#83 cmi.83.patch114.02 KBsun
FAILED: [[SimpleTest]]: [MySQL] 34,345 pass(es), 5 fail(s), and 1 exception(s). View
#83 interdiff.txt9.03 KBsun
#79 cmi.79.patch109.88 KBsun
FAILED: [[SimpleTest]]: [MySQL] 34,333 pass(es), 17 fail(s), and 1 exception(s). View
#77 cmi.77.patch102.43 KBsun
FAILED: [[SimpleTest]]: [MySQL] 34,210 pass(es), 3 fail(s), and 15 exception(s). View
#77 interdiff.txt14.54 KBsun
#75 cmi.patch88 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] 34,191 pass(es), 21 fail(s), and 32 exception(s). View
#73 cmi.patch88 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] 34,174 pass(es), 30 fail(s), and 32 exception(s). View
#68 cmi.68.patch85.98 KBsun
FAILED: [[SimpleTest]]: [MySQL] 34,012 pass(es), 215 fail(s), and 92 exception(s). View
#66 cmi.patch82.01 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] 34,140 pass(es), 52 fail(s), and 90 exception(s). View
#36 cmi.patch78.04 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] 33,230 pass(es), 53 fail(s), and 87 exception(es). View
#15 drupal8.cmi_.11.patch73.91 KBsun
FAILED: [[SimpleTest]]: [MySQL] 34,469 pass(es), 52 fail(s), and 86 exception(es). View
#5 drupal8.cmi_.5.patch70.16 KBsun
FAILED: [[SimpleTest]]: [MySQL] 34,439 pass(es), 50 fail(s), and 88 exception(es). View
#2 cmi.patch70.02 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] 34,443 pass(es), 50 fail(s), and 231 exception(es). View

Comments

cweagans’s picture

subscribe.

heyrocker’s picture

FileSize
70.02 KB
FAILED: [[SimpleTest]]: [MySQL] 34,443 pass(es), 50 fail(s), and 231 exception(es). View

Here we are folks, welcome to the initial core patch for the configuration management initiative. Please note that this is VERY MUCH a work in progress patch. It has known bugs and all sorts of stuff missing and there is lots of cleanup to do. However, it is basically functional and gives an opportunity for public comment on the API along with some examples of how implementations might look.

Before I get into the good stuff, I want to acknowledge the people who contributed to this patch and the system's design to this point, all of whom should be credited when/if this patch lands: Angela Byron (webchick), Yves Chedemois (yched), Larry Garfield (crell), Claes Gyllenwald (letharion), Daniel Kudwien (sun), Karoly Negyesi (chx), Justin Randall (beejeebus), Katherine Senzee (ksenzee), Mark Sonnabaum (msonnabaum), Fabian Sörgvist (fabsor), David Strauss (davidstrauss). I would also like to publicly acknowledge NodeOne for donating 50% of my billable hours to this project. Additionally, my thanks and gratitude to anyone who has allowed me to bounce ideas off them or participated in the gdo discussions to this point. The feedback and response to this has been really fantastic.

For background on the system and its architectural concepts, please review

http://groups.drupal.org/node/191283

It will answer a lot of questions in terms of the how and why.

This patch contains four main areas of interest:

  • Changes to the installer to setup the config directory and install default module configs.
  • The configuration api (core/includes/config.inc).
  • An implementation of the performance settings form using the configuration management system.
  • An implementation of the image styles system using the configuration management system.

IMPORTANT

You will have to apply the patch and then do a brand new fresh Drupal install, including a fresh settings.php. There is currently a bug in the installer that causes it to not work if the database settings form is skipped.

Beyond that this should be working and I'm looking for general feedback and thoughts. The image styles stuff is pretty rough, there are probably still some bugs and there is still a LOT of cleanup and now-unused code since it is getting refactored pretty heavily.

I know there will be lots of thoughts on XML as the file format, and I have added some of my own based on my current experience with the system at http://drupal.org/node/1288090#comment-5340754. Please move all commentary on the file format there.

There are also some issues around converting system_settings_form() to CMI that I punted on for the purposes of this patch, instead choosing to write my own submit function for the form. Please see the issue at
http://drupal.org/node/1324618.

Things I'm interested in feedback on

  • High level thoughts on the API, the architecture, how the code looks and concerns of that nature.
  • One of the things I did with image styles is I removed the whole concept of the default/in code/overidden styles. In a system where we are writing changes directly out to files all the time, it just didn't seem to make any sense any more.
  • I have no performance optimizations at all, so for instance, right now anytime you construct a new config object, you're getting a database hit. I'm sure this can be helped. I'm particularly interested in seeing if it makes any performance difference if we store serialized PHP arrays in the active store as opposed to the raw XML we are using now. I'm interested in both the speed and memory implications there.

Some things that I know are missing

  • Internationalization - This is under discussion (http://groups.drupal.org/node/185609) and is my next priority.
  • Managing updates to configuration - Lots of stuff needs to happen when configuration is re-loaded from disk (think about the changes that happen when a field is added or deleted for instance.) A system around this was proposed at BandCAMP but needs to be implemented and test, more details at http://groups.drupal.org/node/190729.
  • The UI that will be used to reload new config after its been updated.
  • The ability to override settings for server-specific data (aka database settings and the like.)
  • Update functions to convert old database-stored stuff to new config-system stored stuff
  • Tests for the system itself and rewriting of many existing tests.

Finally, if you are interested in helping out, work will be continuing in the CMI sandbox at http://drupal.org/sandbox/heyrocker/1145636, in the branch 8.x-file-config. I don't have a lot of time to be handholding or educating people on the system at the moment, but if you get a good grasp on things and want to start attacking specific issues, ping me and I'll get you commit access.

heyrocker’s picture

Status: Active » Needs review

Status: Needs review » Needs work

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

sun’s picture

Title: Add a file-based configuration api » File-based configuration API
Component: other » base system
Category: task » feature
Priority: Normal » Major
Status: Needs work » Needs review
FileSize
70.16 KB
FAILED: [[SimpleTest]]: [MySQL] 34,439 pass(es), 50 fail(s), and 88 exception(es). View

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

Status: Needs review » Needs work

The last submitted patch, drupal8.cmi_.5.patch, failed testing.

aspilicious’s picture

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

beejeebus’s picture

i pushed a commit that fixes page caching and makes the hook_boot/hook_exit tests pass:

http://drupalcode.org/sandbox/heyrocker/1145636.git/commit/5e86ed702d791...

too tired to get a patch going at this point.

xjm’s picture

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

aspilicious’s picture

Pushed a fix for the config test

EDIT:

Shouldn't

readFromFile()

and

deleteFromActive()

be part of the DrupalConfigVerifiedStorageInterface

fietserwin’s picture

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

amateescu’s picture

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

heyrocker’s picture

It is possible that the Overridden/Restore functionality could remain, since we keep the original copies of the default config along with the module's code files. We would need to be able to link files in the live config directory back to these originals in order to determine whether or not they are changed and display that information. Not sure how we would do that, but it could be done. I don't think it needs to be part of this patch though, we can do it in a followup and if it doesn't get done I don't think its a dealbreaker either (personally.) I made an issue in the sandbox queue about this, and it can be moved into the core queue after this patch lands.

fietserwin’s picture

Status: Needs review » Needs work
Issue tags: -Configuration system

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.

sun’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system
FileSize
73.91 KB
FAILED: [[SimpleTest]]: [MySQL] 34,469 pass(es), 52 fail(s), and 86 exception(es). View

[ - 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?
yched’s picture

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.

yched’s picture

Status: Needs work » Needs review

crosspost

yched’s picture

Issue tags: +Configuration system

and resetting tag as well.

David Strauss’s picture

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.

aspilicious’s picture

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

Status: Needs review » Needs work

The last submitted patch, drupal8.cmi_.11.patch, failed testing.

beejeebus’s picture

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.

we don't have a configuration system with data tree, we have a directory of arbitrarily named files. as such, forcing people to think about files makes the API and system more consistent.

it was also pointed out that it would be difficult to figure out where the file path ended, and the data within the file started.

those who support the idea can probably put it better than me, because i completely disagree with the argument :-)

beejeebus’s picture

re #16, my latest thoughts here - http://groups.drupal.org/node/190729#comment-662963

next task is to try to explain via use cases why we don't need to write to files except on export.

pounard’s picture

Deleted because duplicate.

chx’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes

Updated issue summary.

pounard’s picture

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.

sun’s picture

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.

beejeebus’s picture

Issue summary: View changes

Added branch information

sun’s picture

Issue summary: View changes

Updated issue summary.

beejeebus’s picture

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

granularity of the object? it's not about the object, it's about files - you can't use this interface without understanding that you are reading values clumped together in a file.

EDIT - this is unclear, even to me, please disregard.

beejeebus’s picture

Issue summary: View changes

Updated issue summary.

heyrocker’s picture

@sun thank you for the detailed review and architecture description! Hopefully this will be very helpful for future reviewers. Some comments on the concerns

Cr1: I am not against this is we can figure out a way to make it work, and having a colon delimit the file and the key is the way I would suggest if we decide we want to go this route. However we also shouldn't prevent the possibility of being able to init a config object and do a bunch of stuff in it (as bojanz mentioned in #20.) It's possible we can support both maybe? There are serious issues and edge cases with doing image.styles.large.name.effects and forcing the code to figure out where to break from name-of-file to data-in-file. David and Larry and Chx spent a good amount of time on this at our first sprint in Denver and that was when we moved towards this model because it was becoming too problematic.

Cr2: Yes this is just something we're going to have to be more careful with. In many respects, some config changes will essentially be API changes. However in some ways that is good. If a module you are integrating with removes a config value as part of an upgrade, wouldn't you want to know about it? If a config value isn't set, then you should probably deal with it in a different way than if it has a value, and there is no way to make this distinction in the current variable system. For module developers it does create a little more work - when you decide you need a new config value, you actually have to go into your module's default config file, add the value, install it, and reload config before you can use it. I don't personally think this is a huge burden to add though. Will be curious to hear what others think.

Cr3: Yes, that's correct. my_module would provide either my_module.xml or maybe my_module.settings.xml, and within that would be the keys. This actually forces values to be namespaced, whereas with the variables system, modules would (and did) simply ignore any naming conventions within variables.

Ma1: Some chainability is already possible. For instance, you can chain any function from config() (aka print config('image.styles.large')->get('name'); works.) To get the others to work, the functions would need to return the config object back, so set() would need to return $this. Not a big deal conceptually, but want to make sure it makes sense everywhere (except get() as aspilicious notes.)

Ma4: Do these functions have some general utility outside of the config system? If so, it might make more sense to move them into common.inc or something. At least the xml->array->xml functions might qualify in this way.

Beyond that I don't really have any issues with the suggested cleanups and such.

pounard’s picture

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

aspilicious’s picture

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

heyrocker’s picture

@pounard this situation is pretty easy right now.

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

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

That situation is however one of the cases that makes it very difficult to put everything in one string without a special delimiter (like a colon).

@aspilicious The second module installed would overwrite the first module's file. However I would consider that a bug in the modules, not a deficiency in the config system. Modules should namespace their config files with their module name, just like they have to in various other areas of Drupal. I don't really see that requirement as a problem.

pounard’s picture

@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');

?

aspilicious’s picture

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

fago’s picture

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?

beejeebus’s picture

just pushed a new branch called 8.x-file-config-reload, with this commit:

http://drupalcode.org/sandbox/heyrocker/1145636.git/commit/f57b542

hejrocker - do you want this in another issue?

discussed this a bit with hejrocker in IRC, but the code doesn't reflect our discussions 100%. not really happy with it, but it's something more than vapour-ware to talk about.

- i've only implemented a reload from disc --> active store, the 'git pull' use case. i have NFI how to do the other way, or how that fits with the current design.

- we're not making config CRUD operations cause reloads, because the current idea is that code that calls config() CRUD operations should know whether a reload is necessary or not, not the config system. so why don't we just let the code that calls config() CRUD do what needs to be done? it seems a config reload hook is only necessary when we don't have any such code, such as when loading in new config from disc. in that case, we do need something to allow 'business code' to run.

- no reload on CRUD #2: if config CRUD always writes to disc to keep it in sync, then we need an in-memory way to keep track of all config changes in case a reload is called. some sort of automagic tracking, and an API for accessing it, that is ready to be used in the (rare?) case calling code decides that a CRUD operation needs to trigger a reload. or do we make calling code responsible for passing in the changes? or ...?

smaller issues from the implementation:

- i thought it would be a good idea to let modules know what has changed up front, as every single module will need to check for this to know if they should react in some way.

- as the code is now, it's impossible to get changed files loaded unless they've been signed by $drupal_config_key. do we really want to force knowledge of $drupal_config_key outside of a production environment?

heyrocker’s picture

FileSize
78.04 KB
FAILED: [[SimpleTest]]: [MySQL] 33,230 pass(es), 53 fail(s), and 87 exception(es). View

This is a reroll to core since a bunch of stuff broke with recent conversions of the core cache system to PSR-0. It also includes a lot of cleanups from aspilicious, ksenzee and sun. Some of the bigger changes sitting in branches will have to wait for closer inspection which may not happen for a couple weeks as im in the middle of moving back to the states.

heyrocker’s picture

Status: Needs work » Needs review
heyrocker’s picture

I just realized I merged in the currently-broken head. Will reroll again when all is fixed.

pounard’s picture

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.

Status: Needs review » Needs work

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

nedjo’s picture

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.

Dries’s picture

Assigned: Unassigned » Dries

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

Dries’s picture

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.

cweagans’s picture

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.

Dries’s picture

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

moshe weitzman’s picture

Assigned: Dries » Unassigned

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\">");

  // 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();
  }
Dries’s picture

Assigned: Unassigned » Dries

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

@dries Verified makes sense when you think about the fact that data in the active store has been confirmed to be good - it has no syntax errors (otherwise it would not have been loaded) it signature matches the actual data (thus it hasn't been tampered with.) We are probably going to make another class that directly access the files instead of the active store for the update mechanism, this class would specifically NOT have "verified" in its class name. It's an important distinction worth keeping but it could probably be spelled out more clearly. I agree Drupal is probably redundant.

There have been a lot of suggestions about class naming both here and in IRC, but I haven't had the chance to process them all. The move is taking a lot of my mental energy for the next couple weeks.

The duplication of config file is more for modules to be able to ship with default configuration, and for the system to be able to revert to that default configuration. It has nothing to do with security actually. A lot of these comments definitely need fleshing out.

@moshe The gymnastics with json_encode()/decode() are simply the fastest way to get from SimpleXML to an array. I agree it looks hacky and weird but it is very efficient. All that said, I am more and more becoming convinced that XML was a mistake and that the things it buys us are to painful to implement (see discussion at http://drupal.org/node/1288090#comment-5340754). 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.

@pounard that line is stock in Drupal 7 already, the only thing I did is add XML. The rest are already blocked by default.

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?

geerlingguy’s picture

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.

beejeebus’s picture

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

heyrocker’s picture

@beejeebus If we lock the ability to write files during a reload (which has been long discussed) then do we have a problem? We have known for a while we will need to lock things down on reload and we will, and in fact with everyone writing through a single api it should be reasonably simple to do when we start putting everything together. I don't think this is a particularly big deal to fix myself. You don't want your pushed changes overwritten? Either you set this lock manually, or you set as part of your deployment process. Every process has steps to be followed and this can be one of them.

As I mentioned in the original post, we don't even have a consensus on how the update process will look, but these are solveable problems and I personally don't think they reflect badly on the design. If we need to write this before the patch lands that's fine, and maybe its not a bad idea as it would provide a workflow for people to see.

For the record I'm not ragging on beejeebus who has been an enormous help to CMI through the process.

beejeebus’s picture

re #51, yep, locking out writes during the reload would work.

for it to be effective, the locking has to start before the new files start appearing in the config dir. getting the new files into the config dir is not going to be anything the config system is aware of, so the lock has to be set manually, or by a process outside of drupal code. there is nothing in the design that makes this just work, instead you need an extra step - 1. lock all config writes, 2. git pull/scp/make your edits, 3. run the reload, 4. unlock writes.

i can live with "you have to manually lock all writes, load your change files into the config dir, reload, then unlock", but i don't like it as a design, because the "loading new config from disc" is, IMO, one of the most important use cases for this API.

catch’s picture

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.

Sborsody’s picture

"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? :) ).

Crell’s picture

@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: composer.json for modules). 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.

heyrocker’s picture

@catch @crell One way or another, either format is going to require a schema definition of some type. For instance, we will need to have properties like 'translateable' (already discussed this with Gabor) and some of these will be private to the file. XML has this natively with attributes, JSON does not and will require creating some drupalism around it. I'm not necessarily against this but it should be part of the discussion. I do see that, if we look at general trends in the 'web world' whatever that means, JSON seems to be gaining a lot of ground as a config format.

Sborsody’s picture

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

heyrocker’s picture

@sborsody there is already plans to make a setting to bypass writing to files, but the assumption currently is that it will be writing files by default

Sborsody’s picture

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.

Dries’s picture

Assigned: Dries » Unassigned

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.

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-config

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

EclipseGc’s picture

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.

David Strauss’s picture

Issue tags: -D8MI, -language-config

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.

David Strauss’s picture

Issue tags: +D8MI, +language-config

Fix tag deletion.

heyrocker’s picture

Status: Needs work » Needs review
FileSize
82.01 KB
FAILED: [[SimpleTest]]: [MySQL] 34,140 pass(es), 52 fail(s), and 90 exception(s). View

This is a new patch containing

- Conversion of existing classes to PSR-0 (thanks to EclipseGC)
- Addition of ability to retrieve the entire data array for a file by passing no key to $config->get()

Also trying to get a handle on where our current core test failures are.

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
85.98 KB
FAILED: [[SimpleTest]]: [MySQL] 34,012 pass(es), 215 fail(s), and 92 exception(s). View

Fixed SimpleTest integration.

Dries’s picture

Status: Needs review » Needs work

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.

beejeebus’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, cmi.68.patch, failed testing.

Dave Reid’s picture

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

heyrocker’s picture

Status: Needs work » Needs review
FileSize
88 KB
FAILED: [[SimpleTest]]: [MySQL] 34,174 pass(es), 30 fail(s), and 32 exception(s). View

Another fix to see if we can get the testing profile working

Status: Needs review » Needs work

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

heyrocker’s picture

Status: Needs work » Needs review
FileSize
88 KB
FAILED: [[SimpleTest]]: [MySQL] 34,191 pass(es), 21 fail(s), and 32 exception(s). View

This patch removes .xml from the global .htaccess, which we believe is the cause of the aggregator failures. Working on adding new functionality to auto-create a .htaccess in the config folder at install-time.

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
14.54 KB
102.43 KB
FAILED: [[SimpleTest]]: [MySQL] 34,210 pass(es), 3 fail(s), and 15 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, cmi.77.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
109.88 KB
FAILED: [[SimpleTest]]: [MySQL] 34,333 pass(es), 17 fail(s), and 1 exception(s). View

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

catch’s picture

Status: Needs review » Needs work

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.

catch’s picture

Status: Needs work » Needs review

dreditor status change.

Status: Needs review » Needs work

The last submitted patch, cmi.79.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
9.03 KB
114.02 KB
FAILED: [[SimpleTest]]: [MySQL] 34,345 pass(es), 5 fail(s), and 1 exception(s). View

Attached patch fixes the ImageDimensionsUnitTest once for all.

Interdiff is against #79.

Status: Needs review » Needs work

The last submitted patch, cmi.83.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
17.65 KB
110.9 KB
FAILED: [[SimpleTest]]: [MySQL] 33,832 pass(es), 13 fail(s), and 1 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, cmi.85.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
110.99 KB
FAILED: [[SimpleTest]]: [MySQL] 34,333 pass(es), 5 fail(s), and 1 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, cmi.87.patch, failed testing.

heyrocker’s picture

The ieid for image effects saved through this test is not getting generated, and thus the tests for image effects are failing because they are posting to URLs that don't exist. I have not dug into the code to see why this is, but that's what is going on.

heyrocker’s picture

Status: Needs work » Needs review
FileSize
114.08 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cmi_4.patch. Unable to apply patch. See the log in the details link for more information. View

OK I think we should be down to two with this one. The last two are related to imagefields not getting updated properly when a style is updated. Can anyone look at those?

Status: Needs review » Needs work
Issue tags: -Configuration system, -D8MI, -language-config

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

heyrocker’s picture

Status: Needs work » Needs review

#90: cmi.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system, +D8MI, +language-config

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
113.69 KB
FAILED: [[SimpleTest]]: [MySQL] 34,386 pass(es), 2 fail(s), and 0 exception(s). View
heyrocker’s picture

FileSize
113.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cmi_5.patch. Unable to apply patch. See the log in the details link for more information. View

Hm lets try this

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
16.73 KB
111.03 KB
FAILED: [[SimpleTest]]: [MySQL] 34,385 pass(es), 2 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, cmi.97.patch, failed testing.

David Strauss’s picture

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

effulgentsia’s picture

FileSize
111.26 KB
PASSED: [[SimpleTest]]: [MySQL] 34,369 pass(es). View

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

sun’s picture

Status: Needs work » Needs review

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

catch’s picture

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

heyrocker’s picture

@david I hope to make followup issues on those and other topics over the weekend. There are a lot of things floating around and I want to make sure they get captured.

@catch when I made that comment about PSR-0, I had forgotten there was still procedural stuff in config.inc. So the comment should probably just be removed for now, and a followup issue created about possibly converting those functions to classes.

@sun

- Configuration must not be additionally cached by a module in any way (static cache / database cache).

Can you elaborate on why this is a problem?

- 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)
- 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 duplication issue is really tied to XML as much as anything, and while it's a little messy I don't really have a problem with it to be honest. As we discuss the file format again (yet another followup discussion) this can be discussed at the same time. Regardless I don;t think it should be a blocker for this to be committed. The other two I think are good candidates for followups as well.

- Some modules invoke drupal_alter() on configuration (e.g., image styles).

Yes, the implications of this have to be carefully considered

- 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().
- Need a way to list config object names/suffixes _after_ a specified prefix.
- Config keys must not contain periods (within a specific key).

I think these cleanups should be pretty easy to get in now, while we're also writing tests.

heyrocker’s picture

This patch adds extensive tests for get(), set(), and clear(). There are probably some variations on casting still not covered (object->array is one I just thought of) but it covers most of the possibilities. I also looked into some of sun's todos:

- We cannot rename clear() to unset() because unset is reserved. I remember now that his is the whole reason I called it clear() in the first place.
- Sun added some very nice casting functions which are here and part of the tests now.

heyrocker’s picture

FileSize
129.79 KB
PASSED: [[SimpleTest]]: [MySQL] 34,397 pass(es). View

and the patch

sun’s picture

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.

heyrocker’s picture

Re 2: OK, so as long as the effect is never individually passed around, we can just leave the machine name as its key and refer to it through there ($style->effects->image_desaturate->weight = 5). Yes, this makes a lot of sense.

Re 3: This also makes a lot of sense. At first glance it SEEMS like anyone who would previously alter() a value could simply change / add to the config by hand. One implication of this is that when that module is uninstalled it would also need to undo its changes (assumedly through hook_uninstall()) as opposed to with an alter() where this would just happen as a matter of consequence. This dovetails somewhat into a discussion we had this weekend about cleaning up configuration on module uninstall.

effulgentsia’s picture

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.

yched’s picture

Issue tags: -Configuration system

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)

yched’s picture

Unintended tag change

hefox’s picture

Issue tags: +Configuration system

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.

heyrocker’s picture

Note that, at least thus far, $conf will remain pretty much unchanged (albeit with a new access mechanism most likely.) The config system currently relies on the database, which means it comes in way late in boostrap. Anyone whose solution currently relies on mucking with $conf will still be able to do that without much change.

bojanz’s picture

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’s picture

In response to @catch in #80, I ran some simple benchmarks tonight checking page caching with the patch and without. I randomly generated five nodes with devel and loaded a cached home page five times with the patch and five times without. This generated a range of data, and I took the middle one in both cases. They are available here:

http://heyrocker.com/cmi.html
http://heyrocker.com/no-cmi.html

The high level view is that the CMI version is a slight regression, which is not completely surprising. We have no caching at all (not even static caching on repeated requests for the same data) and the code hasn't been seriously optimized. Catch already pointed out a call to db_table_exists() which I can avoid, I'll roll that into a new patch tomorrow. There are definitely a lot more function calls in the cmi version.

I have been hoping to be able to avoid adding another caching layer on top of the config system in order to keep the architecture simple, but if we have to we have to.

heyrocker’s picture

FileSize
118.14 KB
PASSED: [[SimpleTest]]: [MySQL] 34,612 pass(es). View

New patch. Includes the following:

- Removes the check on db_table_exists() in read()
- Adds a ton more tests
- Rerolled to head

Based on the specs we had laid out last weekend as far as the requirements for this patch to land, I believe we have met those goals. I know that catch and others have had comments about the object design, but we are planning to address those in followups, even if they are forced to introduce API changes. Again, the main goal here is to unblock WSCCI and get something in place for them and the plugin people to work from.

In the last few days I have also added these followup issues:

#1448330: [META] Discuss internationalization of configuration
#1447686: Allow importing and synchronizing configuration when files are updated
#1444766: Determine method for automatically cleaning up configuration after a module is uninstalled
#1444620: Remove file signing from configuration system

There is also discussion of changing the classes around so that they extend a basic key/value store class, which would have a lot of use in situations around Drupal in general. That discussion is happening at

#1202336: Add a key/value store API

I think we're ready to go if everything comes back green.

cosmicdreams’s picture

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

Tor Arne Thune’s picture

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

heyrocker’s picture

Yes, as soon as this patch lands we will open up a huge pile of followups about converting various subsystems to the new system

sun’s picture

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

Tor Arne Thune’s picture

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

cweagans’s picture

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

cosmicdreams’s picture

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

catch’s picture

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

heyrocker’s picture

FileSize
118.01 KB
PASSED: [[SimpleTest]]: [MySQL] 34,613 pass(es). View

@cwaegans attached patch cleans up these issues. I think I left one of the inline @todos because it made a lot more sense where it stood. The d.o doxygen standards do allow for inline @todos (see http://drupal.org/node/1354).

@catch So if I understand this right we would go from this

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

to assumedly a singleton of some sort (?) and something like this

$config->get('system.performance', 'cache');
$config->get('system.performance', 'aggregate_js');
$config->get('system.performance', 'aggregate_css');

right? I admit my OO design skills are not the greatest so I want to make sure I am getting it. If so then guess I see this as a DX vs performance issue. I agree it would be helpful (even likely necessary) to be able to do static caching of requests since right now the get() process is really inefficient because we may check all sorts of things multiple (even dozens) of times in a page load and we're hitting the database for each one right now. But there is no doubt the DX is a lot uglier there and the original process is way more smooth.

This also harkens back to the earlier discussion about possibly combining the filename and key into a single parameter such as

$config->get('system.performance:aggregate_css');

As far as another config class, I think this could become useful down the road as other systems begin to want to use a file-based storage system but they don't make sense within the framework of our two-tier configuration. For instance, there has been talk of putting the module list into a file-based system. However it makes no sense to have such a thing in this config system. We actually need the module list in order to operate and we don't (yet) plan to be available this early. So a thought I had was you could make a new class that wraps all the same functionality DrupalConfig does but operates on a different set of files and in a different way (like it doesn't have an active store.) And again, this all works back towards making all these classes be more generic and decoupled for greater flexibility which we've already started discussing elsewhere.

catch’s picture

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.

sun’s picture

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.

heyrocker’s picture

Ah yes, this does seem to make a lot more sense. However, given that we're already discussing reworking the classes I think it makes sense to work these ideas into that rearchitecture rather than doing it now and tearing it apart again later.

cosmicdreams’s picture

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

heyrocker’s picture

No I'm arguing those should be pushed off for a followup patch and we're ready to go as is right now. What needs to happen is someone who is not me needs to RTBC it, preferably someone who has been working on it and has familiarity with it.

webchick’s picture

Status: Needs review » Reviewed & tested by the community

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.

cweagans’s picture

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.

fago’s picture

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.

catch’s picture

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.

fago’s picture

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.

sun’s picture

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

heyrocker’s picture

Does it really make sense to profile the system now, when we know we're already going to rework it pretty majorly? We know we will be (almost certainly) adding static caching and optimizing the code in ways that will make the current version pretty irrelevant. The main goal here is just to unblock the other initiatives.

As for the reloading of config, please make sure your comments are put to #1447686: Allow importing and synchronizing configuration when files are updated

catch’s picture

Status: Reviewed & tested by the community » Needs work
 /**
+ * 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).

heyrocker’s picture

Status: Needs work » Needs review
FileSize
117.98 KB
PASSED: [[SimpleTest]]: [MySQL] 34,819 pass(es). View

New patch rerolled to HEAD and fixing the comments above. I discussed the upgrade path with catch on IRC and we agreed that if it doesn't get done before this gets committed then that should be opened as a new critical bug. It shouldn't be that tough to write for the two subsystems that have been implemented.

webchick’s picture

Assigned: Unassigned » Dries

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.

webchick’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

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.

David_Rothstein’s picture

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

heyrocker’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
117.98 KB
PASSED: [[SimpleTest]]: [MySQL] 34,830 pass(es). View

I think the second answer is the best one as far as that goes. This definitely needs a bug report but I don't know if its critical or not, I guess it is since it prevents Drupal from working. I might try and fix that before commit if I can get to it before Dries gets to this.

This patch is a reroll to HEAD, I merged my branch with the changes from the PSR-0 DB:TNG commit and am running it past the bot as a precaution. If It comes back green I'm just going to put this back to RTBC.

heyrocker’s picture

Status: Needs review » Reviewed & tested by the community

and we are green!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

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.

heyrocker’s picture

Status: Fixed » Needs work
Issue tags: +Needs change record

Reopening because we need the following

- change notice
- CHANGELOG.txt entry

heyrocker’s picture

Title: File-based configuration API » Change notification: File-based configuration API

Changing title to match standard as well

xjm’s picture

Category: feature » task
Priority: Major » Critical

Setting priority/category.

webchick’s picture

Assigned: Dries » Unassigned

This can come off Dries's plate now.

effulgentsia’s picture

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

xjm’s picture

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

heyrocker’s picture

http://drupal.org/node/1500260

This is mostly the information from webchick's introduction post cleaned up and streamlined a bit. I don't know if it is overkill or not, but I figured it was worth providing some level of detail. I'll open a followup about CHANGELOG.

heyrocker’s picture

Component: base system » configuration system
Status: Needs work » Needs review
Tor Arne Thune’s picture

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

webchick’s picture

Category: task » feature
Priority: Critical » Major
Status: Needs review » Fixed

Hooray!

plach’s picture

Title: Change notification: File-based configuration API » File-based configuration API

Moving the title back to ease future reference.

xjm’s picture

Issue tags: -Needs change record

And, untagging. :)

webchick’s picture

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

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

Anonymous’s picture

Issue summary: View changes

Added a little more detail to the description of the active store