Move CMI import/export directory "staging" to "sync", as it is confused with staging environments

Thanks @Schnitzel.

So I see the case for this (and kind of agree now that I've thought about it), but this would be a massive BC/workflow/expectation break. Unfortunately my gut reaction is that we shouldn't do this during the beta.

It depends I think this is possible in a backwards compatible way.

If we want to change the code to match the name then we need to do something like this.

Here is the smallest possible change.

I can't find the issue right now, but it was suggested before to actually split the double meaning of the current staging directory into separate import and export directories. That would make a lot of sense IMO. And if you specify both to be the same the functionality would be the same as it currently is. We could even consider some sort of compatibility layer, i.e. if you have specified a staging directory, that will override both import and export.

Thoughts?

Re @Schnitzel: Yes, that's certainly one workflow, and probably the most common (and preferred) one. It can totally be served by separating the import and export directories - as a concept - if you just point them to the same physical directory. That also makes the workflow very self-documenting: We don't need to explain what "staging" means, what happens is evident from the fact that import and export directory are identical. But this would a) allow a bunch of other use-cases, and it gets rid of this documentation/naming barrier. I.e. when people simply FTP the YAML files onto their Drupal sites, it's much more self-documenting to put them in a directory called "import" than "staging".

But that's just my 2 cents. I don't want to derail this, so feel free to ignore, if the suggestion is not as constructive as I hoped. I just thought it would be an elegant solution to this, while the added flexibility is a mere side-benefit.

