Problem/Motivation

We have a shiny new configuration system, but no way for users to actually deploy configuration to a new site and import it. We need to create a user interface through which this can happen.

Proposed resolution

Create a user interface to import configuration that has been deployed to a server. This interface should allow users to view the configuration changes that have been made (updates, adds, and deletes) and press a button to accept them. Once this happens, the import should occur. In an ideal world, this is all that would be necessary. It should be very very simple to deploy and import configuration.

After discussion with Bojhan, he made the following mockup for the configuration import screen:

Currently this is implemented with creates and updates working properly. Deletes have turned out to be problematic. Currently the following proposal is on the table:

Config entities maintain a 'manifest' file which lists the objects that each config entity is currently maintaining. This file is a very simple flat listing of config object IDs, and it would always exist, even if it is empty. It would be automatically generated the first time a specific ConfigEntity is constructed, so module maintainers don't have to worry about it at all. Objects are added/removed from this file automatically as they are created/deleted. When config is ready to be deployed, this file will have to be deployed along with the actual objects themselves. Comparing this file to the existing manifest on the target site will generate the create/delete list.

The most recent patches are working with this idea.

Testing instructions - FTP/file copy workflow

  1. Download two copies of Drupal 8 and put them in separate subdirectories. Call one 'Dev' and the other 'Live'. Apply the most recent patch to both of them. (For more information about applying patches see the patch documentation.) Do whatever you need to do on your local development environment to be able to run these downloads at separate URLS (outside the scope of these instructions.)
  2. Install both downloads.
  3. On dev, make some changes to settings. Not everything is currently converted to CMI but the following areas are
    • Performance settings
    • RSS settings
    • Cron settings
    • Site Information
    • Maintenance mode settings
    • Search settings
    • Image Stlyes
  4. Each site has two directories under sites/default/config_[random_string]. One is named 'active' and one named 'staging'. Copy the configuration from 'active' on the 'dev' site to 'staging' on the 'live' site. You can either copy all of the configuration, or just the files that you have changed.
  5. On the 'live' site, visit 'admin/config/development/sync' and you will see the screen listing your configuration changes. Look at them and see that they represent the changes you made. If so, press 'Import All'. The import will run. Check through the live site to make sure the appropriate changes were in fact made.

Testing instructions - git workflow (to automatically deploy from dev site's active to production site's staging)

This is more involved and requires making some changes to settings.php. I have outlined this process at #1697256-93: Create a UI for importing new configuration.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gdd’s picture

Here's one to get the ball rolling #1697266: Allow cherry picking what config to sync

gdd’s picture

Issue tags: +Configuration system

Tagging

sun’s picture

Priority: Major » Normal

Given the we ripped off the UI from the import patch and there is no UI/UX to improve at all currently, I'm demoting this normal. ;)

I'm also not sure whether this issue shouldn't be postponed on #1703168: [Meta] Ensure that configuration system functionality matches expected workflows for users and devs in the first place...?

gdd’s picture

Yes, that is totally correct. It is stilly to work on the UI until the other issue is resolved. I plan on fleshing that out some today.

Bojhan’s picture

Issue tags: +Usability

I would like to actually design this, rather than do touch up on some patch that doesn't exist anymore?

I don't really understand how this slipped through my radar, can you please inform the UX-Team when you are about to do something important UX wise? Somehow feel that all of this whether its this issue or the other will have UI impacts.

Please keep in mind, that making a good UI takes a very long time. If we can already start on parts, I would very much like that. Otherwise it ends up being a rush-job near code freeze, and that is very stressful on me.

gdd’s picture

I had thought just as I was leaving Munich that I should have pinged you about this at the code sprint on the last day. We are making several changes in the plumbing which will address many of the problems listed above as well as simplifying the UI, but I also agree we should come up with a nice design for this. What's the best way to start? I can maybe hack together a wireframe of our current thoughts and go from there?

Bojhan’s picture

@heyrocker Guess I missed that, anyways its history :) lets figure out how to do this. The best way is outlining what you want the system to do and then a wireframe, so we can visualize what it means.

Bojhan’s picture

Title: [meta] Improve UX of configuration synchronization » [meta] UX of importing configuration objects
Issue tags: +Needs issue summary update
FileSize
57.62 KB

I did a quick call with heyrocker to explore what all of this means. This page actually seems quite simple to design, a user comes here when something is added to the import map (or other plugin mechanism). The purpose of this page, is to review whether the right configuration changes are to be imported. To make this an explicit action.

In this initial design, I basically split it all up in separate tables for scan ability - more so, than you can find in plain lists. Introducing table headers, something you can find in contrib but we sadly don't really have for core yet. The import button, is the only action.

config-home-import.jpg

EDIT: Please add an up-to-date issue summary, it is a bit unclear what we are discussing. I am a little concerned that this page exposes a education problem, most site builders that get to this page initially will have no clue how to get anything on this page or what it does. Our 700.000 user base is very diverse, and I want to avoid creating this UI where people have no clue what it is for, and or what to do - similar to the performance page, we should provide good but concise guidance.

I'd love to hear any feedback.

corvus_ch’s picture

Clean and Simple. I like this.

Two additions:

  • Allow to apply each file individually. The link/button can be placed inside the tables in a separate column.
  • I really would like to see what the different files contains. So when hoover over one of the filenames, a diff should apear showing how this file will affect the system.

The later is clearly a nice to have but a helpful one.

sun’s picture

Status: Active » Needs review
FileSize
5.34 KB
13.95 KB

Since we are finally closing down #1626584: Combine configuration system changes to verify they are compatible (yay!), I'm moving the original config module UI implementation over here.

This patch is in no way meant to be ready or complete and should not be understood as a resolution proposal for this issue.

I'd actually love to see some bold proposals here. @heyrocker already mentioned one in #1, so anything along those lines.

Furthermore, I agree with @Bojhan in that there is a fundamental problem space involved: We need to cater for at least two different audiences:

1) People who do not really understand what this is for and all about. They need clear and concise help that explains how this is supposed to work and in which situations it is needed. They also possibly need instructions for how to stage configuration.

2) People (developers) who are used to staging workflows already. This audience is the one that would actually like to see a more advanced UI (showing an actual diff for changed configuration, or selectively importing/exporting individual differences only, etc).

For now, I went ahead and slightly adjusted the implementation to match @Bojhan's mockup in #8 — with the sole purpose of allowing everyone to test the UI:

Added help to config module.
Changed button labels to "Import all" and "Export all".
Replaced fieldsets with headings.

dagmar’s picture

Just to link the issue. #120955: Integrate Diff into Core Maybe can be added into D8.

gdd’s picture

Assigned: Unassigned » gdd

Gonna focus on this in the coming week

webchick’s picture

Testing instructions, copied/modified from #1447686-158: Allow importing and synchronizing configuration when files are updated for posterity.

## DEV SERVER. ##
# Download and extract Drupal 8.x-dev.
wget http://ftp.drupal.org/files/projects/drupal-8.x-dev.tar.gz
tar -zxvf drupal-8.x-dev.tar.gz

# Initialize a git repo there and add all the files.
cd drupal-8.x-dev
git init
git add .
git commit -am "D8 core files."

# Apply & commit the patch.
wget http://drupal.org/files/config.ui_.10.patch
# There's a conflict with config.info due to the packaging script that you need to sort out manually, so use patch.
# git apply --index config.ui_.10.patch
patch -p1 < config.ui_.10.patch
# Add "configure = admin/config/development/sync" to the .info file.
git add core/modules/config/config.admin.inc 
git add core/modules/config/lib/Drupal/config/Tests/ConfigImportUITest.php 
git commit -am "Applying CMI UI patch."

# Install Drupal - dev version.
drush si --db-url=mysql://root:root@localhost/8.x-dev --account-pass=admin -y

# Add the files dir to Git.
git add sites/default/files/config -f
git commit -am "Adding files directory."
# Tons of files get added from the active_X directory.

## PRODUCTION SITE ##
# Re-create a copy  of the site for prod.
cd ../
git clone file:///Users/abyron/Sites/drupal-8.x-dev drupal-8.x-prod
mysql -uroot -proot
DROP DATABASE `8.x-prod`;
CREATE DATABASE `8.x-prod`;
exit
mysqldump -uroot -proot 8.x-dev > dump.sql
mysql -uroot -proot 8.x-prod < dump.sql

# Copy over settings.php.
cd drupal-8.x-prod
cp ../drupal-8.x-dev/sites/default/settings.php sites/default/settings.php
# Edit, point database to prod.
webchick’s picture

Also, I'm getting this on my prod site, though I am not sure if there are special steps required to reproduce the problem. I'll try and figure that out next time I test.

error about file->listAll()

Not bumping down to "needs work," since there's still plenty to review here.

gdd’s picture

Note that is (or more likely when) #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config lands, we will also be enabling/disabling/installing/uninstalling (or some combination thereof) modules through this interface. So that is up to four new categories we will need. One possibility is we could just put these into a 'Modules' table, with the name of the module and what is being changed. Or we could give each category its own heading, but then we have up to 8 tables in an import which feels like it might start getting overwhelming. Could be leaning towards vertical tabs at that point? Dunno.

sun’s picture

we will also be enabling/disabling/installing/uninstalling (or some combination thereof) modules through this interface

I strongly recommend to keep that internal business logic out of the user interface. Modules will still be managed on the Modules page. This is the config import/export UI, and this UI should focus on the actual job at hand. A difference in enabled modules is just one of many differences between active and staging. The user of this UI doesn't need and does not want to know about the internal config import/staging mechanisms.

gdd’s picture

Are you suggesting that the import function should not also manage module install/uninstall? Or that it should hand it and not be exposed in the UI?

In either case I really disagree. Enabling and disabling modules has to be tied to deployment and import if the file that manages contains this information lives in the config directory. That is a pretty big DX concept - anything in here is config, and is managed from this import screen. That is of course not the ONLY place it is managed, you can still manage it from the Extend page (just like you can still manage Views from the UI) but import should acknowledge changes to module status that it sees.

If that is true, then it really has to be exposed to the users as well. Enabling/disabling modules is a pretty big deal with heavy consequences. The user should know that it is happening and have a chance to react if that data is not what they expect it to be. If we don't expose it I think it will be a pretty major UX fail.

Bojhan’s picture

I imagine sun is correct in stating we shouldn't be moving over the UI of enabling/disabling modules. From my understanding all that heyrocker wants is that when you push import, you can also execute the config change of enabling/disabling/uninstalling a module which sounds like what I expected.

I don't really get what the discussion is about, is it about whether we want to show to people like a diff what modules are enabled/disabled/uninstalled?

gdd’s picture

Bojhan is correct, that is all im asking.

My original post was that I thought it was important to show the specific modules being enabled/disabled/installed/uninstalled because these can be destructive and deserve special attention. In other places where we do destructive behavior (like in field deletions) we will specifically call it out in the Deleted section of this page. I figured it was worth the extra level of detail for the module actions.

Anonymous’s picture

i agree with #19. i think this is a case of 'modules are special'.

sun’s picture

yeah, sorry, thanks @Bojhan for clarifying my comment. I also agree with the special-casing for the possibly destructive behavior of importing module/extension list changes.

(That said, I'm entirely not sure whether we want to "stage" the uninstallation of a module/extension, which is an entirely different beast compared to just disabling... but that's a discussion for another issue.)

(I also wonder whether possible module/extension list changes won't make a selective/custom import of individual changes impossible — it would add a huge requirement of managing dependencies for config changes to the config import mechanism; i.e., "You cannot cherry-pick this change, since you chose to not import the addition of Xyz module." — but that's a discussion for another issue.)

