Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DX][BC] allow any config value to be overridden via settings.php #2805

Closed
2 tasks
robertgarrigos opened this issue Aug 21, 2017 · 66 comments · Fixed by backdrop/backdrop#2984
Closed
2 tasks

Comments

@robertgarrigos
Copy link

robertgarrigos commented Aug 21, 2017

Describe your issue or idea

I would like to be able to export/import config files without overwriting some key config values. For instance private files directory. This is a value which might be different in devel and production environments, thus when updating configs it overrides private files directory value and you need to change it manually every time.

It would be great to be able to hardcode some configs in settings.php so they don't get overwritten during config import.

Here's a list of specific variables (and thier respective issues) that we should support in core, even without global variable-override support:


PR backdrop/backdrop#2607 by @herbdool (specific overrides)
PR backdrop/backdrop#2984 by @quicksketch (global overrides)

@serundeputy serundeputy changed the title allow hardcode config values [DX] allow hardcode config values Aug 21, 2017
@jenlampton
Copy link
Member

jenlampton commented Aug 21, 2017

IIRC any setting that could vary in different environments should be using the State system instead of config (or variables).

I'm not sure why file system directories are config and not states, that does seem like something that is likely to change in different environments. I wonder if that may have been an oversight?

@jenlampton jenlampton changed the title [DX] allow hardcode config values [DX] File system directories should use States (instead of Config) Aug 21, 2017
@herbdool
Copy link

That doesn't seem right to me. The file system path should rarely change between environments, no? Any path embedded in the WYSIWYG would stop working. A state is something that can change while using the site which doesn't fit the file system path. That shouldn't change during use.

@jenlampton jenlampton changed the title [DX] File system directories should use States (instead of Config) [DX] allow some config values to be hard-coded or overridden Aug 22, 2017
@jenlampton
Copy link
Member

jenlampton commented Aug 22, 2017

Hm, that's a good point about embedded images.

However, private files most likely wouldn't be embedded, and those are the ones that often have strange "paths" that can vary by environment. (I struggle with this on Drupal sites I currently have hosted on Acquia, for example.) Maybe public/private should be handled differently?

It would be great to be able to hardcode some configs in settings.php so they don't get overwritten during config import.

There was a reason we removed the variable/config overrides from settings.php, but I don't remember exactly what it was anymore. @quicksketch do you recall?

@robertgarrigos I wonder if this might not be a good use-case for a Backdrop version of strongarm module?

@robertgarrigos
Copy link
Author

I found the state function and thought, like @herbdool, that it should be used for changes on the same site, not between environments which I would understand as different sites. In any case, I didn't realise that the private files path would have such an impact in changing environments. I would think that having a config for that would actually avoid such problems.

Any way, this issue was not about a single config value but about the ability to override any config value. Otherwise, how do you do when deploying a site? There is the devel module, the theme debug config, private files path (although I might need to reconsider this), all the performance configs, etc. You would provably use all these config/modules on a devel environment but you would need them off on a staging or production environment. Do you just go to change all those values every time you deploy a change to your project? This is where yes, variable/config overrides with in settings.php would be handy.

@jenlampton using strongarm, wouldn't it be like using a sledgehammer to crack a nut? Strongarm for Drupal requires ctools and features and I guess it would need a heavy rewriting in order to run without those modules and adapt it to the new config management. But yes, you got the concept: I would love to see something like strongarm in backdrop core. Something simpler, though.

@jenlampton
Copy link
Member

jenlampton commented Aug 22, 2017

Otherwise, how do you do when deploying a site? ... Do you just go to change all those values every time you deploy a change to your project?

For me personally, my dev sites are a copy of production, and not vice versa. The configuration files I commit always contain the values I want on production, so deploying is never an issue. My process is to enable the development modules or adjust settings I need for development only in the development environment, and never commit those changes. (I do usually commit modules like devel, but those are never enabled on production.)

The location of all config files is also customizable via settings.php. I usually put my live site config in a directory named live-active and my dev site config in dev-active. By keeping two separate sets of config, I can have any values I want differ between environments.

Then, when I do a config sync from live -> dev, I usually check what's about to be imported, and adjust as needed (for example, copying my dev site copy of system.core.json into my staging directory before import - or making a copy of the json and doing a single-file import afterward.)

I agree, in explaining this it seems like the only reason I'm able to do this work-around is because of how well I understand the system already... it seems like we need an easier way. :)

I'm going to take a look into how Drupal 8 did this, as they have the global $conf in settings.php and also use CMI. That's the same boat we're in...

