| [21:56:20] | * cwjustgotdelta[m is now known as CornWallace[m] |
| [01:20:18] | * farmBOT has joined #farmos |
| [11:48:25] | <mstenta[m]> | jgaehring paul121 symbioquine : FYI I can only join the dev call today until 12:30 |
| [14:58:06] | <mstenta[m]> | symbioquine paul121 : i may end up honing in the scope of the `farm_update` module that i was telling you about on the dev call... |
| [14:58:45] | <mstenta[m]> | there are a few different kinds of config differences that can happen... missing config, inactive config, added config, and overridden config |
| [14:59:14] | <mstenta[m]> | i may just focus on reverting overridden config - that's the easiest to detect and revert |
| [14:59:58] | <mstenta[m]> | i was hoping i could also handle "missing" and "inactive" (creating configuration that is added to a module after it's installed) - but it's turning out to be tricky |
| [15:00:14] | <mstenta[m]> | (and i never really planned on handling "added") |
| [15:00:59] | <mstenta[m]> | the tricky part with missing/inactive is mostly on the "inactive" side... |
| [15:01:26] | <mstenta[m]> | "missing" config would be config in `config/install`, whereas "inactive" is config in `config/optional` |
| [15:01:33] | <mstenta[m]> | it's easy enough to detect and import "missing" config |
| [15:01:48] | <mstenta[m]> | but "inactive" requires also determining whether or not all the dependencies are met |
| [15:02:23] | <mstenta[m]> | so i may just simplify and only focus on "overridden" config to start |
| [15:02:52] | <mstenta[m]> | this means `farm_update` will automatically revert any config that's changed, but if config is added/removed then an update hook will still be necessary |
| [15:05:30] | <paul121[m]> | <mstenta[m]> "(and i never really planned on..." <- seems like this should really be an update hook? |
| [15:06:03] | <paul121[m]> | is that how other contrib modules would handle adding config? |
| [15:06:24] | <mstenta[m]> | oh so "added" is actually the opposite of what you'd think... it means there is *extra* config that you may not want anymore |
| [15:06:25] | <mstenta[m]> | so it would be a delete event |
| [15:06:48] | <mstenta[m]> | which agreed: should NOT happen automatically... except in a very intentional update hook |
| [15:06:48] | <paul121[m]> | oh - just read your last reply haha |
| [15:07:01] | <mstenta[m]> | but we can't really determine if it's added by farmOS or added manually |
| [15:07:08] | <mstenta[m]> | so not gonna touch that with `farm_update` |
| [15:07:34] | <mstenta[m]> | (if a farmOS module removes config for some reason, then that module can add an update hook to delete it if that makes sense) |
| [15:07:48] | <paul121[m]> | hmmm yep |
| [15:08:58] | <mstenta[m]> | the missing/inactive basically means: a module is already installed, but then a new version adds new config in it... since it's already installed, that config will not be automatically created (`config/install` and `config/optional` only get installed when the module is first installed) |
| [15:09:29] | <mstenta[m]> | so we *could* potentially automate the creation of that... but i'm finding the "inactive" ones tricky to determine their dependencies |
| [15:09:38] | <paul121[m]> | for example, if I manually added a `map_popup` view mode to an asset that did not already have one defined (it was using default), then this would be "added" config |
| [15:09:41] | <mstenta[m]> | and it would be weird if we handled "missing" but not "inactive" |
| [15:09:55] | <mstenta[m]> | so i might just stick to "overridden" for now |
| [15:10:09] | <mstenta[m]> | correct. |
| [15:10:57] | <paul121[m]> | gotcha. hmm. there must be some function somewhere that evaluates config dependencies? |
| [15:11:14] | <mstenta[m]> | there is |
| [15:11:22] | <mstenta[m]> | core has a `config.installer` service |
| [15:11:23] | <paul121[m]> | it might be abstracted even lower than config entities - maybe some `DependencyInterface` ? |
| [15:11:41] | <mstenta[m]> | with a `validateDependencies` method |
| [15:12:01] | <mstenta[m]> | unfortunately, that method is `protected` though, so we can't call it directly from our code |
| [15:12:34] | <paul121[m]> | doh |
| [15:12:46] | <mstenta[m]> | the code that does call it also installs the config................ maybe we could use the calling method, i can look closer at that as an option... |
| [15:13:35] | <mstenta[m]> | otherwise, it would mean replicating that method in our calss |
| [15:13:42] | <mstenta[m]> | which is also certainly possible |
| [15:13:59] | <mstenta[m]> | but it feels like scope is growing... hence my thought that i'll just focus on reverting overridden |
| [15:14:05] | <mstenta[m]> | but maybe it's still worth a look... |
| [15:16:31] | <paul121[m]> | does `config.installer` except a list of config to install? or something? |
| [15:23:32] | <mstenta[m]> | The `validateDependencies()` method essentially takes a config name + info about installed modules and config |
| [16:57:09] | <mstenta[m]> | paul121: should we just remove `config_rewrite`? and just use `hook_install()`? |
| [16:59:31] | <mstenta[m]> | we're only using it in `farm_api` (to override `jsonapi`, `jsonapi_extras`, and `simple_oauth` settings), and in `farm_entity` (to override `entity_reference_integrity` settings) |
| [17:00:29] | <mstenta[m]> | i'm inclined to just move those overrides to `hook_install()`s in both, and leave those configs unmanaged after installation |
| [17:00:53] | <mstenta[m]> | `config_rewrite` is neat, but it does add another layer of complexity, and i'm not sure we absolutely need it |
| [17:01:11] | <mstenta[m]> | we were using it for a few other things earlier, but i have converted those to `hook_install()`s already |
| [17:02:04] | <mstenta[m]> | (this is related to the `farm_update` considerations... we would need to patch `config_rewrite` so that `config_update` takes rewrites into account) |
| [17:04:58] | <mstenta[m]> | if i remember correctly, i think i was the one who pushed using `config_rewrite` originally :-) |
| [17:19:03] | <paul121[m]> | ah interesting.. hmm.. yeah it really just is for convenience huh |
| [17:19:27] | <mstenta[m]> | i think my main motivation was to keep a high YML/PHP ratio ;-) |
| [17:20:26] | <mstenta[m]> | notably, `config/rewrite` ONLY gets evaluated on install (just like `config/install` and `config/optional`) so it's really not much different than `hook_install()` in practice |
| [17:20:36] | <mstenta[m]> | and they show as overridden in `config_update_ui` |
| [17:20:56] | <mstenta[m]> | so if we move to using `hook_install()` nothing would really change |
| [17:21:12] | <paul121[m]> | yea.. right.. it's probably better in `hook_install()` anyways |
| [17:21:14] | <mstenta[m]> | and might be better for overall config visibility for end users who want to have more control themselves |
| [17:21:38] | <mstenta[m]> | i will probably just exclude all of these configs from `farm_update` reverts either way |
| [17:21:38] | <paul121[m]> | it would still show as overridden though, yea? |
| [17:21:42] | <mstenta[m]> | yes |
| [17:22:18] | <mstenta[m]> | just like it does now |
| [17:22:48] | <paul121[m]> | yeah. so I guess some weird situation would be if `simple_oauth` changed its config schema |
| [17:23:07] | <paul121[m]> | but we would need to do a `hook_update` for that anyways |
| [17:23:24] | <mstenta[m]> | yea true |
| [17:23:45] | <mstenta[m]> | also, i noticed we're actually only overriding 3 settings in that one (i've already started the process of moving to `hook_install()` haha) |
| [17:23:55] | <mstenta[m]> | the others are the same as defaults |
| [17:24:26] | <mstenta[m]> | the two key path settings (which don't have defaults anyway), and `access_token_expiration` (which we increase from 300 to 3600) |
| [17:24:47] | <paul121[m]> | ah cool |
| [17:34:25] | <mstenta[m]> | want to take a quick look? https://github.com/farmOS/farmOS/compare/2.x...mstenta:2.x-remove_config... |
| [17:40:25] | <paul121[m]> | yea looks good! |
| [17:41:07] | <paul121[m]> | the `farm_ui_dashboard` and `farm_ui_user` are kinda the kicker that make this seem like the correct way to do things |