With regard to the actual UI, I'm actually not sure what else we can do. We don't have a Diff tool/component in core (and quickly searching the net didn't yield a good/modern PHP library either), so a diff feature most likely can't be part of the initial UI. That pretty much leaves us with the current UI proposal, which is implemented in the latest patch... (?)

gdd’s picture

Note that with the new paradigm (FileStorage + two directories) we are in some ways back to where we started, aka that unless you have a full set of configuration in your import directory when you import, everything missing will be interpreted as a delete. So for instance take the following example.

- Dev installs new Drupal site on localhost and at a hosting provider.
- Dev creates a view.
- Dev pushes view to import directory at hosting provider.
- Dev visits import page.
- Import page lists one new view and all other config as to be deleted.

One way I thought of to get around this is that when a module is installed, the default config is installed to both the 'staging' directory and the 'active' directory. This guarantees that 'staging' always starts with a complete set of config from which to start.

Thoughts?

gdd’s picture

This would also require that config being dynamically generated by modules (think about the config files for the default content types) end up in there somehow as well. That becomes trickier. Still, we need a way to deal with this issue.

moshe weitzman’s picture

FYI, drush config-edit allows for import of just one file. It does so by exporting the file for edit, opening editor, and then importing the one file (we had to use low level functions for this, instead of config_import()). See http://drupalcode.org/project/drush.git/blob/35c2f62dabb2bd764b834c574cd...

My point is that it is possible to import just one file, and we should allow for that in the web UI, IMO.

gdd’s picture

Right now the configuration looks at your staging directory and compares it to your active directory, then it says 'Here are the differences, do you want to import them?' One of those differences is that a missing file is interpreted as the config having been deleted (like a content type, for instance.) The end result of this is that if all your config is missing, then it is all interpreted as wanting to be deleted. That's why we need everything in staging to do proper comparisons.

Ways we could revisit this assumption. We could come up with a naming convention for deleted config (node.type.article.deleted.yml or something) although then that has to become a reserved word. We could also add a deleted flag inside files to indicate they are a delete action (with the same reservation as above.) Say, if the config file is changed such that it has only one key, and that key is 'delete: TRUE', then this is considered a delete action.

The nice thing about the way it is now is when doing listings of all the config of a type (say all the views, indicated by grabbing all the files that begin views.view.*.yml) you don't have to worry about filtering out files that exist but are actually deleted. Even worse, in that second option, you would have to actually open them all to figure it out. I guess we could actually prefix the files (aka deleted.node.type.article.yml) so they don't get listed by anything, but can be easily identified as deletes. There is no module with that namespace now so it currently wouldn't collide with anything. It certainly would make some parts of life easier (but probably some harder that aren't coming to atm but are sure to be brought up later.)

Totally up for talking this through more.

moshe weitzman’s picture

Naive question - couldn't the UI let you adjust its guesses about add/edit/delete? So you could uncheck all but one namespace if you just want it to import one file. Imports would go through the same code path that config-edit does, since they can be "partial"

tstoeckler’s picture

IMO both a) Importing all configuration b) Importing a single file are valid use-cases and should ideally be supported. When doing "Import all" we could handle the delete case, by checking which files are missing and (either automatically or optionally) deleting those.

I think the case for a) is a single developer that clicks a bunch of things together on dev and at a certain point simply wants copy everything over to staging.

For a multi-developer set-up where one developer is is working on a new view, and another is changing a content type you want to specifically choose only one. (Technically it would be possible to only git pull the actual files you want to change so that you could still always "Import all". I still think it makes to support the "Import only this" case.) Contrib could add a Features-like solution on top of that where you import a couple of related/dependent config files. (Assuming we don't solve that in core.)

In terms of a concrete UI, maybe a simple tableselect would be enough. In the submit we could check if all files are selected and then ask the user, whether he wants to delete missing ones.

gdd’s picture

I'm really reluctant to implement cherry-picking imports in core, because there are very common use cases where the dependencies are really nasty, and the implications of partially importing things can be pretty bad. I always assumed this would get implemented in contrib, but then it's something you have to seek out and find.

moshe weitzman’s picture

If that is the case, I would recommend that there is a prominent Export button which gets you a clean set of files to work on. You would then make your changes and then check them in or perform an import.

gdd’s picture

We have that now but we want to get rid of it to simplify the process (which also spawned my discussion above about alternate ways to identify deletes.)

gdd’s picture

Returned to this issue today. The more I think about it, the more I think that the 'missing file equals deletion of config' metaphor doesn't really work. This really only applies to config implemented as ConfigEntities, most settings-style config will *never* be deleted (excepting when a module is uninstalled.)

As a thought experiment I started working on a patch today that changes ConfigEntity::delete() so that when that config is deleted, it actually renames the config to deleted..yml, and refactors the import process to acknowledge these files as deleted. This actually cleans up the import process A LOT. Everything makes sense - you just copy config from your dev site's active directory to the live site's import directory and it all works without an export step. However it does have some downsides:

- The delete..yml files in the active directory on the dev site have to be cleaned up by hand. The system has nothing else it can do with them.
- It breaks version control history.

I don't mind those tradeoffs but I bet others will vehemently disagree.

In discussion with alexpott in IRC he came up with the idea of some config being inherently 'deleteable' or 'undeleteable', tracking it potentially in the config metadata. I'm not sure this helps, I have to think it through and I ran out of time to continue our discussion.

The more I think about this issue the more strongly I feel that missing=deleted is broken and we need something else, because if you always need to export a full set of config before you copy over files to import, then this process is going to be really convoluted and more complex than it has to be. We really need to find a way around that.

Anonymous’s picture

Hmmm, I don't have any answers yet, but I'll think on it.

Anonymous’s picture

thought about this some more, and discussed this with cafuego.

i don't think we should implement 'i just want to export this one change to production' without a validate step during import. something like what was in the pre-denver import patch, which allowed modules to block an import before any changes are made.

exporting the whole dev env tree, in a situation where changes to production only happen via import, is relatively safe, because you know you're pushing a working set (at least in as far as your dev setup works).

once we move to 'just push some subset of changes' (and that's really what this means), then you can no longer be reasonably sure that what you push contains all the changes needed to create a viable config on your target environment, even if you follow the 'only make changes via import' workflow.

so, regardless of how we do 'this file should be deleted', i think we need to address validation on import.

gdd’s picture

Here is a patch that does the delete-as-rename workflow. It is very rough and somewhat hacky in a couple places, I post it mainly for discussion purposes. It is not all that different from the previous patch, with the following changes:

- All references to 'export' are removed
- Behavior of ConfigStorageController::delete() modified to rename files instead of delete them
- Behavior of config_synch_get_changes() modified to acknowledge these files
- I removed the tests to make the patch easier to review
- This workflow introduces a situation where 'delete' means something different depending on what you're doing. If I go to the UI and delete an image style, I just want it renamed. However during synch, I actually want to delete the file for real. I hacked in a solution here but a better one would require a bit of rearchitecture.

In the end, if need be, we can commit the export-to-import patch before feature freeze and iterate on it from there I guess. I do want to keep talking about ideas and rolling things around though.

gdd’s picture

FileSize
9.44 KB

Ooops, this is going to fail wildly I assume.

gdd’s picture

Bojhan asked for a screenshot. Here is the sync screen with one changed image style, one new one, and one deleted one.

Status: Needs review » Needs work

The last submitted patch, ui.patch, failed testing.

yched’s picture

I'm currently touring Québec :-), and lengthy, articulate posts are a bit difficult for me - sorry, I know you're a bit alone on that topic :-/.

I just have a really bad gut feeling about moving towards these "delete.x" files.
IMO, a deleted item is not in the config anymore, period - as far as the config files are involved it should be as if the item never existed. It's not the role of the config files to keep tracks of past states of the config history. This sounds like it introduces some amount of hysteresis in the system.
Plus, we now have delete.xx files lying around, for which the corresponding items might have been actually deleted on some site instance but not on some other, and that we don't really know when we can clean them up.

It seems to me that this would badly mess up with the config repository for something that should be fixed one layer above : flow of config import/sync.
I'd say the current behavior is correct : if you sync your config with a set of files where item 'foo' doesn'st exist, then item 'foo' is deleted. That's what you asked for.
Maybe what we want is to separate 'sync' and 'import' operations :
- 'sync' does real synchronization, absent files equals deletion of the corresponding config item
- 'import', well, imports a set of config files (that are not expected to be a real, full config dir)- it does creation and update, but no deletes.

Side note: yeah, it so happens that some - rare - subsystems (Field API is the only one I know of) need to keep track of deleted items for some time, because actually "doing" the deletion takes a while. But it's the task of the subsystem to keep those items around (and that belongs in "state" : field pending deletion but not fully deleted yet on this given instance of the site), and decide when to actually purge. But as far as the config knows, the admin ask for deletion, the field is gone.

corvus_ch’s picture

+1 for yched's idea with determing between import and synchronization. Having tose delete.x files lying around smells a little bit fishy and I fear that this can become a real mess during the time and a lot of deleting has been done.

Anonymous’s picture

re. #38, that works for me, and i think we should just create a new directory for partial imports. my bikeshed colour aside, adding a directory that the config system knows it should only insert or update files from will be very, very easy to add.

however, my concerns in #33 still stand - when not dealing with a whole config tree, there is no reasonable way to know if the import will be valid, so we need a validation step. (or just big warnings about how you get to keep both pieces.)

gdd’s picture

I agree that having the delete files is a little hacky, I'm just trying to work through some different ideas.

I am not sure how I feel about import vs synchronization. We are already having enough trouble explaining how to do one of these tasks, much less two. Can we really expect to be able to explain to users when to use which directory? It makes sense for us from an architectural standpoint but it seems like it will be really confusing for users.

As I was thinking through the workflow as we currently have it, figuring out how to sync missing files is going to be difficult especially if you're outside the confines of version control. For instance:

1) User starts a site on a local dev server, builds some content types and views, etc.
2) User ftp's their entire local active directory into the remote site's import directory.
3) Import on live site goes as planned.
4) User decides to delete a content type.
5) User ftp's their entire local active directory into the remote site's import directory.
6) Nothing happens, because the deleted content type still exists on the remote site (it was removed from local active but still exists on remote import.)

I'm sure there are ways to avoid this (like we could clear out the import directory when an import is done) but this would have other consequences (like you always have to get the full tree into the import directory every time instead of just the first time.)

webchick’s picture

We seem to be stumped. Is it worth taking a break here to go out on a research mission and grab some ideas from other projects?

gdd’s picture

I'm willing to look again. I did do some research at the outset and didn't find anything that really approximated what we are trying to do here (especially in terms of ease of use for newbish users.)

gdd’s picture

I came up with an idea last night that I think is very promising, and was met with favorable response by beejeebus, timplunkett, and xjm. The idea is that config entities maintain a 'manifest' file which lists the objects that each config entity is currently maintaining. This file is a very simple flat listing of config object IDs, and it would always exist, even if it is empty. It would be automatically generated the first time a specific ConfigEntity is constructed, so module maintainers don't have to worry about it at all. Objects are added/removed from this file automatically as they are created/deleted. When config is ready to be deployed, this file will have to be deployed along with the actual objects themselves. Comparing this file to the existing manifest on the target site will generate the create/delete list.

Here's a sample manifest file for image styles as it would look after an initial installation:

- image.style.large
- image.style.medium
- image.style.thumbnail

This solves a lot of problems for us. Because the file always exists, we don't have to rely on the non-existence of a file to manage deletes (and thus we avoid all the issues we've been fighting above.) While it does slightly increase file clutter, the manifest file always has a use, even when its empty, so it's not useless clutter (like for instance, the delete.* files are after they've already been deployed and imported.) It is also possible that these manifest files could be used to store some additional data that is useful for listings. I had a talk with dawehner and damiankloip this morning, discussing whether or not these files would be useful to store enabled/disabled for views. It would allow them to generate listings without opening every file to check its enabled/disabled state (they were already discussing creating a similar separate file for this.)

Also note that this only implements a create/delete workflow for config implemented as config entities. This is as it should be, as static settings-style config has no use for this functionality. It exists when a module is installed, it doesn't when a module is uninstalled. Yes this could change midstream in a module update, but in that case it is up to the module to handle it anyways.

The only downside is that people doing deployments have to remember to deploy the manifest file in addition to the actual object files if creates/deletes are involved. I find this to be a pretty small downside though, compared to the various things we've been struggling with above. We don't have to export-to-import which is a UX disaster, and we don't have useless files cluttering up everything.

First pass patch attached. Still needs a lot of docs. Also doesn't currently handle removing manifest files when modules are uninstalled, which will have to happen.

I personally, am very convinced this is the way forward. It's not perfect but it's the closest we've got.

gdd’s picture

OK this patch is currently borked because the config entities aren't touched at installtime, so the initial manifest file never gets created. Either we would need to figure out a way to do that, or module devs would have to create an initial manifest file themselves. Either way, this concept still warrants discussion.

gdd’s picture

Here is an updated patch, it reworks config_install_default_config() to make installation work properly. The code isn't pretty but it works. The biggest change is that instead of calling config_get_sync_changes() to determine which files to install, it just builds that list by hand. All we're ever doing is adding files in that function, and I don't mind special-casing it there. I also removed the export UI tests since they aren't applicable anymore.

To test, I took this patch and applied it to two checkouts of the current Views work, checked out from http://drupal.org/sandbox/tim.plunkett/1799554 (use the VDC branch.) Then I installed both sites from scratch, calling one 'drupal_dev' and one 'drupal_live'. I then installed Views and Views UI on both sites (they aren't currently installed by default in the standard profile.) On drupal_dev I made the following changes:

- Enabled the tracker view (but made no edits)
- Enabled the comments view and edited it to show 10 items per page instead of 5
- Added a new view called 'Test' which was just all the defaults from the wizard

