-
Notifications
You must be signed in to change notification settings - Fork 41
Allow configuration storage in alternative locations (for example: the database). #2277
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
Comments
Getting Backdrop to store configuration in the database required the following changes:
This PR is based on and includes the patch for #2275. To use the patch, set I am able to use this patch to stand up a site and store configuration changes in the database. The tests are, of course, a complete mess. |
Test are fixed by adding a new Bootstrap stage: Still some work to be done (import/export is broken, for starters), but I'd like someone else to look at the extra bootstrap stage. |
Thanks @sentaidigital for making this issue! It's an item I've been thinking about on and off for quite a while. Using a database storage could make a lot of sense in various hosting situations. You're totally right that in a high-availability scenario, the database is much more accessible than the file system (though that should be shared too, and the options are a lot better now than they were a few years ago, e.g. Gluster and Amazon EFS). Pantheon in particular could benefit from this change, because it would make it so that if you downsynced your database, it would also include your configuration. The configuration directory isn't easily accessible on Pantheon anyway, so storing the configuration in the database might give you a multiple wins. The downsides are that database-storage is less accessible and visible than the file system for developers. Config files being easily accessible makes the entire operation of Backdrop seem more transparent and simple than it all being stored in the database. But overall, I think this is a great option to have, just as long as the default is the file system with the option to switch to the database. Speaking of which, do you have any ideas on how you could change your config storage type other than reinstalling? Seems like it might be a good candidate for a Drush issue.
I left a full review on the PR. The extra bootstrap phase in particular I think we could improve. Here's my comment from the PR:
For illustration, here's the code from /**
* Constructs a new BackdropDatabaseCache object.
*/
function __construct($bin) {
// All cache tables should be prefixed with 'cache_', except for the
// default 'cache' bin.
if ($bin != 'cache') {
$bin = 'cache_' . $bin;
}
$this->bin = $bin;
// Bootstrap the database if it is not yet available.
if (!function_exists('db_query') || backdrop_get_bootstrap_phase() < BACKDROP_BOOTSTRAP_DATABASE) {
backdrop_bootstrap(BACKDROP_BOOTSTRAP_DATABASE, FALSE);
}
} Seems like we could use that same bootstrap upgrading code in |
If the bootstrap upgrade works, I have no objections.
It's just JSON in the database record. It should be trivial to export from file and import into the database. Even if we add a way to do it in core, a Drush command would still be a good solution. Either way, migrating from one to the other is beyond the scope of this issue. I'd like to make the installer UX updates a separate issue, too. Right now, you have to specify the database in settings.php before you start the install, which is good enough to let more advanced site builders play with and break it while the UI is built. Regarding the JSON part (see comments in PR): Honestly, I'm not happy about the encoded settings part of this patch. My original plan was to store one setting per row in the database (more like the variables table) so developers and sites admins could change individual settings easily with simple SQL statements. I've had to do this a number of times to fix something broken in a D6 or D7 site. (In fact, I submitted a patch for Domain Access to do so.) Editing a scalar or simple array that's been serialized is doable. Editing a large array that has been serialized is error prone, to say the least. However, the data is sent to the Maybe we can change the semantics from full file to individual setting in 2.x, and leave the JSON functions duplicated until then. |
This does not work: // Bootstrap the database if it is not yet available.
if (!function_exists('db_query') || backdrop_get_bootstrap_phase() < BACKDROP_BOOTSTRAP_DATABASE) {
backdrop_bootstrap(BACKDROP_BOOTSTRAP_DATABASE, FALSE);
}
} But this does: // Bootstrap the database if it is not yet available.
if (!function_exists('db_query') || backdrop_get_bootstrap_phase() < BACKDROP_BOOTSTRAP_DATABASE) {
require_once BACKDROP_ROOT . '/core/includes/database/database.inc';
}
} The call to The bootstrap upgrade does work if we change that while loop from: while ($phases && $phase > $stored_phase && $final_phase > $stored_phase) { ... } to while ($phases && ($phase > $stored_phase || $final_phase > $stored_phase)) { ... } |
I was initially skeptical of D8's decision to move the active configuration back into the database, but after using it for a while, I'm starting to think it was a good idea. We really don't want to encourage manual editing the active configuration of a site directly. We should be telling people to export their configuration, edit the files, and then import the changes.
Should we be storing JSON in the database? Even if we are importing and exporting JSON to files, it seems like it might make more sense to store serialized PHP objects in the database records. Again, we don't want people making changes to the active configuration of the site, and should instead be leading them towards the export/change/import workflow. |
The most recent The |
@quicksketch if you have no objections, I'd like to move some of the changes here to the #2275, including any additions to |
Removed the API additions that are now in #2275 and cleaned up the patch to remove the reverted bootstrap changes. This should make the patch easier to review and do a better job keeping both issues in scope. |
Hi @sentaidigital and thanks for this PR. Unfortunately, I'm not yet sure this is the kind of change we should be making in Backdrop. My instincts are telling me that this kind of feature addition goes directly against Backdrop's principles.
This is an edge-case scenario. Changes specifically for this group should not be put in Backdrop core without careful consideration. If we start adding features that benefit the only minority of sites, we are going to end up with code bloat and increased complexity that adversely affects the majority.
You need to capture both the files and the database to get an exact snapshot. If you keep the config in your files directory (as it is by default) there's nothing else to download. Only when you move the configuration out of the files directory do you now need to capture a third thing. And even so, is it really that much more work to grab a config directory than it is to grab the files directory? Is it worth the cost?
But this isn't always wanted. How would you then down-sync your content only (without configuration)? I don't think Pantheon would add a checkbox for "Database with config" and one for "Database without config" in their UI. We separated content from configuration so that these things could be managed separately, and this feature would defeat that purpose.
These are some pretty serious downsides. Even if we don't want developers to be changing active config directly, having the files easily accessible is a huge win. It helps people learn what's going on more quickly, and that's very important for less-savvy developers. I may be wrong about the possible benefits to our majority use-case, but I'd be curious to hear more perspectives. Either way, I don't think this is a decision we should rush into, and I'm not sure why this issue was tagged with 1.6. We need to more carefully consider the implications of increasing the complexity and decreasing the visibility in exchange for a feature that may only benefit the 1%. |
This PR doesn't mandate how developers or vendors should approach configuration management in Backdrop. It only provides an option to store configuration in a database instead of in flat files. Developers, regardless of their savvy, can still use the default Nor does this require that database configuration be stored in the main database. Defining a second database connection Nor does it require the staged configuration to live in the database. Developers can still edit the configuration files in the staging directory and then import it in to the active config in the database. Actually, this is how I expect a site using database configuration to be managed. The implementation here is completely backwards compatible. Existing installations will continue to run with their file-based configuration, exactly as before. In fact, until a future PR updates the install process, the option to use the database for configuration will require someone to set it in their There's not a lot of complexity, either. The PR amounts to one additional class and a switch to choose between Yes, the load balanced environment argument is an edge case, and isn't the best argument to sell this. Security is a better one. Databases tend to be behind firewalls and only accessible by the webheads, which protects the configuration better than an easily misconfigured web server directive protecting one directory in an otherwise published There is also a DX argument to be made. I generally don't need to copy down the files directory, because I can look past the missing images or broken download links. Grabbing the entire configuration and data in a single operation is easier. Which is not to say grabbing both files and database is hard, only that one operation is easier than two operations, and harder to mess up. |
I'm still concerned about adding a feature that only benefits some extreme minority of sites, increases the amount of code we'd need to maintain, increases the overall complexity of the system, and decreases the visibility of how configuration files work when the feature is used. All of those things go against Backdrop's philosophy. That concern doesn't mean we shouldn't do this, or that it's a bad idea. All it means is that this decision needs to be considered carefully and not just merged in because the code looks good. What I'm asking for is more eyes on the problem/solution, more voices weighing in with opinions, and some more time to think about whether it's worthwhile to make this kind of change. We haven't had any PRs like this that go directly against our principles yet, it will be a good test of our process to see how we handle the decision. |
I don't think we/Backdrop should relegate high performance websites as an edge case. It seems perfectly reasonable to me that there will be sites that need load balancing and high performance techniques and it seems likely to me that those will be the ones that give Backdrop CMS the confidence boost for others to adopt. The tipping point for me is performance. @sentaidigital can we get some metrics:
If we can demonstrate that the performance is better or on par with the current situation then I think this is a good add with no impact for people that spin up Backdrop on shared hosting. They will never know about this setting until/and if they ever need it. I know we target small to medium size businesses and non-profits. I've worked on several non profit sites that are high traffic with load balancing and cacheing in place.
That is my 2¢; let's see if we can get those metrics. Here is a write up on how @quicksketch did some benchmarking: https://backdropcms.org/news/backdrop-vs-drupal-7-benchmarks-php-55-56-and-70 |
Why is less than 1% of sites not an edgecase? That's the exact definition of an edgecase!
Yes, I agree with this. Maybe high-profile sites using backdrop will sell others on using it, but that doesn't mean everything they need to do run a Backdrop site needs to go into core. I don't care so much about performance while the setting is ON, though seeing the metrics might be interesting. There should be a cost associated with keeping the config in the db, but we've never been able to measure it (before now). FTR: I'm less concerned with this particular issue and more concerned with how Backdrop will handle these edge-case feature requests, and how we do the cost/benefit analysis to figure out if/when requests like this should go in. |
I am doing a lot of CI/CD and multiple nodes systems. I keep configs in github repository as a part of the project and all config changes on production brunch get deployed to multiple servers at the same time. Good part of it is that you can rollback configuration without affecting database data. Also you can track who and when did changes to specific config and what was a reason. I suggest you to explore this option. |
@jenlampton - I usually works on edge-case and performance critical tasks. |
Quick datapoints: Test run of this PR using PHP 7 and file-based config: Link run_tests.php takes 4m05s There are a lot of variables here. The server may have been under higher load during the stock tests, which were run several days ago. @Gormartsen, I'd appreciate your help coming up with more reliable numbers. Edit: If these numbers hold, using the database speeds up the test suite by 9%. Edit 2: Database: Test run duration: 36 min 36 sec Database seems to be no worse than files. |
It seems as though this check is failing and sending back the default when bee is used:
In |
When running Then I don't know why that setting is no longer set! |
Thanks @indigoxela, you guessed correctly. I had left-over files that made it so the config files were being found even though they weren't the ones actually being used. I remember we had this problem with
Now seems like a good time to at least try fixing this in core, I think it might be as simple as not wiping out the |
By removing a single line that reset the global |
Do we have any core committers who haven’t been involved in this issue in some way? It’s looking great to me… |
Looks like I didn't fully fix |
I put in a new fix for the tests, the failure in |
7.5 years, 140 comments, 4 pull requests, many hours of puzzlement later, merged into 1.x as a new feature to be included in Big thanks to @sentaidigital and @jlfranklin for starting things off, and to @quicksketch and @hosef for the heavy lifting of getting tests working at the end. With reviews and assists in the middle from @herbdool, @laryn, @indigoxela, @mikemccaffrey, @jenlampton, @serundeputy, @Gormartsen, @klonos, @alexfinnarn, @kiamlaluno, @argiepiano. A great team effort! 🎉🎉🎉 Reopening for the needed change record. |
Reopening for Change Record |
There is one storage class available for
Config
to use:ConfigFileStorage
. I'd like the option of using aConfigDatabaseStorage
class that would save the same data, but in the database instead of in flat files.I can think of the following reasons why:
PR: backdrop/backdrop#4453
Replaces: backdrop/backdrop#1613Summary:
This is a feature request for an option that will likely only be used by a minority of sites running Backdrop. As you can see in the discussion below, there are other reasons it might still be a candidate for core inclusion. This option would be disabled by default, so there would be no change for existing sites, or new installs.
Here is a list of pros and cons for PMC review:
Pros
Cons:
The text was updated successfully, but these errors were encountered: