Problem

  • 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.

Goal

  • Implement this functionality.

Details

  • 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

  1. 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.)
  2. 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.
  3. Modules are called in the order of the dependency chain defined in info files.
  4. Aside from this, the work to be done is left entirely to the modules to detect and react to.

Notes

  • 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.
    1. 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.
    2. 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
CommentFileSizeAuthor
#184 config.import.184.patch7.63 KBsun
#184 interdiff.txt12.19 KBsun
#166 config.import.166.patch19.81 KBsun
#161 config.import.161.patch19.81 KBsun
#161 interdiff.txt762 bytessun
#156 config.import.156.patch19.45 KBgdd
#155 config.import.155.patch19.39 KBgdd
#154 config.import.154.patch19.07 KBsun
#154 interdiff.txt2.78 KBsun
#150 config.import.150.patch17.85 KBsun
#150 interdiff.txt9.46 KBsun
#146 config.import.146.patch10.8 KBsun
#146 interdiff.txt2.64 KBsun
#144 config.import.144.patch9.98 KBsun
#131 config.import.131.patch27.17 KBsun
#131 interdiff.txt6.77 KBsun
#129 config.import.129.patch27.67 KBsun
#129 interdiff.txt7.19 KBsun
#117 config.import.117.patch27.62 KBsun
#113 config.import.113.patch27.54 KBsun
#113 interdiff.txt3.99 KBsun
#108 config.import.108.patch27.4 KBsun
#108 interdiff.txt7.91 KBsun
#106 1447686-106-config-sync.patch29.36 KBAnonymous (not verified)
#105 config.import.105.patch31.04 KBsun
#105 interdiff.txt4.05 KBsun
#103 config.import.103.patch30.76 KBsun
#103 interdiff.txt2.3 KBsun
#102 config-import-form.png7.64 KBsun
#102 config.import.102.patch30.46 KBsun
#102 interdiff.txt15.42 KBsun
#98 config.import.98.patch25.84 KBsun
#96 config.import.96.patch26.81 KBsun
#95 config.import.95.patch35.37 KBsun
#93 config.import.93.patch35.9 KBsun
#91 config.import.91.patch35.91 KBsun
#87 config.import.87.patch27.58 KBsun
#43 1447686-reload-image-test.diff30.85 KBDavid Strauss
#32 1447686-31-reload.patch28.63 KBAnonymous (not verified)
#31 1447686-31-reload.patch28.62 KBAnonymous (not verified)
#29 0001-1447686-29-reload.patch.patch25.2 KBPol
#28 1447686-28-reload.patch26.56 KBAnonymous (not verified)
#25 0001-1447686-25-reload.patch19.67 KBPol
#23 20.png71.63 KBPol
#23 1447686-23-reload.patch14.64 KBPol
#20 1447686-20-reload.patch14.85 KBPol
#19 19.png70.2 KBPol
#17 1447686-17-reload.patch18.61 KBAnonymous (not verified)
#14 1447686-11-reload.patch14.62 KBPol
#12 1447686-10-reload.patch14.59 KBPol
#11 18.png31.49 KBPol
#9 1447686-9-reload.patch18.22 KBAnonymous (not verified)
#6 1447686-6-reload.patch17.09 KBAnonymous (not verified)
#5 1447686-5-reload.patch15.34 KBAnonymous (not verified)
#4 1447686-4-reload.patch14.03 KBAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gdd’s picture

Issue summary: View changes

Updated issue summary.

Anonymous’s picture

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:

http://drupalcode.org/sandbox/heyrocker/1145636.git/shortlog/refs/heads/...

gdd’s picture

Priority: Normal » Critical

Now that the patch has landed, this is a critical followup task

sun’s picture

Issue tags: +Configuration system
Anonymous’s picture

Status: Active » Needs review
FileSize
14.03 KB

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.

Anonymous’s picture

FileSize
15.34 KB

added an implementation of image_config_reload_from_disc(). looks pretty ugly, but seems to work.

Anonymous’s picture

FileSize
17.09 KB

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.

cweagans’s picture

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).

Pol’s picture

Status: Needs review » Needs work

Hello,

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.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
18.22 KB

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().

Pol’s picture

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.

Pol’s picture

FileSize
31.49 KB

I've updated the UI, tell me if you like it !

Pol’s picture

FileSize
14.59 KB

Here's the patch.

Status: Needs review » Needs work

The last submitted patch, 1447686-10-reload.patch, failed testing.

Pol’s picture

Status: Needs work » Needs review
FileSize
14.62 KB
(21:43:17) beejeebus: drupol: can you can you make that $config_files instead of $configs ?

Done.

Status: Needs review » Needs work

The last submitted patch, 1447686-11-reload.patch, failed testing.

Anonymous’s picture

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.

Anonymous’s picture

FileSize
18.61 KB

updated patch against 8.x with the UI from Pol.

Anonymous’s picture

Category: feature » task
Status: Needs work » Needs review
Pol’s picture

FileSize
70.2 KB

Beejeebus,

About the menu, are you ok with this ?

Pol’s picture

FileSize
14.85 KB

Patch updated with menu entry visible in admin/config.

cweagans’s picture

No, config doesn't need to be its own section at admin/config. Just stick it under Development.

cweagans’s picture

Status: Needs review » Needs work
Pol’s picture

FileSize
14.64 KB
71.63 KB

Patch updated.

Pol’s picture

Status: Needs work » Needs review
Pol’s picture

FileSize
19.67 KB

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 ?

aspilicious’s picture

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!

Anonymous’s picture

Status: Needs review » Needs work

heyrocker pointed out that we should move the menu callback UI from the system.module into config.module.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
26.56 KB

updated patch adds tests.

Pol’s picture

Patch updated.

Removed config configuration from system.module and system.admin.inc.
Moved them to config.module and config.admin.inc.

Status: Needs review » Needs work

The last submitted patch, 0001-1447686-29-reload.patch.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
28.62 KB

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.

Anonymous’s picture

FileSize
28.63 KB

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.

ksenzee’s picture

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).

Owen Barton’s picture

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.

yched’s picture

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]

Owen Barton’s picture

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).

yched’s picture

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.

Owen Barton’s picture

Deployment can still be in two (or more) passes - I am only talking about validation, before any deployment starts.

agentrickard’s picture

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

David Strauss’s picture

"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.

David Strauss’s picture

And now, I'll actually check out the code. :-)

Anonymous’s picture

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.

David Strauss’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 1447686-reload-image-test.diff, failed testing.

yched’s picture

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.

sun’s picture

Status: 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.

Issues:

- 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

yched’s picture

#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.

Anonymous’s picture

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.

yched’s picture

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.

Anonymous’s picture

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.

yched’s picture

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 ?

ksenzee’s picture

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

Anonymous’s picture

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.

yched’s picture

@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.

ksenzee’s picture

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.

David Strauss’s picture

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.

David Strauss’s picture

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.

Anonymous’s picture

"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.

David Strauss’s picture

"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.

ksenzee’s picture

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?

Anonymous’s picture

"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?

David Strauss’s picture

"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.

David Strauss’s picture

"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.

Owen Barton’s picture

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!).

gdd’s picture

One thing everyone has with Features that we currently have no way to track is the ability to tell when a Feature has been 'overridden' aka changed since delivery. We don't have anything like this. When we push files we detect which ones are changed since last reload, but we have no idea if it has changed in BOTH places.

A simple way to manage this is to add a flag to the active store. When someone changes config in the UI, the flag flips to true. After a reload all the flags get reset. This way when new config is shipped to this site, we have a way to know what has changed since the last reload. We don't even necessarily have to do anything but the most simple reaction to it in core (basically ask 'Go forward with disk config or bail on the whole thing?') but it would be available for contrib modules to work with and do more complex and detailed operations with.

This really seems like something that would be useful and would address some of the worries I'm seeing in this thread.

Thoughts?

Anonymous’s picture

Status: Needs review » Needs work

i'll defer to david on this, i'm not going to fight any more.

philippejadin’s picture

Playing devil's advocate

Isn't it a bit like reinventing version control ?

gdd’s picture

That has always been my feeling, and its what I was trying to get across to this guy when he blogged about this

http://brightbacon.com/blog/drupal/configuration-management-initiative-l...