@jenlampton jenlampton changed the title [DX] allow some config values to be hard-coded or overridden [DX] allow some config values to be hard-coded or overridden via settings.php Aug 22, 2017
@robertgarrigos
Copy link
Author

it seems like we need an easier way

Agree ;-)

@olafgrabienski
Copy link

For Drupal 8, there is the Configuration Split module. Maybe an interesting approach also for Backdrop?

@herbdool
Copy link

@jenlampton This is what Nate mentioned to me a while back in gitter (don't have the link anymore):

$conf only affects variable_set/get(), which are deprecated as they're not config-management friendly. If it's something like an authorization token you're storing, $settings in settings.php is the best bet, then use settings_get(). We've had a few modules that want things in config or settings.php, and we don't have a universal way of overriding config_get(). So what I've seen is doing a settings_get() first, and if empty, use config_get()
$setting = settings_get('my_setting');
$setting = isset($setting) ? $setting : config_get('my_module.settings', 'my_setting');

So the module needs to support this alternative way of overriding config and I think few do. I made it so it would work with devel modules like Reroute Email so it could be set per environment.

I'm not sure if overall this would be a need for the 80% or not.

@jenlampton jenlampton added this to the 1.9.0 milestone Aug 24, 2017
@jenlampton
Copy link
Member

I feel like the solution we had in D7 was a good one. for the few settings you wanted to override per environment you could specify those in settings.php. Something at the core level that takes this into account after config sync, and on config read would likely be the best way to handle it. I'm not sure something like that even could live in contrib. Maybe?

@jlfranklin
Copy link
Member

@robertgarrigos , Silkscreen 1.8-dev supports this. You'll need to grab the latest Silkscreen 1.x branch, the Layered configuration driver and the Memory configuration driver, then follow the set up instructions in the Layered README.md.

Silkscreen is a drop-in replacement for Backdrop, much like Pressflow was for Drupal 6. The current 1.x branch is missing the last 8 weeks of Backdrop development, because I'm waiting for BD 1.8 to drop before doing the next merge.

@robertgarrigos
Copy link
Author

thanks @jlfranklin I'll have a look.

@jenlampton jenlampton removed this from the 1.9.0 milestone Jan 15, 2018
@opi
Copy link

opi commented Jul 31, 2018

For the example, these are some settings (from Drupal) that I'd like to override locally, but still want the production value to be saved in my config files:

# Debug
$conf['error_level'] = ERROR_REPORTING_DISPLAY_ALL;
$conf['theme_debug'] = TRUE;
# Reroute email
$conf['reroute_email_address'] = "user@domain";
$conf['reroute_email_enable'] = true;
$conf['reroute_email_enable_message'] = true;
# Performance & caching
$conf['cache'] = FALSE;
$conf['block_cache'] = FALSE;
$conf['preprocess_css'] = FALSE;
$conf['preprocess_js'] = FALSE;
# LESS
$conf['less_devel'] = TRUE;
$conf['less_watch'] = FALSE;

Source https://gist.github.com/opi/fda934602521d69291c308f030a01c68

@klonos
Copy link
Member

klonos commented Apr 16, 2019

Related: #3300

@herbdool
Copy link

It's not an easy solution and not sure we can avoid an API change. In D7 the problem was that if you saved a form that had overridden values those values got saved into the db, which probably confuses people. D8 came up with a complex solution to avoid this https://www.drupal.org/docs/8/api/configuration-api/configuration-override-system. I'm not sure if a generic solution could be simple or not, or just identify a few variables and add specific PRs for them alone.

@herbdool
Copy link

I think this is the accepted approach for checking if there's an override in settings.php for a variable in config:

      $custom_theme = settings_get('maintenance_theme');
      if (empty($custom_theme)) {
        $custom_theme = config_get('system.core', 'maintenance_theme');
      }

https://github.com/backdrop/backdrop/blob/4426b2be634baa40dab541b2c6a248fe6b1ad6c6/core/includes/theme.maintenance.inc

I'm not sure if it's worth overhauling this but if people submit a PR then they also need to account for places in core and contrib that are already using this approach. And need to ensure that the override won't get saved to config if it's in a form.

@herbdool
Copy link

Here's an example of a non-intrusive, barebones approach for one variables, the error_level: backdrop/backdrop#2607 -- it only changes where error_level is called that it first checks the settings file.

It's backwards compatible and simple. And the override won't get saved to the config file accidentally. But it doesn't include any way of showing the end user that the variable is overridden in the settings file when they're on the form. That could be a different issue since it would be good to first allow people to use this and then improve the UX.

@jlfranklin
Copy link
Member

It's not an easy solution and not sure we can avoid an API change. In D7 the problem was that if you saved a form that had overridden values those values got saved into the db, which probably confuses people. D8 came up with a complex solution to avoid this.

D7 would save the value, but the form would continue to show the override. D8 (unless they've changed it) shows the database value, even when it's not in effect. I found D8 lying to me about the current state of the setting to be much more frustrating than D7.

I think this is the accepted approach. [...]

That's clearly built to be used in specific instances, and unfortunately it won't work in the general case. There are plenty of settings where the override could legitimately be something where empty() returns TRUE. If we patch CMI to check for overrides, we'll have to inspect the underlying settings variable directly to see if the array_key_exists().

@herbdool
Copy link

@jlfranklin that's why I took a different approach in my sample PR backdrop/backdrop#2607 by wrapping settings_get around the config_get the former which checks if the array key is set.

I figured we'd need a way to show which form items are overridden, but was thinking that for a minimal viable solution might be good to get something merged before that's figured out. We'd need to show a message for each form item that's overridden. Or disable them - easier to implement but still doesn't explain the override status.

@jenlampton
Copy link
Member

jenlampton commented Apr 22, 2019

I wonder it would be worth looking into adding a hook that would allow people to define the config names for keys that could be overridden. In core we could define the ~5 that we know are needed most often, but it would open the door to allowing contrib to add more.

We'd need to turn it around so config_get() called settings_get() instead, but only for those 5 magical keys.

If there was a match, we could also set a message to the user saying that X value had been overridden, and changing the setting in the UI may have no affect. It may even be possible to disable that form element too, if the hook provided the config name, a human-readable label (for the message), and a form API key (or keys) to disable the element.

@herbdool
Copy link

I've updated my proof of concept PR to provide some UX so admins can see if a variable is overridden. It uses a #pre_render and a new #config_variable property of the element (only way to know what config file and variable is associated with the form element).

Screenshot from 2019-04-23 16-23-48

I'm sure it could be prettier but wanted to share as a concept.

@jenlampton I don't like the idea of config_get calling settings_get, even if for only some variables. This seems like it invites an infinite loop. People can already call settings_get('variable', config_get('config_file.settings', 'variable)) so if they do that then it could end up calling itself ad infinitum.

