Problem/Motivation
I really think we should not call the staging directory of CMI "staging". I spoke to a lot of people that already use Drupal 8 in production and they all agree that it's a bad name. Because it is confused a lot with actual staging environments. Some CMI Workflows actually have separate folders for different hosting environments (like "to_dev", "from_prod", "staging") and then it is even more confusing.
Proposed resolution
In #1741804: Implement a config import/staging directory the staging directory was introduced. There it was decided to call it "staging" as the proposed "import" name was not good enough, as we use that folder also for exporting of configuration.
So I would suggest to actually call it "import-export" or if we don't like the two name system, maybe "synchronization" or "sync"? (we call it anyway synchronization in the UI)
I just think we will start to confuse a lot of people, because when you tell somebody "staging" they immediately think about staging environments
Remaining tasks
+ Agree
+ Write Patch
+ Create change record
- Inform contrib modules to use CONFIG_SYNC_DIRECTORY instead of CONFIG_STAGING_DIRECTORY
User interface changes
-
API changes
- Constant name CONFIG_STAGING_DIRECTORY probably changes
Change record: https://www.drupal.org/node/2574957
Beta phase evaluation
Issue category | Bug |
---|---|
Issue priority | Major because the earlier we fix this issue, the less people are already confused by it. Especially before the RC1 |
Prioritized changes | The main goal of this issue is to make a better developer experience. 8 ships with CMI, which nobody ever knew before, let's make it as streamlined as possible, so that we don't scare people away. |
Is BC? | Yes, fully backwords compatible to modules that still depend on CONFIG_STAGING_DIRECTORY |
Comment | File | Size | Author |
---|---|---|---|
#70 | interdiff.2487588.69.70.txt | 6.87 KB | YesCT |
#70 | 2487588.70.patch | 113.29 KB | YesCT |
#69 | interdiff.2487588.66.69.txt | 594 bytes | YesCT |
#69 | 2487588.69.patch | 112.79 KB | YesCT |
#67 | interdiff.2487588.63.66.txt | 683 bytes | YesCT |
Comments
Comment #1
xjmThanks @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.
Comment #2
xjmComment #3
alexpottIt depends I think this is possible in a backwards compatible way.
Comment #4
alexpottIf we want to change the code to match the name then we need to do something like this.
Comment #5
alexpottHere is the smallest possible change.
Comment #6
Schnitzel CreditAttribution: Schnitzel at Amazee Labs commentednice, thanks @alexpott for just doing that =)
I'm in favor of #4 as I think it's not only confusing for people to have a folder that is called "staging" but also during the backend development.
Anyway, I think though, that import is not the right name, as we also use it for exporting *yay*.
How about "sync"?
Comment #7
tstoecklerI can't find the issue right now, but it was suggested before to actually split the double meaning of the current
staging
directory into separateimport
andexport
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 havespecified
astaging
directory, that will override bothimport
andexport
.Thoughts?
Comment #8
Schnitzel CreditAttribution: Schnitzel at Amazee Labs commented@tstoeckler
this all depends on which GIT workflows we as a community suggest in #1831818: Document git config import workflow.
My current understanding is, that you drush config export on the DEV site, add this code to GIT and on the PROD site you make a drush config import again. And per default drush would be configured to take the same directory for both actions.
But of course we could suggest to config export into the export directory on DEV, add to GIT, but then you would need to drush config import from the export directory? Which is a bit confusing. Or would we tell the people to copy from export to import directory?
In my understanding of having config in GIT: there should be one single directory that contains the config that matches the current code, and in there we should import and export to.
Comment #9
tstoecklerRe @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.
Comment #10
moshe weitzman CreditAttribution: moshe weitzman at Acquia commentedIMO, 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.
Comment #11
alexpottIn #2247291: Reorder tabs in configuration UI @Eli-T has pointed out how this exactly same confusion exists in the current UI.
Comment #12
Leksat CreditAttribution: Leksat at Amazee Labs commentedI personally more like
sync
rather thanimport-export
, becauseimport-export
could also beimport_export
andImportExport
whilesync
is alwayssync
.Attached patch changes
staging
tosync
in all places, including comments and variable names. So, now the core contains only two occurrences of wordstaging
, 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.
Comment #14
Leksat CreditAttribution: Leksat at Amazee Labs commentedI forgot to rename some files in the previous patch.
Comment #17
anavarreComment #18
cilefen CreditAttribution: cilefen as a volunteer commentedThis needs thoughtful beta evaluation as soon as possible.
Comment #19
anavarreFYI #2487592: CMI: don't ship with a default "active" directory that is empty in most Drupal installations just went in.
Comment #20
Schnitzel CreditAttribution: Schnitzel at Amazee Labs commentedgonna work on that in Barcelona
Comment #21
Schnitzel CreditAttribution: Schnitzel at Amazee Labs commentedreroll
Comment #24
Schnitzel CreditAttribution: Schnitzel at Amazee Labs commentedanother reroll, there was a typo in there and some files missing
Comment #25
Schnitzel CreditAttribution: Schnitzel at Amazee Labs commentedComment #26
Leksat CreditAttribution: Leksat at Amazee Labs commentedAdded 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
Comment #27
Leksat CreditAttribution: Leksat at Amazee Labs commentedCreated a draft change record: https://www.drupal.org/node/2574957
Comment #28
Eli-TThe patch in #26 changes strings that have been changed following UX review in #2572537: Update the UI texts for the Configuration Manager module.
Comment #29
moshe weitzman CreditAttribution: moshe weitzman at Acquia commentedHave 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.
Comment #30
Schnitzel CreditAttribution: Schnitzel at Amazee Labs commentedComment #31
Schnitzel CreditAttribution: Schnitzel at Amazee Labs commentedYes, see #7 and #8.
I think my take and also the take from @alexpott (discussed with him at DrupalCon Barcelona) is, that we actually don't need the staging/sync/import/export directories at all. Because the UI could just create a temporary folder when it is importing the full configuration, exporting anyway send a tarball via the Browser.
And the big discussion we have right now about how the GIT/rsync/anyother workflow should work, is a lot depending on people thinking to need to use the staging directory, which is completely wrong. You need it if you wanna use rsync CMI workflows, but you don't need it if you have git and drush config-import workflows.
So it's great that we have a possibility for config files to be imported via the UI with rsync, but that does not mean that we need to create a folder by default. We could just explain people in the settings.default.php how to do it and make the code less depended on an actual folder and just create the folder if we really need it.
But what we also agreed to, that this change is much bigger and needs involvement of a lot more people.
So we say: if we cannot remove the folder completely, we wanna at least name it in a way that people are not confused about it.
And therefore I would keep 1 folder and call it sync, like the UI also calls it :)
Comment #32
Schnitzel CreditAttribution: Schnitzel at Amazee Labs commentedwe should also adapt the exception to throw that 'sync' does not exist, because currently it would throw that 'staging' does not exist even when we request the 'sync' directory, which would be confusing a lot :)
Comment #33
Leksat CreditAttribution: Leksat at Amazee Labs commented@Schnitzel, regarding #32, the fallback happens only if "staging" directory is set, so no exception is thrown in this case.
Comment #34
Schnitzel CreditAttribution: Schnitzel at Amazee Labs commented@Leksat
Oh! correct, thanks for clarification :)
I think this is good then! RTBC, but as it conflicts with #2572537: Update the UI texts for the Configuration Manager module, let's wait till that one got in and then we can adapt your patch to change these strings as well.
Comment #36
Schnitzel CreditAttribution: Schnitzel at Amazee Labs commented#2572537: Update the UI texts for the Configuration Manager module is in, retesting :)
Comment #38
anavarreAttempting a reroll.
Comment #39
anavarreComment #40
Schnitzel CreditAttribution: Schnitzel at Amazee Labs commentednice! thanks :)
Comment #41
Schnitzel CreditAttribution: Schnitzel at Amazee Labs commentedComment #42
Schnitzel CreditAttribution: Schnitzel at Amazee Labs commentedadded beta evaluation
Comment #44
alexpottRerolled - conflict with #2520540: Enforced configuration dependencies shouldn't have to be repeated in the calculated dependencies
Diff of #38 to this patch... just a context difference.
Comment #45
xjmReviewing this.
Comment #46
xjm(Partial review notes.)
For the CR.
I think there should also be an @see to CONFIG_SYNC_DIRECTORY? Could be fixed on commit/followup/whatevs.
Happy BC.
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.
Are these string rewrites intentional or a merge artifact? I'll look back on the issue.
Comment #47
YesCT CreditAttribution: YesCT commentedI 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.]
Comment #48
Leksat CreditAttribution: Leksat at Amazee Labs commentedThere is a draft of a change record: https://www.drupal.org/node/2574957
Comment #49
Schnitzel CreditAttribution: Schnitzel at Amazee Labs commentedNo they should not be changed that hard, we just created new strings in https://www.drupal.org/node/2572537 and looks like in the reroll that got wrong.
Will fix that.
Comment #51
Schnitzel CreditAttribution: Schnitzel at Amazee Labs commentedfixed #2487588: Move CMI import/export directory "staging" to "sync", as it is confused with staging environments .2 #2487588: Move CMI import/export directory "staging" to "sync", as it is confused with staging environments .5 thanks for review @xjm!
Comment #52
xjmAlso for that "active" followup.
Comment #53
Schnitzel CreditAttribution: Schnitzel at Amazee Labs commentedFollow up will happen here:
#2580569: Update documentation that mentions active CMI directory. active was removed.
Comment #54
YesCT CreditAttribution: YesCT commentedchange 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...
Comment #55
xjmReviewed with mad word diff regex skills and confirmed the new patch does just this one thing. :)
Also:
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.
Comment #56
alexpottWe 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.
Comment #57
xjmUnassigning for someone to fix that while I muck about manually testing things. :)
Comment #58
xjmI 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.Comment #59
moshe weitzman CreditAttribution: moshe weitzman at Acquia commentedI will update Drush quickly if noone else does.
Comment #60
YesCT CreditAttribution: YesCT commentedsearched 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
Comment #61
xjmThe CR exists now.
Comment #62
YesCT CreditAttribution: YesCT commentedI will try and do #56
Comment #63
YesCT CreditAttribution: YesCT commentedhere 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.]
Comment #64
alexpottLet's do 2 things here... add a comment saying config.storage.staging is deprecated.
But also swap it to be like this:
Because then if any site is swapping out the config.storage.staging service everything will magically still work.
Comment #65
YesCT CreditAttribution: YesCT commentedon it.
Comment #66
YesCT CreditAttribution: YesCT commentedcomments in config files are not parsed by api.drupal.org, but used that notation anyway as we are used to it.
Comment #67
YesCT CreditAttribution: YesCT commentedactual interdiff.
Comment #68
alexpottThe Yaml looks great. I think we should put the deprecation notice above the service definition and use the standard text... like so...
Comment #69
YesCT CreditAttribution: YesCT commentedok. :)
Comment #70
YesCT CreditAttribution: YesCT commentedtodo syntax doesn't use :
https://www.drupal.org/node/1354#todo
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.)
Comment #71
alexpottThis looks great.
Comment #72
xjmmkay
Comment #73
xjmI 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!
Comment #78
alexpottComment #79
moshe weitzman CreditAttribution: moshe weitzman at Acquia commentedDrush has been updated for this change. Nice work all.
Comment #80
k4 CreditAttribution: k4 commentedUpgrading 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
Comment #81
anavarre@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..
Comment #83
joshuautley CreditAttribution: joshuautley commented#81 worked for me