Problem

Early on in the CMI discussions, it was determined that we had to support partial deployment of configuration - if you add a view, you should be able to deploy just that view. Unfortunately this also caused several architectural problems

  • Deploying deletes of configuration is impossible without a full accounting of the items available in each site. To get around this the manifest system was created, however manifests also made deployment more confusing, as it meant every add or delete of a ConfigEntity was associated with two files rather than one.
  • It is far more difficult to manage dependencies in a partial deployment scenario. For instance, I could attempt to deploy a field instance without deploying the field definition or the bundle it is attached to. We can add this work to a verification step, but it is a lot more work we are creating for ourselves.
  • After doing an import we had to delete all config from the staging directory to prevent it from being imported again. This causes issues with git-based deployment mechanisms and is not completely intuitive.

Solution

Given these and other related problems, we should eliminate the ability to do partial deployments and instead require that the whole config tree be deployed at all times. Additionally, we will ad a UI to export and import the entirety of your config via a single tarball. This will actually increase usability for non-technical users, as it means they no longer have to dive into their directory structure, find the weirdly named config directory, and move files between two differently named directories. This is a pretty big UX win. This decision was made at DrupalCon Portland in a meeting with the CMI maintainers and major contributors, and was approved by Dries.

For those users that are adamant about needing partial deployment, they can write their own by swapping out the import mechanism. This will not be supported by core though.

CommentFileSizeAuthor
#22 19-22-interdiff.txt699 bytesalexpott
#22 1998576-22-config-tree-import.patch54.5 KBalexpott
#19 1998576-19-config-tree-import.patch53.82 KBswentel
#19 interdiff.txt1.13 KBswentel
#17 16-17-interdiff.txt2.69 KBalexpott
#17 1998576-17-config-tree-import.patch53.82 KBalexpott
#16 1998576-16-config-tree-import.patch53.8 KBAnonymous (not verified)
#9 1998576-9-config-tree-import.patch52.92 KBAnonymous (not verified)
#8 1998576-8-config-tree-import.patch52.69 KBAnonymous (not verified)
#7 1998576-7-config-tree-import.patch51.93 KBAnonymous (not verified)
#5 1998576-5-config-tree-import.patch54.72 KBAnonymous (not verified)
#4 1998576-4-config-tree-import.patch42.48 KBAnonymous (not verified)
#3 1998576-2-config-tree-import.patch.patch28.39 KBAnonymous (not verified)
#2 1998576-2-config-tree-import.patch30.24 KBAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

I'm guessing this will complicate #1998204: config_install_default_config() is not safe to use in hook_update_N() a bit.
Right now we're using config_install_default_config() to install new config of a module, but my first idea was to do per-file imports during upgrade. Not sure how that jives with this.

Anonymous’s picture

Status: Active » Needs review
FileSize
30.24 KB

i'm working in the 1998576-config-tree-import branch.

here's a work in progress patch, config import and config import UI tests are working.

Anonymous’s picture

woops, ignore that patch, here's one with a better rebase.

Anonymous’s picture

updated patch, i've removed all the 'manifest' strings i could find in core, and all the tests that referenced manifests are passing for me. oh hai bot.

Anonymous’s picture

i've added a dumb UI for exporting and importing a config.tar.gz file.

only manually tested by going to admin/config/development/export and admin/config/development/import.

alexpott’s picture

re #1 it does not complicate it at all :)

The config installer uses a storagecomparer to only create new config... :)

Anonymous’s picture

hmm, the last patch had some user.module hunks that it shouldn't.

new patch with those removed.

Anonymous’s picture

updated patch, adds admin screen menu items, slightly less ugly export form.

Anonymous’s picture

reroll to keep up with 8.x

gdd’s picture

Issue summary: View changes

Updated issue summary.

gdd’s picture

Issue summary: View changes

Updated issue summary.

gdd’s picture

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigImportForm.phpundefined
@@ -0,0 +1,53 @@
+      \Drupal::service('config.storage.staging')->deleteAll();

We actually don't need to do this anymore. If we always assume a full config tree, then we can just leave the files after doing an import. I guess one question is do we even keep the existing import UI? Or do we just replace it with the one-click download/upload?

I also expanded the issue summary to make it more clear why this is needed.

gdd’s picture

Issue summary: View changes

Updated issue summary.

Anonymous’s picture

delete - huh, good catch, i'd talked about that but forgot to take it out.

as for the import UI, i think we need it still, just to allow for a sanity check.

maybe we should add a 'skip confirmation of changes'(or whatever wording makes sense), unchecked by default? so people who Know What They Are Doing can skip the import form?

Status: Needs review » Needs work

The last submitted patch, 1998576-9-config-tree-import.patch, failed testing.

gdd’s picture

I had a thought in the shower today while considering the import screen question. This screen actually provides a lot of very useful information that is lost in the single upload/import operation - whether config is overridden, what config changed, ability to view diffs, etc. What if we made the upload operation just that - upload and extract only. Then after config is extracted to staging, we redirect to the current import screen with a message like 'Your configuration has been uploaded. Please review the changes below and click "Import Changes" to make them active."

This allows us to have the single button upload but still maintain what is really important functionality. We could even remove the current screen from the visible menu to reduce the chances of users ending up there accidentally. Just have 'Download configuration' and 'Upload configuration'.

I really like this idea.

Anonymous’s picture

re #13 - yep, the current patch just redirects after upload, so you land on the import screen with a message. i'd be happy for it to land that way.

gdd’s picture

Oh perfect! I hadn't actually tried the upload side and I guess I missed it in the patch.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
53.8 KB

updated patch takes out the delete code from the import form.

also added some slightly less stupid UI strings, but i could really do with some feedback on those.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
53.82 KB
2.69 KB

This patch is fantastic, the removal of manifests is a huge win, the simplicity of the one button export, import, synchronise is a huge win for usability and less wtf's from new and advanced users.

let's get this in and worry about the strings before string freeze.

rerolled due to userng...

interdiff proves i did no significant change...

swentel’s picture

Two nitpicks, but I won't set it to needs work for it.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImporterTest.phpundefined
@@ -77,6 +80,20 @@ function testNoImport() {
+   * Tests that trying to import from an empty staging configuration directory fails.

80 chars

+++ b/core/modules/system/lib/Drupal/system/SystemConfigSubscriber.phpundefined
@@ -0,0 +1,39 @@
+ * System Config subscriber

Missing end point

swentel’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.13 KB
53.82 KB

Nitpicks

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

This is RTBC when #19 is green :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1998576-19-config-tree-import.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
54.5 KB
699 bytes

This should fix the failing tests....

gdd’s picture

Status: Needs review » Reviewed & tested by the community

This is a great patch and a great architectural adjustment that is going to make everyone's lives easier. Huge thumbs up to beejeebus and alexpott. Ship it!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Makes loads of sense. Committed/pushed to 8.x.

kim.pepper’s picture

Wow. This is going to make working with config in D8 soooooo much easier. Thank you everyone who worked on this!

xjm’s picture

Priority: Normal » Major

Retroactive priority bump. :)

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.