@jenlampton
Copy link
Member

jenlampton commented Apr 23, 2019

I like the UX in the proof of concept @herbdool, but I don't like the implementation. The idea of needing to hard-code override support into every use of every variable in core turns my stomach.

People can already call settings_get('variable', config_get('config_file.settings', 'variable)) so if they do that then it could end up calling itself ad infinitum.

Oh jeez, yeah. I'd forgotten about that.

Maybe we need something new? Something that can take values set in settings.php and hook into config_get() in a way that works for contrib. Maybe overrides_get()?

@quicksketch
Copy link
Member

Merged into 1.x for 1.16.0. Thanks everyone for the great suggestions, code, reviews, tests, and feedback!

@herbdool
Copy link

Where would we add some documentation for this? And perhaps a follow-up issue for adding to the docs? @klonos you might know?

@klonos
Copy link
Member

klonos commented Jun 18, 2020

We already have inline documentation in the settings.php file itself @herbdool so we got that end covered. We could also add a change record in https://api.backdropcms.org/change-records since this seems more like a develop-y thing, or it could go under https://api.backdropcms.org/advanced-installation. Anyone has any better idea?

@herbdool
Copy link

@klonos there's a "needs change record" label on this so at least that. Though I'm not sure of the workflow for adding that.

Also something in the advanced section would be helpful. Someplace so if someone does an internet search it'll come up.

@klonos
Copy link
Member

klonos commented Jun 18, 2020

I've created a draft change record for this here: https://api.backdropcms.org/change-records/any-config-value-can-be-overridden-via-settingsphp

I tried to provide examples for things like specifying values that are arrays, and for nested settings.

I could use some 👀 on it re the validity of the examples.

PS: Once we have polished wording and examples in the change record to cover most common use cases, I think that it might be worth updating the docblock in the actual settings.php file.

@klonos
Copy link
Member

klonos commented Jun 18, 2020

...I'm reopening this issue for visibility purposes. Let's close once documentation and change record have been published.

@klonos klonos reopened this Jun 18, 2020
@herbdool
Copy link

@klonos I would preview it but I don't have a user account on the API site. Perhaps you or @jenlampton could give me one?

@jenlampton
Copy link
Member

A welcome message with further instructions has been e-mailed to the new user herbdool.

@herbdool
Copy link

thanks @jenlampton

@klonos I updated it. I used examples from the code here https://github.com/quicksketch/backdrop/blob/61ee9aa44d93808bf01bbd49991ebdbdee981857/core/includes/config.inc#L662.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants