text:
- We have a shiny new configuration system, but the primary workflow we are targeting is not in place - push configuration files to a second site and have that new configuration take effect.
- Implement this functionality.
- Files pushed to a new site should not take effect immediately, something will have to be kicked off to accept them.
- There needs to be a place in admin to do this, but also an API that can be hooked into by Drush and other tools.
- When a reload happens, modules need to be notified of these changes so that they can act on it.
- This notification has to happen in a specific order (new node types need to be created before field instances can be added, same between fields and instances, etc...)
- This notification has to include extensive information about the config since modules may want to act on changes to other modules' config.
- We need to avoid race conditions wherein the code that has been pushed to the second server gets overwritten by configuration changes happening on the live site.
Proposed resolution
- When config is reloaded a new hook is fired for modules to respond to. This hook passes on pointers to the old config (currently in the active store) and the new config (currently in the files.)
- Modules examine the differences between these two in order to determine the actions they need to take. New additions can be considered a create, missing configuration can be considered a delete.
- Modules are called in the order of the dependency chain defined in info files.
- Aside from this, the work to be done is left entirely to the modules to detect and react to.
- The idea of using the module dependencies for ordering is entirely experimental and may not work at all. It will need extensive testing and research. Other ideas welcomed.
- This will require a bit of reworking of the current class architecture to achieve properly.
- There are two thoughts as far as avoiding race conditions.
- Currently files are automatically written when configuration changes are made. This means that when files are pushed, in order to avoid changes being overwritten, the admin on tha target site would essentially need to be set to be read-only until the configuration is reloaded. This is a feature we plan on adding anyways, but this switch would need to be flipped manually before config is pushed/pulled.
- The other option is that we don't write files automatically, instead we just write the active store. When you want to push config to the live site, you have to manually export the config, commit it, and push it live. This adds a step for end users, but reduces complexity in the deployment process.
- The ideal target use case for this is the field system. If we can make that work, we should be in pretty good shape.
- Further discussion can be found at http://groups.drupal.org/node/190729
PASSED: [[SimpleTest]]: [MySQL] 37,241 pass(es). View
PASSED: [[SimpleTest]]: [MySQL] 37,313 pass(es). View
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config.import.161.patch. Unable to apply patch. See the log in the details link for more information. View
|762 bytes
Comment #1beejeebus CreditAttribution: beejeebus commented
just a note that a) i intend to work on this when the first patch lands and b) first, early crappy code lives in the 8.x-file-config-reload branch in the CMI sandbox:
Comment #3sun CreditAttribution: sun commented
Comment #4beejeebus CreditAttribution: beejeebus commented
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 35,057 pass(es). View
here's an EARLY cut at some reload code. i've created a patch against the CMI sandbox 8.x branch. the action is happening in the 1447686-reload branch.
i discussed reload with heyrocker and msonnabaum, and there will likely be a lot of changes to the class hierarchy coming down the track, but i've tried to avoid that and just get something working.
i've also avoided touching the 'write to files all the time' question - we can deal with that later.
Comment #5beejeebus CreditAttribution: beejeebus commented
added an implementation of image_config_reload_from_disc(). looks pretty ugly, but seems to work.
Comment #6beejeebus CreditAttribution: beejeebus commented
thanks to some memory jogging from davidstrauss, implemented a first cut at the 'rerun my implementation after other modules have finished' functionality to hook_config_reload_from_disc. updated patch attached.
Comment #7cweagans CreditAttribution: cweagans commented
This is a bit pedantic, but I think it should be "disk" instead of "disc", per http://support.apple.com/kb/HT2300?viewlocale=en_US&locale=en_US and pretty much any source on the web that I found (I chose the apple one because it was nice and succinct).
Comment #8Pol CreditAttribution: Pol commented
|Needs review
|» Needs work
I've tested this patch and it doesn't seems to reload anything.
Maybe I'm doing something wrong, I don't know, but when I go to /admin/config/config/reload, no changes are detected even after modifying the XML files (adding or editing)
My setup is a Drupal 8 (branch 8.x dev), freshly checked out + patch in #6.
I'm going to look further into this.
Comment #9beejeebus CreditAttribution: beejeebus commented
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 35,648 pass(es). View
updated patch.
- added hook_reload_validation() and hook_reload_error(), to allow modules to check config changes and react to an error respectively
- refactored config_reload().
Comment #10Pol CreditAttribution: Pol commented
Hello Beejeebus,
I've tested your patch and changing a config file doesn't do anything.
To test your patch, I change settings in the xmls from the image module and see if the changes are detected and reloaded.
Comment #11Pol CreditAttribution: Pol commented
I've updated the UI, tell me if you like it !
Comment #12Pol CreditAttribution: Pol commented
Here's the patch.
Comment #14Pol CreditAttribution: Pol commented
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 35,662 pass(es). View
(21:43:17) beejeebus: drupol: can you can you make that $config_files instead of $configs ?
Comment #16beejeebus CreditAttribution: beejeebus commented
alllrighty, the patch at #14 is committed, thanks!
next bit is to make a menu entry at 'admin' or similar so we can find this functionality easily.
Comment #17beejeebus CreditAttribution: beejeebus commented
updated patch against 8.x with the UI from Pol.
Comment #18beejeebus CreditAttribution: beejeebus commented
|» task
|Needs work
|» Needs review
Comment #19Pol CreditAttribution: Pol commented
About the menu, are you ok with this ?
Comment #20Pol CreditAttribution: Pol commented
Patch updated with menu entry visible in admin/config.
Comment #21cweagans CreditAttribution: cweagans commented
No, config doesn't need to be its own section at admin/config. Just stick it under Development.
Comment #22cweagans CreditAttribution: cweagans commented
|Needs review
|» Needs work
Comment #23Pol CreditAttribution: Pol commented
PASSED: [[SimpleTest]]: [MySQL] 35,660 pass(es). View
Patch updated.
Comment #24Pol CreditAttribution: Pol commented
|Needs work
|» Needs review
Comment #25Pol CreditAttribution: Pol commented
Here's a better patch.
I understand why my patch were 14Kb instead of 18Kb, when using
git diff, it doesn't includes untracked files (DrupalConfigFile.php & DrupalConfigTree.php).
I had to create a branch, commit my stuff to it, then
git format-patch 8.x.. to get it working.
Is there a better way to do ?
Comment #26aspilicious CreditAttribution: aspilicious commented
Don't forget to write proper docs when this is rdy.
For example: some functions need @return or @throws statements.
drupal.org/node/1354 is the best node in the world!
Comment #27beejeebus CreditAttribution: beejeebus commented
|Needs review
|» Needs work
heyrocker pointed out that we should move the menu callback UI from the system.module into config.module.
Comment #28beejeebus CreditAttribution: beejeebus commented
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 35,711 pass(es). View
updated patch adds tests.
Comment #29Pol CreditAttribution: Pol commented
FAILED: [[SimpleTest]]: [MySQL] 35,675 pass(es), 13 fail(s), and 5 exception(s). View
Patch updated.
Removed config configuration from
system.module and
Moved them to
config.module and
Comment #31beejeebus CreditAttribution: beejeebus commented
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 35,705 pass(es). View
looks like #29 was missing some files from config_test - this patch adds them, and incorporates the rest of #29 into #28.
i've also added a config.api.php, which is stubbity stub stub right now.
Comment #32beejeebus CreditAttribution: beejeebus commented
ksenzee looked at the patch and suggested that we need better naming for config_reload_run_hooks().
i've renamed config_reload_run_hooks() to config_invoke_reload_hooks() after some discussion with xjm.
Comment #33ksenzee CreditAttribution: ksenzee commented
I think config_invoke_reload_hooks() is an improvement. I forgot to mention that cweagans is right about disc/disk. Disc is specifically a CD or DVD (or a vertebra in your back).
Comment #34Owen Barton CreditAttribution: Owen Barton commented
I was thinking about this, and wandering if getting "$config_changes, $active_config_tree, $file_config_tree" data structures is going to allow modules to simply and reliably/correctly validate their configuration.
An example that comes to mind is views module - to validate a view it is ideal to check that the various structures that view filters, fields etc refer to actually exist, and are in the configuration that the view is configured to expect. For example, a node field being the expected type, or being configured as single value v.s. multiple value, matching a filter configuration in a view. Obviously, if the configuration it relates to does not match the modules own configuration, we would hope validation would fail.
From views perspective, getting data in a "$config_changes, $active_config_tree, $file_config_tree" means that a field or filter plugin would need to not only know how to validate it's own configuration, but also find and validate (against what it is expecting) the data structures for modules that it relates to (e.g. field modules). It seems like because this operates on the configuration data structure alone, it would need to be a different code path (and hence perhaps significant amounts of custom code) than the existing validation that a field or filter plugin might implement, because the latter would typically work with the APIs the modules they refer to, because they already need to use these APIs for building their actual functionality it makes sense to use them for validation (rather than examining the "raw" configuration objects for the module they use). This could be the "same" module (e.g. flag views adding views support for flag), but it would still need to be examining the flag configuration data structures directly, rather than using flag APIs that and parse and present those data structures in the same way the flag views plugin validation expects.
If this is the case, it seems like if existing validation pathways couldn't be used, then either the volume of validation logic needed would double, or each module would need to manage it's own layer of indirection (such that it's API could be passed a proposed configuration to present back in validation routines, rather than using the active one) or the quality of the validation would be reduced if modules only validated their "own" configuration, but didn't compare that with the configuration of the modules they integrate against what their configuration expects. In the latter case the view might validate, but may result in a broken site, and editing the view would not let it save due to the internal (more correct) validation failing.
I am not sure I have a clear strategy for how to resolve this - an approach that comes to mind is to essentially present the proposed (file) configuration as the active configuration during validation, but without storing it in the active store (in D7 terms this would look like switching $conf in hook_boot, a bit like strongarm etc). This could allow modules existing, API based, validation routines to work without needing to inspect other modules data structures, or each module needing to add their own layer of indirection somehow.
This is probably not sufficient alone, since some modules (e.g. field) need to perform operations on other state (e.g. table schemas) based on the configuration, and being instantiated in the active state only would not be sufficient for them to determine if the proposed change can succeed. Hence I think a 2-step validation operation might be the way to go:
- First, validate that your modules own configuration looks correct, and that any internal state transitions seem like they can be accomplished correctly - this can use the "$config_changes, $active_config_tree, $file_config_tree" approach proposed here.
- If this passes, switch to presenting the proposed configuration as the active one (but without actually updating the active store) and allow modules to trigger existing "higher order" validation functions, such as views plugins validating themselves (using the same logic they would use when saving a view). The only gotcha is that APIs used in this step would need to be careful not to rely on the state transitions being made yet.
- If this passes also, they we are good to load the configuration into the active store.
Comment #35yched CreditAttribution: yched commented
I think that the CONFIG_DEFER_RELOAD flag was proposed during the BADcamp sprint for these kind of cases.
Other example : field_config_reload() having to deal with a new field instance on a node type that gets deployed during the same 'reload'. The node type needs to exist before the field instance can be created.
Following up on the example in #34, if there are things in the list of $config_changes that the view might depend on, then views_config_reload() returns CONFIG_DEFER_RELOAD, so that those changes get handled first - until views_config_reload() gets called with $config_changes containing none of its dependencies, which it can assume have been merged.
That would mean that "accepted" config changes are iterately saved after each "invoke hook_config_reload() pass", instead of "iterate on hook_config_reload() until noone returns CONFIG_DEFER_RELOAD, *then* save all", which is what the current patch does.
[edit: reworded/clarified a bit]
Comment #36Owen Barton CreditAttribution: Owen Barton commented
I am not sure loading changes in 2 stages addresses what I am talking about, which is really just about validation. We want all validation to occur before we start any loading actions (changing table schemas etc).
Comment #37yched CreditAttribution: yched commented
We want all validation to occur before we start any loading actions (changing table schemas etc).
This would make the field API use case mentioned above impossible : a "deploy" that both creates a node type and adds field instances to it is impossible to deploy in one pass.
Comment #38Owen Barton CreditAttribution: Owen Barton commented
Deployment can still be in two (or more) passes - I am only talking about validation, before any deployment starts.
Comment #39agentrickard CreditAttribution: agentrickard commented
Working on the following test case:
* Add a new image style via configuration in image_module_test.
* Check that config reload recognizes the change.
* Check that reload adds the change.
Working in the branch
1447686-reload-image-test in https://drupal.org/node/1145636
Comment #40David Strauss CreditAttribution: David Strauss commented
"Deployment can still be in two (or more) passes - I am only talking about validation, before any deployment starts."
The model we're building for configuration deployment assumes that downstream recipients of configuration (like test/QA/live) get something coherent and valid. The CONFIG_DEFER_RELOAD flag is only for ordering. It just so happens that having CONFIG_DEFER_RELOAD flag fire on the same hooks on two passes indicates that we're not dealing with something valid. It's more like an assert() in code that allows failing fairly fast.
The recommended workflow for testing and validation of configuration will still involve a test or staging environment where the production database and files get synced to test application of updated code and configuration. Implementing a configuration system where configuration can be 100% validated before application is extremely difficult without transactional DDL in the database (not available in MySQL and many other DBs).
"There are two thoughts as far as avoiding race conditions."
This isn't a true race condition; it's more of a conflict. The safest approach is adding a sort of vector clock to the active store so it tracks the timestamp or hash of the file last loaded for a specific part of the configuration. Before syncing config out to disk, the config system can check whether the hash of file matches the hash of the file originally loaded into the active configuration store or that the active store last wrote to disk itself. This will indicate whether the active configuration is "derived" from the one on disk. If the active configuration isn't "derived" from the one on disk, writing the active configuration (possibly with an additional change) back to disk may reverse a change pulled from elsewhere.
That said, solving this well isn't too important. It's always possible in these sorts of complex workflows to just write out the configuration and sanity-check with git to ensure nothing's been reverted.
Comment #41David Strauss CreditAttribution: David Strauss commented
And now, I'll actually check out the code. :-)
Comment #42beejeebus CreditAttribution: beejeebus commented
re #39 - the problem here is that there's already an image_test module at core/modules/simpletest/tests/image_test.module, and this is the one that is being loaded by the test. so we just need to rename to image_config_test or add the functionality to the existing image_test module.
Comment #43David Strauss CreditAttribution: David Strauss commented
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1447686-reload-image-test.diff. Unable to apply patch. See the log in the details link for more information. View
Here's the diff from a fresh update of 8.x to Ken's work. I made this for my review, but I figured I'd post it here.
Comment #45yched CreditAttribution: yched commented
At any rate, I think my remark in #35 still stands : we need to write the "accepted" config after each pass, so some progress is done before the next pass. Otherwise, there is no reason a module that needed to return CONFIG_DEFER_RELOAD the 1st time would return anything different the 2nd time.
Comment #46sun CreditAttribution: sun commented
|Needs work
|» Needs review
I've reviewed the patch in #32 in detail and I think it's pretty much ready to land, except of some minor issues that can be fixed quickly (listed below).
This makes a very good second milestone and gets the basic reload/sync functionality in. We can follow up with converting entity types into configuration, and after that, attacking the massive field problem space.
- disc -> disk
- lock implemmentation code looks a bit odd; check/compare with other lock implementations in core
- config_reload_* function namespaces
- config_reload -> config_sync
- ensure module hooks cannot alter the config changes by reference; save the exact imported config
- drupal_flush_all_caches() after reload
- $old and $new vs. $file and $active
- lock active store during reload hooks, until config system itself writes the config
- test CONFIG_RELOAD_DEFER
- separate issues: installer, convert entity/field tables into config
Comment #47yched CreditAttribution: yched commented
#46 crossposts #45 :-)
Also, re #39: the test case should probably also make sure that the images folder for the style is flushed when the style is updated.
Comment #48beejeebus CreditAttribution: beejeebus commented
re #45 - we don't write any config until the end of all the module reload hooks are called, and no exceptions have been thrown.
that is, it's not module's responsibility to write the changes to the active store, but just to react and do whatever work they need to do.
Comment #49yched CreditAttribution: yched commented
re #48 : I don't see why, and if so, as I wrote in #47, and after re-reading David Strauss's #40, I don't understand why we have CONFIG_DEFER_RELOAD at all.
Comment #50beejeebus CreditAttribution: beejeebus commented
re #40 - i don't agree with this approach, if it means we don't allow modules to reject a given set of config changes before we make changes that we might not be able to back out of.
'it works in test/stage' is not enough, IMO.
Comment #51yched CreditAttribution: yched commented
Validation happened before, when the config is changed on the 'source' site (typically dev), i.e. written TO disk through the UI or API calls. Then ensuring deployment of the config file is consistent is the job of the deployer, not of the 'reload' mechanism IMO. If you do a partial deployment, that's your responsability. Test it on QA first, that's what it's for.
Embarking in two-ways validation sounds like a giant can of worms - and not a minor onus on the many core/contrib subsystems that will use CMI. I hardly see that implemented reliably and consistently across all of contrib.
Again, if nothing changed between pass n and n+1, why would mymodule_config_reload() return 'OK dude' on pass n+1 if it returned CONFIG_DEFER_RELOAD on pass n ?
Comment #52ksenzee CreditAttribution: ksenzee commented
Nothing in the config may have changed, but the state of the system may have changed.
First pass:
- field system sees a new field instance on woozle entity types, tries to create it, sees that the woozle entity does not yet exist, returns CONFIG_DEFER_RELOAD
- entity system sees new woozle entity type, creates it
Second pass:
- field system sees a new field instance on woozle entity types, tries to create it, succeeds
- entity system sees woozle entity type, notices that it already exists, does nothing
Comment #53beejeebus CreditAttribution: beejeebus commented
re #40 and #51 - meh. i'll wait until heyrocker makes a call, then implement it that way.
i don't think there's anything unclear about the positions, and i'm happy to implement stuff i don't agree with. i've been saying the current design is broken for so long i'm kind of over it.
Comment #54yched CreditAttribution: yched commented
@ksenzee: that's my point, what you describe relies on the fact that the «new entity type» change gets applied at some point between pass 1 and 2.
Comment #55ksenzee CreditAttribution: ksenzee commented
Does applying the new entity type require that the entity system alter the config that got passed to it? That's what I thought you were saying in #45. It seems to me that the configuration data has to be unalterable, and that modules may react to it but not try to change it. But I'm entering the discussion late so I may not be understanding correctly.
Comment #56David Strauss CreditAttribution: David Strauss commented
For the record, CONFIG_DEFER_RELOAD is merely an ordering mechanism that happens to indicate a circular dependency (and, thus, a failed application of config) under certain circumstances. If we want to add some level of configuration validation, we could do a first pass through all systems that allows no changes but does allow veto of applying the changes. I don't recommend doing this right now given the complexity of testing dependent configuration between modules. There's little purpose in a validation system that doesn't validate the hard cases.
"Nothing in the config may have changed, but the state of the system may have changed.
First pass:
- field system sees a new field instance on woozle entity types, tries to create it, sees that the woozle entity does not yet exist, returns CONFIG_DEFER_RELOAD
- entity system sees new woozle entity type, creates it
Second pass:
- field system sees a new field instance on woozle entity types, tries to create it, succeeds
- entity system sees woozle entity type, notices that it already exists, does nothing"
This is a good example, but I don't think we should fire the hook for a system that didn't fire CONFIG_DEFER_RELOAD during the previous pass. (In this example, the change would be not calling the entity system at all in the second pass.) After all, a system is welcome to do whatever it wants and return CONFIG_DEFER_RELOAD if it wants to do more later. Maybe call it CONFIG_PLEASE_CALL_AGAIN?
"that's my point, what you describe relies on the fact that the «new entity type» change gets applied at some point between pass 1 and 2."
No, this change does not happen *between* passes 1 and 2. The change happens *during* the entity system's configuration phase in pass 1.
"It seems to me that the configuration data has to be unalterable, and that modules may react to it but not try to change it. But I'm entering the discussion late so I may not be understanding correctly."
That's correct. Running configuration for a system should not alter the n or n+1 configuration values. In fact, the n configuration is only provided to guide application of the n+1 configuration.
Comment #57David Strauss CreditAttribution: David Strauss commented
Specifically, CONFIG_DEFER_RELOAD provides an ordering mechanism that doesn't require actually embedding limited rules for ordering in any explicit system. Modules can decide that the dependencies for applying their own configuration have yet to be met however they want.
Comment #58beejeebus CreditAttribution: beejeebus commented
"This is a good example, but I don't think we should fire the hook for a system that didn't fire CONFIG_DEFER_RELOAD during the previous pass. (In this example, the change would be not calling the entity system at all in the second pass.) After all, a system is welcome to do whatever it wants and return CONFIG_DEFER_RELOAD if it wants to do more later. Maybe call it CONFIG_PLEASE_CALL_AGAIN?"
this is how the code runs now in the patch - only modules that return the constant are called again. i like the improvement to naming CONFIG_DEFER_RELOAD, other suggestions welcome.
"If we want to add some level of configuration validation, we could do a first pass through all systems that allows no changes but does allow veto of applying the changes. I don't recommend doing this right now given the complexity of testing dependent configuration between modules. There's little purpose in a validation system that doesn't validate the hard cases."
the current patch works this way - the validation run allows modules to veto the config set before any changes have been made.
i'm not sure what you're suggesting re validation, but it sounds like a 'suck it and see' approach. 'well, it worked when i clicked around and then committed my config files on my dev site, so it should import on your dev setup' etc. i completely disagree, but meh.
Comment #59David Strauss CreditAttribution: David Strauss commented
"i'm not sure what you're suggesting re validation, but it sounds like a 'suck it and see' approach. 'well, it worked when i clicked around and then committed my config files on my dev site, so it should import on your dev setup' etc. i completely disagree, but meh."
Committing configuration solely evolved by using the UI from an existing, working configuration should always work. If it doesn't, there's a bug in some subsystem's application of configuration.
The only case in which a configuration *should* fail is following manual alteration or merging of incompatible changes. And, frankly, such manual alteration or merging is a situation where *anything* could fail (e.g. if I merge your change of adding a call to function a() with my change of renaming function a() to a_plus_one()).
Many of Drupal's critical core and contrib systems require the sort of speculative update testing I'm recommending here. Specifically, I'm recommending syncing database+files into a test environment populated with candidate code+config for deployment and running "drush updatedb" as well as converging to the on-disk configuration. This is not any additional work versus current best-practice workflows. In fact, it's both less laborious than re-applying many changes by hand and less likely to result in a different config applied to production than was tested.
Comment #60ksenzee CreditAttribution: ksenzee commented
Committing configuration solely evolved by using the UI from an existing, working configuration should always work. If it doesn't, there's a bug in some subsystem's application of configuration.
Lots of things should always work, unless there's a bug. :) We can pretty much be guaranteed that at some point, there will be a bug that makes someone's production site fail when they update their config. (Especially considering that not every site owner handles dev/staging/prod correctly.) How do we want to handle that failure? Do we let modules try to detect it in advance, or not? Do we try to handle it gracefully in some other way?
the current patch works this way - the validation run allows modules to veto the config set before any changes have been made.
This being the case, what is the argument for removing that validation run?
Comment #61beejeebus CreditAttribution: beejeebus commented
"Committing configuration solely evolved by using the UI from an existing, working configuration should always work. If it doesn't, there's a bug in some subsystem's application of configuration.
The only case in which a configuration *should* fail is following manual alteration or merging of incompatible changes. And, frankly, such manual alteration or merging is a situation where *anything* could fail (e.g. if I merge your change of adding a call to function a() with my change of renaming function a() to a_plus_one())."
huh? except where the target system has also had some clicky-clicky changes. but that'll never happen, right?
Comment #62David Strauss CreditAttribution: David Strauss commented
"huh? except where the target system has also had some clicky-clicky changes. but that'll never happen, right?"
Well, then you have configuration to merge back downstream to your test, staging, or dev environment before you deploy. Anything else would require a code+config merge to happen right in the production environment, which I'm fine being risky because it's a dumb thing to do. I also don't appreciate your patronizing tone; you're not going to convince me of anything that way.
I also don't understand why you're talking about this sort of workflow as a novel concept people will mess up all the time. Everything we're doing here is how Features and ctools exportables have always worked. No one I've ever met who's used those tools has expected to merge exportables or Features directly in production.
Comment #63David Strauss CreditAttribution: David Strauss commented
"Lots of things should always work, unless there's a bug. :) We can pretty much be guaranteed that at some point, there will be a bug that makes someone's production site fail when they update their config. (Especially considering that not every site owner handles dev/staging/prod correctly.) How do we want to handle that failure? Do we let modules try to detect it in advance, or not? Do we try to handle it gracefully in some other way?"
I'm okay with modules having some opportunity to validate it in advance, but it's never going to be comprehensive without major changes to all of Drupal core -- and probably to other systems Drupal relies on, like MySQL. Whatever we do, it's going to be so non-comprehensive that I wouldn't even document it as a facility for end users to rely on.
"the current patch works this way - the validation run allows modules to veto the config set before any changes have been made. This being the case, what is the argument for removing that validation run?"
I'm okay with leaving it in; it's a pretty lightweight check at this point. But, the bottom line is that we have no checks like this for hook_update_N(), the old variables system, exportables, or Features. You could make all the same arguments about them. I don't see why a partial validation solution for one of the many systems that could break production is all that valuable.
Comment #64Owen Barton CreditAttribution: Owen Barton commented
There are more scenarios to consider here than just the "features driven workflow" - for example:
- Admin installs Module A provides some default configuration (e.g. a content type with some fields).
- Admin changes some of this configuration, perhaps deletes a field or two.
- Admin goes to the core provided project installer interface and downloads and installs Module B.
- Module B depends on Module A, and provides some configuration that expects what Module A provides.
In this case, if we don't provide a way for Module B to check that the specific configuration it references is in place, then it will likely break in one way or another. Please note that this is a simple non-VCS site managed using the provided core project installer. I don't think it is realistic to say that every Drupal site (including personal sites etc) can _only_ be managed via VCS and a separate staging site, even if this is a best practice, and dropping the project installer would displease quite a few people too :)
Even considering the "features driven workflow" scenario, not having solid validation is one of the things that drives me crazy (in a bad way) about features. You can pull the latest configuration and revert the latest features on a staging site, but there can be issues (for example due to a developer merging things in a funky way) that can generate problems in fairly obscure and easily missed places, increasing testing costs (manual or automated both). Yet a good portion of these issues are picked up by the built in validation routines when submitting the appropriate form.
I think providing the best systems we can to highlight these kind of configuration integrity problems (without resorting to site specific tests in a staging environment) is an extremely valuable use of our collective energy here.
I think it is fine for the "second stage" validation I described to be a separate issue. I am not sure I agree that getting a high level of validation requires transactional DDL - for example, if we run a validation hook with the proposed configuration "active" at some level (but not saved), then it doesn't seem too difficult to write validation routines to avoid relying on the schema being changed yet - this would not be hard in the majority of cases - for example if a views filter validation needs to check it's field exists and has compatible configuration, the core field function it calls would normally just provide the field configuration in the expected format - there is no need for it to actually go and read the table schema. For references that crosses the configuration-entity boundary this approach wouldn't work (although it would fail-safe in many cases), but I think that is a pretty reasonable architectural tradeoff (and I am sure someone will write the exportable entity contrib for sites that need it!).
Comment #66beejeebus CreditAttribution: beejeebus commented
|Needs review
|» Needs work
i'll defer to david on this, i'm not going to fight any more.
Comment #67philippejadin CreditAttribution: philippejadin commented
Playing devil's advocate
Isn't it a bit like reinventing version control ?
Comment #69philippejadin CreditAttribution: philippejadin commented
Just thinking more about this. Why not let the modules using CMI handle this? This is new functionality after all, and this will probably not be handled by CMI alone.
Let me give an example :
- user creates a new content type and attaches two fields to it. This config is stored in xml files on a test server
- user moves the xml files to the production server
- user visits admin ui of the production server
- content type / fields administration ui shows the new available content type, but they are grayed out, because they are available in config, but are not yet created in DB.
- user can click some button to enable the new content type (or use drush specific command, provided by field module)
- the content type module takes care of creating the new content type, based on the config.
- if the configuration changes again (for example, content type is deleted in config but there are some content tables available in DB), the UI would show it as well.
- when the content type is created, user can visit the field UI, and after "add new field" and "add existing field", he sees "enable field found in config, but not yet created in DB", with again the available fields shown in gray. He can enable them from the UI or drush
In this example the ui is provided by the content-type / field module, because only the field module knows how to deal with new and removed configuration.
I think that for complex example like this, it should be handled by the modules themselfes.
In case of views, it's a bit different, because as far as I know, views could be instantiated "on the fly" without needing any information from the DB, they could be created directly from the config provided by CMI. In this case, where no database changes are needed, I would use the proposed solution of flagging configuration, and provide a generic UI to allow user to upgrade existing config.
This may help separate concerns, and let the modules do the complex work of choosing which upgrade to do first, as long as the configuration system provides clean api for that.
This is super exciting by the way :-)
Comment #70ksenzee CreditAttribution: ksenzee commented
I'm not sure the system described in #69 would move us forward. We need to be able to deploy new configuration all at once via a drush command, or we haven't really solved the deployment problem.
I do like heyrocker's idea in #65 of adding a flag to track differences between active and canonical storage. I created a separate issue for it at #1515312: Add snapshots of last loaded config for tracking whether config has changed. Should we postpone further work on this issue until that one is resolved?
Comment #72catch CreditAttribution: catch commented
It seems like this would be easier to solve if UI changes didn't automatically write back to the file store, can someone explain why the current system is doing this, and why we can't drop that to simplify this process?
Comment #73beejeebus CreditAttribution: beejeebus commented
i'm not working on this anymore, and have done nothing on the patch since drupalcon denver, so #43 would be the latest code.
Comment #74ksenzee CreditAttribution: ksenzee commented
catch: I think that's what beejeebus suggested at http://groups.drupal.org/node/190729#comment-636018 as well.
heyrocker, yched and I had a good discussion of this issue in IRC today. Hopefully neither of them will mind if I paste an excerpt (emphasis added):
(12:13:01 PM) heyrocker: I assume the active config would change as modules act on it right?
(12:13:05 PM) heyrocker: Does that not happen right now?
(12:13:18 PM) yched: heyrocker: that's it - no,does not happen
(12:13:24 PM) heyrocker: hmm
(12:13:43 PM) yched: right now, there are a series of passes until no module returns CONFIG_DEFER_RELOAD
(12:13:46 PM) heyrocker: right
(12:13:56 PM) yched: THEN config gets applied to the active store
(12:14:14 PM) heyrocker: Why is that a problem?
(12:14:34 PM) yched: so my pb is : if I needed to return DEFER 1st time around, and nothing changed when I'm called again, I'll just return DEFER again for the same reasons
(12:14:59 PM) ksenzee: When you say "nothing changed"
(12:15:03 PM) heyrocker: well something will have changed somewhere right?
(12:15:08 PM) ksenzee: The config data itself does not change, but the system state does, right?
(12:15:21 PM) yched: heyrocker: not in the active store (not with current patch)
(12:16:02 PM) yched: my hook_config_reload() will call the "usual API" to do its checks (like : does this node type exist, before I can create a field on it)
(12:16:08 PM) heyrocker: So there may be situations where you need to take data from the new config, and make your own secondary config changes and save them somewhere for people down the line to use?
(12:16:12 PM) yched: this "usual API" reads from the active store
To reword my scenario in #52, from what I think is yched's point of view:
First pass:
- field system looks at the new config, sees that there should be a new field instance on woozle entity types, asks the entity system if it has a woozle entity, gets back "no, no woozles here", returns CONFIG_DEFER_RELOAD
- entity system sees new woozle entity type and creates it, but has no way of recording that the new entity has been created because it can't save its config object to the active store.
Second pass:
- field system sees a new field instance on woozle entity types, asks the entity system "do we have woozle entities yet?". The entity system checks the active store (since that's where it keeps its information, right?) and sees no woozle entity because it couldn't save to the active store during the last pass. Field system says "oh, no woozles yet" and returns CONFIG_DEFER_RELOAD, and now we are in an endless loop.
What I was failing to understand in #52 is that the field system intends to use the configuration system as its only means of storing system state. I kind of figured it would have a more reliable method, like checking to see whether the DB tables exist, or something. But of course that makes no sense. If the config system is going to be useful, it has to be useful all the time.
One way forward is what I think several of the comments above imply: use the active store freely, all the time, even during reloads. Change it around as needed. That way the "usual APIs" work during reloads. Don't worry if the active store and the file store end up being different at the end of the reload; just overwrite the file store at the end. This is certainly practical, and I'll happily write a patch to implement it if that's what we need to get moving here. But architecturally something about it bothers me. I'm not sure I can explain it well, but I'll try.
As CMI was first envisioned, there was no concept of an "active store" versus a file store, right? It just stored everything in files. Then the active store was introduced as a performance enhancement, because writing through on every save is too slow. But you could write through at any time without messing anything up. Now we're talking about using the active store for something very different than just caching: we're talking about divorcing it from the file store entirely, at least during reload, so that we can make changes to the active store that are never intended to be written through to the file store - changes that represent partial application of the state from the file store and that would, in fact, corrupt the file store if they were written through in the middle of a reload pass. ("What do you mean, I have woozle entities with no fields on their bundles?? I never configured my site that way.")
So my architectural question is this: if we had encountered this problem before we invented the active store, how would we have solved it? Does it just so happen that the active store is the right solution for both "file I/O is slow" and "I need a mutable version of config during reload"? Or are we making the active store do double duty just because we already have it?
Comment #76sun CreditAttribution: sun commented
I discussed parts of the CONFIG_DEFER_RELOAD detail with @beejeebus in Denver already. My review in #46 contained:
- lock active store during reload hooks, until config system itself writes the config
The reasoning for that was the idea that the active/in-production configuration should be changed exactly to those values that are being imported (fwiw, I'd love to see a more decent terminology in this discussion and patch; "reload" makes no sense at all).
However, the arguments raised for the defer case infinitely deferring totally make sense, too. When a particular config change is deferred, then another config change must be applied to the active/production configuration at some point, or the config change won't ever be not deferred. In turn, this means we want and need to change the active/production configuration for each config change during the reload/import/synchronization.
That said, the reload/import/sync hook implementation is a special, advanced facility for more complex modules relying on configuration. The majority will be simple configuration values (such as the performance/cache setting) and they won't need a reload/import/sync hook implementation. Because of that, the underlying consideration for above mentioned review issue still stands; only the configuration system itself should write the new imported/synced configuration values into the active/production store. The optional reload/import/sync hook only cares for executing dependent or related changes for a particular config value.
Thus, we want to move the writing of each new imported/synced configuration object into the reload/import/sync loop -- instead of performing the write for all config objects/changes at the end.
This should resolve the CONFIG_DEFER_RELOAD issue, since the active/production configuration is changed for each particular config change, and because, by default, config() relies on the active/production configuration. (We might want to enforce the latter in some way, but that can be a follow-up issue.)
I still would not want anything else in the system to perform any configuration value changes in the active/production configuration during a reload/import/sync, because that would inherently mean that there are new differences between the configuration stores introduced while both are being synced. Thus, I'd still like to see the configuration stores being locked for writes during the operation. Only the config system itself must be able to write.
Lastly, the automated write to the file store whenever configuration is saved looked bogus to me since day one. We definitely need to remove that, as it entirely breaks the reload/sync idea in the first place, no matter how you look at it. It was introduced based on the idea that, on a developer site, you'd always want to save all of your config changes to disk, so you can VCS commit and push them to staging or production later on. However, that's the abnormal, special case in the workflow, and instead of doing so, we rather need to introduce a export/write-to-disk operation that complements the import operation (which could be automated again through a special config system flag that you can enable on your local site during development).
Comment #77yched CreditAttribution: yched commented
Thus, we want to move the writing of each new imported/synced configuration object into the reload/import/sync loop -- instead of performing the write for all config objects/changes at the end.
Yes ! That's what I've been advocating since #35 :-)
Also, to clarify :
The whole purpose of hook_config_reload() is to let subsystems perform their "side effects" in reaction to incoming config files changes. . E.g : for Image styles API, clear the image derivative directory for a style that got modified / deleted;
for Field API, create the db tables for a new field, delete values for deleted fields / instances, clear a couple caches, etc...
If your config change has no side effect ("simple configuration values") then you don't need to implement hook_config_reload().
Those actions cannot be easily undone (or are plain destructive). Aiming for "no writes to the active store until the very end of the process when we are fully sure that nothing goes wrong, so that we can bail out at any given time" is delusional. There *will* be side effects outside of the config store anyway.
*If* we want to provide extra safety against importing invalid set of config file changes (i.e "blocked" on unresolved DEFER return values), it needs to happen in a previous step, involving a different hook (or maybe the same hook in some sort of "dry-run" mode ?). Dunno how doable that is, but IMO that can and should be discussed in a followup. hook_config_reload() is the place where stuff actually happens.
Comment #78beejeebus CreditAttribution: beejeebus commented
Comment #79yched CreditAttribution: yched commented
Still re @sun #76 [edit: and re @beejebus #77, that was edited out later on]:
only the configuration system itself should write the new imported/synced configuration values into the active/production store. The optional reload/import/sync hook only cares for executing dependent or related changes for a particular config value.
That was my initial take too. The regular config API is$ $config->save(), and it writes to both stores. The flow in "reload' is different and specific : write into active store some data that comes from the file store. I'd tend to think that this specific flow should be the responsibility of the config system, and not done in hook_cf_reload() implementations :
- hook_cf_reload() implementations just a) return "change accepted" (and do their side effect actions), or b) return DEFER
- the reload loop then takes care of committing the accepted changes to the active store, and calls the next hook implementation (or iterates on the next pass if there were DEFERs)
This being said, ksenzee had an interesting take on that in yesterday's IRC discussion. Basically, have $config objects that are passed to hook_cf_reload() be tainted with some specific '_reloading' flag, that $config->save() interprets as "skip saving to file store".
To explain a little further, here's how we live in a world with CMI (that's mostly a rephrase of the second part of this post-BADcamp comment on g.d.o) :
Imagine some subsystem ('mysubsystem') that provides $thing objects. It provides the
mysubsystem_create($thing) API function, called by the UI and for programmatic creation of "things" by 3rd party.
Like in D7 and earlier, what this does is :
1) perform its own business action ("side effects" : create a table, clear a cache...)
2) save config : in D8 that's $thing->save() - writes to both stores.
Then, when reacting to a new "thing" confg file, mysubsystem_config_reload() cannot call mysubsystem_create() because it only wants to do 1) and not 2) : the "write to the active store" part will be made outside hook_config_reload().
You thus need to duplicate or otherwise factor out the code for 1). Or pass an extra param like :
function mysubsystem_create($thing, $write_to_config = TRUE) {
// Take the business action :
// - Create a db table,
// - Clear some cache...
if ($write_to_config) {
// Save the corresponding config (both active and files)
$config = build_the_corresponding_config_object($thing);
* Implements hook_config_reload().
function field_config_reload() {
if (I figure out a new $thing needs to be created) {
mysubsystem_create($thing, FALSE);
That's painful for each subsystem in core and contrib to figure out and re-implement correctly on their own.
With ksenzee's proposal, mysubsystem_create($thing) simply works within mysubsystem_config_reload() because $thing->save() is smart and saves to the stores that are relevant to the flow we're dealing with (regular "UI / API to both stores", or "sync/reload files to active store").
But: this means that mysubsystem_create($thing) takes $thing as a Config object (or subclass), which AFAIK is a slight shift from the current directions CMI was taking so far.
Comment #81Crell CreditAttribution: Crell commented
Having more data available can certainly make more things possible. My concern is 1) Complexity (since we have yet another dataset to pay attention to) and 2) Size. I know these files are not necessarily going to be big, but once you start talking about, say, every user on d.o having their own preferences plus their own dashboard layout, you run into a LOT of data to be snaoshotting.
Comment #82cweagans CreditAttribution: cweagans commented
True, but one could argue that user preferences/dashboard layout settings are simply overrides of some default configuration set, and only the defaults need to have detailed snapshots. All of the user preferences/dashboard settings, IMO, should be treated like content on a site (i.e., you don't want to drag that data around to any place where you set up a dev environment, for example)
Comment #83yched CreditAttribution: yched commented
Keeping a separate store for "the in-reloading / last reloaded" state does sounds like a valuable thing to have, notably for contrib enrichments/control on the config flow.
as the reload process goes along, we could write only to the 'last reloaded' config area, which can be used as our working area as it were during the reload process. When reload is successfully complete, we copy those changes into the active store. A huge upside of this is that if the reload process fatals, we still have the active store clean and pure and can recover from the failed reload with no extra effort
Still not sure about that. As I wrote earlier, hook_config_reload()'s main purpose is to perform 'state' changes (db / filesystem). So even if we defer the actual writing to the active store until the very end of the reload process, all the hook_config_reload() implementations will still have run meanwhile, which makes the in-between state kind of awkward - active store not being consistent with the 'state' of the site.
- What happens for config reads that happen in concurrent requests ? They read from the unchanged, not-in-sync with actual state, active store ? OK, I guess we can state that "if you don't put your site offline before starting a conf reload you know what you're doing".
- More importantly, what kind of API calls are we allowed to make within hook_config_reload() ? If I use API funcs that fetch on CMI content ("does this node type exist ?", "does this field/instance exist ?"...) , they will read the active store, that does not reflect the config changes that have been "committed" yet.
I still think config changes should be written to the active store as soon as they're accepted, and we want to reduce the time window where site state is not in sync with the active store. Once the reload process starts, there's no real going back, failure conditions need to be detected beforehands. And yes, having a snapshot of the last reload would probably help detect them.
Comment #85yched CreditAttribution: yched commented
we can make the API such that you get to choose where you read from depending on your needs : files, active store, last loaded
I don't see us adding an extra $config_source param to each API func that reads from CMI, so I assume you mean that config($config_name) automatically reads from the right source depending on some internal logic / global state ?
I can't say I feel very comfortable with the idea. Sounds like the kind of black magic that could lead to debug hell.
Also, this would mean that no-one can cache above config(), since the function might read from different sources without the caller knowing it ?
In short, for now I still go by #83 : 'last loaded' is a useful piece of info to have, but during reload, best thing AFAICT is to write changes to active on the fly.
Comment #86David Strauss CreditAttribution: David Strauss commented
The size concern is reasonable, and we can technically achieve the basic safety level by merely keeping a hash of the last-loaded/last-exported config. The key is being able to detect divergence of either the active or on-disk config from the last point of common ancestry (whether that was load time or export time).
If we can detect that, we can wave our hands and say, "Hey, this changed on both sides. I hope you know what you're doing." Or, for old SimCity players, "YOU CAN'T CUT BACK ON CONFIG REVIEW! YOU WILL REGRET THIS!"
Storing the full common-ancestor configuration is a sort of way to wink at contrib and encourage development of neat tools for comparing and merging configurations. It's possible such a contrib module could even farm out delicate merges to the modules actually owning that part of the config.
Comment #87sun CreditAttribution: sun commented
|Implement a mechanism for configuration to be reloaded when files are updated
|» Allow to import and synchronize configuration when files are updated
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 35,028 pass(es). View
- Better title. "reload", DIE.
- Performed a range of clean-ups and fixes based on #46, based on @beejeebus' cmi branch from back then.
- Still misses a lock on all storages so that no module can write to any storage during the import/sync operation. However, we can do that in a follow-up issue (which I thought existed already, but unable to find it).
- Did not do anything about the more complex CONFIG_DEFER_SYNC case or additional backup for later 3-way merges. Frankly, I think both of them should be separate follow-up issues, instead of blocking this initial patch.
Code of attached patch lives in the config-import-1447686-sun branch of cmi (based on @beejeebus' original reload branch, but rebased against HEAD, since the history of that was a bit messy).
Comment #88sun CreditAttribution: sun commented
And it even passed tests :)
@beejeebus did this in the original patch and I left it unchanged:
+++ b/core/lib/Drupal/Core/Config/DrupalConfigFile.php
@@ -0,0 +1,151 @@
+class DrupalConfigFile implements DrupalConfigVerifiedStorageInterface {
DrupalConfigFile is actually 80% similar to the current SignedFile class. But instead of being a completely custom thing, it acts as an alternative storage driver next to the database driver.
I have always argued for making the file storage just an alternative driver, which is in no way more special than the SQL driver. Doing this pro-actively prevents us from introducing capabilities to one storage, which are not supported or impossible to implement in another.
So in turn, the Configuration API boils down to a subsystem that is capable of working with multiple storage engines to CRUD + List configuration. In turn, the operation we're discussing in this issue boils down to "synchronizing key/values between 2-N storages."
The re-introduction of $conf overrides in #1484690: Implement $conf overrides in the configuration system inherently bumped the number of storage engines being synchronized from 2 to 3, because $conf is nothing else than an in-memory storage based on hard-coded values (write-locked by design).
Due to that, we started to discuss #1202336: Add a key/value store API, because there isn't and shouldn't be anything special about the actual data being synchronized between the stores. And regardless of the outcome of that issue, I think that is the direction we need to take with the config system as well as import/sync operation: An abstract controller that internally handles one to many storages and is able to synchronize records between them. Continuing the refactoring in this direction should be a separate issue though.
Back to DrupalConfigFile, #1444620: Remove file signing from configuration system performs a similar operation, but renames the class name to FileStorage instead, which sounds more appropriate. We likely want to get that in first and then merge this DrupalConfigFile here into FileStorage.
Comment #89sun CreditAttribution: sun commented
#1444620: Remove file signing from configuration system landed. After further inspection of the current code vs. intended changes here, I'd suggest to do #1567812: Remove "Verified" from configuration class names first (patch already looks RTBC to me). I'll re-roll and adjust this patch here accordingly once that one landed.
Comment #90yched CreditAttribution: yched commented
Thanks @sun.
My only worry with handling CONFIG_DEFER_SYNC in a followup is that we have a discussion of like 50 comments about this right above. I'm not really looking forward to start again in a new issue :-/
Comment #91sun CreditAttribution: sun commented
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
Rebased, rerolled, reloaded for #1567812: Remove "Verified" from configuration class names
Interdiff to #87: http://drupalcode.org/sandbox/heyrocker/1145636.git/commit/f537ce7
@yched: Also happy to continue the advanced import/sync operation here - as long as we can commit the basic changes at some point soonish. It unblocks work on the import UI and other things being introduced, which can totally happen in parallel.
Comment #93sun CreditAttribution: sun commented
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 36,595 pass(es). View
d'oh, sorry.
Comment #94sun CreditAttribution: sun commented
I realize that the removal of DrupalConfigTree and DrupalConfigFile and replacement with a built-in DrupalConfig::load() method (as well as StorageInterface::setName()) is a big change compared to the earlier patches in this issue. The new code doesn't look perfect to me. Especially the synchronization methods (writeToActive() etc.) need to be removed from StorageInterface later on. But I believe the new code goes at least in the right direction, as outlined in #88 and #1560060: [meta] Configuration system roadmap and architecture clean-up.
Comment #95sun CreditAttribution: sun commented
Resolved merge conflicts against HEAD.
Comment #96sun CreditAttribution: sun commented
FAILED: [[SimpleTest]]: [MySQL] 36,680 pass(es), 6 fail(s), and 5 exception(s). View
Re-rolled, merged, and resolved against HEAD.
Comment #98sun CreditAttribution: sun commented
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 36,694 pass(es). View
I've rewritten the newly introduced test case entirely.
This looks RTBC to me. We still want to continue the discussion on the advanced synchronization/defer cases, but committing this now will allow all of us to see facts + a much better picture.
Comment #99aspilicious CreditAttribution: aspilicious commented
I really don't like all these documentation todo's...
I hope someone cleans those files when #1560060: [meta] Configuration system roadmap and architecture clean-up is done or almost done.
Comment #100beejeebus CreditAttribution: beejeebus commented
|Needs review
|» Reviewed & tested by the community
i think this is ready to fly.
Comment #101catch CreditAttribution: catch commented
|Reviewed & tested by the community
|» Needs work
'') {
+ * (optional) A storage driver class name to use for the DrupalConfig object.
+ * Defaults to DrupalVerifiedStorageSQL.
+ $storage_class = (isset($conf['config_default_storage']) ? $conf['config_default_storage'] : 'Drupal\Core\Config\DatabaseStorage');
+ }
+ return new DrupalConfig(new $storage_class($name));
Not the same class name. Also if we do this I think it should use a dedicated global like the databases array rather than $conf. $conf has only ever been useful for things that could possibly be in either settings.php or the database (although we've ignored that for things like cache backends, but with variable_get() going away we should clean it up again).
Also while this adds pluggability for the active store, it removes the ability to specify a custom configuration class itself. I never understood why we had the latter when we didn't have the former, but just dropping one feature and adding the other needs a lot of discussion that's not happened here. There's also been no discussion about the implications of allowing callers to specify their own storage class. I know merlinofchaos has floated that idea to allow Views to store it's configuration in normalized db tables (which may or may not be a requirement), but at the moment I'm really not keen on allow people to do things like hard code their configuration calls to the db store (or any other configuration backend) since that undermines making it pluggable.
The main reason for this appears to be making it easier to get an instance of things from the file store, that feels more like something we should keep as internal as possible to the config API (like an obscurely named function we don't tell anyone about? I don't really have good ideas on that bit).
+++ b/core/includes/config.incundefined
@@ -83,6 +91,201 @@ function config_get_storage_names_with_prefix($prefix = '') {
+ do {
+ if (!$lock_acquired = lock_acquire(__FUNCTION__, 5)) {
+ lock_wait(__FUNCTION__, 5);
+ continue;
Why 5 lock attempts at 5 seconds each? lock_wait() already returns as soon as it can. Just lock_wait() is fine here then there's no need for do while.
+++ b/core/includes/config.incundefined
@@ -83,6 +91,201 @@ function config_get_storage_names_with_prefix($prefix = '') {
+ $target_storage = config(NULL, 'Drupal\Core\Config\DatabaseStorage');
This goes against making the storage class pluggable. If I have something else specified in $conf, then it won't get synced to? Also this goes back to allowing people to specify the storage class. If I can unilaterally call config('foo', 'MyCustomConfigStore') then there is no way for the system to have any idea that this is an active store that needs to be synced to.
+++ b/core/includes/config.incundefined
@@ -83,6 +91,201 @@ function config_get_storage_names_with_prefix($prefix = '') {
+ // Get all module data so we can find find weights and sort.
+ $module_data = system_rebuild_module_data();
+ $sorted_modules = array();
+ foreach ($modules as $module) {
+ $sorted_modules[$module] = $module_data[$module]->sort;
+ }
+ arsort($sorted_modules);
+ return array_keys($sorted_modules);
module_list() is already sorted, should not be necessary to scan the filesystem etc. Also this introduces a direct dependency of config.inc on system module. module_list() would be an indirect dependency but that's at least a step better.
+++ b/core/includes/config.incundefined
@@ -83,6 +91,201 @@ function config_get_storage_names_with_prefix($prefix = '') {
+ // We allow modules to signal that they would like to be rerun after all
+ // other modules by returning CONFIG_DEFER_SYNC. Loop until there are no
+ // modules left that indicate they would like to be rerun, checking that we're
+ // not stuck rerunning the same list of modules over and over at each cycle.
The config defer stuff feels like it's something that should be centrally handled by the module system. i.e. there are other use cases for "please let me run my hook again a bit later", and we could potentially use that to replace hook_module_implements_alter() one day. I'm not necessarily opposed to committing this baked into config to unblock this issue, although I need to think about that a bit more, but can we open a follow-up to discuss factoring it out at least? There too much stuff in here that knows too much about the module system.
+++ b/core/modules/config/config.admin.incundefined
@@ -0,0 +1,58 @@
+ * Form constructor for configuration import form.
+ *
+ * @see config_admin_import_form_submit()
+ */
What does this form look like?
+++ b/core/modules/config/config.api.phpundefined
@@ -0,0 +1,35 @@
+ // TODO: working example.
Let's fill in the @todos, would make the patch easier to review.
+++ b/core/modules/config/config.moduleundefined
@@ -1 +1,34 @@
+function config_permission() {
Does this really need it's own permission? Feels like administer site configuration is probably enough.
+++ b/core/modules/config/config_test/config_test.infoundefined
--- /dev/null
+++ b/core/modules/config/config_test/config_test.moduleundefined
+++ b/core/modules/config/config_test/config_test.moduleundefined
+++ b/core/modules/config/config_test/config_test.moduleundefined
@@ -0,0 +1,36 @@
@@ -0,0 +1,36 @@
I don't see any tests for the defer part of this, but that's also one of the trickiest parts of the patch.
+++ b/core/modules/image/image.moduleundefined
@@ -495,6 +495,46 @@ function image_path_flush($path) {
+function image_config_sync($config_changes, $target_storage, $source_storage) {
+ foreach ($config_changes['new'] as $file_name) {
+ if (strpos($file_name, 'image.style.') === 0) {
+ $style = $source_storage->load($file_name)->get();
+ $style['is_new'] = TRUE;
+ module_invoke_all('image_style_save', $style);
+ image_style_flush($style);
Those image_style_flush() calls are adding a tonne of cache clears, during an operation that's about to call drupal_flush_all_caches(), this and the deletion issue mentioned earlier could do with their own dedicated follow-up issues.
Comment #102sun CreditAttribution: sun commented
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 36,685 pass(es). View
- It is not the intention of this patch and generally out of the scope of this issue to introduce a better pluggability for config() / DrupalConfig / storage engine. Therefore, I've removed the addition of the $conf-based default value, and restored the original $config_class argument that allows to override DrupalConfig. Consequently, the function signature and also usage of config() looks very odd now, but I'm perfectly fine with that as a temporary state. I'm highly confident that we'll be able to clean this up after doing further architectural revamps.
- You're totally right on the lock code. I've redone that, using the variable_initialize() as inspiration.
- see 1)
- I've clarified the sort by dependencies in comments, and removed it from the other places where it wasn't really necessary. It's perfectly possible that this entire functionality belongs into config.module instead. If possible, I'd like to defer that decision to later.
- Created #1603656: Incorporate config "defer" algorithm into module hook system? for that discussion.
- The form is very basic, only gets the job done. Working on a nice UI is part of the reason for why I'd like to push this initial patch through.
Screenshot: (and Stark doesn't make it prettier ;))
- Added API docs.
- Yes, I think a separate permission makes sense and is needed. The import/sync operation will typically be performed by a different user role on a site. The purpose of the import/sync operation is to identify configuration differences, so you naturally want to allow others to change the site configuration, but not to perform a synchronization. But anyway, that's a minor detail for this patch, which will be re-evaluated for the Config UI work.
- Yes, the defer part is what generally needs more live data, experience, and discussion. The idea is to defer the "defer" part (SCNR ;)) to the point at which we're actually able to test it, which means at least entity, node, and field modules being converted to the config system, whereas most of those "typed" conversions depend on #1552396: Convert vocabularies into configuration. Thus, no tests for defer here.
- image_style_flush() mainly removes existing derivative images for a style. It's true that it also flushes two caches, and you're probably right that those should be moved into the appropriate form submit handlers or whatever, but I don't really see how that is related to this issue/patch.
Comment #103sun CreditAttribution: sun commented
PASSED: [[SimpleTest]]: [MySQL] 36,696 pass(es). View
@beejeebus doesn't want config_sync() to be re-entrant with the new lock code, so I've removed that.
Comment #104pounard CreditAttribution: pounard commented
const CONFIG_DEFER_SYNC = 'CONFIG_DEFER_SYNC'; Why don't you use integers as value for such class constants? Seems both more natural and more efficient (and is a good practice too).
function config_get_changes_from_disk() is an awkward function name, config down layer is a collection of physical files in a filesystem, yes, but whether or not it's a disk under the FS is no matter to us. Maybe something such as
config_get_changes_from_files() would be a better name?
Comment #105sun CreditAttribution: sun commented
PASSED: [[SimpleTest]]: [MySQL] 36,694 pass(es). View
Fixed the config_sync_get_changes() function name and added docs for CONFIG_DEFER_SYNC.
Again, please note that the entire "defer" part will be discussed further, re-evaluated, and very potentially changed entirely. The current goal is to get this patch in, so we at least have the basic import/sync functionality and are able to advance on that.
Comment #106beejeebus CreditAttribution: beejeebus commented
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1447686-106-config-sync.patch. Unable to apply patch. See the log in the details link for more information. View
discussed this a bunch with sun and catch, and we decided we should rip out the defer stuff to make this patch simpler, then tackle the defer/dependency stuff in a follow up issue.
so, attached patch takes out the CONFIG_DEFER stuff.
Comment #108sun CreditAttribution: sun commented
|» sun
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 36,696 pass(es). View
#106 also removed the actual hook_config_sync() invocation ;) (and I'm glad the tests at least cover that already :))
Furthermore, we also just discussed to hardcode the special $target_storage and $source_storage uses instead of trying to introduce a not really thought-through false-pluggability on config(). Making the whole thing actually and really pluggable is a very different and much larger discussion.
Comment #109beejeebus CreditAttribution: beejeebus commented
|Needs review
|» Reviewed & tested by the community
ok, this one really does look good to go.
i've created #1605460: Implement dependencies and defer process for importing configuration as a follow up to revisit the config defer stuff.
Comment #110xjm CreditAttribution: xjm commented
Comment #111cweagans CreditAttribution: cweagans commented
|Allow to import and synchronize configuration when files are updated
|» Allow importing and synchronizing configuration when files are updated
Sorry, this was bugging me.
Comment #112catch CreditAttribution: catch commented
|Reviewed & tested by the community
|» Needs work
Sorry folks CNW again, although less than last time.
+++ b/core/includes/config.incundefined
@@ -86,3 +88,154 @@ function config_get_storage_names_with_prefix($prefix = '') {
+ if (!lock_acquire(__FUNCTION__)) {
+ // Another request is synchronizing configuration. Wait for it to complete,
+ // then return a negative result for UI purposes. We do not make a
+ // difference between an actual synchronization error and a failed lock,
+ // because a concurrent request synchronizing configuration is an edge-case
+ // in the first place and would mean that more than one developer or
+ // site builder attempts to do it without coordinating with others.
+ lock_wait(__FUNCTION__);
+ return FALSE;
+ }
Sorry now this comment confuses me as well. If we know we're going to return FALSE if a lock isn't acquired, then why bother waiting until the other process finishes before returning? i.e. can't we immediately return FALSE here? Is the idea to wait until it's done so the UI shows the change? If so, it feels like we could do that in a submit handler or something instead of the API function.
+++ b/core/includes/config.incundefined
@@ -86,3 +88,154 @@ function config_get_storage_names_with_prefix($prefix = '') {
+ // @todo Lock writes to all storages to prevent other/unintended config
Nitpick, but I don't think storages is a word.
+++ b/core/includes/config.incundefined
@@ -86,3 +88,154 @@ function config_get_storage_names_with_prefix($prefix = '') {
+ // Allow all modules to deny configuration changes.
+ foreach (module_implements('config_sync_validate') as $module) {
+ $config_changes = $config_changes_copy;
+ $function = $module . '_config_sync_validate';
+ $function($config_changes, $target_storage, $source_storage);
+ }
+ // Allow modules to react to the configuration changes.
+ foreach (module_implements('config_sync') as $module) {
+ $config_changes = $config_changes_copy;
+ $function = $module . '_config_sync';
+ $function($config_changes, $target_storage, $source_storage);
+ }
we can't just use module_invoke_all() here?
+++ b/core/includes/config.incundefined
@@ -86,3 +88,154 @@ function config_get_storage_names_with_prefix($prefix = '') {
+ }
+ catch (ConfigException $e) {
+ // Just keep going, because we need to allow all modules to react even if
+ // some of them are behaving badly.
+ }
Shouldn't this log or something? Why fail silently?
+++ b/core/includes/config.incundefined
@@ -86,3 +88,154 @@ function config_get_storage_names_with_prefix($prefix = '') {
+ if ($active_config->get() != $file_config->get()) {
+ $config_changes['changed'][] = $name;
Is it OK for this to not be a strict comparison?
+++ b/core/lib/Drupal/Core/Config/FileStorage.phpundefined
@@ -120,8 +121,8 @@ class FileStorage {
public function delete() {
- // Needs error handling and etc.
- @drupal_unlink($this->getFilePath());
+ // @todo Error handling.
+ return @drupal_unlink($this->getFilePath());
Just a note there's an existing issue for allowing try/catch instead of @ #1247666: Replace @function calls with try/catch ErrorException blocks.
+++ b/core/modules/image/image.moduleundefined
@@ -495,6 +495,46 @@ function image_path_flush($path) {
+ * Implements hook_config_sync().
+ */
+function image_config_sync($config_changes, $target_storage, $source_storage) {
+ foreach ($config_changes['new'] as $file_name) {
+ if (strpos($file_name, 'image.style.') === 0) {
+ $style = $source_storage->load($file_name)->get();
+ $style['is_new'] = TRUE;
+ module_invoke_all('image_style_save', $style);
+ image_style_flush($style);
OK I didn't really spot this in earlier reviews, but comparing this with the vocabulary patch it's starting to feel wrong.
This patch means there's going to be three ways for the configuration to be changed:
1. Saving a form.
2. Calling config() API functions directly.
3. The reload operation introduced here.
Given there's three possible code paths, is all the code in this function really specific to just this one? I'm assuming not and it was mostly copied from elsewhere.
As importantly, won't directly saving configuration mean none of this stuff gets fired? Am I responsible for all of this myself if I do that? This seems like it's going to be an issue for any configuration that's not a straight port from variables. This patch doesn't need to cover all that, but it feels like we're adding to a pre-existing problem here, and the vocabulary issue is the only places it's really being discussed.
+++ b/core/modules/image/image.moduleundefined
@@ -495,6 +495,46 @@ function image_path_flush($path) {
+ // @todo image_style_delete() supports the notion of a "replacement style"
+ // to be used by other modules instead of the deleted style. Good idea.
+ // But squeezing that into a "delete" operation is the worst idea ever.
+ // Regardless of Image module insanity, add a 'replaced' stack to
+ // config_sync()? And how can that work? If an 'old_ID' key would be a
Can we open an explicit follow-up issue for this?
Comment #113sun CreditAttribution: sun commented
FAILED: [[SimpleTest]]: [MySQL] 36,719 pass(es), 0 fail(s), and 18 exception(s). View
+ if ($active_config->get() != $file_config->get()) {
+ $config_changes['changed'][] = $name;
Is it OK for this to not be a strict comparison?
It actually has to be a loose comparison, as long as #1608912: (Re-)adding keys to configuration breaks the intended goal of being able to diff files is not resolved.
- I'm not able to make sense of #1247666: Replace @function calls with try/catch ErrorException blocks right now, so leaving the error-suppression in here.
- We'll have to make decision now:
A) Either we're pushed back to the drawing board from scratch in order to investigate the picture-perfect solution for hook_config_sync() implementations, as well as MODULE_THING_save($config) API functions, as well as probably the entire CONFIG_DEFER_SYNC stuff.
B) Or we move on with this current result and continue to explore a better/ideal design in the real world.
Personal note:
I'm slowly but certainly running out of steam with the config system completion/clean-up/re-arch patches. For one, because we're doing way too granular reviews on things that will most likely change anyway later on. Second, because almost all of the issues/patches step on each other's toes, requiring the same or similar base changes. We've defined a roadmap to decrease the chance for this to happen, and at the same time, attempt to steer config system contributors to issues that need to be resolved first to unblock other work.
In general, I already considered whether it wouldn't be better to roll back the complete thing entirely and do all of the re-design and development work in a sandbox instead. However, that would prevent us from moving forward with simpler things in parallel (such as converting variables into config), has organizational issues (such as a missing testbot in sandboxes), and lastly, it seems we weren't able to properly identify all problem areas in the design work when the config system lived in a sandbox branch in the first place, so it seems like re-designing the whole thing within Drupal core's actual HEAD is the only sane approach.
Finally, I also have the impression that most core contributors are not really able to "see" and understand where the config system is heading, without actually seeing it in action on their local development sites. Since all of these changes pretty much affect all modules and everything that exists for Drupal, it would make hella sense to have everyone on board and being involved in the developments (or at least discussions on particular/focused details of the rather large problem space).
Thus, I only see us succeeding with option B), but which really means to loosen the strictness of reviews for config system changes, shortening the delays between RTBC and commits, and rather plan for a dedicated and coordinated config system clean-up effort after feature freeze, at which point we can still clean up (UI) locks, exception logging, return values, phpDoc, comments, and whatever else to make the API and UI rock solid.
[Also, most probably I should totally be in Barcelona in order to hash out most of these things in person with others, but I totally fail in organizing such things :-/]
- Created #1609418: Configuration objects are not unique (and added to the roadmap)
Comment #114sun CreditAttribution: sun commented
|Needs work
|» Needs review
Comment #116catch CreditAttribution: catch commented
|Needs work
|» Needs review
#1 OK that's fine, and #1530652: Switch from == to === to prevent mistakes and make brute force password attacks harder isn't in yet which is trying to change it across the board.
#2. yeah also fine, just pointing out the @todo already has an open issue.
A) Either we're pushed back to the drawing board from scratch in order to investigate the picture-perfect solution for hook_config_sync() implementations, as well as MODULE_THING_save($config) API functions, as well as probably the entire CONFIG_DEFER_SYNC stuff.
B) Or we move on with this current result and continue to explore a better/ideal design in the real world.
Well there's option #3 which is open a specific (IMO critical) issue to discuss it, since there is already image styles with hooks reacting to config changes that feels like enough of a real example, it's just a shame no-one spotted this issue when the initial CMI patch went in.
Also I think you missed this line:
This patch doesn't need to cover all that, but it feels like we're adding to a pre-existing problem here, and the vocabulary issue is the only places it's really being discussed.
Since there's not a general issue to discuss that stuff, nor a specific plan to deal with all of it, I need to either ask it in here, or ignore it completely.
For one, because we're doing way too granular reviews on things that will most likely change anyway later on.
I don't think either the defer stuff or the hook invocation issue is remotely too granular. The hook invocation issue is a pre-existing problem with converting a whole swath of stuff that people want to convert, whereas this issue is adding a feature (albeit one that's critical to the config system doing what it wants). - The fact this patch makes that issue slightly worse is a good sign that it needs to be dealt with sooner rather than later. I can live with the code duplication and multiple slightly different code paths now, as long as we've got a high priority follow-up to untangle it. Opened #1609760: hook_image_style_*() is not invoked for image styles upon Image module installation.
Comment #117sun CreditAttribution: sun commented
Sorry, forgot the other hook arguments in the last patch.
Comment #118sun CreditAttribution: sun commented
Comment #119sun CreditAttribution: sun commented
|Needs review
|» Reviewed & tested by the community
re: @catch/#116: 3)
Yeah, please don't get me wrong. I absolutely love in particular your reviews, as you happen to spot architectural issues/problems (instead of nitpicks) that either no one thought of, or everyone collectively ignored. ;)
The particular topic of API functions vs. hook invocations vs. hook_config_sync() was raised by @yched earlier on already. I think it is closely related to #1605460: Implement dependencies and defer process for importing configuration, but in fact, not the same, as that one should focus on the CONFIG_SYNC_DEFER problem space, which we'll need to sort out for at least Field API to work correctly. Thus, I've created #1614274: Code in hook_config_sync() implementations duplicates module API functions to resolve that particular topic.
Therefore, tentatively setting status back to RTBC.
Comment #120chx CreditAttribution: chx commented
|Reviewed & tested by the community
|» Needs review
I would like to review this before it gets in.
Edit: and by this I do not mean "defer indefinitely" I mean I am reading this and I need a little time.
Comment #121alexpott CreditAttribution: alexpott commented
|Needs review
|» Needs work
+++ b/core/includes/config.inc
@@ -86,3 +88,148 @@ function config_get_storage_names_with_prefix($prefix = '') {
+ $target_config = config($name);
+ $source_config = new DrupalConfig(new FileStorage($name));
+ $target_config->setData($source_config->get());
+ $target_config->save();
This code will rewrite the file you are synchronising. Which firstly is wasting time... we're reading it in and then rewriting with exactly the same values and secondly potentially confusing as on import a file's timestamps will indicate that it's changed.
That's said fixing this will require a lot architectural rework that can be done elsewhere so I suggest we add a @todo before the $target_config->save() to note that we are writing to both the file and the database and should only be writing to the db.
Comment #122alexpott CreditAttribution: alexpott commented
|Needs work
|» Needs review
Did not mean to change the status - sorry...
Comment #123beejeebus CreditAttribution: beejeebus commented
#121 - yes, yes it will. yes, yes it is ... wasteful. and introduces nasty, nasty races. and other stuff i don't want to raise again.
i'd like to see us import from a different directory than the one we write to, but i don't want to start that discussion now, but rather in follow up issues.
Comment #124bleen CreditAttribution: bleen commented
A couple of minor grammatical errors:
+++ b/core/includes/config.incundefined
@@ -86,3 +88,148 @@ function config_get_storage_names_with_prefix($prefix = '') {
+ // Another request is synchronizing configuration.
+ // Return a negative result for UI purposes. We do not make a difference
+ // between an actual synchronization error and a failed lock, because a
+ // concurrent request synchronizing configuration is an edge-case in the
+ // first place and would mean that more than one developer or site builder
// Another request is synchronizing configuration.
// Return a negative result for UI purposes. We do not differentiate between
// an actual synchronization error and a failed lock because concurrent
// synchronizations are an edge-case happening only when multiple developers
// or site builders attempt to do it without coordinating.
+++ b/core/includes/config.incundefined
@@ -86,3 +88,148 @@ function config_get_storage_names_with_prefix($prefix = '') {
+ // Do not trigger subsequent synchronization operations if there are no
+ // changes in either category.
// Do not trigger subsequent synchronization operations if there are no
// changes in any category.
Comment #125beejeebus CreditAttribution: beejeebus commented
discussed the patch with chx in IRC.
he was concerned about about special hooks during sync, instead suggesting that we add hooks as part of the config object, so that the same code runs during a ->save() as during a sync.
here's a gist i wrote during the discussion: https://gist.github.com/2861942
and one from chx: https://gist.github.com/2861969
not changing the status or writing any code until we get more feedback.
Comment #126chx CreditAttribution: chx commented
This is completely in line with what I posted to the meta issue about validation: http://drupal.org/node/1560060#comment-6073876
Comment #127catch CreditAttribution: catch commented
I've fixed the issue title for #1609760: hook_image_style_*() is not invoked for image styles upon Image module installation to make it more obvious what the problem is, and marked #1614274: Code in hook_config_sync() implementations duplicates module API functions as a duplicate of it.
#1609760: hook_image_style_*() is not invoked for image styles upon Image module installation is starting to feel like an absolute blocker to conversion of non-variables to the configuration system, and possibly this issue since it's adding a third code path when we already have divergence between the the two that are in core. I'd at least want us to have more discussion on that issue about how to tackle this before committing this one, currently it's only me and sun on there.
Comment #128chx CreditAttribution: chx commented
Filed a patch to the hook invocation issue with the new hook family.
Comment #129sun CreditAttribution: sun commented
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config.import.129.patch. Unable to apply patch. See the log in the details link for more information. View
The only possible conclusion that can be drawn from #1609760: hook_image_style_*() is not invoked for image styles upon Image module installation is this.
The patch will not apply against HEAD, but the change in direction needs review nevertheless (see interdiff).
EDIT: To clarify:
This change turns hook_config_sync() into "change dispatcher" instead.
- hook_config_sync() remains to be optional.
- It allows a module to handle a particular configuration change on its own, using its own API functions, and possibly also higher-level configuration data object wrappers (e.g.,
$view = new View($new_config);).
- If the hook implementation returns TRUE, then the config sync operation assumes that the module handled the configuration change and removes it from its stack of changes.
- Any remaining changes that were not handled by a module are synchronized by the config system itself via direct read/write operations through the involved storages.
Comment #131sun CreditAttribution: sun commented
|Needs work
|» Needs review
FAILED: [[SimpleTest]]: [MySQL] 36,836 pass(es), 1 fail(s), and 0 exception(s). View
Merged HEAD, adjusted docs.
Comment #132chx CreditAttribution: chx commented
Thanks for the hard work. IMO once #1605324: Configuration system cleanup and rearchitecture goes in we can do a lot better and have proper config objects that aware whether themselves are saved or not and act accordingly. We can fire a hook before save and if any config object gets saved then that's that. The objects themselves can avoid saving twice, it will become an internal business to optimize out double saves. Or we can keep the $module_config_sync if that's what people want but still, we will call ->save() on every config object and the config object being an object can decide whether it needs saving or not.
Edit: To clarify, add a $tainted property to the ConfigObject class, flip to TRUE in set(), FALSE in save() and only do the storage write if it's TRUE.
Comment #133chx CreditAttribution: chx commented
However. That's just my idea. I have been overruled before in one of the issues entangled with this. That's how it is and I shouldn't fight this hard either. Sorry if I did.
Edit: note I did not CNW or postponed this issue.
Comment #135sun CreditAttribution: sun commented
|Needs work
|» Postponed
Postponing on #1605324: Configuration system cleanup and rearchitecture -- which is sorta RTBC and will hopefully land in the next hours.
Comment #136sun CreditAttribution: sun commented
Now that #1605324: Configuration system cleanup and rearchitecture has landed, I'm actively working on this issue again.
My top priority is to get the goals of the "bare" import functionality of this issue aligned with the functionality that is required to fix #1609760: hook_image_style_*() is not invoked for image styles upon Image module installation. As before, I'm combining both in #1626584: Combine configuration system changes to verify they are compatible in order to make sure that the proposed changes are compatible.
I also thought about incorporating #132; i.e., using a flag on a Config object to determine whether it has been handled by a module callback or not.
First of all, we should add a basic $isNew property + method, since we need that anyway: #1666632: Add Config::isNew() to allow code to determine whether a config object already exists
But regarding the import mechanism, I see large problems with the idea of leveraging any kind of flag on a Config object:
- Performance/memory hog:
The current import code dispatches a configuration change to the module callback (if any) and removes it from the list of differing config object names upon success. All config changes that were not handled by module callbacks are synchronized directly afterwards:
$remaining_changes = config_import_invoke_owner($config_changes, $source_storage, $target_storage); config_sync_changes($remaining_changes, $source_storage, $target_storage);
If we would change this, then config_sync_changes() would have to get the full stack of instantiated config objects instead of a simple list of names.
There most likely will be hundreds, or possibly even thousands of configuration changes that need to be imported. The current code has no issue with that, since it basically handles only one config object at a time. Relying on and checking
$config->isNew()(or another flag) requires to instantiate all config objects and to keep them in memory, so they can be passed forward to config_sync_changes().
- Changes not only involve saving, but also deleting:
Regardless of whether the flag is $isNew, $tainted, or $dirty, I don't see how the concept could really and reliably work for configuration that needs to be deleted.
- There is more than one config object involved per each change:
The import mechanism has to provide both the config object from the source storage as well as the config object from the target storage, in order to allow the module callback to properly handle a configuration change:
function MODULE_config_import($op, $name, $new_config, $old_config)
This means that $isNew could have been changed on one of $new_config and $old_config, and we cannot really be sure on which. It "should" be $new_config, but that's a big assumption.
- EDIT: Separation of concerns
(was previously second part of last point, separated for clarity)
I'm also not sure whether we really want to force module callbacks into having to use $new_config directly. In fact, in Barcelona, @merlinofchaos kept on insisting that it's possible that Views and other similarly complex modules may not want to deal with $config at all.
They only want to extract the configuration data from the passed in config object(s) and pass that on to one (or more) module/thingie-specific CRUD controllers, which do not have any knowledge about a "configuration system" to begin with.
In any case, the most concerning point for me is the performance issue.
Comment #137sun CreditAttribution: sun commented
Please note that we intend to commit a few parts of the current code via #1609760: hook_image_style_*() is not invoked for image styles upon Image module installation already, in order to fix that critical bug and thus free up core's critical/major thresholds. The other/actual import functionality code is not contained.
As long as we're waiting for those other commits, I'd love to see some more feedback and discussion on #132 - #136, as well as any other issues on the current code. It doesn't make sense for me to "extract" the changes for this issue right now, because there are too many dependencies, and once those other commits happened, the remaining changes are pretty much the same (remaining) as those being visible in #1626584: Combine configuration system changes to verify they are compatible
Comment #138chx CreditAttribution: chx commented
|» Needs work
isNew has nothing to do with this. I get your problem with tainted, let me offer this:
module_invoke($module, 'config_sync', $op, $name, $new_config, $old_config);
if (!$new_config->isTainted()) {
Notice the huge difference: the module does not and can not determine whether the object will be saved later. That's the responsibility of the object. If it is not tainted (ie it was saved) then we do not need to save it later -- but we know it was saved some time already. OOP. Encapsulation. Responsibilities. Stuff like that.
Comment #139sun CreditAttribution: sun commented
|Needs work
|» Postponed
I understand. However:
A) That would still be incompatible with #136.4, i.e., disallowing custom CRUD controllers.
While I'd personally agree that there "should not" be any such things like custom CRUD controllers for dynamic configuration thingies in the first place (because I still believe the config system itself is the CRUD controller), @merlinofchaos strongly disagreed with that. It is possible that we came to an agreement (not sure) that the "regular" cases don't need a custom CRUD controller. But nevertheless, he still wants the system to allow to do that. This custom module API can be anything, ranging from complex OO stuff to dead simple arrays (check: image styles). As soon as the config is converted to something else, we're effectively losing the architectural purpose of any kind of flag or property on the config object. Instead, the regular flow of the module API creates a new config object at some point and saves that. In turn, that would make the module callback more complex in the end.
B) It makes the bold assumption of #136.3: $new_config has to be saved as being passed.
This is similar to A), but not the same. Something as unholy and simple as
$old_config->setData($new_config->get())->save(); breaks the assumption.
Please note that I've edited #136 to separate the second part of 3) into a new 4) for clarity, since that should have been an own point in the first place.
Comment #140chx CreditAttribution: chx commented
My assumption is that yes every single config object needs to exist in file and in the active store and if a module has a need for additional storage, be my guest. Replacement, not so much.
Comment #141effulgentsia CreditAttribution: effulgentsia commented
|» Needs work
Per #136, this seems no longer postponed.
Comment #142sun CreditAttribution: sun commented
|Needs work
|» Postponed
Sorry for reverting the status, but there still seems to be a disagreement on a question of #1609760: hook_image_style_*() is not invoked for image styles upon Image module installation that seems to be a detail, but apparently questions the entire module callback concept. We need an agreement/sign-off/hammer on that issue, before we can reasonably move on with re-rolling/rebasing the other issues/branches. As soon as the status of that issue is cleared, I promise there will be a new patch to review here. (In fact, I wanted to do that already, but just in time that issue got re-opened.)
Comment #143merlinofchaos CreditAttribution: merlinofchaos commented
Just to clarify: I don't wish to replace $config, I just wish to make sure that it is separate where necessary. I have backed off my assertion that my ConfigurableThingy won't necessarily use it directly, and my latest implementation does actually use it and can get value out of it.
I do need to be able to have additional storage; heck, Field API needs that (where additional storage == the creation/modification of tables). So that's non-negotiable.
Comment #144sun CreditAttribution: sun commented
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 37,282 pass(es). View
#1609760: hook_image_style_*() is not invoked for image styles upon Image module installation still needs a final decision, but there hasn't been any progress on that issue in the past days.
Since we cannot postpone this issue forever and time to feature freeze is running, I've re-rolled/re-extracted this patch, despite the lack of final decision.
Attached patch contains the remaining changes for the config import functionality. All changes for #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc) have been removed:
f1a4353 Removed irrelevant thingie changes.
7c39b7c Deleted irrelevant thingie files.
69f1e2c Removed ConfigTest thingie hook invocation assertions from ConfigImportTest.
With all of that out of the way, the remaining changes are pretty small :)
Comment #145sun CreditAttribution: sun commented
Comment #146sun CreditAttribution: sun commented
PASSED: [[SimpleTest]]: [MySQL] 37,319 pass(es). View
Can we move forward here?
Attached patch incorporates the last @catch's review issues from #112, now that they're possible to do (specifically regarding #1530652: Switch from == to === to prevent mistakes and make brute force password attacks harder + #1608912: (Re-)adding keys to configuration breaks the intended goal of being able to diff files).
@bleen18's minor docs issue has been incorporated as well.
Aside from that:
- The module API callback and hook invocation issue has been resolved via #1609760: hook_image_style_*() is not invoked for image styles upon Image module installation
- Error suppression is still left to #1247666: Replace @function calls with try/catch ErrorException blocks
- Properly handling renames is left to #1609418: Configuration objects are not unique
- Proper dependency handling is still left to #1605460: Implement dependencies and defer process for importing configuration
Comment #147cosmicdreams CreditAttribution: cosmicdreams commented
@sun, what is necessary for us to move forward. How can I help?
Comment #148sun CreditAttribution: sun commented
The most challenging part (the module API callback) landed with #1609760: hook_image_style_*() is not invoked for image styles upon Image module installation already, so the remaining changes of this patch are relatively trivial and actually were RTBC already. I still consider this patch RTBC and would like to see it land ASAP, so we can start to work on a proper UI and stuff.
That might need some final confirmations though.
Aside from that, we should get back to converting more settings forms to the configuration system and completing/adjusting previous conversions where necessary, now that #1599554: Tutorial/guidelines for how to convert variables into configuration is resolved. :)
Comment #150sun CreditAttribution: sun commented
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 37,383 pass(es). View
756dd4d Added tests for import/export UI.
df008cc Merge branch 'config-next-1626584-sun' into config-import-1447686-sun
Comment #152webchick CreditAttribution: webchick commented
|Reviewed & tested by the community
|» Needs review
Ok, here's how I tested this.
Bootstrap code:
# 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.
git apply --index config.import.150.patch
git commit -am "Applying latest import/export 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.
# NOTE: At this point the files/config_yakkayakka folder is empty.
git add sites/default/files -f
git commit -am "Adding files directory."
# Next, create a "prod" version of the site and install it.
cd ../
git clone file:///Users/abyron/Sites/drupal-8.x-dev drupal-8.x-prod
cd drupal-8.x-prod
drush si --db-url=mysql://root:root@localhost/8.x-prod --account-pass=admin -y
Next, I went to the prod site at http://localhost/drupal-8.x-prod/ and created a new Article node with a large image attached. I confirmed that on the full node view, this was output at 480x480px.
Next, I went to my dev site at http://localhost/drupal-8.x-dev/node#overlay=admin/config/media/image-st... and changed the height/width of the large image style to to 450x450px instead.
Problem Instead of editing this, it created a new scale action. (This is a known bug, covered in #1508872: Image effects duplicate on edit). I deleted the old action, so the only thing left was "Scale 450x450 (upscaling allowed)".
I expected this to write to my files/config_yaddayadda and it did not. heyrocker informed me that we changed to an explicit import/export operation. So in order to see this, I needed to enable the config module.
Problem I would not have known to do this without his help. We should make sure this module is enabled for all updates and on installation, at least for the standard profile (one could make the argument that minimal profile users will use Drush, but I'm not so sure they won't be confounded by this, too..).
I enabled the Configuration manager module through the module UI.
Problem I have no idea where to go next. The module is missing a link to the configuration page in its .info file.
Find the page at http://localhost/drupal-8.x-dev/admin/config/development/sync and ummm...what?
And the corollary Export page:
Problem I... literally have no idea what to do here. It seems like it's defaulting to a destructive action, given the "Delete (7)" heading, but it's not clear what values are being deleted or what values they are going to be replaced with. There's no help text to provide any guidance. I have no idea why I'm seeing everything here and not just the image.style.large as I would expect.
I'm going to go out on a limb though and go to the "Export" tab and click "Export." I get a green confirmation and then a note that there are no configuration changes. Clicking back to "Import" says the same.
Now when I
ls under sites/default/files/config_yokkoyokko I get files. Hooray. I add and commit them to git:
git add -f sites/default/files
git commit -am "Files. I has them."
Now, back over to prod.
I enable the config module through the UI again, and again go to admin/config/development/sync.
Problem Like on my dev site, it tells me all of my config values are changed despite me never having changed any of them. This is very confusing.
I click Import, without having done
git pull first, so I can see it tell me I have no config changes.
Next, I do
git pull from the prod site, and reload the page, and...
Problem: It still tells me I have no configuration changes to import. :( Clearing cache doesn't help.
So... unless I'm wildly mistaken, this isn't working. What step did I miss?
Comment #153webchick CreditAttribution: webchick commented
|Needs review
|» Needs work
I've been informed that I screwed up the following:
I click Import, without having done git pull first, so I can see it tell me I have no config changes.
Doing this destroyed all of my configuration data, because of the earlier problem about the directory being empty by default. So image styles? Gone. Sad panda.
I'm going to call this "needs work." :P
Comment #154sun CreditAttribution: sun commented
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 37,203 pass(es). View
9dabce4 Fixed config import screen deletes all configuration initially (stop-gap UX fix).
89dd92d Updated ConfigImportUITest::testImportLock() for new empty source storage behavior.
2c80423 Added assertions for new empty source storage UI behavior.
Comment #157webchick CreditAttribution: webchick commented
Ok. Basically same deal as above. Config module's enabled by default. Yay.
Now when I get to the Import tab on my dev site, I see:
Clicking "Export" takes me to the second screenshot in #152.
I click the Export button there. Same as before ("There are no configuration changes.").
Back to prod. On import page I get the same screenshot. Go to Export tab, click Export button.
Back to Import. Just a message that there are no configuration changes + an Import button. This will be the default screen that people see when they are about to push their configuration changes to production:
PROBLEM: For kicks, I clicked the Import button anyway, even though there are no changes to implement. It gives an error:
"The import failed due to an error. Any errors have been logged."
It probably should instead either grey out the button so I can't click it, or else show me something less terrifying looking when this happens. Follow-up.
git pull. It correctly brought over my files.
PROBLEM: Now I reload and... still says "There are no configuration changes." :(
The reason this is happening is because there are two configuration directories, since each site was a fresh install. Greg says in order to get this working I need to edit settings.php on prod and point it to the dev site's config dir. I will try and do this shortly.
Comment #158webchick CreditAttribution: webchick commented
Ok. Talked this over with heyrocker and eaton at mwds. They correctly pointed out that the workflow I'm doing here to simulate a multiserver environment is a bit wonky, and is thus hitting an edge case that happens when you install from scratch twice. Because in a normal situation, you would actually build the site out on your development environment, and then copy *both* the files and database over, and then either manually create or copy/paste settings.php over, and then ta-da.
So. One more time.
# 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.import.156.patch
git apply --index config.import.156.patch
git commit -am "Applying latest import/export 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.
# NOTE: At this point the files/config_yakkayakka folder is empty.
git add sites/default/files -f
git commit -am "Adding files directory."
# 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`;
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.
NOW. Going to try again. :P
Comment #159webchick CreditAttribution: webchick commented
|Needs review
|» Needs work
PROBLEM We lost the adjustment to standard.info to make config module enabled by default. Needs work for that. Manually enabled the module in both dev and prod. Configure link is working. Yay.
Added new node, uploaded big image, confirmed it was 480x480.
Add desaturate action, update style.
Click Export.
git add -f sites/default/files
git commit -am "Files. I has them."
Click Export.
Back to Import.
git pull
error: The following untracked working tree files would be overwritten by merge:
Please move or remove them before you can merge.
Comment #160webchick CreditAttribution: webchick commented
Ok, right. I don't want to export on production using this method. I only want to import.
*Rewind* to (by deleting the contents of the config directory on prod):
git add -f sites/default/files
git commit -am "Files. I has them."
git pull
I click Import.
I go to my node (in a different browser), and....
Comment #161sun CreditAttribution: sun commented
|Needs work
|» Reviewed & tested by the community
|762 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config.import.161.patch. Unable to apply patch. See the log in the details link for more information. View
Lacking interdiffs, I really hope that these two changes were the only ones.
b4d56dd Added 'configure' property to .info file, and enabled Config module for Standard profile.
Comment #162sun CreditAttribution: sun commented
Comment #164webchick CreditAttribution: webchick commented
Ok, sat down with Dries to go over this functionality. His feedback is the following (note: neither he nor I have read this entire issue, and Greg isn't here, please don't get mad, cos I know this was discussed above. :)):
1) We should get rid of "Export." We should automatically write through to the files upon change to the YAML files below, so the files are the canonical version and the database is a cache.
2) We should rename Import to something like "Reload files from disk" or "Reload configuration."
To elaborate on #1, I was || this close to having a repeat of my "lost all production configuration data" scenario (except on dev) because when you have made configuration changes on dev, and you go to the initial "Configuration synchronization" page, you get this:
Which looks identical to this:
So the inclination to click that button on the "Import" page and destroy my changes I had worked so very hard on on my dev site was very strong.
Removing export would also eliminate at least 3 of the "I'm stuck" blockers I've hit while reviewing this patch. It's a much more natural way for it to work. I know it was changed, presumably for a good reason, but it's a huge WTF, unfortunately; will talk to Greg tomorrow.
Comment #165webchick CreditAttribution: webchick commented
Restoring tags.
Comment #166sun CreditAttribution: sun commented
Re-rolled against HEAD.
Comment #167tstoeckler CreditAttribution: tstoeckler commented
|Needs work
|» Needs review
Testbot, plz.
Comment #168sun CreditAttribution: sun commented
|Needs review
|» Postponed
I fail to see how such "decisions" can be made out of the blue and how they're related to this issue in the first place.
Additionally, it has been clarified more than once that the UX of this technical functionality was never taken into account, not even for a minute. There is at least one follow-up issue for designing a proper UI + UX. The goal of this patch is to introduce the low-level import/export functions and a minimal way to allow Drupal core developers to actually trigger them through the UI, which heavily simplifies the work on all the config conversion patches.
The changes that are suddenly being asked for here require a range of other issues to be resolved first. Essentially putting this issue on a halt for another couple of weeks/months.
Comment #169sun CreditAttribution: sun commented
Created/changed the following dependencies into critical tasks:
Comment #170sun CreditAttribution: sun commented
The writing to both DatabaseStorage and FileStorage was removed, because it inherently conflicts with the idea of having differences between current and exported configuration. The moment you write, you eliminate any possible differences.
Comment #171webchick CreditAttribution: webchick commented
Can you elaborate on why we need to have differences between current and exported configurations? Or point me to the right comment/issue where I can read up on this myself? I'm really not interested in blocking this issue, especially on anything StorageDispatcher-related, since that seems completely "meh, if we get to it" material; this issue is actually critical.
Originally, the way CMI worked was the files were wrote-through directly on save. This would allow:
a) When I
git add my files directory after installation, it would already include all of my site's default configuration, and these would already be available on my production site when I
git clone from there.
b) When I first went to the Configuration sync (Import) page, it would simply say "No configuration changes." Which is exactly what you'd expect when you haven't changed anything yet.
c) Each time I save a configuration form,
git diff would allow me to iteratively inspect my changes to ensure they're correct. Right now, I only get them all in one "export" shot, and can't triage them individually.
d) When I went to the Production site's Configuration sync (Import) page, it would simply say "system.site" (or whatever), not a weird error asking you to export your changes, nor a list of all possible values in the site. This is exactly what you'd expect when you've only changed one thing and moved it over.
In other words, changing this behaviour to how it used to be would've solved at least 80% of my blocking issues while trying to review this functionality, and it would also make the UI here passable, instead of a recipe for data loss.
Comment #172sun CreditAttribution: sun commented
- Whether the config system writes to multiple storages or not is out of scope for this issue. This patch does not touch nor change that part of the config system.
- Concurrent/dispatched writes to the active store and the file storage just simply means that there will never be any configuration differences for anyone who is not using git. It's dead simple: If you write to both, then there cannot be any differences. As another direct consequence, you will not be able to see in the UI whether a site's configuration has been changed.
- The import mechanism must not write to the file storage during an import. All hell breaks lose otherwise. The user will not know what data will be imported.
- The moment you think about git deployments only, you can forget about the entire UI. People who are using git do not need a UI.
I'm essentially back to #1626584-83: Combine configuration system changes to verify they are compatible.
Comment #174webchick CreditAttribution: webchick commented
I don't think that's true, though. I hit exactly that case in #159 due do Doing It Wrong™. Git correctly gave me a merge conflict and wouldn't allow me to overwrite my production site until I figured it out.
Comment #175webchick CreditAttribution: webchick commented
And even in the FTP case, most (all?) clients will give you a warning/error when you're uploading a file that's older than the one on disk in the destination.
Comment #176catch CreditAttribution: catch commented
don't think that's true, though. I hit exactly that case in #159 due do Doing It Wrong™. Git correctly gave me a merge conflict and wouldn't allow me to overwrite my production site until I figured it out.
There are lots of production workflows which aren't
git pull, and whether ftp clients warn you about uploading newer files or not depends on the client and configuration. I don't think we should rely on either of those behaviours to stop people getting their production sites out of whack.
There's also the case where you push the new configuration to the site and someone submits a form straight afterwards, which would revert the changes just pushed, potentially reverting things back to their previous state or some kind of hybrid since a form will have multiple configuration values and/or objects which may or may not have each been explicitly changed.
Comment #178webchick CreditAttribution: webchick commented
Can you point me to where these discussions took place? I'm trying to follow along, but it's hard without the context. At a glance, I would say someone overwriting production files that were just pushed by clicking something in their DB is exactly how it's supposed to work: the last person to push something wins. catch's second scenario seems to imply we just need to add locking around this.
Comment #179yched CreditAttribution: yched commented
What if we have separate directories for import and export / writethrough ?
Something like : each site instance specifies in its settings.php what are its import and export folders (or at least the default ones, maybe overridable in the import/export UI / drush command on a case by case basis)
on stage server:
export (or writethrough ?) to files/config_xxx/stage
import from files/config_xxx/prod (or occasionally import from another folder to rollback)
on prod server:
export to files/config_xxx/prod
import from files/config_xxx/stage (or occasionally import from another folder to rollback)
/prod and /stage would both be pushed/pulled to git.
Comment #180beejeebus CreditAttribution: beejeebus commented
#178 - i think you mean clicking something in the UI?
just saving over changes before they are imported is certainly one way it could work. importing a subset of the changes may break sites, but it would simplify the system a lot.
Comment #181beejeebus CreditAttribution: beejeebus commented
#179 - yes. that.
Comment #184sun CreditAttribution: sun commented
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 37,241 pass(es). View
9ce873f Removed Configuration Import/Export UI.
Comment #185webchick CreditAttribution: webchick commented
|Needs review
|» Fixed
Ok, testbot gives this a pass, so this looks good to go.
Committed and pushed to 8.x. WOOHOO.
Comment #186.0(not verified) CreditAttribution: commented
Clarify dependency issues