There's only so much we can (or should) do in the system. However I think my suggestion above is a pretty easy thing we could add without too much pain.

philippejadin’s picture

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 :-)

ksenzee’s picture

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?

gdd’s picture

This and internationalization are basically the most important open issues for CMI at the moment. Does it make sense to wait to see what happens with #1515312: Add snapshots of last loaded config for tracking whether config has changed or should we move forward? This is holding up a lot of really important work (particularly around conversion of the field system) and I'd like to see it get in if at all possible. Where are we at with the patch itself? I'd like to see some more comments there and leave the 'what do we do about changes on live' as a followup.

catch’s picture

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?

Anonymous’s picture

i'm not working on this anymore, and have done nothing on the patch since drupalcon denver, so #43 would be the latest code.

ksenzee’s picture

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?

gdd’s picture

@catch The writing of UI changes directly to the file system has absolutely nothing to do with this, this is about changes being pushed from dev that conflict with changes already made in live. Those live changes are written to the active store either way, and the changes being pushed from dev to live will overwrite or conflict with them. The writing of changes directly to the file system does open up other issues, but they are separarate from tracking changes that have been overridden. Note that I'm not against this happening, I just want to make sure this issue doesn't get sidetracked.

@ksenzee Actually performance had nothing to do with the introduction of the active store, it was much more about having a set of configuration that we could always rely on, even if the files themselves got borked with syntax errors/being hacked/etc.

I have more to say and think about about ksenzee's post butit has to wait for when I'm less asleep.

sun’s picture

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).

yched’s picture

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.

Anonymous’s picture

.

yched’s picture

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);
    $config->save();
  }
}

