This is a spin-off from #1703168: [Meta] Ensure that configuration system functionality matches expected workflows for users and devs.

The patch adds a config import directory, and updates the tests, installer and config sync code to use it.

Files: 
CommentFileSizeAuthor
#43 1741804_43.patch23.73 KBchx
PASSED: [[SimpleTest]]: [MySQL] 40,339 pass(es). View
#38 1741804_38.patch24.02 KBchx
FAILED: [[SimpleTest]]: [MySQL] 40,341 pass(es), 0 fail(s), and 1 exception(s). View
#38 interdiff_27_38.txt12.81 KBchx
#38 interdiff_34_38.txt5.88 KBchx
#34 config.directories.34.patch23.4 KBsun
PASSED: [[SimpleTest]]: [MySQL] 40,364 pass(es). View
#34 interdiff.txt13.22 KBsun
#29 config.directories.29.patch24.48 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#29 interdiff.txt12.45 KBsun
#27 1741804-27-config-import-directory.patch18.29 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 40,746 pass(es). View
#26 1741804-26-config-import-directory.patch17.8 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 40,739 pass(es). View
#24 1741804-24-config-import-directory.patch17.8 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] 40,734 pass(es), 0 fail(s), and 34 exception(s). View
#20 1741804-20-config-import-directory.patch17.62 KBmatason
FAILED: [[SimpleTest]]: [MySQL] 40,548 pass(es), 3 fail(s), and 1,117 exception(s). View
#15 1741804-15-config-import-directory.patch17.77 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 40,569 pass(es). View
#11 1741804-11-config-import-directory.patch17.77 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 40,356 pass(es). View
#9 1741804-9-config-import-directory.patch17.61 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 40,366 pass(es). View
#8 1741804-8-config-import-directory.patch11.12 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 40,348 pass(es). View
#6 1741804-6-config-import-directory.patch11.12 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 40,254 pass(es). View
config-import-directory.patch9.47 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] 40,284 pass(es), 150 fail(s), and 191 exception(s). View

Comments

Status: Needs review » Needs work

The last submitted patch, config-import-directory.patch, failed testing.

beejeebus’s picture

Status: Needs work » Needs review

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

beejeebus’s picture

hmmm. so our upgrade path tests don't work unless your parent drupal has a writable settings.php. i don't know enough about those tests to know if that's a known, expected behaviour or not.

Status: Needs review » Needs work

The last submitted patch, config-import-directory.patch, failed testing.

beejeebus’s picture

Issue tags: +Configuration system

tagging.

beejeebus’s picture

Status: Needs work » Needs review
FileSize
11.12 KB
PASSED: [[SimpleTest]]: [MySQL] 40,254 pass(es). View

zomg, i had to export another global. sigh.

tstoeckler’s picture

Status: Needs review » Needs work
+++ 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.

beejeebus’s picture

Status: Needs work » Needs review
FileSize
11.12 KB
PASSED: [[SimpleTest]]: [MySQL] 40,348 pass(es). View

new patch just fixes typo from #7.

alexpott’s picture

FileSize
17.61 KB
PASSED: [[SimpleTest]]: [MySQL] 40,366 pass(es). View
+++ 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

beejeebus’s picture

Status: Needs review » Needs work

i'm not sure i buy the reasoning exactly, but i like making config_get_config_directory() take a param, so i'm happy if we move forward from the patch in #9.

+    $path = conf_path() . '/files/' . $config_directories[$type];

lets make that code check that $config_directories[$type] is set, and throw an exception if it isn't. Bad Things can happen in that case, and we want to blow up immediately with a specific error.

beejeebus’s picture

Status: Needs work » Needs review
FileSize
17.77 KB
PASSED: [[SimpleTest]]: [MySQL] 40,356 pass(es). View

attached patch implements #10.

chx’s picture

Status: Needs review » Needs work

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.

beejeebus’s picture

@alexpott if u get to this before I do, I prefer import, but I'll don't care too much either way.

alexpott’s picture

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

beejeebus’s picture

Status: Needs work » Needs review
FileSize
17.77 KB
PASSED: [[SimpleTest]]: [MySQL] 40,569 pass(es). View

i've painted the bikeshed a fetching shade of staging - all comments now refer to a staging directory :-)

andyceo’s picture

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

chx’s picture

Status: Needs review » Reviewed & tested by the community

Looks nice.

sun’s picture

Status: Reviewed & tested by the community » Needs work

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?

matason’s picture

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.

matason’s picture

Status: Needs work » Needs review
FileSize
17.62 KB
FAILED: [[SimpleTest]]: [MySQL] 40,548 pass(es), 3 fail(s), and 1,117 exception(s). View

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.

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

The last submitted patch, 1741804-20-config-import-directory.patch, failed testing.

beejeebus’s picture

Status: Needs work » Needs review

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

The last submitted patch, 1741804-20-config-import-directory.patch, failed testing.

beejeebus’s picture

Status: Needs work » Needs review
FileSize
17.8 KB
FAILED: [[SimpleTest]]: [MySQL] 40,734 pass(es), 0 fail(s), and 34 exception(s). View

fixed brokenosity in prepareEnvironment() and implemented #18.

Status: Needs review » Needs work

The last submitted patch, 1741804-24-config-import-directory.patch, failed testing.

beejeebus’s picture

Status: Needs work » Needs review
FileSize
17.8 KB
PASSED: [[SimpleTest]]: [MySQL] 40,739 pass(es). View

blurgh, fix typo in config_get_config_directory.

beejeebus’s picture

FileSize
18.29 KB
PASSED: [[SimpleTest]]: [MySQL] 40,746 pass(es). View

in #drupal-contribute chx asked me to make the code in drupal_install_config_directories() use a loop, and i think its a good suggestion.

new patch is the same as 26 apart from that.

dagmar’s picture

Status: Needs review » Needs work

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.

sun’s picture

Status: Needs work » Needs review
FileSize
12.45 KB
24.48 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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

sun’s picture

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 ?

Status: Needs review » Needs work

The last submitted patch, config.directories.29.patch, failed testing.

yched’s picture

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 ?

sun’s picture

Title: implement a config import directory » Implement a config import/staging directory

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.

sun’s picture

Status: Needs work » Needs review
FileSize
13.22 KB
23.4 KB
PASSED: [[SimpleTest]]: [MySQL] 40,364 pass(es). View

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.

beejeebus’s picture

sun can drive this home.

sun’s picture

Status: Needs review » Reviewed & tested by the community

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

sun’s picture

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

chx’s picture

FileSize
5.88 KB
12.81 KB
24.02 KB
FAILED: [[SimpleTest]]: [MySQL] 40,341 pass(es), 0 fail(s), and 1 exception(s). View

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.

chx’s picture

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

Status: Reviewed & tested by the community » Needs work
Issue tags: -Configuration system

The last submitted patch, 1741804_38.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

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

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

The last submitted patch, 1741804_38.patch, failed testing.

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
23.73 KB
PASSED: [[SimpleTest]]: [MySQL] 40,339 pass(es). View

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

alexpott’s picture

+1 for this patch...

webchick’s picture

Status: Reviewed & tested by the community » Fixed

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!

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