Implement a config import/staging directory

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportTest.php
@@ -73,15 +73,15 @@ class ConfigImportTest extends WebTestBase {
+    // Verify the new configuration does not exist in the import direcotry.

Typo of "direcotry"

Otherwise, looks really great, though.

+++ b/core/includes/install.incundefined
@@ -254,8 +254,8 @@ function drupal_rewrite_settings($settings = array()) {
+  global $config_directory_name, $config_import_directory_name;

I think we should move to a format like this:

 $config_directories = array(
   'active' => '/some/directory/outside/webroot',
   'staging' => '/another/directory/outside/webroot',
 );

Because it makes it consistent with global $databases; and then a site can add more... which allows for some very interesting workflows.

No interdiff because it's a different approach.

Patch attached is a potential implementation

Thanks, I think this is going great! I am not sure how I feel about calling it staging... it feels bikeshed-y to debate that so I will pass.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportTest.phpundefined
@@ -73,15 +73,15 @@ class ConfigImportTest extends WebTestBase {
+    // Verify the new configuration does not exist in the import directory.
+    $file_storage = new FileStorage(array('directory' => config_get_config_directory(CONFIG_STAGING_DIRECTORY)));

import or staging? Comment says one thing, the code another.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportTest.phpundefined
@@ -112,8 +112,9 @@ class ConfigImportTest extends WebTestBase {
+    // import directory.
+    $file_storage = new FileStorage(array('directory' => config_get_config_directory(CONFIG_STAGING_DIRECTORY)));

Same.

But you import and export into this folder :) that's why I went for "staging"... I might actually prefer "stage" (hmmm... I feel like I'm entering a bikeshed ;) )

config-import-directory.patch queued for re-testing.

Looks nice.

Awesome! Thanks for moving this forward, @beejeebus!

It makes sense to do this as a separate patch as preparation for switching to FileStorage for the active store.

A few issues though:

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -688,7 +688,7 @@ abstract class TestBase {
+    $this->originalConfigDirectories = $GLOBALS['config_directories'];

@@ -720,9 +720,14 @@ abstract class TestBase {
+    $GLOBALS['config_directory'] = array(
...
+    $this->configActiveDirectory = $this->originalFileDirectory . '/' . $GLOBALS['config_directory'][CONFIG_ACTIVE_DIRECTORY];
+    $this->configStagingDirectory = $this->originalFileDirectory . '/' . $GLOBALS['config_directory'][CONFIG_STAGING_DIRECTORY];

@@ -780,7 +785,7 @@ abstract class TestBase {
+    $GLOBALS['config_directories'] = $this->originalConfigDirectories;

1) $GLOBALS['config_directory']
should be
$GLOBALS['config_directories']
I think?

2) Not really happy about polluting $this with these individual properties... can we just set $this->configDirectories to essentially hold the already resolved values of the global variable?

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -720,9 +720,14 @@ abstract class TestBase {
+    file_prepare_directory($this->configActiveDirectory, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS);
+    file_prepare_directory($this->configStagingDirectory, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS);

We can do it in a follow-up patch, but ideally we should just be calling into install_ensure_config_directory().

I prepared that in #1626584-112: Combine configuration system changes to verify they are compatible already. Earlier comments on that issue explain some more problems with the Drupal installer. I'm happy to perform these changes along with the issue/patch that replaces the DatabaseStorage to FileStorage for the active store though. That said, I'm not sure whether we want to change install_ensure_config_directory() to create all config directories in one fell swoop, as I've done in there?

By the looks of it, I think we can just call config_get_config_directory() since this function checks drupal_valid_test_ua() and returns an appropriate path, I'll test out my theory and come back with a patch if it works.

The attached patch implements sun's suggestion however to make use of install_ensure_config_directory() we'd need to require_once(DRUPAL_ROOT . '/core/includes/install.inc'); which I suspect we don't want to do? Instead I have stuck with a call to file_prepare_directory(); for each config directory.

Code looks good, however I see some issues with the docs:

+++ b/core/includes/bootstrap.incundefined
@@ -296,6 +296,20 @@ const REGISTRY_WRITE_LOOKUP_CACHE = 2;
 /**
+ * $config_directories key for active directory.
+ *
+ * @see config_get_config_directory
+ */
+const CONFIG_ACTIVE_DIRECTORY = 'active';
+
+/**
+ * $config_directories key for staging directory.
+ *
+ * @see config_get_config_directory
+ */

Could we have a better explanation about what means 'active' and 'staging'?

If developers try to get this information reading the docs of config_get_config_directory it says: Drupal core provide 'active' and 'staging' but doens't really explain which directory will be used to export and which is used to import configurations.

+++ b/sites/default/default.settings.phpundefined
@@ -257,9 +257,12 @@ $drupal_hash_salt = '';
  * Example:
- *   $config_directory_name = '/some/directory/outside/webroot';
+ *   $config_directories = array(
+ *     CONFIG_ACTIVE_DIRECTORY => '/some/directory/outside/webroot',
+ *     CONFIG_STAGING_DIRECTORY => '/another/directory/outside/webroot',
+ *   );

Same for this settings. It should be more clear the difference between staging and active.

Fixed phpDoc and Exception of config_get_config_directory().
Added staging storage as a separate DIC service.

Some of these changes/adjustments are essentially extracted from #1626584-113: Combine configuration system changes to verify they are compatible

That said, the service name config.state was a quick naming choice. Do we want to rename the two config storage services into config.storage.active and config.storage.staging ?

Naming nitpick :
I'm a little worried that the 'staging' config dir is confusing. The most commonly used meaning of the word is "one of the servers in a typical dev-stage-live deployment line". So having a 'staging' config dir on each of the 'dev', 'staging' and 'live' servers sounds like a WTF.

Can't we have the 'active' and 'import' stores ?

Hrm. Spent quite some time to resolve the Drupal installer errors, but pretty much ended up with merging a range of changes from the next branch...

The failures are currently "hidden" with the original patch here, since it doesn't register the config staging/state storage as a service on the DIC. As soon as you do that (and we want to do that), the installer calls into drupal_container() for rendering the maintenance theme and other stuff, but drupal_container() calls into config_get_config_directory(), which throws an exception, because the requested config directory does not exist yet.

I'm not entirely sure whether to proceed with this patch without the DIC service, or whether we want to merge in the required adjustments for the Drupal installer here. As mentioned, we'll need to do those anyway later on. I somewhat lean towards not including them here, but yet, reducing the amount of conflicts.

Will roll a revised patch in a minute.

Fixed installer creates config directories too early.
Reverted config.state DIC service.

re: #32: @yched: "import" doesn't really fly either, since it is also used for exporting config. That's more or less the reason why I chose to go with "state" in the service name... None of the names are really ideal, but if possible, I'd prefer to defer the discussion on a proper name to a separate issue, so we can move on with all the other technical implementation stuff in parallel.

My changes to this patch are pretty minor -- merely ensured that they're compatible with the new CachedFileStorage implementation, so I think this is RTBC.

Let's move the discussion about the "staging"/"state"/"import" config directory name into the original issue:
#1703168: [Meta] Ensure that configuration system functionality matches expected workflows for users and devs

(That said, I'm already working on incorporating this patch into the next branch and stumbled over some new + odd hiccups in the installer, but we can fix/adjust this code when we're actually switching to the CachedFileStorage.)

Moved the discussion on "staging" into #1703168-47: [Meta] Ensure that configuration system functionality matches expected workflows for users and devs

Would be great if we could quickly move forward here, since this patch is blocking #1702080: Introduce canonical FileStorage + (regular) cache

I have added a use Exception to boostrap.inc because that's our code standard like it or not, renamed variables in the test -- because it was called $state and it was not storing state of any kind IMO -- but that's all. It's just renames. I have attached two interdiffs, one to sun's latest and one to Justin's because sun's patches were changing wildly back and forth making it harder to compare to Justin's patch.

And totally yes to #37 let's get this moving! I totally approve of the RTBC with the renames I added (which are in a test, anyways).

#38: 1741804_38.patch queued for re-testing.

Weird. I get no fails. Here it is again.

+1 for this patch...

Awesome!

This patch is pretty straight-forward. It changes from one config directory to two and as such ends up making a bunch of adjustments throughout the system that remove the assumption that there's only one.

The only thing I really don't like is that the distinction between CONFIG_ACTIVE_DIRECTORY and CONFIG_STAGING_DIRECTORY is not very intuitive; the names definitely need work. I don't think that's worth holding up the architecture fix, however, which is essential to moving forward on CMI. But we should either file a major/critical task specifically about revisiting the naming, or not close #1703168: [Meta] Ensure that configuration system functionality matches expected workflows for users and devs (which is already a critical task) until this is done. I have a feeling the naming discussion will be easier once more of the CMI work is completed, so I don't mind holding off on that for a few more patches.

Committed and pushed to 8.x. ROCK!