/**
 * 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.

gdd’s picture

I had a brief discussion with David about some of the reload issues this week at the d.o sprint here in Portland. Our talk was centered around the whole issue of 'what happens when something on prod has been overridden since last reload' but it is possible that it might help in the other issues as well.

Basically the idea is that whenever we reload config, we take that a snapshot of that config and save it away somewhere else. What this gives us at any point in time is the following:

1) The config in the files
2) The config in the active store
3) The config that was last reloaded

This means we can actually determine not only that the active store has drifted since last reload, but also in what way it has drifted. It allows us a little more detail too. If I change a setting on prod, then decide to revert it, the active store is still the same as the last reload even though it was at some point modified. We couldn't know this with the simple flag solution. This offers the opportunity for contrib to do some really cool 3-way diff stuff to determine how and where config has changed. While this does add some complexity to our solution, I really like it a lot.

This might also offer us some opportunity to solve some of the problems listed above. For instance, 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! This is a pretty big deal IMO.

Really interested to hear thoughts on this, as it seems like a solution that could really unblock a lot of the issues we're having here.

Crell’s picture

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.

cweagans’s picture

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)

yched’s picture

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.

gdd’s picture

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.

Well we can make the API such that you get to choose where you read from depending on your needs:

- files
- active store
- last loaded

So which you read from is up to the process that is running. That said, your point about state changes being made along the way (which sun also made to me in IRC) is a good one, and I am continually forgetting that the field system makes real unrecoverable changes to the database schema as it does its work. Obviously it would also be problematic if the field system writes to active while other systems write only to 'last loaded' (field would likely need to write to both.) However, I also think that the field system is almost the only place where this happens. For 90% of other use cases, the system I outlined in #80 would work really well.

Yced does this sound like it can work for you?

yched’s picture

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.

David Strauss’s picture

@Crell

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.

sun’s picture

Title: Implement a mechanism for configuration to be reloaded when files are updated » Allow to import and synchronize configuration when files are updated
Status: Needs work » Needs review
FileSize
27.58 KB
  1. Better title. "reload", DIE.
  2. Performed a range of clean-ups and fixes based on #46, based on @beejeebus' cmi branch from back then.
  3. 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).
  4. 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).

sun’s picture

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.

sun’s picture

#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.

yched’s picture

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 :-/

sun’s picture

FileSize
35.91 KB

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.

Status: Needs review » Needs work

The last submitted patch, config.import.91.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
35.9 KB

d'oh, sorry.

sun’s picture

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.

sun’s picture

FileSize
35.37 KB

Resolved merge conflicts against HEAD.

sun’s picture

FileSize
26.81 KB

Re-rolled, merged, and resolved against HEAD.

Status: Needs review » Needs work

The last submitted patch, config.import.96.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
25.84 KB

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.

aspilicious’s picture

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.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

i think this is ready to fly.

catch’s picture

Status: 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 @@
+<?php

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.

sun’s picture

Status: Needs work » Needs review
FileSize
15.42 KB
30.46 KB
7.64 KB
  1. 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.
  2. You're totally right on the lock code. I've redone that, using the variable_initialize() as inspiration.
  3. see 1)
  4. 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.
  5. Created #1603656: Incorporate config "defer" algorithm into module hook system? for that discussion.
  6. 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 ;))

    config-import-form.png

  7. Added API docs.
  8. 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.
  9. 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.
  10. 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.
sun’s picture

FileSize
2.3 KB
30.76 KB

@beejeebus doesn't want config_sync() to be re-entrant with the new lock code, so I've removed that.

pounard’s picture

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).

EDIT:
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?

sun’s picture

FileSize
4.05 KB
31.04 KB

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.

Anonymous’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 1447686-106-config-sync.patch, failed testing.

sun’s picture

Assigned: Unassigned » sun
Status: Needs work » Needs review
FileSize
7.91 KB
27.4 KB

#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.

Anonymous’s picture

Status: 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.

xjm’s picture

Issue tags: +VDC

Tagging.

cweagans’s picture

Title: Allow to import and synchronize configuration when files are updated » Allow importing and synchronizing configuration when files are updated

Sorry, this was bugging me.

catch’s picture

Status: 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?

sun’s picture

FileSize
3.99 KB
27.54 KB
  1. + 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.

  2. 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.
  3. 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 :-/]

  4. Created #1609418: Configuration objects are not unique (and added to the roadmap)
sun’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, config.import.113.patch, failed testing.

catch’s picture

Status: Needs work » Needs review

#1 OK that's fine, and #1530652: [META] 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.

#3.

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.

sun’s picture

FileSize
27.62 KB

Sorry, forgot the other hook arguments in the last patch.

sun’s picture

#117: config.import.117.patch queued for re-testing.

sun’s picture

Status: 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.

chx’s picture

Status: 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.

alexpott’s picture

Status: 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.

alexpott’s picture

Status: Needs work » Needs review

Did not mean to change the status - sorry...

Anonymous’s picture

#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.

bleen’s picture

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.

Anonymous’s picture

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.

chx’s picture

This is completely in line with what I posted to the meta issue about validation: http://drupal.org/node/1560060#comment-6073876

catch’s picture

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.

chx’s picture

Filed a patch to the hook invocation issue with the new hook family.

sun’s picture

FileSize
7.19 KB
27.67 KB

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.

Status: Needs review » Needs work

The last submitted patch, config.import.129.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
6.77 KB
27.17 KB

Merged HEAD, adjusted docs.

chx’s picture

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.

chx’s picture

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.

Status: Needs review » Needs work

The last submitted patch, config.import.131.patch, failed testing.

sun’s picture

Status: Needs work » Postponed

Postponing on #1605324: Configuration system cleanup and rearchitecture -- which is sorta RTBC and will hopefully land in the next hours.

sun’s picture

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:

  1. 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().

  2. 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.

  3. 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.

  4. 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.

sun’s picture

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

chx’s picture

Status: Postponed » 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()) {
     unset($remaining_changes[$op][$key]);
   }

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.

sun’s picture

Status: 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.

chx’s picture

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.

effulgentsia’s picture

Status: Postponed » Needs work

Per #136, this seems no longer postponed.

sun’s picture

Status: 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.)

merlinofchaos’s picture

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.

sun’s picture

Status: Postponed » Needs review
FileSize
9.98 KB

#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 :)

sun’s picture

#144: config.import.144.patch queued for re-testing.

sun’s picture

FileSize
2.64 KB
10.8 KB

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: [META] 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

cosmicdreams’s picture

@sun, what is necessary for us to move forward. How can I help?

sun’s picture

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. :)

gdd’s picture

Status: Needs review » Needs work

After applying the patch, I enabled the configuration module (should this be enabled by default on install?) and went to the import form and got a parse error because the function config_import_get_changes() doesn't exist. It appears to me that this was renamed at some point to config_sync_get_changes() but this one instance got missed? The function signature doesn't match there either. We should probably add tests for this form I suppose.

From a code perspective I don't see any issues here. A lot of the plumbing has in fact been committed already and this patch just brings it together. I would like to run it by hand a couple times though, just to see it in action. That's really my only holdup to RTBC at this point.

sun’s picture

Status: Needs work » Needs review
FileSize
9.46 KB
17.85 KB

756dd4d Added tests for import/export UI.
df008cc Merge branch 'config-next-1626584-sun' into config-import-1447686-sun

gdd’s picture

Status: Needs review » Reviewed & tested by the community

I spent some time playing around with this today. The code is good and it was pretty thrilling to be able to move config changes between sites and watch them take effect after so much time not being able to. On the downside, the UX on the feature is really confusing as it stands right now. However since this is holding up so many patches right now I think we should commit it, and I've opened a major task to work on improving the UX of the sync process at #1697256: Create a UI for importing new configuration.

I realize there are a lot of followups here, but this is a huge milestone. Thanks to everyone who worked on this.

webchick’s picture

Status: 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?

Import screen. Table with a heading of 'Delete (7)' and then a row of key names like 'image.style.large' and so on, with an 'Import' button at the bottom.

And the corollary Export page:

Export screen. Same, except 'Create' instead, and the button says 'Export'.

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.

Successful export message, no table of changes.

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?

webchick’s picture

Status: 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

sun’s picture

Status: Needs work » Needs review
FileSize
2.78 KB
19.07 KB

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.

gdd’s picture

FileSize
19.39 KB

To clarify, the configuration system currently interprets a missing configuration file as a request to delete that configuration. Since you ship with no configuration files installed, it incorrectly interprets this state as meaning you want to delete all configuration. Sun's patch introduces a stopgap fix, which instructs the user to export their configuration before they can import any, preventing unintended data loss. This is just a stopgap feature until we can attend to the greater UX issues in #1697256: Create a UI for importing new configuration.

I just tested this patch and it works as needed. The only addition in my attached patch is it turns on the config module by default on installation of the standard profile as requested. I will let someone else RTBC but I think we're ready to roll here if webchick is OK with it.

gdd’s picture

FileSize
19.45 KB

Looking at this with webchick right now, she asked that I add the Configure link for the modules page.

webchick’s picture

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:

Error message saying There is no base configuration. Export it first.

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:

 Just a message that there are no configuration changes + an Import button.

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.

--

Did 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.

webchick’s picture

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.

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

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

# Apply & commit the patch.
wget http://drupal.org/files/config.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."

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

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

NOW. Going to try again. :P

webchick’s picture

Status: 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.

PRODUCTION;
Added new node, uploaded big image, confirmed it was 480x480.

DEV:
admin/config/media/image-styles/edit/large
Add desaturate action, update style.

DEV:
admin/config/development/sync
Error about not having exported yet.

admin/config/development/sync/export
Export table lists all values

Click Export.
Configuration exported successfully; No configuration changes.

DEV:

git add -f sites/default/files
git commit -am "Files. I has them."

PROD:
admin/config/development/sync
Error about not having exported yet.

admin/config/development/sync/export
Export table lists all values

Click Export.
Configuration exported successfully; No configuration changes.

Back to Import.
admin/config/development/sync
No configuration changes.

PROD:

git pull
error: The following untracked working tree files would be overwritten by merge:
	sites/default/files/config_2ezZuUbVrI_6zBIztQNzbaV_-4su34rvuallqWnekOQ/image.style.large.yml
	sites/default/files/config_2ezZuUbVrI_6zBIztQNzbaV_-4su34rvuallqWnekOQ/image.style.medium.yml
	sites/default/files/config_2ezZuUbVrI_6zBIztQNzbaV_-4su34rvuallqWnekOQ/image.style.thumbnail.yml
	sites/default/files/config_2ezZuUbVrI_6zBIztQNzbaV_-4su34rvuallqWnekOQ/system.cron.yml
	sites/default/files/config_2ezZuUbVrI_6zBIztQNzbaV_-4su34rvuallqWnekOQ/system.performance.yml
	sites/default/files/config_2ezZuUbVrI_6zBIztQNzbaV_-4su34rvuallqWnekOQ/system.rss-publishing.yml
	sites/default/files/config_2ezZuUbVrI_6zBIztQNzbaV_-4su34rvuallqWnekOQ/system.site.yml
Please move or remove them before you can merge.
Aborting

:(

webchick’s picture

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."

PROD:
admin/config/development/sync
There is no base configuration. Export it first.

git pull

Reload.

Only shows image.style.large

YAYYYYY!!!

I click Import.
There are no configuration changes.

I go to my node (in a different browser), and....

Black and white picture!

YAY!!!!!!!!!!!

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
762 bytes
19.81 KB

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.

sun’s picture

Issue tags: +API addition

Tagging.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, config.import.161.patch, failed testing.

webchick’s picture

Issue tags: -API addition

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:

1 change: system.site (import)

Which looks identical to this:

1 change: system.site (export)

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.

webchick’s picture

Issue tags: +API addition

Restoring tags.

sun’s picture

FileSize
19.81 KB

Re-rolled against HEAD.

tstoeckler’s picture

Status: Needs work » Needs review

Testbot, plz.

sun’s picture

Status: 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.

Postponing.

sun’s picture

sun’s picture

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.

webchick’s picture

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.

sun’s picture

  1. 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.
  2. 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.
  3. 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.
  4. 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.

gdd’s picture

Also there are problems even before you import. Say I push a bunch of config to the live site at the same time someone on said live site is changing said config. There are all sorts of ugly race conditions that happen here that absolutely will result in destruction of data. You can say 'oh they shouldnt be doing that' all you want, but they will, and when they do it will destroy data and the reason will be much hairier for people to figure out.

webchick’s picture

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.

webchick’s picture

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.

catch’s picture

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.

gdd’s picture

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.

Yes this is the situation I was discussing above in #172. I actually really pushed for writethrough being on default for months, but over time beejeebus and others convinced me it was going to be a problem. Also the community at large has let me know that telling people "Dont change the live site" is an unacceptable answer, they see allowing clients to change Views and other config themselves on their live site as a big selling point of the Drupal experience, and that current workflows right now support this.

webchick’s picture

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.

yched’s picture

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.

Anonymous’s picture

#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.

Anonymous’s picture

#179 - yes. that.

gdd’s picture

One of the problems with the import/export directory solution is that you can't just git pull your changes to the live site. You would want export (aka writethrough) on your dev site to land in import on your live site, and that wouldn't happen.

I had a really long talk today with dries, webchick, merlin, msonnabaum, chx and some others. Most of this talk was about the pros and cons of writethrough, and alternate implementations. No consensus was found, but I think we all agreed that there probably should have been a more public discussion around this and workflows in general before now. While there was a lot of discussion about writethrough in various issues and events over the last year, it was never put into its own issue for focus. So I have opened #1703168: [Meta] Ensure that configuration system functionality matches expected workflows for users and devs as a critical task to determine that.

However this should not hold up the plumbing part of this patch, which is essential for systems like Views and Fields to proceed with implementations. HOWEVER while we're still discussing workflows and functionality that will impact the UI down the road, I don't think it makes an enormous amount of sense to commit any UI for this functionality right now. I realize how important it is for this functionality to be exposed to users to play with it, however I also dont think we're doing anyone any favors exposing it in its current state. We also don't need the UI for merlin and yched to go forward with their work. So what I would really like to do is pull the UI out of this patch and just commit the plumbing so we can start playing with it and move on to the hard work of #1605460: Implement dependencies and defer process for importing configuration and other issues. That plumbing is really the important part here, so lets get this in and fight about the rest later.

gdd’s picture

Status: Postponed » Needs work
sun’s picture

Status: Needs work » Needs review
FileSize
12.19 KB
7.63 KB

9ce873f Removed Configuration Import/Export UI.

webchick’s picture

Status: Needs review » Fixed

Ok, testbot gives this a pass, so this looks good to go.

Committed and pushed to 8.x. WOOHOO.

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

Anonymous’s picture

Issue summary: View changes

Clarify dependency issues