I did not test a delete, since Views' delete is not currently working. I then moved all the config files from the active directory on drupal_dev to the staging directory on drupal_live, and visited the config sync screen. I got the following result:

Screen Shot 2012-10-17 at 10.36.31 AM.png

You can see this shows several changes other than the Views.

- system.cron is because the cron key was different on each site. It's possible that this is more appropriate as state, I'll take this to a followup.
- system.site is because the default site name is different for each site. I decided this is a change I don't want to import.
- system.modules is the enabled module list, and we don't support importing these changes yet.

Given this I deleted those three files from the drupal_live staging directory and was left with only the views changes. I then clicked 'Import All' and let it do its thing. At the end, it still listed views.view.test as needing to be added, this is a glitch with the manifest file getting updated in the import process somewhere. However, most importantly, that view was created and the two changes did in fact import correctly!!! While there are a few caveats above, that's a pretty big deal IMO, and it demonstrates the workflow we've been striving for since the beginning working properly.

Would love some more reviews and discussion here.

webchick’s picture

Status: Needs work » Needs review

Exciting!! Marking needs review.

webchick’s picture

Priority: Normal » Major

Also, no. This is not normal. :P

webchick’s picture

Heh, trying that again. :)

gdd’s picture

Priority: Major » Critical

I would go even farther, I actually thought this was already critical.

Status: Needs review » Needs work

The last submitted patch, 1697256-config-import-47.patch, failed testing.

alexpott’s picture

I like the idea of manifests... I'm not sure that we need to a file per config entity or just one big manifest file. I've been testing this out with the breakpoints module as this is the config entity we have in core and it's apparent that #1813100: Allow different naming schema for yaml files is going to be important... that issue needs more eyes!

Patch attached passes some more tests...

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -65,6 +65,13 @@ public function __construct($entityType) {
+
+    // Create the manifest file if it doesn't exist.
+    // @todo Not sure this is the best place for this to live
+    $config = config('manifest.' . $this->entityInfo['config prefix']);
+    if ($config->isNew()) {
+      $config->save();

This isn't necessary... we can just create the file on first save.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -229,6 +236,14 @@ public function create(array $values) {
+    $config = config('manifest.' . $this->entityInfo['config prefix']);
+    $manifest = $config->get();
+    if (!isset($manifest[$this->entityInfo['config prefix'] . '.' . $entity->id()])) {
+      $manifest[] = $this->entityInfo['config prefix'] . '.' . $entity->id();
+    }
+    $config->setData($manifest)->save();

This needs to be done on save() and not create. Since the breakpoint config entity has not set it's id yet.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -256,6 +271,13 @@ public function delete($ids) {
+    $config = config('manifest.' . $this->entityInfo['config prefix']);
+    $manifest = $config->get();
+    unset($manifest[$this->entityInfo['config prefix'] . '.' . $entity->id()]);
+    $config->setAll($manifest)->save();

this can be changed to config::clear()

alexpott’s picture

Status: Needs work » Needs review

go testbot go!

alexpott’s picture

Seems like the patch causes a loop... Been testing for 50+ minutes :(

Status: Needs review » Needs work

The last submitted patch, 1697256-config-import-55.patch, failed testing.

alexpott’s picture

Oops I was wrong ... No loop :) and less exceptions and fails. So that's a win.

gdd’s picture

I was also thinking of the idea of one big manifest file yesterday. It would make the workflow easier because you could say 'Just push the config objects you want plus this one other file' as opposed to 'Just push the config objects you want and their associated manifest files' but I'm wondering if it messes up more targeted deployments. I am going to work on this patch some more today so I'll give it a try.

gdd’s picture

Title: [meta] UX of importing configuration objects » Create a UI for importing new configuration

Also changing title to be more relevant, since this isn't really a meta-issue anymore.

gdd’s picture

One thing the manifest files will make more difficult is doing very precisely targeted deployments. For instance, say you create two new views, but you only want to deploy one. You deploy the one view and the manifest file which lists both. What happens here? Is this a bug ('You didn't deploy this other view you wanted to add') or a feature ('Your missing file is intentional so I will ignore it.') Also this impacts the ability to use the manifest files for listings, which I'm not necessarily against, but its something I've talked about with the Views team. This problem just grows when you go to one big manifest file vs targeted ones. Until we discuss those implications some more I think I'm going to hold off on working on one big one.

gdd’s picture

Attached is a new patch and interdiff. Major points of interest:

- The manifests are now keyed, which makes them easier to remove entries from.
- Added config_uninstall_default_config() as a match to config_install_default_config(). Note that config uninstallation is a mess right now, which we'll deal with in followups. There are several that touch on this issue and I'd like to merge them.
- Added config_get_module_config_entities() to gather all the config entities provided by a single module. This function is ugly but is only needed temporarily until #1763974: Convert entity type info into plugins lands, which will provide this as a properly implemented common function.
- Fixed a bug in which multiple ids passed to ConfigEntity::delete() were not getting removed from the manifest, only the last one was.
- Removed config_export() and all references to it (undoubtedly breaking a lot of tests in the process.)
- Fixed a bug in the code that prevented duplicate entries in the manifest file.
- Cleanups here and there.

Outstanding issues:
- We currently can't uninstall the manifest files, because doing so involves firing hook_entity_info() to collect information about the config entities available from the module, but of course this is not available at uninstall time. This limitation will also go away when #1763974: Convert entity type info into plugins lands.
- If config is deleted in the import process, it will still show in the change list after the process is done running. This has to do with the staging version of the manifest not getting updated. Pondering whether or not it is smart to empty the staging directory after a import is run?
- Tests need to be fixed.
- Could probably use some helper functions around managing the manifest files.

I've been doing a lot of testing of this along with #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity) and it's been working really well. I'm really happy with how this is going.

gdd’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1697256-config-import-63.patch, failed testing.

gdd’s picture

Status: Needs work » Needs review
FileSize
21.61 KB

pfeh

Status: Needs review » Needs work

The last submitted patch, 1697256-config-import-63.patch, failed testing.

alexpott’s picture

Doing this review from iPad so no dreditor :(

But one thing jumps out around the saving to the manifest files. The in_array() won't work. And now we are using id's as keys we could do some thing like this...

$manifest_config = config(...);
If (!$manifest_config->get($entity->id())) {
  $manifest_config->set($entity->id(), $entity->uuid)->save();
}

Saving the uuid could allow for some interesting validation / conflict detection on import. The other ice thing about this is less duplication of the config entity prefix. It's only in the manifest file name.

Dries’s picture

Issue tags: +Favorite-of-Dries

This looks promising. The issue summary is lacking though. Would love to see some more screenshots.

yched’s picture

Not sure either where to look for the exact use cases we're trying to solve/fix - is the summary up to date ?

Coz I do fail to see the point of supporting "import config from an /import folder containing a partial config tree (e.g only a view.foo.yml file).
I'd tend to think the import folder should always contain a full tree of the exact set of config you want your site to end up in. Partial deployments would be managed manually, outside CMI (manual file copy / git merge, then sync on the resulting full tree).

This being said, I do dislike manifest files much less then deleted.foo files :-)

tim.plunkett’s picture

For the H3 @todo in there, I opened #1817996: Add sane default styles for table <caption>

gdd’s picture

Issue summary: View changes

Updated issue summary.

gdd’s picture

I updated the issue summary today. There really aren't any more screenshots. There's only one screen involved, and it's the one I showed in #46. However for anyone who wants to test this, here are some testing instructions.

  1. Download two copies of Drupal 8 and put them in separate subdirectories. Call one 'Dev' and the other 'Live'. Apply the patch in #63 to both of them. (For more information about applying patches see the patch documentation.) Do whatever you need to do on your local development environment to be able to run these downloads at separate URLS (outside the scope of these instructions.)
  2. Install both downloads.
  3. On dev, make some changes to settings. Not everything is currently converted to CMI but the following areas are
    • Performance settings
    • RSS settings
    • Cron settings
    • Site Information
    • Maintenance mode settings
    • Search settings
  4. Each site has two directories under sites/default/config. One is named 'active_' and one named 'staging_'. Copy the configuration from 'active' on the 'dev' site to 'staging' on the 'live' site. You can either copy all of the configuration, or just the files that you have changed.
  5. On the 'live' site, visit 'admin/config/development/config' and you will see the screen listing your configuration changes. Look at them and see that they represent the changes you made. If so, press 'Import All'. The import will run. Check through the live site to make sure the appropriate changes were in fact made.

This tests normal static settings. If you also want to test the manifest file functionality that is under discussion, you can do this by applying #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity) before doing your installations, and making changes to Image Styles in addition to the settings.

gdd’s picture

Issue summary: View changes

Updated issue summary.

gdd’s picture

Issue summary: View changes

Updated issue summary.

gdd’s picture

Issue summary: View changes

Updated issue summary.

webchick’s picture

Steps to test:

# SET UP DEVELOPMENT SITE

cd ~/Sites

# Download and extract 8.x tarball.
wget http://ftp.drupal.org/files/projects/drupal-8.x-dev.tar.gz
tar -zxvf drupal-8.x-dev.tar.gz

# Initialize a git repo.
cd drupal-8.x-dev
git init
git add .
git commit -am "Drupal 8 base files."

# Fix up .info file for patching.
vi core/modules/config/config.info # Delete everything after "core = 8.x"
git commit -am "Stupid packaging script."

# Apply latest CMI UI patch.
wget http://drupal.org/files/1697256-config-import-63_0.patch
git apply --index 1697256-config-import-63_0.patch
git commit -am "Latest CMI UI patch."

# Apply latest image styles CMI patch.
wget http://drupal.org/files/1782244-image-styles-config-entity-26.patch
git apply --index 1782244-image-styles-config-entity-26.patch
git commit -am "Latest image styles CMI patch."

# Install Drupal.
mysql -uroot -proot -e "DROP DATABASE IF EXISTS 8xdev; CREATE DATABASE 8xdev;"
drush si --account-pass=admin --db-url=mysql://root:root@localhost/8xdev -y

# Add a text file to the staging directory, since Git is dumb.
vi sites/default/files/config/staging_XXX/README.txt

# Add files directory and sites.php file.
git add git add sites/*
git commit -am "Adding config files."

# SET UP PRODUCTION WEBSITE.

# Clone database.
mysql -uroot -proot -e "DROP DATABASE IF EXISTS 8xprod; CREATE DATABASE 8xprod;"
mysqldump -uroot -proot 8xdev | mysql -uroot -proot 8xprod

# Clone files.
cd ..
git clone file:///Users/abyron/Sites/drupal-8.x-dev drupal-8.x-prod
cd drupal-8.x-prod
cp ../drupal-8.x-dev/sites/default/settings.php sites/default/settings.php
vi sites/default/settings.php # Change 8xdev to 8xprod, uncomment $conf['system.site']['name'] and set as 'Prod site'

# FANCY GIT CRAP TO MAKE GIT PULL WORK. (thanks timplunkett!)
cd ../drupal-8.x-dev
git remote add origin file:///Users/abyron/Sites/drupal-8.x-prod
git pull origin master

Now.

ON DEV
1) Went to Configuration > Site information > Slogan. Added a slogan of "Your mom." Clicked "Save configuration." Confirmed it showed up on the front page.
2) Went to Configuration > Development > Synchronize configuration. It currently says "No configuration changes," which is what I would expect:

No changes to import yet.

PROBLEM: That text there needs some love. Is it kosher to expose the exact configuration directory name in the UI, for example? Could we just say something instead like "Import configuration that is waiting to be staged."?

PROBLEM: For kicks, I click "Import all" anyway. I get a fatal error that disappears quickly, and then I am brought back to the same screen without the Overlay.

Fatal error is: Call to undefined function lock_wait() in modules/config/config.admin.inc on line 96

3) Go to the file system, enter git status. As expected I see:

#        modified:   sites/default/files/config/active_svho7NGP1EsSDPRxc6OIFIBIM3Zhd6jMz2oOWBmBJbY/system.site.yml

git diff shows:

diff --git a/sites/default/files/config/active_svho7NGP1EsSDPRxc6OIFIBIM3Zhd6jMz2oOWBmBJbY/system.site.yml b/sites/default/files/config/active_svho7NGP1EsSD
index 14a0ec1..21acb4c 100644
--- a/sites/default/files/config/active_svho7NGP1EsSDPRxc6OIFIBIM3Zhd6jMz2oOWBmBJbY/system.site.yml
+++ b/sites/default/files/config/active_svho7NGP1EsSDPRxc6OIFIBIM3Zhd6jMz2oOWBmBJbY/system.site.yml
@@ -1,6 +1,6 @@
 name: Site-Install
 mail: admin@example.com
-slogan: ''
+slogan: 'Your mom.'
 page:
   403: ''
   404: ''

So far, so good! :)

4) I commit that change:

git commit -am "Updated the slogan."

PRODUCTION SITE

1) I pull to get that change.

git pull --rebase

2) I reload http://localhost/drupal-8.x-prod/. No slogan.

3) I clear the cache:

drush cc all

4) I reload http://localhost/drupal-8.x-prod/ again. YAY! SLOGAN!

So far, this is all working exactly as I would envision it working. GREAT job. Light years beyond where this was at MWDS in July.

5) I go to admin/config/development/sync and… uh oh.

PROBLEM:

Drupal\Core\Config\StorageException: sites/default/files/config/staging_9Bea8FtYr2mBpzIQVa7Pt838yfQh4AfobDYIlbPDIik/ not found. in Drupal\Core\Config\FileStorage->listAll() (line 190 of/Users/abyron/Sites/drupal-8.x-prod/core/lib/Drupal/Core/Config/FileStorage.php).

"The website encountered an unexpected error. Please try again later."

Oopsie. ;)

I... have no idea why this is. It might be on my end. I had sites/*/files in my ~/.gitignore. I commented that out, but still both locations tell me:

# On branch master
nothing to commit (working directory clean)

Even though the dev site has both an active and staging dir, and the prod site has only active.

Starting over again. :( Git-- :P

Anonymous’s picture

so i don't think the test steps in #73 are right.

i guess there's still the idea out there that you can just track your active config, push it around between envs, and good things will happen.

the git pull on the prod site is not how this is intended to work, right? or did i miss something? the changes from dev need to get into the staging dir on prod, then be imported. some simple thing will work if you skip that step, but lots of other things wont. and your site will be horribly broken.

webchick’s picture

Ok, so at the time that you install Drupal, there is no sites/default/files/config/staging_[stuff] directory. That seems like a bug.

webchick’s picture

WAIT. No. The problem is Git. Because the staging directory is empty it doesn't show up as something to add. Grumble, grumble.

PROBLEM: Can we instantiate both active/staging directories with a README.txt file inside?

webchick’s picture

Ok, now that I manually added a staging_XXX/README.txt file, I'm back in business. Let's continue the test.

Now let's try something fancier. Config Entities! And because people don't ever listen when you tell them not to change config on production, let's do it anyway and see how CMI reacts.

PROD SITE

1) Go to Configuration >> Media >> Image styles, click "edit" on "Large."

2) Add a Desaturate effect.

3) Edit the Scale effect, and move height/width to 100px.

4) git status shows:

# On branch master
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	modified:   sites/default/files/config/active_QmcJFt2eHcdpS7bzsRKW-k5Uk6B__nFDayhEH8RCmSI/image.style.large.yml
#
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#	sites/default/files/styles/
no changes added to commit (use "git add" and/or "git commit -a")

That seems right.

git diff shows:

diff --git a/sites/default/files/config/active_QmcJFt2eHcdpS7bzsRKW-k5Uk6B__nFDayhEH8RCmSI/image.style.large.yml b/sites/default/files/config/active_QmcJFt2
index 1f9b20b..261e63d 100644
--- a/sites/default/files/config/active_QmcJFt2eHcdpS7bzsRKW-k5Uk6B__nFDayhEH8RCmSI/image.style.large.yml
+++ b/sites/default/files/config/active_QmcJFt2eHcdpS7bzsRKW-k5Uk6B__nFDayhEH8RCmSI/image.style.large.yml
@@ -2,11 +2,21 @@ name: large
 label: 'Large (480x480)'
 effects:
   ddd73aa7-4bd6-4c85-b600-bdf2b1628d1d:
+    ieid: ddd73aa7-4bd6-4c85-b600-bdf2b1628d1d
     name: image_scale
     data:
-      width: '480'
-      height: '480'
+      width: '100'
+      height: '100'
       upscale: '1'
     weight: '0'
-    ieid: ddd73aa7-4bd6-4c85-b600-bdf2b1628d1d
+  77b99a37-63b9-47e8-be01-1b2f98af4416:
+    label: Desaturate
+    help: 'Desaturate converts an image to grayscale.'
+    'effect callback': image_desaturate_effect
+    'dimensions passthrough': '1'
+    module: image
+    name: image_desaturate
+    data: {  }
+    weight: '2'
+    ieid: 77b99a37-63b9-47e8-be01-1b2f98af4416
 langcode: und

PROBLEM: It's a little weird that ieid moves around. Presumably this is because we removed sorting in that other patch.

PROBLEM: It's also weird that 77b99a37-63b9-47e8-be01-1b2f98af4416 is duplicated twice in the new image effect; once for ieid and once for the machine name of the thing. However, I'm pretty sure I remember this patch and the reason we did that is because it's possible to rename an image effect (is it though?)

Otherwise though, this looks as expected.

5) Commit it.

git commit -am "Image styles."

DEV SITE

DO NOT PULL YET. Let's get an article added with the old image style and see what happens.

1) node/add/article, add something with an image. Verify it looks normal on node/1.

2) git pull --rebase

fatal: No remote repository specified. Please, specify either a URL or a
remote name from which new revisions should be fetched.

Stupid Git. :\ AGAIN. Sigh.

webchick’s picture

Tim gave me the magickal dark incantations of:

cd ~/Sites/drupal-8.x-dev
git remote add origin file:///Users/abyron/Sites/drupal-8.x-prod
git pull origin master

Updated instructions, and we are cooking with gas again. :)

webchick’s picture

Now then. AHEM.

DEV SITE

DO NOT PULL YET. Let's get an article added with the old image style and see what happens.

1) node/add/article, add something with an image. Verify it looks normal on node/1.

2) git pull --rebase

Image style updates get pulled in:

Fast-forward
 .../image.style.large.yml                          |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

3) Reload http://localhost/drupal-8.x-dev/node/1. No change.

4) drush cc all

5) Shift+reload http://localhost/drupal-8.x-dev/node/1.

PROBLEM Interesting. The 100x100 made it over, but not the desaturate. Despite the fact it's in the YAML file. Tried this in a totally different browser just in case it was some kind of weird caching issue bit same deal. Back to admin/config/media/image-styles/edit/large, confirmed that the desaturate option is there. Preview looks right. Clicked "Update style." Now http://localhost/drupal-8.x-dev/node/1 shows greyscale. Missing a hook_config_something_something maybe?

PROBLEM: Also interesting. After clicking "Update style" and doing git diff, I see:

diff --git a/sites/default/files/config/active_QmcJFt2eHcdpS7bzsRKW-k5Uk6B__nFDayhEH8RCmSI/image.style.large.yml b/sites/default/files/config/active_QmcJFt2
index 261e63d..4117bc7 100644
--- a/sites/default/files/config/active_QmcJFt2eHcdpS7bzsRKW-k5Uk6B__nFDayhEH8RCmSI/image.style.large.yml
+++ b/sites/default/files/config/active_QmcJFt2eHcdpS7bzsRKW-k5Uk6B__nFDayhEH8RCmSI/image.style.large.yml
@@ -2,19 +2,14 @@ name: large
 label: 'Large (480x480)'
 effects:
   ddd73aa7-4bd6-4c85-b600-bdf2b1628d1d:
-    ieid: ddd73aa7-4bd6-4c85-b600-bdf2b1628d1d
     name: image_scale
     data:
       width: '100'
       height: '100'
       upscale: '1'
     weight: '0'
+    ieid: ddd73aa7-4bd6-4c85-b600-bdf2b1628d1d
   77b99a37-63b9-47e8-be01-1b2f98af4416:
-    label: Desaturate
-    help: 'Desaturate converts an image to grayscale.'
-    'effect callback': image_desaturate_effect
-    'dimensions passthrough': '1'
-    module: image
     name: image_desaturate
     data: {  }
     weight: '2'

Why would it be deleting those values?

webchick’s picture

Now, finally, everyone's favourite test case: Conflicts!

DEV SITE

1) Go to Configuration > System > Site information, change Slogan to "His mom."

2) Commit it.

git commit -am "His mom."

PROD SITE

1) Go to Configuration > System > Site information, change Slogan to "Her mom."

2) Commit it.

git commit -am "Her mom."

PROBLEM Side note. git diff here shows:

diff --git a/sites/default/files/config/active_QmcJFt2eHcdpS7bzsRKW-k5Uk6B__nFDayhEH8RCmSI/system.site.yml b/sites/default/files/config/active_QmcJFt2eHcdpS
index 21acb4c..150a6d8 100644
--- a/sites/default/files/config/active_QmcJFt2eHcdpS7bzsRKW-k5Uk6B__nFDayhEH8RCmSI/system.site.yml
+++ b/sites/default/files/config/active_QmcJFt2eHcdpS7bzsRKW-k5Uk6B__nFDayhEH8RCmSI/system.site.yml
@@ -1,6 +1,6 @@
-name: Site-Install
+name: 'Prod site'
 mail: admin@example.com
-slogan: 'Your mom.'
+slogan: 'Her mom.'
 page:
   403: ''
   404: ''

I've hard-coded $conf['system']['site.name'], and when saving the form this makes its way into the diff. This might actually cause conflict problems all on its own without me having to play with slogans. ;)

3) Drum rolllllll.....

$ git pull --rebase
remote: Counting objects: 15, done.
remote: Compressing objects: 100% (8/8), done.
remote: Total 8 (delta 4), reused 0 (delta 0)
Unpacking objects: 100% (8/8), done.
From file:///Users/abyron/Sites/drupal-8.x-dev
   dee7555..61c4233  master     -> origin/master
First, rewinding head to replay your work on top of it...
Applying: Her mom.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging sites/default/files/config/active_QmcJFt2eHcdpS7bzsRKW-k5Uk6B__nFDayhEH8RCmSI/system.site.yml
CONFLICT (content): Merge conflict in sites/default/files/config/active_QmcJFt2eHcdpS7bzsRKW-k5Uk6B__nFDayhEH8RCmSI/system.site.yml
Failed to merge in the changes.
Patch failed at 0001 Her mom.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To check out the original branch and stop rebasing run "git rebase --abort".

Bingo!

4) Reload http://localhost/drupal-8.x-prod/. Site slogan still shows "Her mom," as expected.

5) Clear cache for fun. ;) ;)

drush cc all

PROBLEM

Additional uncaught exception thrown while handling exception.

Original

Symfony\Component\Yaml\Exception\ParseException: Unable to parse at line 3 (near &quot;&lt;&lt;&lt;&lt;&lt;&lt;&lt; HEAD&quot;). in Symfony\Component\Yaml\Parser->parse() (line 238 of /Users/abyron/Sites/drupal-8.x-prod/core/vendor/symfony/yaml/Symfony/Component/Yaml/Parser.php).

Additional

Symfony\Component\Yaml\Exception\ParseException: Unable to parse at line 3 (near &quot;&lt;&lt;&lt;&lt;&lt;&lt;&lt; HEAD&quot;). in Symfony\Component\Yaml\Parser->parse() (line 238 of /Users/abyron/Sites/drupal-8.x-prod/core/vendor/symfony/yaml/Symfony/Component/Yaml/Parser.php).

;)

5) Deal with the conflict:

vi sites/default/files/config/active_QmcJFt2eHcdpS7bzsRKW-k5Uk6B__nFDayhEH8RCmSI/system.site.yml
### Change:
<<<<<<< HEAD
slogan: 'His mom.'
=======
slogan: 'Her mom.'
>>>>>>> Her mom.
### to:
slogan: 'Her mom.'

# Mark conflict resolved.
git add sites/default/files/config/active_QmcJFt2eHcdpS7bzsRKW-k5Uk6B__nFDayhEH8RCmSI/system.site.yml
git commit -am "Stupid wankers who change stuff on production. GEEZ."

6) Reload http://localhost/drupal-8.x-prod/ again. IT WORKS! Except for these errors, which are gone on the next re-load:

Notice: Undefined index: backtrace in _drupal_log_error() (line 188 of core/includes/errors.inc).
Symfony\Component\Yaml\Exception\ParseException: Unable to parse at line 3 (near "<<<<<<< HEAD"). in Symfony\Component\Yaml\Parser->parse() (line 238 of core/vendor/symfony/yaml/Symfony/Component/Yaml/Parser.php).
webchick’s picture

So, in summary of the last ~3 hours of testing the "I don't read docs" way:

- Add a README.txt to both the active and staging directories, so Git will pick them up. Git can't deal with empty directories. Yes, it's a dumb workaround, but we do this in other places, e.g. modules/ and themes/

- Fix the wording on the import screen (which, btw, I never had to use, so I'm sure I tested this all wrong). Suggestion is to change "Import configuration that is placed in [directory]" to just "Import configuration that is waiting to be staged."

- Fix the fatal error that happens when you click "Import all" without any files waiting: Fatal error: Call to undefined function lock_wait() in modules/config/config.admin.inc on line 96

- Though it shouldn't happen anymore once README.txt exists, make the import UI a little more forgiving if the staging directory doesn't exist. Instead of blowing up with:

Drupal\Core\Config\StorageException: sites/default/files/config/staging_9Bea8FtYr2mBpzIQVa7Pt838yfQh4AfobDYIlbPDIik/ not found. in Drupal\Core\Config\FileStorage->listAll() (line 190 of/Users/abyron/Sites/drupal-8.x-prod/core/lib/Drupal/Core/Config/FileStorage.php).

It should probably say "Could not find [dir-name] directory. Please ensure it exists in [config-path]."

Other issues found, not part of this patch:

- The system should ideally be more forgiving when it encounters invalid YAML. Right now it blows up with:

Additional uncaught exception thrown while handling exception.

Original

Symfony\Component\Yaml\Exception\ParseException: Unable to parse at line 3 (near &quot;&lt;&lt;&lt;&lt;&lt;&lt;&lt; HEAD&quot;). in Symfony\Component\Yaml\Parser->parse() (line 238 of /Users/abyron/Sites/drupal-8.x-prod/core/vendor/symfony/yaml/Symfony/Component/Yaml/Parser.php).

Additional

Symfony\Component\Yaml\Exception\ParseException: Unable to parse at line 3 (near &quot;&lt;&lt;&lt;&lt;&lt;&lt;&lt; HEAD&quot;). in Symfony\Component\Yaml\Parser->parse() (line 238 of /Users/abyron/Sites/drupal-8.x-prod/core/vendor/symfony/yaml/Symfony/Component/Yaml/Parser.php).

- There's something wonky with the image styles stuff. It didn't pick up a new image effect until I clicked "Update styles" manually. It also saved the effect label and some other info to YAML, then removed it again when I clicked "Update styles."

- It's a little weird that ieid keeps moving around. Presumably this is because we removed sorting in that other patch.

- It's also weird that 77b99a37-63b9-47e8-be01-1b2f98af4416 is duplicated twice in the new image effect; once for ieid and once for the machine name of the thing. However, I'm pretty sure I remember this patch and the reason we did that is because it's possible to rename an image effect (is it though?).

Now, to read how I was actually supposed to test it. ;)

webchick’s picture

Right, so as outlined in IRC the way I tested it is an unsupported way that's not intended to work, and breaks when new things are added, esp. things with dependencies like e.g. Fields. Thought so. ;) In which case we definitely need INSTALL.txt updated to reflect the set up instructions in the issue summary here. It's one thing to deliberately not read the docs so you can go into a patch fresh, but quite another thing when there are no docs to know how to perform the set up properly. :)

webchick’s picture

Issue summary: View changes

Updated issue summary.

gdd’s picture

Issue summary: View changes

Updated issue summary.

Anonymous’s picture

i created #1821306: create a way to back out of unintentional / unsupported active config changes to work on the 'just copy my config' case and how to handle it.

webchick’s picture

Ok, new testing instructions. (And yes, heyrocker, I know you said not to use Git in my testing, but I really want to be able to track what changes are happening when and where and am not that observant without version control. ;))

cd ~/Sites

# Download and extract 8.x tarball.
wget http://ftp.drupal.org/files/projects/drupal-8.x-dev.tar.gz

# SET UP DEVELOPMENT WEBSITE.
tar -zxvf drupal-8.x-dev.tar.gz
mv drupal-8.x-dev 8.x-dev

# Initialize a git repo.
cd 8.x-dev
git init
git add .
git commit -am "Drupal 8 base files."

# Fix up .info file for patching.
vi core/modules/config/config.info # Delete everything after "core = 8.x"
git commit -am "Stupid packaging script."

# Apply latest CMI UI patch.
wget http://drupal.org/files/1697256-config-import-63_0.patch
git apply --index 1697256-config-import-63_0.patch
git commit -am "Latest CMI UI patch."

# SET UP PRODUCTION WEBSITE.
cd ..
git clone file:///Users/abyron/Sites/8.x-dev 8.x-prod

# INSTALL DEV SITE
cd 8.x-dev
mysql -uroot -proot -e "DROP DATABASE IF EXISTS 8xdev; CREATE DATABASE 8xdev;"
drush si --account-pass=admin --db-url=mysql://root:root@localhost/8xdev -y

# Add a text file to the staging directory, since Git is dumb.
touch sites/default/files/config/staging_XXX/README.txt