IMO, this is exactly the sort of change we should do now, in order to spare thousands of confused devs in the next 5 years. Right now, there are 20 something live d8 sites that would suffer "massive BC break". I think we should go further than the latest patch proposes and rename the constant to IMPORT_EXPORT (i'm ambivalent on that). I volunteer to update drush accordingly.

In #2247291: Reorder tabs in configuration UI @Eli-T has pointed out how this exactly same confusion exists in the current UI.

I personally more like sync rather than import-export, because import-export could also be import_export and ImportExport while sync is always sync.

Attached patch changes staging to sync in all places, including comments and variable names. So, now the core contains only two occurrences of word staging, where it means staging server.

There is no backward compatibility layer in the patch. I believe an upgrade path could be provided in the head2head module.

I forgot to rename some files in the previous patch.

This needs thoughtful beta evaluation as soon as possible.

FYI #2487592: CMI: don't ship with a default "active" directory that is empty in most Drupal installations just went in.

Added staging-to-sync fallback to not break existing installations.
Also created D9 followup #2574943: Trigger E_USER_DEPRECATED when CONFIG_STAGING_DIRECTORY fallback is called

Created a draft change record: https://www.drupal.org/node/2574957

The patch in #26 changes strings that have been changed following UX review in #2572537: Update the UI texts for the Configuration Manager module.

Have we discussed splitting into two folders - one for import and one for export? I actually think thats the clearest. It also helps security since you would usually make the import one read-only as it is git controlled, while the web server is allowed to write to export.

@Schnitzel, regarding #32, the fallback happens only if "staging" directory is set, so no exception is thrown in this case.

Attempting a reroll.

Rerolled - conflict with #2520540: Enforced configuration dependencies shouldn't have to be repeated in the calculated dependencies

102c102
< index 4a2b41b..b12b660 100644
---
> index e9e1194..9a9a9e6 100644
105c105
< @@ -178,8 +178,8 @@ function drupal_get_database_types() {
---
> @@ -202,8 +202,8 @@ function drupal_get_database_types() {
116c116
< @@ -187,7 +187,7 @@ function drupal_get_database_types() {
---
> @@ -211,7 +211,7 @@ function drupal_get_database_types() {
125c125
< @@ -463,9 +463,9 @@ function drupal_install_config_directories() {
---
> @@ -487,9 +487,9 @@ function drupal_install_config_directories() {
138c138
< @@ -481,19 +481,19 @@ function drupal_install_config_directories() {
---
> @@ -505,19 +505,19 @@ function drupal_install_config_directories() {
162c162
< @@ -501,7 +501,7 @@ function drupal_install_config_directories() {
---
> @@ -525,7 +525,7 @@ function drupal_install_config_directories() {
196c196
< index a762de9..5266ff4 100644
---
> index 80180ce..7de5058 100644
206c206
<   * get the current dependencies without recalculation, you can use
---
>   * get the current dependencies (without recalculation), you can use
1727c1727
< similarity index 78%
---
> similarity index 79%
1730c1730
< index 5c67c7b..e47a615 100644
---
> index 394c83b..83c9aae 100644
1744c1744
< similarity index 77%
---
> similarity index 79%
1747c1747
< index 6113d7e..fee4cbe 100644
---
> index d035ef8..d692f78 100644
1912c1912
< index 0e2bf5c..f617201 100644
---
> index 394c767..f11b567 100644
1943c1943
< index 65edaeb..9c0f46b 100644
---
> index 7abfa2f..2270958 100644
1946c1946
< @@ -1513,7 +1513,7 @@ public function configImporter() {
---
> @@ -1518,7 +1518,7 @@ public function configImporter() {
2094c2094
< index e8a7c1b..6e43ab2 100644
---
> index 5a3f359..b98dea2 100644
2097,2098c2097,2098
< @@ -138,7 +138,7 @@ public function testCalculateDependencies() {
<        $dependencies = $view->calculateDependencies();
---
> @@ -138,7 +138,7 @@ public function testGetDependencies() {
>        $dependencies = $view->getDependencies();
2107c2107
< index 085009c..5271a35 100644
---
> index 6b95f4d..fd7b864 100644
2128c2128
< @@ -942,7 +942,7 @@ protected function configImporter() {
---
> @@ -943,7 +943,7 @@ protected function configImporter() {
2137c2137
< @@ -1087,7 +1087,7 @@ public function __get($name) {
---
> @@ -1088,7 +1088,7 @@ public function __get($name) {
2160c2160
< index 1ea5d72..d93cc09 100644
---
> index 7d3a3ae..6a45d25 100644

Diff of #38 to this patch... just a context difference.

Reviewing this.

(Partial review notes.)

1. +++ b/core/core.services.yml
   @@ -290,9 +290,9 @@ services:
   -  config.storage.staging:
   +  config.storage.sync:
   
   +++ b/core/includes/bootstrap.inc
   @@ -109,9 +109,19 @@
   +const CONFIG_SYNC_DIRECTORY = 'sync';
   
   +++ b/core/lib/Drupal/Core/Config/FileStorageFactory.php
   @@ -25,12 +25,12 @@ static function getActive() {
   -  static function getStaging() {
   ...
   +  static function getSync() {
   

   For the CR.

2. +++ b/core/includes/bootstrap.inc
   @@ -109,9 +109,19 @@
     * $config_directories key for staging directory.
     *
     * @see config_get_config_directory()
   + *
   + * @deprecated in Drupal 8.0.x and will be removed before 9.0.0. The staging
   + *   directory was renamed to sync.
   

   I think there should also be an @see to CONFIG_SYNC_DIRECTORY? Could be fixed on commit/followup/whatevs.

3. +++ b/core/includes/bootstrap.inc
   @@ -140,6 +150,11 @@
   +  // @todo: Remove fallback in Drupal 9. https://www.drupal.org/node/2574943
   +  if ($type == CONFIG_SYNC_DIRECTORY && !isset($config_directories[CONFIG_SYNC_DIRECTORY]) && isset($config_directories[CONFIG_STAGING_DIRECTORY])) {
   +    $type = CONFIG_STAGING_DIRECTORY;
   +  }
   

   Happy BC.

4. +++ b/core/includes/install.inc
   @@ -211,7 +211,7 @@ function drupal_get_database_types() {
     *   $config_directories['active'] = 'config_hash/active';
   
   @@ -525,7 +525,7 @@ function drupal_install_config_directories() {
     *   Type of config directory to return. Drupal core provides 'active' and
   

   Looks like this was missed in #2487592: CMI: don't ship with a default "active" directory that is empty in most Drupal installations I guess? Followup.

5. +++ b/core/modules/config/config.module
   @@ -33,7 +33,7 @@ function config_help($route_name, RouteMatchInterface $route_match) {
   -      $output .= '<p>' . t('Compare the configuration uploaded to your staging directory with the active configuration before completing the import.') . '</p>';
   +      $output .= '<p>' . t('Import configuration that is placed in your sync directory. All changes, deletions, renames, and additions are listed below.') . '</p>';
   
   @@ -43,7 +43,7 @@ function config_help($route_name, RouteMatchInterface $route_match) {
   -      $output .= '<p>' . t('Upload a full site configuration archive to the staging directory. It can then be compared and imported on the Synchronize page.') . '</p>';
   +      $output .= '<p>' . t('The full import page can be used to import a full set of configuration for this site by uploading a gzipped or bzipped tar file consisting of all configuration YAML files. The results will be placed in a the sync directory, so they can be compared in the Synchronize tab. There the import can be finalized.') . '</p>';
   

   Are these string rewrites intentional or a merge artifact? I'll look back on the issue.

I will work on the change record.

[edit: wait, I see one was made in #27, but it didn't have the issue linked, so it was not showing on the sidebar. editing that one.]

There is a draft of a change record: https://www.drupal.org/node/2574957

+++ b/sites/default/default.settings.php
@@ -223,13 +223,13 @@
- * directories used for configuration data. On install, "active" and "staging"
...
+ * directories used for configuration data. On install, "active" and "sync"
...
- * The default location for the active and staging directories is inside a
+ * The default location for the active and sync directories is inside a

Also for that "active" followup.

change record is now showing up in the sidebar here, and needs review: https://www.drupal.org/node/2574957

[edit: I'm searching for staging in change records to see if there are some that should be updated https://www.drupal.org/list-changes/published/drupal?keywords_descriptio...

Reviewed with mad word diff regex skills and confirmed the new patch does just this one thing. :)

Also:

[mandelbrot:maintainer | Mon 13:46:04] $ grep -ri "staging" * | grep -v "~" | grep -v "vendor"
core/includes/bootstrap.inc: * $config_directories key for staging directory.
core/includes/bootstrap.inc: * @deprecated in Drupal 8.0.x and will be removed before 9.0.0. The staging
core/includes/bootstrap.inc:const CONFIG_STAGING_DIRECTORY = 'staging';
core/includes/bootstrap.inc:  if ($type == CONFIG_SYNC_DIRECTORY && !isset($config_directories[CONFIG_SYNC_DIRECTORY]) && isset($config_directories[CONFIG_STAGING_DIRECTORY])) {
core/includes/bootstrap.inc:    $type = CONFIG_STAGING_DIRECTORY;
core/modules/serialization/src/EntityResolver/EntityResolverInterface.php:   * on example.com and staging.example.com), it is generally unsuitable for use
sites/default/default.settings.php: * Use settings.local.php to override variables on secondary (staging,,

The first three are the deprecated constant itself; the next three are the BC fallback; and the last two are unrelated things that actually refer to a staging environment.

CR looks great.

Now I'm just going to test it a bit.

+++ b/core/core.services.yml
@@ -290,9 +290,9 @@ services:
-  config.storage.staging:
+  config.storage.sync:

We could leave the config.storage.staging in core.services and just alias it to config.storage.sync

I think renaming the method in the factory is fine since no one should be using that directly.

Unassigning for someone to fix that while I muck about manually testing things. :)

I tested this manually with an existing (pre-beta 16) site: applied the patch, ran update.php, proceeded to sync config with another install of the same site with no problems. Yay APIs and yay happy BC. I also tested a newly installed site and confirmed config sync still functions as expected there too.

drush cex -y works fine for the existing site, but breaks for the new site. We will need a PR for drush for this change so that drush works with either. Anyone up to file that?

I was also going to test https://www.drupal.org/project/config_installer but that is broken since the conf_path() removal, it seems.

I will update Drush quickly if noone else does.

searched for CRs mentioning staging
no change to https://docs.google.com/spreadsheets/d/1glIXozp3GHj23bYAQMufqs1mQ-xnRZCD...
https://www.drupal.org/node/2220437/revisions/view/7161595/8957641
https://www.drupal.org/node/2268523/revisions/view/7254309/8957627
https://www.drupal.org/node/2241059/revisions/view/7392715/8957659 <- maybe revert this one
https://www.drupal.org/node/2164727/revisions/view/7149123/8957863 <- might need more changes
no change to https://www.drupal.org/node/1548406
no change to https://www.drupal.org/node/1352228

The CR exists now.

I will try and do #56

here goes.

I think the difference in patch size, is my git diff settings to show a rename. but there could be something else. a diff of the patch files might be good to do on review.

[edit: it's fine, diffing my patch to the one in #44 shows the changes from the #51. so all good.]

+++ b/core/core.services.yml
@@ -290,6 +290,8 @@ services:
+  config.storage.staging:
+    alias: config.storage.sync
   config.storage.sync:
     class: Drupal\Core\Config\FileStorage
     factory: Drupal\Core\Config\FileStorageFactory::getSync

Let's do 2 things here... add a comment saying config.storage.staging is deprecated.

But also swap it to be like this:

  config.storage.staging:
     class: Drupal\Core\Config\FileStorage
     factory: Drupal\Core\Config\FileStorageFactory::getSync
   config.storage.sync:
     alias: config.storage.staging

Because then if any site is swapping out the config.storage.staging service everything will magically still work.

on it.

comments in config files are not parsed by api.drupal.org, but used that notation anyway as we are used to it.

actual interdiff.

+++ b/core/core.services.yml
@@ -291,8 +291,12 @@ services:
       - { name: backend_overridable }
   config.storage.staging:
+    # @deprecated staging is renamed to sync.
+    # @see CONFIG_STAGING_DIRECTORY
     class: Drupal\Core\Config\FileStorage
-    factory: Drupal\Core\Config\FileStorageFactory::getStaging
+    factory: Drupal\Core\Config\FileStorageFactory::getSync
+  config.storage.sync:
+    alias: config.storage.staging
   config.storage.snapshot:

The Yaml looks great. I think we should put the deprecation notice above the service definition and use the standard text... like so...

   # @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.0.
   #   Use config.storage.sync instead.
   config.storage.staging:
     class: Drupal\Core\Config\FileStorage
     factory: Drupal\Core\Config\FileStorageFactory::getSync

ok. :)

1. +++ b/core/includes/bootstrap.inc
   @@ -140,6 +151,11 @@
   +  // @todo: Remove fallback in Drupal 9. https://www.drupal.org/node/2574943
   

   todo syntax doesn't use :
   https://www.drupal.org/node/1354#todo

2. +++ b/core/includes/install.inc
   @@ -505,19 +505,19 @@ function drupal_install_config_directories() {
   -  // Put a README.txt into the staging config directory. This is required so
   +  // Put a README.txt into the sync config directory. This is required so
      // that they can later be added to git. Since this directory is auto-
   

   since line shorter, can wrap closer to 80 chars.

went through the whole patch, wrapped some other things also.

another couple changes to add a period to the end of a sentence (in lines this patch was changing anyway.)

This looks great.

mkay

I checked the word diff and the grep again. All is well. +1 for the BC with the service alias.

I'm glad we got this in! Committed and pushed to 8.0.x, adn updated and published the CR. Thanks, all!

Drush has been updated for this change. Nice work all.

Upgrading core from beta15 to rc1:

/admin/reports/status:
Exception: The configuration directory type 'sync' does not exist in config_get_config_directory() (line 162 of core/includes/bootstrap.inc).

/update.php:
Not present
Your /settings.php file must define the $config_directories variable as an array containing the name of a directories in which configuration files can be written.

What is the recommend upgrade path?

creating /sites/default/files/config_xyz/sync by hand does not help

$config_directories = array(); -> is always an empty array, even when installing rc1 from scratch

@k4 you don't only need to have the sync directory created but also the following in settings.php

$config_directories['sync'] = 'sites/default/files/config_HASH/sync';

Change config_HASH by your unique HASH and default by anything else you might have chosen to go with..

#81 worked for me