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
- 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.)
- Install both downloads.
- 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
- 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.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#136 | 1697256-config-import-135.patch | 29.22 KB | gdd |
#133 | 1697256-config-import-133.patch | 28.34 KB | gdd |
#128 | 1697256-config-import-128.patch | 28.73 KB | gdd |
#127 | 3_new_views.png | 12.11 KB | xjm |
#127 | 2_updated.png | 13.98 KB | xjm |
Comments
Comment #1
gddHere's one to get the ball rolling #1697266: Allow cherry picking what config to sync
Comment #2
gddTagging
Comment #3
sunGiven 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...?
Comment #4
gddYes, 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.
Comment #5
Bojhan CreditAttribution: Bojhan commentedI 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.
Comment #6
gddI 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?
Comment #7
Bojhan CreditAttribution: Bojhan commented@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.
Comment #8
Bojhan CreditAttribution: Bojhan commentedI 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.
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.
Comment #9
corvus_ch CreditAttribution: corvus_ch commentedClean and Simple. I like this.
Two additions:
The later is clearly a nice to have but a helpful one.
Comment #10
sunSince 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.
Comment #11
dagmarJust to link the issue. #120955: Integrate Diff into Core Maybe can be added into D8.
Comment #12
gddGonna focus on this in the coming week
Comment #13
webchickTesting instructions, copied/modified from #1447686-158: Allow importing and synchronizing configuration when files are updated for posterity.
Comment #14
webchickAlso, 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.
Not bumping down to "needs work," since there's still plenty to review here.
Comment #15
gddNote 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.
Comment #16
sunI 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.
Comment #17
gddAre 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.
Comment #18
Bojhan CreditAttribution: Bojhan commentedI 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?
Comment #19
gddBojhan 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.
Comment #20
Anonymous (not verified) CreditAttribution: Anonymous commentedi agree with #19. i think this is a case of 'modules are special'.
Comment #21
sunyeah, 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... (?)
Comment #22
gddNote 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?
Comment #23
gddThis 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.
Comment #24
moshe weitzman CreditAttribution: moshe weitzman commentedFYI, 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.
Comment #25
gddRight 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.
Comment #26
moshe weitzman CreditAttribution: moshe weitzman commentedNaive 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"
Comment #27
tstoecklerIMO 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.
Comment #28
gddI'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.
Comment #29
moshe weitzman CreditAttribution: moshe weitzman commentedIf 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.
Comment #30
gddWe 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.)
Comment #31
gddReturned 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.
Comment #32
Anonymous (not verified) CreditAttribution: Anonymous commentedHmmm, I don't have any answers yet, but I'll think on it.
Comment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedthought 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.
Comment #34
gddHere 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.
Comment #35
gddOoops, this is going to fail wildly I assume.
Comment #36
gddBojhan asked for a screenshot. Here is the sync screen with one changed image style, one new one, and one deleted one.
Comment #38
yched CreditAttribution: yched commentedI'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.
Comment #39
corvus_ch CreditAttribution: corvus_ch commented+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.
Comment #40
Anonymous (not verified) CreditAttribution: Anonymous commentedre. #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.)
Comment #41
gddI 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.)
Comment #42
webchickWe 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?
Comment #43
gddI'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.)
Comment #44
gddI 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:
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.
Comment #45
gddOK 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.
Comment #46
gddHere 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:
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.
Comment #47
webchickExciting!! Marking needs review.
Comment #49
webchickAlso, no. This is not normal. :P
Comment #52
webchickHeh, trying that again. :)
Comment #53
gddI would go even farther, I actually thought this was already critical.
Comment #55
alexpottI 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...
This isn't necessary... we can just create the file on first save.
This needs to be done on save() and not create. Since the breakpoint config entity has not set it's id yet.
this can be changed to
config::clear()
Comment #56
alexpottgo testbot go!
Comment #57
alexpottSeems like the patch causes a loop... Been testing for 50+ minutes :(
Comment #59
alexpottOops I was wrong ... No loop :) and less exceptions and fails. So that's a win.
Comment #60
gddI 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.
Comment #61
gddAlso changing title to be more relevant, since this isn't really a meta-issue anymore.
Comment #62
gddOne 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.
Comment #63
gddAttached 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.
Comment #64
gddComment #66
gddpfeh
Comment #68
alexpottDoing 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...
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.
Comment #69
Dries CreditAttribution: Dries commentedThis looks promising. The issue summary is lacking though. Would love to see some more screenshots.
Comment #70
yched CreditAttribution: yched commentedNot 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 :-)
Comment #71
tim.plunkettFor the H3 @todo in there, I opened #1817996: Add sane default styles for table <caption>
Comment #71.0
gddUpdated issue summary.
Comment #72
gddI 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.
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.
Comment #72.0
gddUpdated issue summary.
Comment #72.1
gddUpdated issue summary.
Comment #72.2
gddUpdated issue summary.
Comment #73
webchickSteps to test:
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:
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:git diff
shows:So far, so good! :)
4) I commit that change:
PRODUCTION SITE
1) I pull to get that change.
2) I reload http://localhost/drupal-8.x-prod/. No slogan.
3) I clear the cache:
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:
"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:
Even though the dev site has both an active and staging dir, and the prod site has only active.
Starting over again. :( Git-- :P
Comment #74
Anonymous (not verified) CreditAttribution: Anonymous commentedso 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.
Comment #75
webchickOk, so at the time that you install Drupal, there is no sites/default/files/config/staging_[stuff] directory. That seems like a bug.
Comment #76
webchickWAIT. 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?
Comment #77
webchickOk, 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:That seems right.
git diff
shows: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.
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.
Comment #78
webchickTim gave me the magickal dark incantations of:
Updated instructions, and we are cooking with gas again. :)
Comment #79
webchickNow 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:
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:Why would it be deleting those values?
Comment #80
webchickNow, 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: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.....
Bingo!
4) Reload http://localhost/drupal-8.x-prod/. Site slogan still shows "Her mom," as expected.
5) Clear cache for fun. ;) ;)
PROBLEM
;)
5) Deal with the conflict:
6) Reload http://localhost/drupal-8.x-prod/ again. IT WORKS! Except for these errors, which are gone on the next re-load:
Comment #81
webchickSo, 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:
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:
- 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. ;)
Comment #82
webchickRight, 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. :)
Comment #82.0
webchickUpdated issue summary.
Comment #82.1
gddUpdated issue summary.
Comment #83
Anonymous (not verified) CreditAttribution: Anonymous commentedi 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.
Comment #84
webchickOk, 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. ;))
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:
to:
and:
… respectively.
New test coming up, first have to go take the dog outside. ;) Be back soon.
Comment #85
webchickOh, also the net result of this site creation workflow is you end up with 4 different directories under files/config:
This is a little hard to track. One thing heyrocker suggested on the phone today was an issue to change the directory structure to:
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
Comment #86
webchickThe basics: A simple settings change.
DEV SITE
1. Go to admin/config/system/site-information
2. Enter Slogan: "A spiffy site slogan!"
3. Save.
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
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
Woohoo!
5. Commit.
PROD SITE
1. Pull in changes from dev
2. Stage configuration.
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.
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.
8. Go to admin/config/development/maintenance. See pirate speak. Yay!
9. Now let's see what git diff says.
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.
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.
Comment #87
gddThis will be solved when the new context system goes in.
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.
Comment #88
webchickFancier: 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.
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.
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.
5. Reload node/1, verify that image is still big and colourful.
6.
drush cc all
, same thing.7. Stage the changes.
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.
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... :\
Diff confirms. :\
I think I remember hearing somewhere this was a pre-existing bug, maybe add to the testing instructions?
Comment #89
gddYup I will be putting this in the next patch.
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)
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
Comment #90
gddI 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.
Comment #91
webchickFiled 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.
Comment #92
Bojhan CreditAttribution: Bojhan commentedI 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.
Comment #92.0
Bojhan CreditAttribution: Bojhan commentedUpdated issue summary.
Comment #92.1
gddUpdated issue summary.
Comment #93
gddThis 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.
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:So lets get those changes committed
Now move into drupal_live and pull
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!
Comment #93.0
gddUpdated issue summary.
Comment #94
gddI 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
Comment #95
gddComment #97
alexpottPatch 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:
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 throughconfig_install_default_config()
.Comment #98
gddMarking as susceptible to feature freeze
Comment #99
moshe weitzman CreditAttribution: moshe weitzman commentedThat tag is a duplicate of the Category selector (specifically the Feature option).
Comment #100
damiankloip CreditAttribution: damiankloip commentedI haven't actually tested this patch yet, but generally the code etc... looks great.
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.
Yes, unfortunately there is not really another way to determine this... So I guess this can be removed.
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:
Comment #101
damiankloip CreditAttribution: damiankloip commentedAlso, is it worth thinking about adding some sort of helper to get the manifest config name for a specific entity type?
Comment #102
xjmIs it possible to pull the manifest for this out into its own patch? It would be useful for other issues as well.
Comment #103
gddPossibly, 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.
Comment #104
damiankloip CreditAttribution: damiankloip commentedI 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 :)
Comment #105
xjm#97: 1697256-config-import-97.patch queued for re-testing.
Comment #106.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #106.1
xjmUpdated issue summary.
Comment #106.2
xjmUpdated issue summary.
Comment #107
xjmI tested the "file copy workflow" as follows:
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 inhook_help()
or on d.o.Comment #108
xjmI 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.Comment #109
sunI'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:
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.
Comment #110
gddThe 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.
Comment #111
gddHere'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.
Comment #112
gddRerolled 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™.
Comment #114
alexpott#112: 1697256-config-import-112.patch queued for re-testing.
Comment #116
alexpottPatch 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.Comment #117
alexpottGo bot go!
Comment #118
damiankloip CreditAttribution: damiankloip commentedI 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:
Comment #119
tstoecklerI think instanceof is preferred over is_subclass_of()
Comment #120
damiankloip CreditAttribution: damiankloip commentedNo, I don't think so, not when we don't have an instance of the class.
Comment #121
tstoecklerSorry, brainfail...
Comment #122
gddI 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.
Comment #123
xjmThis still doesn't exist; let's make sure we at least have a stub there and followups.
Comment #124
gddIt actually does exist, but the url is wrong in the code. I may as well fix it since the bot is borked anyways.
Comment #125
gddComment #126
gddBah ignore that last one. Interdiff is still good though.
Comment #127
xjmI 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: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...
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.
Comment #128
gddI 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.
Comment #130
tim.plunkett#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().
Comment #131
xjmWe also need tests for the last several bugs we've fixed here. @beejeebus said the other night that he might work on those.
Comment #132
Anonymous (not verified) CreditAttribution: Anonymous commentedre #131, i didn't get to writing those tests, please don't wait for me.
Comment #133
gddRerolled 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.
Comment #134
gddComment #136
gddThe 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.
Comment #137
xjmLooks 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.
Comment #138
sunSome earlier comments mentioned proposed changes that are massive architectural flaws from my perspective, so I want to review this.
Comment #139
webchickCOMMITTED 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.
Comment #140
webchickOops. Didn't see #138. sun, please do open a follow-up for any concerns you have, so we make sure we get this right.
Comment #141
sunSeriously? 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.
This has nothing to do with "default" config.
The config_prefix does not necessarily contain one dot only.
Manifest is saved before configuration is imported.
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.
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.
WTF? Why on earth was config_export() removed?
And why didn't that ring all available alarm bells?
Why all this complexity? Why doesn't this check for the 'config_prefix' property?
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.
Comment #142
cweagans@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.
Comment #143
cweagansComment #144
Bojhan CreditAttribution: Bojhan commentedDid 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.
Comment #145
cweagansThe 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.
Comment #146
Anonymous (not verified) CreditAttribution: Anonymous commentedi'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.
Comment #147
xjm@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:
Comment #148
gddThe 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.
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.
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.
Comment #149
Jose Reyero CreditAttribution: Jose Reyero commentedJust 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?
Comment #150
gddThere 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.
Comment #152
yched CreditAttribution: yched commentedSorry 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 :
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 ?
Comment #152.0
yched CreditAttribution: yched commentedUpdated issue summary.