Updated: Comment #0
Problem/Motivation
If a module dev decides to add a config form to their module, they need to write a config file. Without this, they would be forced to reinstall their module to properly get the file in the active store
If your coworker wants to give you a new content type they were working on, without this you need to do a full site config import.
Proposed resolution
Add a form to "import" a single config file for advanced purposes. Clarify that this will NOT resolve any dependencies, and could break their site.
Remaining tasks
User interface changes
Added admin pages
API changes
Addition
Related Issues
#1898794: Allow config entities to be exported. (Resolve regression from views in Drupal 7).
#1831798: Update hook_help() for config manager module
Comment | File | Size | Author |
---|---|---|---|
#34 | cmi-2099363-34.patch | 31.65 KB | tim.plunkett |
#34 | interdiff.txt | 1.11 KB | tim.plunkett |
#32 | cmi-2099363-32.patch | 31.52 KB | tim.plunkett |
#32 | interdiff.txt | 3.33 KB | tim.plunkett |
#30 | Screen Shot 2013-10-25 at 00.30.24.png | 43.57 KB | swentel |
Comments
Comment #1
tim.plunkettHere is my proposed approach.
This is the initial "single export" form, with a dropdown to pick what type of configuration
For "simple configuration", generally the MODULE.settings.yml (anything not a config entity), you have a dropdown
Selecting one populates the import textarea with the YAML
The same thing works for all config entity types
This is the "single import" form for a "simple configuration", where you have to specify the filename
For config entities, you choose the type
Comment #2
dawehnerJust did a little bit of manual testing:
There it was possible to give another name ... so maybe we have to provide such a entity ID import textfield?
Do we really still need to add old style tabs?
As written I guess it would make sense to be able to specify the ID of the new config entity.
Just in general I think it is odd to never ask the entity manager whether the stuff was really created.
Comment #3
tim.plunkettNew tab structure:
Also now has a confirmation form because @xjm and @alexpott talked me into it
Major props to @Berdir for the $form_state['rebuild'] = TRUE; suggestion
EDIT: I need to open a follow-up for the ability to specify a custom ID for your config entity, but that can wait until after dinner.
Comment #4
tim.plunkettOpened #2101691: Allow a custom ID to be specified when using single import for config entity as a follow-up.
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedi think this is ready.
discussed validation issues with @timplunkett and @moshe in IRC, but they aren't created by this patch.
we need to fix config entity validation elsewhere.
Comment #6
chx CreditAttribution: chx commentedI don't think so. This very nicely shows that encode / decode is doesnt even belong to the config storage interface. It needs to be moved to a staging (or whatever we call it) interface and then filestorage can implement that and be happy about it. If you switch the active store to a redis / mongodb / heck mysql w/ php serialize store what do you expect from encode ? Nothing normal. Conversely, the staging storage shouldn't be a service but should be hardwired to the YAML FileStorage we have pretty much the same as modules are phptemplate wired in D7 and for the same reasons.
Edit: we can hardwire it into a method in ConfigFactory or something.
Comment #7
tim.plunkettThe new interface and hardcoding should be left to a new issue.
This change will at least force the ExportForm to give us something in a format that importing can read.
EDIT: #2105689: Acknowledge that our import/export config storage is not swappable is the new issue
Comment #8
tim.plunkettNope, that won't work, because config.storage.staging is hardcoded to look in /staging. We can't just use it to read out /active, at least not yet.
I'd still very much like to see #3 go in, since it correctly uses the APIs available to it.
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedtalked to timplunkett and chx, and we agreed to RTBC #3 again. fixing the other issue shouldn't block this.
Comment #11
webchickNot looked at the code, but here's a UI review. This patch makes me very happy!!! :D
I'd put export first, then import, personally. That's the logical progression people will do.
I'd also put full ahead of single, since most of the time that's what people should be doing.
Let's alphabetize this list. It's bound to get even longer when contrib modules are added, so finding things will be an issue otherwise.
Can the "what's happening here?" message be moved to the white area, rather than the title, so it doesn't get cut off?
Also, can we kill the import/export tabs from here? That's strange.
Not sure if this is true for other things, but for Blocks anyway, we need some kind of disambiguation between the two "System help" blocks. "System help (Bartik)" vs. "System help (Seven)" for example.
Since "Configuration type" on import dictates which API functions are called, it's important that it's set correctly. How about set it to nothing by default and make it required, so people are forced to make a choice?
Comment #12
webchickComment #13
tim.plunkettThis fixes "Configuration type" by making it required, default to nothing, and sorted alphabetically.
No idea how best to handle the "Configuration names" part for blocks... :(
Comment #15
tim.plunkett#13: config-import-export-2099363-13.patch queued for re-testing.
Comment #16
dawehnerThe variable name is a bit odd. I think $config would be a bit better. A value set to FALSE is pretty clear all the time.
Comment #17
tim.plunkettI had $config originally, but while writing the form I twice got it switched up with $this->config(). So I think we should leave it as is.
Comment #18
tim.plunkett#13: config-import-export-2099363-13.patch queued for re-testing.
Comment #20
tim.plunkettJust a reroll for new tests using the old path
Comment #21
tim.plunkettI think that the desire to differentiate the blocks could be solved by either #2111341: Introduce EntityManager::options() to build options lists or #1959806: Provide a generic 'entity_autocomplete' Form API element. But I think its out of scope for this issue.
Comment #22
tim.plunkett#20: config-2099363-20.patch queued for re-testing.
Comment #23
mtiftI'm going to be looking at this one today and testing it thoroughly
Comment #24
mtiftLet's start with a reroll
Comment #25
tim.plunkettThat double added the titles.
Comment #26
mtift(I got distracted a lot today.) This looks great to me. I have tested most of the scenarios I can imagine and have not encountered any errors.
I was thinking about adding an export button for the single file export (admin/config/development/configuration/single/export), but I'm wondering if that was left off intentionally because the export file can be altered?
Comment #27
tim.plunkettI don't think a download button is needed, its a giant textarea you can select-all and copy-paste from, same as Views/CTools had in D7.
Comment #28
swentel CreditAttribution: swentel commentedThis one needs a reroll after the local task conversion - started looking into it but got confused and completely distracted.
Would love to test this with fields and instances .. :)
Comment #29
tim.plunkettRerolled.
Comment #30
swentel CreditAttribution: swentel commentedDid some tests with fields. Importing gave some problems
The Field(Instance)StorageController::loadByProperties() assume some extra keys - since we were the ones always calling them, those were always ok. (edited)
Importing something illegal gives a nasty error
Made changes to the field storage controllers and added a try/catch to the import of a config entity to catch the exception and show a more friendly message.
It now comes back to import screen. Since we're in the submit, I wasn't sure how to preserve the data; rebuild gave me back the confirm form.
Comment #32
tim.plunkettOh yay, unit tests for local tasks!
Fixing the naming insanity.
Also, a couple people asked about hook_help, but there is already #1831798: Update hook_help() for config manager module .
Comment #33
mtiftThis is ready to go. Applies cleanly. Code looks good. Tested exporting/importing image styles, text formats, and more.
This will definitely benefit from some scary "Are you sure you want to import this config and risk totally breaking your site?" warnings to be provided in #1831798: Update hook_help() for config manager module .
Comment #34
tim.plunkettBack in #13 I fixed the sorting of the config types dropdown, but I only did it for import.
This fixes export in the same way (leaving "Simple configuration" at the top)
Comment #35
webchickSat down with Tim at BADCamp to test this sucker. Overall this both looks and works kick-ass! :) Since this is fixing a regression, it seems to belong as at least a major.
I found a few things while testing:
1) (minor) The options were alphabetized on import, but not on export. Tim fixed that in his latest re-roll.
2) (major/critical) I did something where I copied node.field_tags, and fiddled with it to make it user.field_tags instead, but hit a couple of errors on the way (duplicate UUID, duplicate machine name), but the error I ended up with was that the user_field_tags table already existed. :\ This means that you can currently get into a situation where an entity->save() half-finishes, which needs to be fixed. However, that has nothing to do with this issue, it was merely exposed by it. Will file a follow-up for it.
3) There were some minor UX-ish things, like for example on import you've now lost the name of the file that you had in the export screen, but after talking with Tim, there wasn't really a great way to solve this. Embedding the filename in the YAML output, for example, would make it so that these weren't easily portable. We also discussed the order of the tabs, because this patch makes *single* import/export the second tab after Synchronize. Between Jess, Alex Pott, and Matthew Tift, we couldn't come to consensus on what the order should be, so we decided to just leave it, since it's super easy to change later once we have more user feedback.
So, with that, I'm officially out of things to complain about. :) Sooooo...
Committed and pushed to 8.x. WOOHOO!!
Comment #35.0
webchickadded the hook help