# Add files directory and sites.php file.
git add sites/*
git commit -am "Adding dev config files."

# INSTALL PROD SITE
cd ../8.x-prod
git pull --rebase
mysql -uroot -proot -e "DROP DATABASE IF EXISTS 8xprod; CREATE DATABASE 8xprod;"
drush si --account-pass=admin --db-url=mysql://root:root@localhost/8xprod -y


# Add a text file to the staging directory, since Git is dumb.
touch sites/default/files/config/staging_XXX/README.txt

# Add production version of files directory.
git add sites/*
git commit -am "Adding prod config files."

# FANCY GIT CRAP TO MAKE GIT PULL WORK. (thanks timplunkett!)
cd ../8.x-dev
git remote add origin file:///Users/abyron/Sites/8.x-prod
git pull origin master

Now I have two sites, http://localhost/8.x-dev/ and http://localhost/8.x-prod/. To keep them straight, I edit both sites' settings.php files, and change:

# $conf['system.site']['name'] = 'My Drupal site';

to:

$conf['system.site']['name'] = 'Dev';

and:

$conf['system.site']['name'] = 'Prod';

… respectively.

New test coming up, first have to go take the dog outside. ;) Be back soon.

webchick’s picture

Oh, also the net result of this site creation workflow is you end up with 4 different directories under files/config:

- active_0NQHZw0lAhGfVX2nCmkCkMoDSY_vyYGo23qE5MPOv0Y # dev
- active_Mq8C5-bqo_BHQ0v7a7f1L4ykIWKeaqSuBzAS5NgnPkQ # prod
- staging_B9rgM3e_eNd_Ih4548nIGmeaZPZFTR5qPe7gLXtcR94 # dev
- staging_IF_DCiQ7fmVdPYsow-SGUxlFcAuil-zPKzjo2_-nILI # prod

This is a little hard to track. One thing heyrocker suggested on the phone today was an issue to change the directory structure to:

# dev
- files/config/0NQHZw0lAhGfVX2nCmkCkMoDSY_vyYGo23qE5MPOv0Y/active
- files/config/0NQHZw0lAhGfVX2nCmkCkMoDSY_vyYGo23qE5MPOv0Y/staging
# prod
- files/config/Mq8C5-bqo_BHQ0v7a7f1L4ykIWKeaqSuBzAS5NgnPkQ/active
- files/config/Mq8C5-bqo_BHQ0v7a7f1L4ykIWKeaqSuBzAS5NgnPkQ/staging

Then it's only two mangled-up strings to keep track of and mentally map to their parent environments, rather than 4. +1 from me. Separate issue, of course.

EDIT: And so there is! #1821172: Hash the config directory name, unhash the active and staging directory names

webchick’s picture

FileSize
49.92 KB
33.71 KB

The basics: A simple settings change.

DEV SITE

1. Go to admin/config/system/site-information
2. Enter Slogan: "A spiffy site slogan!"
3. Save.

$ git diff
diff --git a/sites/default/files/config/active_Mq8C5-bqo_BHQ0v7a7f1L4ykIWKeaqSuBzAS5NgnPkQ/system.site.yml b/sites/defau
index 14a0ec1..caf6f46 100644
--- a/sites/default/files/config/active_Mq8C5-bqo_BHQ0v7a7f1L4ykIWKeaqSuBzAS5NgnPkQ/system.site.yml
+++ b/sites/default/files/config/active_Mq8C5-bqo_BHQ0v7a7f1L4ykIWKeaqSuBzAS5NgnPkQ/system.site.yml
@@ -1,6 +1,6 @@
-name: Site-Install
+name: Dev
 mail: admin@example.com
-slogan: ''
+slogan: 'A spiffy site slogan!'
 page:
   403: ''
   404: ''

PROBLEM: Hrm. Is there a way to strip out $conf overrides from making changes to the files? Else i don't see how it will ever be possible to stage things like this with the system. :(

EDIT: Being dealt with at #1763640: Introduce config context to make original config and different overrides accessible

git reset --hard
drush cc all

Slogan is reverted.

Ahem. NEW The basics: A simple settings change.

1. Go to admin/config/development/maintenance
2. Change the maintenance message to "Yarrr! @site be currently under maintenance. We be back shortly. Thank ye fer ye patience." in honour of Talk Like A Pirate Day.
3. Save.
4. Check diff

$ git diff
diff --git a/sites/default/files/config/active_Mq8C5-bqo_BHQ0v7a7f1L4ykIWKeaqSuBzAS5NgnPkQ/system.maintenance.yml b/site
index f559d68..52052b0 100644
--- a/sites/default/files/config/active_Mq8C5-bqo_BHQ0v7a7f1L4ykIWKeaqSuBzAS5NgnPkQ/system.maintenance.yml
+++ b/sites/default/files/config/active_Mq8C5-bqo_BHQ0v7a7f1L4ykIWKeaqSuBzAS5NgnPkQ/system.maintenance.yml
@@ -1,2 +1,2 @@
 enabled: '0'
-message: '@site is currently under maintenance. We should be back shortly. Thank you for your patience.'
+message: 'Yarrr! @site be currently under maintenance. We be back shortly. Thank ye fer ye patience.'

Woohoo!

5. Commit.

git commit -am "I be changin the maintenance message."

PROD SITE

1. Pull in changes from dev

cd ../8.x-prod
git pull --rebase

2. Stage configuration.

cp sites/default/files/config/active_Mq8C5-bqo_BHQ0v7a7f1L4ykIWKeaqSuBzAS5NgnPkQ/* sites/default/files/config/staging_IF_DCiQ7fmVdPYsow-SGUxlFcAuil-zPKzjo2_-nILI

3. Go to admin/config/development/maintenance and verify it's still in proper English.
4. drush cc all, same thing.
5. Go to prod/admin/config/development/sync
6. Here's what I see.

2 files changed, system.cron and system.maintenance

PROBLEM Er. How and why did system.cron change? That's really odd.

PROBLEM And of course now, what I naturally want to do is see a "diff" of those changes before clicking "Import all" since I am not sure what they are. Heck, even if I thought I did know what they were, I would still want to be able to view the diff to verify that was indeed true. :\

7. Throwing all caution to the wind, I click "Import all." Looks good.

No configuration left to import.

8. Go to admin/config/development/maintenance. See pirate speak. Yay!

9. Now let's see what git diff says.

$ git diff
diff --git a/sites/default/files/config/active_0NQHZw0lAhGfVX2nCmkCkMoDSY_vyYGo23qE5MPOv0Y/system.cron.yml b/sites/defau
index 98a0eed..bc0f075 100644
--- a/sites/default/files/config/active_0NQHZw0lAhGfVX2nCmkCkMoDSY_vyYGo23qE5MPOv0Y/system.cron.yml
+++ b/sites/default/files/config/active_0NQHZw0lAhGfVX2nCmkCkMoDSY_vyYGo23qE5MPOv0Y/system.cron.yml
@@ -1,4 +1,4 @@
-key: KtVN51EvJZ8B9g0exckBDheXx590S2YndavfxyFwLUc
+key: JUWTUeiYFKy5idV6X-5Ml2pK2lFJEHLPkch3t2kZU6w
 threshold:
   autorun: '10800'
   requirements_warning: '172800'
diff --git a/sites/default/files/config/active_0NQHZw0lAhGfVX2nCmkCkMoDSY_vyYGo23qE5MPOv0Y/system.maintenance.yml b/site
index f559d68..52052b0 100644
--- a/sites/default/files/config/active_0NQHZw0lAhGfVX2nCmkCkMoDSY_vyYGo23qE5MPOv0Y/system.maintenance.yml
+++ b/sites/default/files/config/active_0NQHZw0lAhGfVX2nCmkCkMoDSY_vyYGo23qE5MPOv0Y/system.maintenance.yml
@@ -1,2 +1,2 @@
 enabled: '0'
-message: '@site is currently under maintenance. We should be back shortly. Thank you for your patience.'
+message: 'Yarrr! @site be currently under maintenance. We be back shortly. Thank ye fer ye patience.'

PROBLEM Oh, crap. :( Staging my configuration ended up over-writing my production cron key with the one from my dev site. :( :( That seems like a pretty huge problem...

10. Manually undo that, commit the change so the two sites are in sync again.

git checkout sites/default/files/config/active_0NQHZw0lAhGfVX2nCmkCkMoDSY_vyYGo23qE5MPOv0Y/system.cron.yml
git commit -am "Pirate speak maintenance. Yarrr."

PROBLEM For a version control-based workflow, I'm now doing twice the committing than I would do in a Features-based workflow. That's kind of annoying, especially given that I'd ideally like to sync up commit messages and the like.

gdd’s picture

PROBLEM: Hrm. Is there a way to strip out $conf overrides from making changes to the files? Else i don't see how it will ever be possible to stage things like this with the system. :(

This will be solved when the new context system goes in.

PROBLEM Er. How and why did system.cron change? That's really odd.

As noted in my initial test, there needs to be an issue to move the cron key to the state system (although I have not made that issue yet.) I can see this going either way, because in many cases you will indeed want the same cron key on all your sites. On the other hand, you could manage that in $conf if you want, so I think it does belong in state.

webchick’s picture

FileSize
126.16 KB

Fancier: Deleting a yaml file.

PROBLEM I actually skipped this one cos I have no idea how to stage a deletion. Copying all files from dev_active -> prod_staging obviously won't work, because the old YML file will still be sitting there so it'll appear as though there's no change. Seems like we need to clear out the staging dir after import.

Even More Fancier: Image styles

Once again, making this change from prod this time, pushing to dev, just for kicks.

PROD SITE
1. Go to node/add/article and add an article w/ a large picture.
2. Save.
3. Verify that at node/1 it shows the picture at more or less full size.
4. Go to admin/config/media/image-styles/edit/large
5. Add a "desaturate" action.
6. Edit the scale action and change to 200x200.
7. Go back to node/1, verify picture is now small and grey.
8. Diff it.

$ git diff
diff --git a/sites/default/files/config/active_0NQHZw0lAhGfVX2nCmkCkMoDSY_vyYGo23qE5MPOv0Y/image.style.large.yml b/sites
index 1f9b20b..8c83ac7 100644
--- a/sites/default/files/config/active_0NQHZw0lAhGfVX2nCmkCkMoDSY_vyYGo23qE5MPOv0Y/image.style.large.yml
+++ b/sites/default/files/config/active_0NQHZw0lAhGfVX2nCmkCkMoDSY_vyYGo23qE5MPOv0Y/image.style.large.yml
@@ -2,11 +2,21 @@ name: large
 label: 'Large (480x480)'
 effects:
   ddd73aa7-4bd6-4c85-b600-bdf2b1628d1d:
+    ieid: ddd73aa7-4bd6-4c85-b600-bdf2b1628d1d
     name: image_scale
     data:
-      width: '480'
-      height: '480'
+      width: '200'
+      height: '200'
       upscale: '1'
     weight: '0'
-    ieid: ddd73aa7-4bd6-4c85-b600-bdf2b1628d1d
+  a7efdeec-9c8a-49ab-9f52-82b145a69733:
+    label: Desaturate
+    help: 'Desaturate converts an image to grayscale.'
+    'effect callback': image_desaturate_effect
+    'dimensions passthrough': '1'
+    module: image
+    name: image_desaturate
+    data: {  }
+    weight: '2'
+    ieid: a7efdeec-9c8a-49ab-9f52-82b145a69733
 langcode: und

PROBLEM Same deal as above about the ieid moving around. Separate issue.
PROBLEM Same deal as above about label / help / etc. getting saved to the YML file. Separate issue.

9. Commit it.

git commit -am "Image stylez"

DEV SITE

1. Go to node/add/article and add an article w/ a large picture.
2. Save.
3. Verify that at node/1 it shows the picture at more or less full size.
4. Pull down changes from production.

$ git pull origin master
remote: Counting objects: 22, done.
remote: Compressing objects: 100% (15/15), done.
remote: Total 15 (delta 8), reused 0 (delta 0)
Unpacking objects: 100% (15/15), done.
From file:///Users/abyron/Sites/8.x-prod
 * branch            master     -> FETCH_HEAD
Updating 7c6769c..3bb23f9
Fast-forward
 .../image.style.large.yml                          |   16 +++++++++++++---
 .../system.maintenance.yml                         |    2 +-
 2 files changed, 14 insertions(+), 4 deletions(-)

5. Reload node/1, verify that image is still big and colourful.
6. drush cc all, same thing.
7. Stage the changes.

cp sites/default/files/config/active_0NQHZw0lAhGfVX2nCmkCkMoDSY_vyYGo23qE5MPOv0Y/* sites/default/files/config/staging_B9rgM3e_eNd_Ih4548nIGmeaZPZFTR5qPe7gLXtcR94/

8. Go to admin/config/development/sync. Two changed rows: image.style.large and system.cron.

PROBLEM Same deal as above w/ system.cron.

9. Click "Import all"
10. Go back to node/1, should now be small and grey.
11. Take care of system.cron.yml, and commit the change.

git checkout sites/default/files/config/active_Mq8C5-bqo_BHQ0v7a7f1L4ykIWKeaqSuBzAS5NgnPkQ/system.cron.yml 
git commit -am "Image stylez."

Overall, looks good. Let's remove the desaturate now.

DEV SITE

1. Go to admin/config/media/image-styles/edit/large
2. Hit "delete" on desaturate and confirm.

PROBLEM It says it deleted it, but didn't actually delete it... :\

Message says desaturate is deleted, but it's actually still there.

Diff confirms. :\

I think I remember hearing somewhere this was a pre-existing bug, maybe add to the testing instructions?

gdd’s picture

Seems like we need to clear out the staging dir after import.

Yup I will be putting this in the next patch.

PROBLEM Same deal as above about label / help / etc. getting saved to the YML file. Separate issue.

Yeah this needs a separate issue for image styles. (#1821516: Information from hook_image_effect_info() should not be saved into image style config files)

I think I remember hearing somewhere this was a pre-existing bug, maybe add to the testing instructions?

Crap I've never seen that before :( We should probably verify that independently of this testing to see whether its a bug or not.

Here's the issue about moving cron_key to state: #1821530: Move cron key from configuration to state system

gdd’s picture

I will also update the instructions to include testing instructions for deletes. In addition I had a long talk with webchick about increasing the utility of the config import screen by adding the ability to see a diff of the files before an import is run. For instance with the system.cron situation, where something changed that you didn't expect, it would have been very useful to know exactly what changed before running the import. While I agreed this would be very helpful information, I am really concerned about time constraints if it has to go in before feature freeze. I always assumed this would be something contrib would take care of. We will need to spend a lot of time on the UX of such a feature, since there will often be many config objects changed, each of them with many individual changes. If it can wait until after feature freeze then I think we can regroup with bojhan in December and see where we're at and what it would take to get this in (design an interface that works well, evaluate diff libraries, implement.) Webchick is going to create a major followup on this.

webchick’s picture

Filed that at #1821548: Add a "diff" of some kind to the CMI UI. I think it's more important to get the UI in at all than make it perfect, so that can be a follow-up task (but it's definitely a major one).
Also filed #1821534: Can't delete image effects from an image style for the image effects bug, which is not the fault of this patch.

My takeaway from all of that is I don't think the hard commit blockers have changed much since #81; namely, let's fix the fatal errors and get some baseline docs in here. If you can easily fix clearing the staging dir too, then that's great.

Bojhan’s picture

I am wondering if we can do some kind of empty table pattern here, that's always much more beautiful.

@webchick Thanks for the thorough review, that helps me a lot understand the flow. It's clear we need diff, so lets do that post freeze. It shouldn't be too hard from a UX standpoint, the only real thing I am concerned about is the IA label. But that could be a seperate issue.

Bojhan’s picture

Issue summary: View changes

Updated issue summary.

gdd’s picture

Issue summary: View changes

Updated issue summary.

gdd’s picture

This is a full start to finish setup for someone wanting to test in a more traditional commit/pull/update workflow. The base concept at work here is that in settings.php you set your active directory on the dev site to be your staging directory on the live site (and vice versa if you also want to push code the other way.) For a larger explanation see the chart and discussion at #1703168-38: [Meta] Ensure that configuration system functionality matches expected workflows for users and devs.

# SETUP DEV SITE
mkdir drupal_dev
cd drupal_dev
wget http://ftp.drupal.org/files/projects/drupal-8.x-dev.tar.gz
tar --strip-components=1 -zxvf drupal-8.x-dev.tar.gz
rm *.gz
git init
git add .
git commit -am "Drupal"

# FIX CONFIG.INFO SO THE PATCH WILL APPLY
vi core/modules/config/config.info 
# Delete everything after "core = 8.x"
git commit -am "Stupid packaging script."

# APPLY THE PATCH
wget http://drupal.org/files/1697256-config-import-63_0.patch
git apply --index 1697256-config-import-63_0.patch
git commit -am "Latest CMI UI patch."

# SETUP SETTINGS.PHP
cd drupal_dev/sites/default
cp default.settings.php settings.php
mkdir files
chmod -R 777 *

# EDIT SETTINGS.PHP AND REPLACE LINE 273 WITH THE FOLLOWING
$config_directories = array(
  CONFIG_ACTIVE_DIRECTORY => array('path' => 'config_xyzzy/dev_to_live'),
  CONFIG_STAGING_DIRECTORY => array('path' => 'config_xyzzy/live_to_dev'),
);

# INSTALL DRUPAL AS YOU SEE FIT

# ADD CONFIG TO GIT
cd config_xyzzy/live_to_dev/
touch README.txt
cd ../../..
git add files
git commit -m "Adding files directory" files
cd ../../..

# SETUP LIVE SITE
git clone file:///Users/gdd/Sites/drupal_dev drupal_live

# SETUP SETTINGS.PHP
cd drupal_live/sites/default
cp default.settings.php settings.php
mkdir files
chmod -R 777 *

# EDIT SETTINGS.PHP AND REPLACE LINE 273 WITH THE FOLLOWING
# NOTE THAT THE ACTIVE AND STAGING DIRECTORIES ARE REVERSED FROM ABOVE
$config_directories = array(
  CONFIG_ACTIVE_DIRECTORY => array('path' => 'config_xyzzy/live_to_dev'),
  CONFIG_STAGING_DIRECTORY => array('path' => 'config_xyzzy/dev_to_live'),
);

# INSTALL DRUPAL AS YOU SEE FIT

# ADD CONFIG TO GIT
git add files
git commit -am "Adding live site config"

At this point if you go to /admin/config/development/sync on 'drupal_live' you will see three changes listed - system.cron, system.module and system.site. system.cron is the cron_key, and there is an issue to move this out of config, so ignore it for the time being. system.module is the module list and not implemented at this point. system.site is the site name, and we're going to update that to be the same as the dev site anyways.

Go to drupal_dev and make some changes. You should change other stuff around as you wish, but in my test I did the following:

- Added desaturate to large style
- Add a new style called 'new_style' with a rotate
- Deleted medium style
- Turn on caching

If I go into git in drupal_dev right now, and do git status I see the following:

 M sites/default/files/config_xyzzy/dev_to_live/image.style.large.yml
 D sites/default/files/config_xyzzy/dev_to_live/image.style.medium.yml
 M sites/default/files/config_xyzzy/dev_to_live/manifest.image.style.yml
 M sites/default/files/config_xyzzy/dev_to_live/system.performance.yml
?? sites/default/files/config_xyzzy/dev_to_live/image.style.new_style.yml

So lets get those changes committed

git add sites/default/files/config_xyzzy/
git commit -am "New configuration for deployment"

Now move into drupal_live and pull

cd ../drupal_live

git pull
remote: Counting objects: 20, done.
remote: Compressing objects: 100% (11/11), done.
remote: Total 11 (delta 4), reused 0 (delta 0)
Unpacking objects: 100% (11/11), done.
From file:///Users/gdd/Sites/drupal_dev
   c0fe8b7..07291c4  master     -> origin/master
Auto-merging sites/default/files/config_xyzzy/dev_to_live/image.style.large.yml
Removing sites/default/files/config_xyzzy/dev_to_live/image.style.medium.yml
Auto-merging sites/default/files/config_xyzzy/dev_to_live/manifest.image.style.yml
Auto-merging sites/default/files/config_xyzzy/dev_to_live/system.performance.yml
Merge made by recursive.
 .../config_xyzzy/dev_to_live/image.style.large.yml |    5 +++++
 .../dev_to_live/image.style.medium.yml             |   12 ------------
 .../dev_to_live/image.style.new_style.yml          |   12 ++++++++++++
 .../dev_to_live/manifest.image.style.yml           |    1 -
 .../dev_to_live/system.performance.yml             |    2 +-
 5 files changed, 18 insertions(+), 14 deletions(-)
 delete mode 100755 sites/default/files/config_xyzzy/dev_to_live/image.style.medium.yml
 create mode 100644 sites/default/files/config_xyzzy/dev_to_live/image.style.new_style.yml

Vist admin/config/development/sync on drupal_live. In addition to the changes discussed above, you should see the changes you made listed as well. Click 'Import All' and your new config should be live!

gdd’s picture

Issue summary: View changes

Updated issue summary.

gdd’s picture

I had a meeting with webchick earlier this week to discuss some of her concerns around this patch. We both agreed that documentation around proper config processes needs improvement, but we also both agreed that it shouldn't be a commit blocker. I made the following changes in response to her feedback.

- Added a README.txt to each config directory so that if empty, they can be added to git. I purposely did not do a lot of error-handling code around the file_put_contents() there. By the time we get to that code, we already know the config directory exists and that it is writeable. Not much else to check once you know that.
- Removed the full staging directory file path from the admin screen. This is both a security precaution and makes the screen less ugly.
- Created a new handbook page for deployment processes and linked to it in the README files. It's just a shell now but we can grow it.

Sorry no interdiff, I merged to current head to make sure there were no conflicts since there have been several major commits in the last few days.

There are also several followup issues now that deal with some of the things found in this review

#1821172: Hash the config directory name, unhash the active and staging directory names
#1821530: Move cron key from configuration to state system
#1821306: create a way to back out of unintentional / unsupported active config changes
#1821548: Add a "diff" of some kind to the CMI UI
#1763640: Introduce config context to make original config and different overrides accessible

gdd’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1697256-config-import-94.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
27.72 KB
11.78 KB

Patch attached should fix configimport and configimportUI tests to use new manifest system. Additionally this patch fixes the lock()->wait configUI test - however this means that a test run will take 30 seconds longer. I'm not a huge fan of the code:

function config_admin_import_form_submit($form, &$form_state) {
  if (config_import()) {
    drupal_set_message(t('The configuration was imported successfully.'));
  }
  else {
    // Another request may be synchronizing configuration already. Wait for it
    // to complete before returning the error, so already synchronized changes
    // do not appear again.
    lock()->wait(CONFIG_IMPORT_LOCK);
    drupal_set_message(t('The import failed due to an error. Any errors have been logged.'), 'error');
  }
}

Since this means a user could wait for 30 secs for an error message... we should be failing fast. And the error message could be more helpful.. perhaps The import failed due to an error or a concurrent import. Any errors have been logged.

The manifest change means that we can no longer just create a new config object by just sticking it in the staging directory and running import. This is by design and (obviously) has resulted in big changes to the tests.

Additionally whilst thinking about this patch I'm wondering how a module will create new config during a hook_update_N(). At the moment this would be non-trivial. The only way to create new simple (i.e non config entity objects) is through config_install_default_config().

gdd’s picture

Issue tags: +feature freeze

Marking as susceptible to feature freeze

moshe weitzman’s picture

That tag is a duplicate of the Category selector (specifically the Feature option).

damiankloip’s picture

Status: Needs review » Needs work

I haven't actually tested this patch yet, but generally the code etc... looks great.

+++ b/core/includes/config.incundefined
@@ -213,17 +267,33 @@ function config_import_invoke_owner(array $config_changes, StorageInterface $sou
+  // @todo The proper way to get this info would be to call get_entity_info()

This todo and task has probably changed a bit now after #1763974: Convert entity type info into plugins. Also, see below and other calls to the entity info array, E.g. 'config prefix' will need to change to 'config_prefix' now.

+++ b/core/includes/config.incundefined
@@ -213,17 +267,33 @@ function config_import_invoke_owner(array $config_changes, StorageInterface $sou
+      // @todo is this a sane and/or trustworthy way to determine that this
+      // is in fact a config entity?

Yes, unfortunately there is not really another way to determine this... So I guess this can be removed.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -304,6 +309,14 @@ public function save(EntityInterface $entity) {
+      $manifest[$entity->id()] = $this->entityInfo['config prefix'] . '.' . $entity->id();

We need to change this to store an array of data instead ideally. See #1826602: Allow all configuration entities to be enabled/disabled. We could then store the status for config entities here too (Which is the main reason that bring me here :)). So I could then add a status to my views and we would have something like:

my_view:
  config_name : views.view.my_view
  enabled: 1
damiankloip’s picture

Also, is it worth thinking about adding some sort of helper to get the manifest config name for a specific entity type?

xjm’s picture

Is it possible to pull the manifest for this out into its own patch? It would be useful for other issues as well.

gdd’s picture

Possibly, although outside the import UI it doesn't really have a use (yet.) My impression is that if we can get a couple non-cmi-people reviews (like damian's) then webchick is ready to commit this.

damiankloip’s picture

I think I agree with @heyrocker here. After I reviewed this patch, it's looking pretty complete. So once the stuff I've outlined in #100 has been addressed, I'm happy we can utilise this in views.

Basically, sticking with this issue will be quicker (Now that I have postponed my work on #1826602: Allow all configuration entities to be enabled/disabled), rather than start a new issue and wait for that :)

xjm’s picture

Status: Needs work » Needs review
Issue tags: -Favorite-of-Dries, -Usability, -Needs issue summary update, -Configuration system, -feature freeze

#97: 1697256-config-import-97.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Favorite-of-Dries, +Usability, +Needs issue summary update, +Configuration system, +feature freeze

The last submitted patch, 1697256-config-import-97.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

I tested the "file copy workflow" as follows:

  1. Changed the site name on my dev site.
  2. Copied the entire contents of the dev site's active directory to the production site's staging directory.
  3. Visited admin -> Configuration -> Synchronize configuration.
  4. Saw my change listed, imported as prompted.
  5. Confirmed the site name was changed. All is well.
  6. Added an image style on my dev site and repeated steps 2-3.
  7. Saw that the new style was listed and imported it.
  8. Confirmed the new style was created. Still okay.
  9. Edited the image style in production.
  10. Went to admin -> Configuration -> Synchronize configuration to see what would happen.
  11. Saw a message that the configuration had changed. I had a suspicion what was about to happen, but I pretended to be Jane Site Builder who probably would not, and clicked the "Import all" button.
  12. My change from 9 was reverted. Data loss, etc. Presumably because the old file was still in the staging directory.
  13. Went to my "backup" (git reset, but presumably Jane Site Builder using FTP just had a backup tarball or something).
  14. Added another new image style in dev and copied my dev active config to my production staging again.
  15. I see both my new image style, but also still the message that the other image style has changed.
  16. Now I'm stuck. I know from #12 what is going to happen if I click "Import all." There's no way for me to recover for this through the UI; I have no way to import one but not the other.
  17. Went to admin/help to try to get "advice" on what to do. Directed to http://drupal.org/documentation/modules/config which does not exist. Oops.

Talked to @heyrocker about the above. Presumably the solution to #12 is to empty the staging directory upon import, so he is adding that to the next version of the patch. He also said that core will not provide any per-configuration import for #16, but this could be implemented in contrib. I think it's less of a concern so long as #12 is resolved, and in other scenarios I'd just need to dig in and figure out which file my changes were in.

I think we should add a hook_help() entry that describes the workflow from the summary in user-friendly terms, and/or clarify the documentation on the synch page to explain how to copy from one site's active dir to another site's staging. Documentation for how to address #17 wouldn't be appropriate on the synch page, but would be useful in hook_help() or on d.o.

xjm’s picture

I also think that it would be a good idea to have some sort of reminder for the user that there is unimported config, e.g. hook_requirements(), but @heyrocker disagreed. That can be discussed in a followup and does not need to be a part of this patch.

sun’s picture

I'm sorry for not following up on this issue earlier, and I still didn't have time to review the proposed changes in-depth.

For now, I quickly have to prevent collateral damage:

Talked to @heyrocker about the above. Presumably the solution to #12 is to empty the staging directory upon import, so he is adding that to the next version of the patch.

Please not. That would be an architectural flaw. I will 100% object to that change. Of all possible config directories, the staging directory is definitely under VCS control. Drupal has no business in manipulating or writing to it, except when being explicitly asked to export configuration. Each explicit export will require a subsequent VCS operation - that is by design. Arbitrary writes are not.

gdd’s picture

The point is that with this patch, there is no more export configuration, and thus there is no point in config staying in there after an import is done because we no longer need a full tree to do imports.

gdd’s picture

Status: Needs work » Needs review
FileSize
28.9 KB

Here's a new patch that addresses the following issues:

- Rerolled to HEAD.
- Now works with the new entity_info plugins.
- Made the manifest entries arrays so that they can be used for additional purposes by individual ConfigEntity implementations.
- Empty the staging directory after a successful import. If the import fails then the staging directory remains untouched.
- Registered 'plugin.manager.entity' in bootstrap instead of in CoreBundle. This is necessary so that it can be used in the installer (which we need to do to retrieve entity info while creating the manifest files.) I tried to do this in an installer-specific DIC (we do this already for storage engines) however this breaks down while batch API is running. I know that upgrade.php needs this as well. I'm open to other ideas, since I know we want the bootstrap container to be as slim as possible.
- Made a couple of cleanups.

Reroll to HEAD broke my interdiffs. I think we're really close here.

gdd’s picture

Rerolled to HEAD, the only change is we no longer register the entity manager in bootstrap because it got done over in #1774388: Unit Tests Remastered™.

Status: Needs review » Needs work
Issue tags: -Favorite-of-Dries, -Usability, -Needs issue summary update, -Configuration system, -feature freeze

The last submitted patch, 1697256-config-import-112.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

#112: 1697256-config-import-112.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Favorite-of-Dries, +Usability, +Needs issue summary update, +Configuration system, +feature freeze

The last submitted patch, 1697256-config-import-112.patch, failed testing.

alexpott’s picture

Patch attached fixes concerns raised in #97 about waiting around for a lock and improves config entity detection by not relying on an array key but using is_subclass_of() instead.

alexpott’s picture

Status: Needs work » Needs review

Go bot go!

damiankloip’s picture

+++ b/core/includes/config.incundefined
@@ -211,17 +276,25 @@ function config_import_invoke_owner(array $config_changes, StorageInterface $sou
+function config_get_module_config_entities($module) {
+  // While this is a lot of work to generate, it's not worth static caching
+  // since this function is only called at install/uninstall, and only
+  // once per module.
+  $config_entities = array();
+  $info = entity_get_info();
+  foreach ($info as $entity_type => $entity_info) {
+    if ($entity_info['module'] == $module && is_subclass_of($entity_info['class'], 'Drupal\Core\Config\Entity\ConfigEntityBase')) {
+      $config_entities[$entity_type] = $entity_info;
+    }
   }
-  config_sync_changes($config_changes, $source_storage, $target_storage);
-  return TRUE;
+  return $config_entities;

I think this could use array_filter instead, and also people might not always use the ConfigEntityBase class for their config entities. Instead we should probably do this check on the Interface.

so something like:

return array_filter($info, function($entity_info) use ($module) {
  return ($entity_info['module'] == $module) && is_subclass_of($entity_info['class'], 'Drupal\Core\Config\Entity\ConfigEntityInterface')
});
tstoeckler’s picture

I think instanceof is preferred over is_subclass_of()

damiankloip’s picture

No, I don't think so, not when we don't have an instance of the class.

tstoeckler’s picture

Sorry, brainfail...

gdd’s picture

I actually think config_get_config_entities() should be added as a helper function in entity.inc but I'll save that for a followup. This patch refactors it to use array_filter(). Anything else? Webchick was talking about testing workflow once more.

xjm’s picture

+++ b/core/modules/config/config.moduleundefined
@@ -1 +1,58 @@
+        '!url' => 'http://drupal.org/documentation/modules/config',

This still doesn't exist; let's make sure we at least have a stub there and followups.

gdd’s picture

It actually does exist, but the url is wrong in the code. I may as well fix it since the bot is borked anyways.

gdd’s picture

gdd’s picture

Bah ignore that last one. Interdiff is still good though.

xjm’s picture

FileSize
13.98 KB
12.11 KB

I retested along the lines of #107. I found two bugs. The first is straightforward; the menu item for a view I created did not get added after synching until I additionally cleared the cache, so we probably need a cache clear following synch.

The other bug is more interesting. For some reason, the manifest.views.view.yml file is not being updated after synching. New views are only added to the manifest if they are edited on the production site after being imported. This results in the following odd behavior:

3_new_views.png
In this screenshot, there are three views that are actually new, but also three views that already exist on the production site. All six are listed as new for some reason. And...

2_updated.png
Here none of the views are new, and I've updated two of them. The users_ view is the one I edited on production once previously, and it exists in the production site's manifest. None of the others do somehow.

I wasn't able to reproduce this with image styles so it might be a Views bug, but we weren't able to determine anything Views overrides that might affect this.

gdd’s picture

I have verified that this is a problem with views. hey have not implemented hook_config_import_create() or hook_config_import_delete() catching the config and writing it out through the entity system. This means that the config system treats this as standard static config which requires no special handling and simply dumps it to the target file system. I discussed this with XJM and we both agreed that we should do a very simple implementation of these hooks in this patch so that views will work, and I have filed a followup to see if we can figure out a way to handle this more transparently (#1830830: Figure out a way for ConfigEntities to automatically handle simple CRUD functionality on config import).

I also added a drupal_flush_all_caches() at the end of the import process. However I am worried this is a little too big of a hammer for all situations. For instance, this would clear the page cache which we would really only need on field system changes. I have created a followup to investigate this as well (#1830832: Optimize cache clearing after config import.).

New patch attached, those are the only two changes other than a reroll to HEAD.

Status: Needs review » Needs work

The last submitted patch, 1697256-config-import-128.patch, failed testing.

tim.plunkett’s picture

#1820526: Remove UUID generation code from load() method in ViewStorageController adds an implementation of hook_config_import_create() for Views, and is RTBC. Not sure about hook_config_import_delete().

xjm’s picture

Issue tags: +Needs tests

We also need tests for the last several bugs we've fixed here. @beejeebus said the other night that he might work on those.

Anonymous’s picture

re #131, i didn't get to writing those tests, please don't wait for me.

gdd’s picture

Rerolled since #1820526: Remove UUID generation code from load() method in ViewStorageController has landed. Not sure where those test failures are coming from, that's a stumper.

gdd’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1697256-config-import-133.patch, failed testing.

gdd’s picture

Status: Needs work » Needs review
FileSize
29.22 KB

The problem is that the new call to drupal_flush_all_caches() clears the css cache, which attempts to variable_del('drupal_css_cache_files'), which can't happen because we don't have a variables table because DrupalUnitTestBase no longer creates table. I attempted to enable system model as a part of the test, but there is some issue with the DIC if we do that. I was talking about this with xjm and catch, and we all agreed that it was way too much of a hammer to flush all caches on every import. However we also didn't want to leave it entirely up to the responding modules. You could easily end up flushing some caches 10 times per import that way. Ultimately we moved the call to drupal_flush_all_caches() into the UI form submit function. This way it happens when you import from the UI, but if you're calling the API function (via drush, or an import script, or whatever) you're on your own to choose which caches to clear and when. We can still investigate other options later if we want.

I'm not sure what other tests to add. We already have tests verifying that ConfigEntities work properly. I would think that individual modules should add tests to verify that their individual implementations work properly as well? This was a Views implementation issue so I'd think it is more appropriate there.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Looks like all the bugs I found have been addressed and there's followup issues filed already for the other questions I had. I've also reviewed the added test coverage and it looks good.

I think we should probably test the git workflow above more thoroughly, but that's much easier to do once this is committed.

sun’s picture

Some earlier comments mentioned proposed changes that are massive architectural flaws from my perspective, so I want to review this.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

COMMITTED AND PUSHED TO EIGHT DOT FREAKING EX!!!! YEAH!!!!

It's so great to see this one behind us. Now we can actually get some folks testing the heck out of this during the BADCamp sprints coming up.

webchick’s picture

Oops. Didn't see #138. sun, please do open a follow-up for any concerns you have, so we make sure we get this right.

sun’s picture

Status: Fixed » Active

Seriously? This hasn't been RTBC for a single time, and six minutes after it became so it was committed. That leaves no chance for anyone to review the proposed changes.

The idea of a manifest/index didn't sound so bad. But this usage and implementation looks inappropriate.

The logic and control structures in config_sync_get_changes() is full of bugs. Changes can duplicate creations and deletions, the manifest is entirely ignored for changes. Was this even remotely tested?

The config import code contains no check or protection for the currently enabled extensions, but yet, manifest files are separated per extension. It completely ignores the existence of manifest files in the target storage.

The change is architecturally flawed. The proposed solution only works for config entity objects, but there can be configuration, but not necessarily config entity objects. As such, the committed change does not solve the actual problem. I have no idea why this was tied to config entities. Like, at all.

+function config_uninstall_default_config($type, $name) {

This has nothing to do with "default" config.

@@ -23,15 +28,26 @@ function config_install_default_config($type, $name) {
+        list(, , $id) = explode('.', $config_name);

The config_prefix does not necessarily contain one dot only.

+++ b/core/includes/config.inc
@@ -23,15 +28,26 @@ function config_install_default_config($type, $name) {
+      $manifest_config->setData($manifest_data)->save();

Manifest is saved before configuration is imported.

+++ b/core/includes/config.inc
@@ -40,6 +56,28 @@ function config_install_default_config($type, $name) {
+function config_uninstall_default_config($type, $name) {
...
+  // If this module defines any ConfigEntity types, then delete the manifest
+  // file for each of them.
+  foreach (config_get_module_config_entities($name) as $entity_type) {
+    config('manifest.' . $entity_info['config_prefix'])->delete();
+  }

Uninstallation happens after disabling a module.

You cannot invoke any APIs in disabled modules. The entity info you're trying to retrieve no longer exists.

+++ b/core/includes/config.inc
@@ -80,18 +118,45 @@ function config($name) {
+  // Config entities maintain 'manifest' files that list the objects they
+  // are currently handling. Each file is a simple indexed array of config
+  // object names. In order to generate a list of objects that have been
+  // created or deleted we need to open these files in both the source and
+  // target storage, generate an array of the objects, and compare them.

Why?

Seriously, this is the only comment that marginally attempts to describe why something simple has suddenly turned into something that is incomprehensible and cannot be explained to anyone in a single sentence. And this doesn't even explain it either.

+++ b/core/includes/config.inc
@@ -211,17 +277,21 @@ function config_import_invoke_owner(array $config_changes, StorageInterface $sou
-function config_export() {

WTF? Why on earth was config_export() removed?

And why didn't that ring all available alarm bells?

+++ b/core/includes/config.inc
@@ -211,17 +277,21 @@ function config_import_invoke_owner(array $config_changes, StorageInterface $sou
+  $info = entity_get_info();
+  return array_filter($info, function($entity_info) use ($module) {
+    return ($entity_info['module'] == $module) && is_subclass_of($entity_info['class'], 'Drupal\Core\Config\Entity\ConfigEntityInterface');

Why all this complexity? Why doesn't this check for the 'config_prefix' property?

+++ b/core/includes/install.inc
@@ -284,6 +284,21 @@ function drupal_install_config_directories() {
+    // Put a README.txt into each config directory. This is required so that
+    // they can later be added to git. Since these directories are auto-
+    // created, we have to write out the README rather than just adding it
+    // to the drupal core repo.
...
+      case CONFIG_ACTIVE_DIRECTORY:
+        $text = 'This directory contains the active configuration for your Drupal site. To move this configuration between environments, contents from this directory should be placed in the staging directory on the target server. To make this configuration active, see admin/config/development/sync on the target server.';
+        break;

The active config directory MUST NOT be versioned in the first place. How on earth did we arrive at this utter misunderstanding?

Lastly, the code doesn't properly delimit/namespace manifest files. It also unnecessarily abuses $config->setData() in many locations, which only begs the question of why this is using Config at all.

Whatever.

Committing this was a step backwards, not forwards.

cweagans’s picture

@sun: There has been an issue summary for this issue that details the proposed changes since Oct 20, and there has been a patch here for even longer than that. Given that this is a critical issue and you comment on nearly everything in the queue, you can't have been unaware of that, so why didn't you give your architectural input at that point? Coming in at the *end* of a critical issue with major architectural changes is unacceptable, especially when said architectural changes are presented with an undertone of "Every line of code that has been written is shit and you need to start over". As webchick said, please open a follow-up for your concerns.

cweagans’s picture

Status: Active » Fixed
Bojhan’s picture

Status: Fixed » Active

Did we fix the help text, or where do I go for a followup?

@cweagans Can we stop with all the "this is unacceptable behavior stuff"? Sun could have describes his concerns more nicely, but we shouldn't respond negatively to providing feedback after most of the discussion, that simply happens as contributors can't follow everything.

cweagans’s picture

Priority: Critical » Normal

The issue is not that he's providing feedback. The issue is that he's waiting till the end to demand architectural changes and then asserting that the change is somehow a step backwards for Drupal, which does nothing except infuriate people that poured months of time into this issue and drive even more developers out of core development. That is unacceptable and it needs to stop. He could have just as easily opened a follow-up.

If we can't just close this issue, then there's no reason that it needs to be critical.

Anonymous’s picture

i've created a couple of follow ups based on question sun raised:

@sun - can you create issues for the other things you've identified that can be tackled in follow ups?

as a general response to @141 - the driver for the CMI plumbing changes in this patch is partial imports. whether or not the manifest file implementation can be improved is one thing, but i was under the impression that the supporting partial imports is not negotiable.

i don't like supporting that use case in core - i think it overcomplicates the internals, and tries to make the system too simple. IMO, it's just not that simple. but, if we take supporting that use case as a given, i haven't seen a better design idea than the manifest files, so i don't think ripping it out is the way forward from here.

xjm’s picture

Priority: Normal » Critical
Status: Active » Fixed

@sun, a lot of work has gone into testing and improving this patch over the past week at BADCamp. At this point, from my perspective (and others'), we really need to get something in and iterate it. The answers to a lot of your questions are up further in the issue (see #63 for example).

Let's open followup issues for specific points and keep the original status. We cannot afford to keep blocking critical features and APIs on architectural perfection. Plus, there's lots of opportunity to work on more self-contained followup tasks at the sprints this week.

Filed:

gdd’s picture

The proposed solution only works for config entity objects, but there can be configuration, but not necessarily config entity objects. As such, the committed change does not solve the actual problem. I have no idea why this was tied to config entities. Like, at all.

The manifest files were created to solve the problem of creating and deleting configuration when you don't have a complete tree. Only config managed by config entities will ever be created or deleted in this way (except on uninstall, which will be handled by the module system completely separate from this.) The manifests make no sense in any other context.

WTF? Why on earth was config_export() removed?

Because if we can support import without needing a complete tree (which we now can) then we don't need an export functionality anymore. The entire idea of export-to-import was a usability problem and confusing.

The active config directory MUST NOT be versioned in the first place. How on earth did we arrive at this utter misunderstanding?

I have no idea what you're talking about here. We have been discussing this in the context of a dev/stage/live workflow forever, and in the context of a two-directory system since the idea was proposed. Yched first brought it up and I documented this workflow back in august at #1703168-38: [Meta] Ensure that configuration system functionality matches expected workflows for users and devs.

This was, as you can see, reviewed and tested by no less than six people, including a core maintainer, two config system maintainers, and at least three highly respected core developers. The manifest file solution has been very actively in dev for over two weeks. It was also the only solution we've ever come up with that offered a solution to the problem at hand. Its fine if you don't like it, and I would never claim any code I wrote is perfect, but that doesn't mean you can treat the hard work of all these people with such a disrespecting and dismissive attitude. How we work is just as (if not more) important as the work we do. I look forward to more followups.

Jose Reyero’s picture

Just landed here looking where that 'manifest' thing I've just came accross in the code came from.

Yes, I know, this is another late 'WTF' but I want, just for the record, to be added to the list of people seriously frustrated with patches like this one that start like an UI thing and end up "improving" the very basis of the config system.

So we just got undocummented (or at least I cannot understand what this is, I cannot find any example of it) METADATA stuck into the configuration data, isn't that it?

gdd’s picture

So we just got undocummented (or at least I cannot understand what this is, I cannot find any example of it) METADATA stuck into the configuration data, isn't that it?

There is currently no metadata in the manifest files. It's just a list of active config entities that is used to help manage synching deletes, that's all. While there are proposals to use it for other things, they have not landed and are currently in active discussion.

One thing worth noting is that this data is per ConfigEntity instance whereas the metadata being generated at #1648930: Introduce configuration schema and use for translation is per ConfigEntity type and can't be used for something like registering what module supplied a specific view. While I'm not completely on board with using the manifest files for that (even though I suggested it) it would provide a solution to some seriously tricky problems we have with dependency management.

Status: Fixed » Closed (fixed)

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

yched’s picture

Sorry for posting in this big and old issue, but the right people are subscribed here.

While working on #1817182-24: Add upgrade path tests for contact category config conversion, bumped on odd code in ConfigStorageController::save(), introduced here :

    // Add this entity to the manifest file if necessary.
    $config = config('manifest.' . $this->entityInfo['config_prefix']);
    $manifest = $config->get();
    if (!in_array($this->getConfigPrefix() . $entity->id(), $manifest)) {
      $manifest[$entity->id()] = array(
        'name' => $this->getConfigPrefix() . $entity->id(),
      );
      $config->setData($manifest)->save();
    }

It seems the if (!in_array()) code is never going to be false ? (i.e $manifest never contains $this->getConfigPrefix() . $entity->id()as a direct value, only as a nested value)
Why would this check be needed to begin with ? If already present, we overwrite, no biggie ?

yched’s picture

Issue summary: View changes

Updated issue summary.