backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

Allow configuration storage in alternative locations (for example: the database). #2277

Closed sentaidigital closed 5 months ago

sentaidigital commented 8 years ago

There is one storage class available for Config to use: ConfigFileStorage. I'd like the option of using a ConfigDatabaseStorage 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: https://github.com/backdrop/backdrop/pull/4453 ~Replaces: https://github.com/backdrop/backdrop/pull/1613~


Summary:

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:

sentaidigital commented 8 years ago

Getting Backdrop to store configuration in the database required the following changes:

This PR is based on and includes the patch for backdrop/backdrop-issues#2275.

To use the patch, set $configuration_directories['active'] = 'db:/default/config_active'; and run the installer.

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.

sentaidigital commented 8 years ago

Test are fixed by adding a new Bootstrap stage: BACKDROP_BOOTSTRAP_DATABASE_EARLY, which has all the database bootstrap code, minus the test prefix validation. The validation remains in the original _backdrop_bootstrap_database() function.

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.

quicksketch commented 8 years ago

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'd like someone else to look at the extra bootstrap stage.

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:

Rather than adding a new bootstrap phase, would it be possible to just make the config bootstrap phase "upgrade" the boostrap level in the contructor for the new database config storage class? This is the same thing that BackdropDatabaseCache::__contstruct() does. Even though the cache backend is needed at boostrap level 2, it upgraded the level to 3 so that it can serve the page cache from the database.

For illustration, here's the code from BackdropDatabaseCache():

  /**
   * 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 ConfigDatabaseStorage::__construct() and it would do the job of making the database accessible earlier if you're using database-stored configuration.

sentaidigital commented 8 years ago

If the bootstrap upgrade works, I have no objections.

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.

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 ConfigStorageInterface is everything in the file, not just the deltas, which means write() would have to either delete the entire block of settings before writing out the new data or do some clever merging to prevent unnecessary writes to the database.

Maybe we can change the semantics from full file to individual setting in 2.x, and leave the JSON functions duplicated until then.

sentaidigital commented 8 years ago

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 backdrop_bootstrap(BACKDROP_BOOTSTRAP_DATABASE, FALSE) does not work because it never calls the _backdrop_bootstrap_database() function. When the constructor calls backdrop_bootstrap(), bootstrap does not update the $final_phase (bootstrap.inc:2668) because $new_phase is FALSE and the current $stored_phase and $final_phase are both 0 (BACKDROP_BOOTSTRAP_CONFIGURATION), so the while loop (bootstrap.inc:2673) never runs.

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)) { ... }
mikemccaffrey commented 8 years ago

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.

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.

It's just JSON in the database record. It should be trivial to export from file and import into the database.

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.

sentaidigital commented 8 years ago

The most recent hook_update_N() from #2222 revealed some issues in the update code. A couple patches addressing those issues are now here.

The exportArchive() and new importArchive() member functions are updated, but not tested.

sentaidigital commented 8 years ago

@quicksketch if you have no objections, I'd like to move some of the changes here to the #2275, including any additions to ConfigStorageInterface and their implementation in ConfigFileStorage, including the is_dir() patch from yesterday and the exportArchive() / importArchive() changes.

sentaidigital commented 8 years ago

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.

jenlampton commented 7 years ago

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.

Makes it easier to synchronize config changes among multiple web servers in a load balanced environment.

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.

Makes it easier to capture the entire state of an environment for debugging elsewhere.

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?

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.

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.

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.

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

sentaidigital commented 7 years ago

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 ConfigFileStorage to store configuration in files.

Nor does this require that database configuration be stored in the main database. Defining a second database connection $databases['config'] = array(...) and setting the configuration path to db:/config/active_config would preserve the separation of content and configuration.

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 settings.php before running the install process.

There's not a lot of complexity, either. The PR amounts to one additional class and a switch to choose between ConfigFileStorage and ConfigDatabaseStorage. It's a textbook example of how OO development should be done. The only other piece is a small patch in bootstrap.inc to ensure the database is initialized in time to load the config from the database. If the PR looks like more, it's because the first nine commits are the commits from #2275. Splitting the PR into two, one to refactor and clean up the existing, one to add a new storage class, was done to make them both easier to review.

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 files directory.

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.

jenlampton commented 7 years ago

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.

serundeputy commented 7 years ago

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.

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.

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 and the corresponding post for D8 http://www.jeffgeerling.com/blogs/jeff-geerling/benchmarking-drupal-8-php-7-vs-hhvm

jenlampton commented 7 years ago

I don't think we/Backdrop should relegate high performance websites as an edge case.

Why is less than 1% of sites not an edgecase? That's the exact definition of an edgecase!

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.

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.

Gormartsen commented 7 years ago
  • Makes it easier to synchronize config changes among multiple web servers in a load balanced environment.
  • Makes it easier to capture the entire state of an environment for debugging elsewhere.

Hi @sentaidigital

I am doing a lot of CI/CD and multiple nodes systems. I found that keeping configs in json files actually do make deployment and testing much easier.

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. When properly adopt, it makes development for big projects more secure and stable. And much easier.

Gormartsen commented 7 years ago

@jenlampton - I usually works on edge-case and performance critical tasks. I will be more than happy to do measurements and tests for this type requests and if necessary provide alternative ideas to handle it.

sentaidigital commented 7 years ago

Quick datapoints:

Test run of this PR using PHP 7 and file-based config: Link run_tests.php takes 4m05s Test run of this PR using PHP 7 and db-based config: Link run_tests.php takes 3m42s with 1 exception Same with fixed exception: Link run_tests.php takes 3m43s

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: Running the tests on my own VM:

Database: Test run duration: 36 min 36 sec Files: Test run duration: 38 min 13 sec

Database seems to be no worse than files.

Gormartsen commented 7 years ago

@sentaidigital Tnx for numbers! I will be able to do extra speed tests only after December 7. (I am still in Peru, doing traveling)

I saw latest commit. We do need to came up with proper strategy to do all tests under files and db configs to avoid any possible exceptions in future.

Gormartsen commented 7 years ago

Ps: each test run on new google cloud instance, so here is no effect on tests speed if you run many tests simultaneously

sentaidigital commented 7 years ago

The patch currently uses array_column(), which doesn't exist before PHP 5.5. There is a compatibility function here under the MIT license.

What is core's policy on including such files with non-GPL'd code?

Gormartsen commented 7 years ago

@sentaidigital I created PR backdrop/backdrop#1661

I added small patch to move config/active to /dev/shm (memory based partition).

This way we are comparing speed for configs stored in database in memory (MySQL files in /dev/shm/mysql on test instances ) and in files in memory (/dev/shm)

Config in MySQL: php7 - 3m 43s link php5 failed due to array_column()

Config in files in memory: php7 - 3m 29s link php53 - 7m link

Config in files on SSD drive (backdrop/backdrop#1656): php7 - 3m 42s link 3m 39s link php53 - 7m 26s link

Based on this results I can say that there is no speed improvement from Database based configs for tests.

Speed stay +- the same. Even using /dev/shm for files is not affecting performance too much.

Numbers could be different on SATA based servers due to bigger seek time (But FS usually cache most requested files as well)

jenlampton commented 7 years ago

Is there any reason this can't be done in contrib?

Gormartsen commented 7 years ago

I like an idea of adding API changes to config (like we have for cache) to make it possible to replace default ConfigFileStorage.

For example, XCacheConfigStorage or even MemcacheConfigStorage could be a good contrib modules.

jenlampton commented 7 years ago

I do too. Then this feature (or anything simular needed in the future) could exist in contrib, and we'd be adding less that isn't for our key users. I just have a feeling it will be difficult, and it would be good to discuss the pain-points as those are arguments for core inclusion.

sentaidigital commented 7 years ago

Is there any reason this can't be done in contrib?

Bootstrap. We have to be able to load a module to load the configuration that we don't know is enabled until we load the configuration.

Gormartsen commented 7 years ago

@sentaidigital - same as cache. So yes, we do need patch core to make it possible to replace default config class via settings.php for example.

Gormartsen commented 7 years ago

upd: I mean, we need to change core to make possible something like this:

settings.php

$settings['config_default_class'] = 'ConfigDatabaseStorage';
$settings['config_backends'] = array('modules/config_db/config_db.class.php');
sentaidigital commented 7 years ago

Adding $settings['config_backends'] is a big enough change to warrant its own issue.

Gormartsen commented 7 years ago

@sentaidigital basically I am proposing to change core in a way to make it possible to use contrib module for ConfigDatabaseStorage

This way we minimize core changes and you can have contrib module for your needs. Kinda compromise

sentaidigital commented 7 years ago

A contrib module will eventually need a way to tell install.php that ConfigDatabaseStorage is available as a backend, before any modules are initialized, even core modules like system or user. I know that changes to support install time selection are not in this patch, but this patch also doesn't make such changes harder.

It's not that this is impossible, but as I mentioned in the review of the PR, "doing so would radically change the scope of this issue."

Even a "simple" patch to add $settings['config_backends'] has ripple effects on parts of bootstrap and config_get_config_storage(), which needs to know the difference between db://active_config vs files/active_config. Setting $settings['config_default_class'] doesn't work because the active configuration may be in the database while the staging configuration is in files.

We can use the $settings['config_backends'][$classname] = "path/to/class.inc" to specific the file and the class name, add public static member functions that return TRUE if they can parse/validate the config path, and another to tell install.php whatever it needs. (I haven't even looked at that code.) Of course, the $settings stuff all needs to be set before running install.php, which is an awful lot to ask of less savvy users.

I'd prefer a solution where a site builder needs only drop in the module to get the new storage class, and use the same solution to support new DatabaseConnection classes and BackdropCacheInterface classes, too. The settings.php solution, while it avoids certain chicken-and-egg issues, always felt clunky to me and required an advanced understanding of the system to even know the *_backends exist.

But, it is the chicken-and-egg issues that make it so hard. We can't use the autoloader because it's not even registered until the end of BOOTSTRAP_DATABASE. How early can we scan every module's .info file and how bad of performance hit would that cause? Maybe install.php should do the scan and symlink the backends into a particular directory that early bootstrap scans or write out a backends.php next to settings.php, but then simply adding a module doesn't add the backends, even though they are clearly in the .info file, which is not how anything else in Backdrop works. Do we have to set a special directory and class of module for backends like we do themes and layouts?

The core of the problem is backends are different from other contrib modules. Config, Database, and Cache classes available in contrib introduces a device driver-like class of contrib module. It's been clunked in with Cache modules so far. This has been less of an issue, because the sites that need advanced caching have more savvy developers building and maintaining them, but it's always been a one-off. It's more of an issue for sites that would like to use other simple databases, such as SQLite or the desktop version of SQL Server (SQL CE?), and for site builders used to the D6, D7, and current D8 practice of storing configuration in the database.

These are issues I was hoping to defer to a later time, because they're big enough to warrant their own issue. Compared to a low-level plug-in system, adding a second class implementing ConfigStorageInterface plus a bit of plumbing to support it is kinderspiel.

jenlampton commented 7 years ago

What is core's policy on including such files with non-GPL'd code?

@sentaidigital MIT is GPL-compatible, so adding that function would be fine.

A contrib module will eventually need a way to tell install.php that ConfigDatabaseStorage is available as a backend, before any modules are initialized, even core modules like system or user.

Could we still use the file system for config during install, and simply move all the config into the DB when installation is complete?

The core of the problem is backends are different from other contrib modules. Config, Database, and Cache classes available in contrib introduces a device driver-like class of contrib module. It's been clunked in with Cache modules so far. This has been less of an issue, because the sites that need advanced caching have more savvy developers building and maintaining them, but it's always been a one-off.

The same logic works for this feature too, the sites that need db config storage also have more savvy developers building and maintaining them.

It's more of an issue for sites that would like to use other simple databases, such as SQLite or the desktop version of SQL Server (SQL CE?).

We've dropped support for anything other than MySQL (and MariaDB) from Backdrop, because that is also an extreme edgecase. Sites that would like to use these other simple databases will not be using Backdrop CMS.

These are issues I was hoping to defer to a later time, because they're big enough to warrant their own issue. Compared to a low-level plug-in system, adding a second class implementing ConfigStorageInterface plus a bit of plumbing to support it is kinderspiel.

Even though the code in this PR may be minimal, we don't usually add features to Backdrop unless we're sure that at least 80% of sites will benefit. Here, that is certainly not the case. However, sometimes these decisions are not always so black and white. There are other factors that may influence the decision, and one thing we should consider is what would need to be done in core to make this possible in contrib.

If comparing the two approaches reveals that what has to be done to core to support this kind of feature in contrib is far more complex and has a worse effect on performance than adding it to core, that may encourage us to make an exception in this case.

klonos commented 7 years ago

If comparing the two approaches reveals that what has to be done to core to support this kind of feature in contrib is far more complex and has a worse effect on performance than adding it to core, that may encourage us to make an exception in this case.

This sounds reasonable to me for non-80% cases.

mikemccaffrey commented 7 years ago

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 files directory.

This is a very good point. Our current system of adding the long hash in the config file directory is kinda a kludgy workaround, and it is never good to rely on security through obscurity.

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.

This is my experience with developing drupal sites as well. A single database sync to get the latest content and configuration is much easier than doing a file sync as well, especially on sites that have a large number of files. In fact, I most often use stage_file_proxy and don't sync the files directory to dev.

Even though the code in this PR may be minimal, we don't usually add features to Backdrop unless we're sure that at least 80% of sites will benefit. Here, that is certainly not the case. However, sometimes these decisions are not always so black and white. There are other factors that may influence the decision, and one thing we should consider is what would need to be done in core to make this possible in contrib.

I would not classify this issue as adding a feature to core. I'm leaning towards thinking that this should be our default config storage solution, especially since it would bring us in closer parity to how config storage works in D8, so users don't need to learn yet another way to manage things.

I'd like to propose that we add this storage option to core in the next release or two so we can try it out, make it the default config storage option if it works well in another release, and then remove file storage as an option in 2.x unless there is a clear reason to continue offering that as an option.

Gormartsen commented 7 years ago

@mikemccaffrey - I have strong feeling that BD has to go his own path. I see that getting anything that belong to structure to files is essential to backdrop future.

Using json files to store state and structure data is my favourite feature for next reason:

I don't use UX to import export settings (configs) at all.

Quick example: Task: I need to update layout settings. My steps: 1 - update it on my local env 2 - git commit layout config changes to github 3 - deploy config to production (via zen.ci, circleci or any other CD system or even just git pull) Done.

jenlampton commented 7 years ago

I agree with @Gormartsen. There are other solutions to the issues mentioned above that don't have the downsides of this approach. There won't be very much overlap with Drupal 8 in the long run, and we need to make our own decisions. The people we are targeting with Backdrop are the people who would benefit the least (and suffer the most) from this kind of change. We need to be careful when making this decision that we aren't doing what we want personally, but are instead acting in the best interests of the people this project is intended for.

Backdrop values site builders over coders. Backdrop values contrib developers over core developers.

There are tools for the experts that can be used for config syncs and db syncs and to make the act of copying files easier. Instead, we should be focused on how easy it is to get a non-developer into using Backdrop, to understand how it works, and not be throwing extra hurdles in their way to benefit the pros.

sentaidigital commented 7 years ago

Instead, we should be focused on how easy it is to get a non-developer into using Backdrop, to understand how it works, and not be throwing extra hurdles in their way to benefit the pros.

If we can focus on the patch, what extra hurdle is being added here? The patch does not change the default configuration scheme, nor does it change the install process. Anyone who isn't reading the release notes won't even know they can store configuration in the database.

A future patch will add radio buttons to the installer that read "Config in files" and "Config in database", with the former selected by default. We can bury it in a collapsed "Advanced options" fieldset or vertical tab. The current case will be preserved, another option will be available. A single radio element is not a hurdle.

Ensuring the common case is easy is a great goal. Denying pros the tools they want and need because we're afraid the tools may be confusing turns away the very people we want helping and mentoring the non-developers and the rookie developers. In the end, it just hurts the community.

There should be room in the Backdrop philosophy for the classic Perlism, "easy things should be easy, difficult things should be possible."

Gormartsen commented 7 years ago

I feel sad that I miss developer meeting. That was a hot and important one. Thank you for your opinions guys, it helped me to shape mine.

My opinion can be counted as a DevOps, a hosting company and advanced developer opinion.

Going back to feature request. I like work that is happening in #2383. This feature is contrib developers friendly. It is matching our philosophy.

jenlampton commented 7 years ago

If we can focus on the patch, what extra hurdle is being added here?

Not much, maybe just more code? (that was actually addressing the "make it default" comment from above) :)

There should be room in the Backdrop philosophy for the classic Perlism, "easy things should be easy, difficult things should be possible."

I agree with this 100%, that's why I asked about what's possible to do in contrib. If it's possible (even if it's a hack) that decreases the need to get this change into core, at least right away. I'd love to see a contrib module first to gauge interest, then a core issue if it turns out to be in high demand.

quicksketch commented 7 years ago

The issue for more consistent accessing of the file-based config storage has been merged at https://github.com/backdrop/backdrop-issues/issues/2275.

It might be worth it to reroll https://github.com/backdrop/backdrop/pull/1613 to get an idea of how much effort is now involved here. With the API additions all separated out, it may make the scope of change here more clear.

Thanks again @jlfranklin for your iterative approach in separating these tasks out for evaluation one at a time. I don't think this will be able to make it for 1.6.0, but we're going to be in a much better place either for 1.7 or your new Silkscreen performance fork.

jlfranklin commented 7 years ago

I don't mind refiling the PR, but I thought this one was already declined for core.

jenlampton commented 7 years ago

I thought this one was already declined for core.

It was declined for core right now, but knowing how much change is required will help us re-evaluate in the future. If this is something some people want, then we'll need to be able to measure the change required so we can decide if it's worth the cost for everyone else.

alexfinnarn commented 6 years ago

Hello folks! I've tried out this PR and like the simplicity of only having to change one line in settings.php and the code changes seem nice and tidy.

I'll go through the steps I took to test:

I also repeated the same process with the normal ConfigFileStorage way of working with config. My main reason for testing out this PR was running into this issue on Pantheon: https://github.com/backdrop-ops/backdrop-pantheon/issues/27. The issue seems to be intermittent, and I can't point to any reason why other than something with the Terminus plugin for cloning code or how Valhalla works for projects that aren't Drupal or WP. If someone is evaluating Backdrop on Pantheon and runs into that issue, though, it might turn them off of their investigation.

Where I work, we run a service for hundreds of sites (some would say 1000+) on D7 and are used to updating config using update hooks and Features when deploying code. Due to the nature of our current infrastructure, it would be far easier for us to store the config in the db and remove a blocker. We don't have a process of working with config in files and that change would be difficult for us to make. Granted, doing things this way goes against moving config into files, and I can't really find anything in https://backdropcms.org/philosophy pointing to changes like this one.

So, what is the feeling on this issue after almost two years have gone by? I've read the comments about pros and cons to this change but it only looks like adding a couple of conditional statements and a class that follows a well-defined interface. I can't comment too much on the code or performance considerations, though, only that the PR worked for me in limited testing and I like the idea. Are there specific types of testing I should be doing?

One interesting question to point out on Backdrop philosophy page:

  1. Focus: Include features for the majority. Backdrop core should only include features and tools that benefit the majority of sites that are running it.

What if a few groups who run services with hundreds of sites migrate to Backdrop and they now make up the majority of sites. Does this change then become more in-line with project philosophy? Even going back to the project focusing on site builders...what if the site builders work for these companies? What if the editors prefer to work with db storage and they all work for these companies? Maybe not a topic for this issue in particular, but it is a question that we think about while evaluating Backdrop coming from Drupal and the promise of the PMC being better than BDFL governance. If larger groups go all-in on Backdrop and are still "not the users Backdrop is intended for", then they're probably not going to want to join in and potentially ruin the community's feel.

jlfranklin commented 6 years ago

@alexfinnarn , have you considered running Silkscreen CMS, the fork of Backdrop that includes those patches and continues to track the other changes? It is intended to be a drop-in replacement for Backdrop, like Pressflow was for Drupal 6.

alexfinnarn commented 6 years ago

@jlfranklin I know we've looked at Silkscreen but have been more focused on evaluating the main Backdrop project against other alternatives for a migration path off of D7.

klonos commented 6 years ago

@alexfinnarn without #285 in place, we cannot be 100% sure of what the majority uses and how. I think that so far we have been going by gut feeling so to say. There have been some features added in core for which we have absolutely no hard proof that they would serve the majority or not.

It is true that the project started with only a handful of people, and it was easy to discuss and see what "the majority" felt like. As we grow, I think that #285 will become a necessity if we are to get a good feel of what works for most people. Having said that, there was also no hard proof that everybody hated the overlay.module ๐Ÿ˜œ, but we did remove it. My point is that I feel that we have been doing a good job so far.

Speaking of #285, even when I was reading https://www.colorado.edu/webcentral/2018/04/11/change-my-view-d8-isnt-best-upgrade-path-1000-d7-edu-sites I was thinking "Well something like that would surely throw our stats off" ๐Ÿ˜„ ...I am sure that cases like that would come along over time, but I have hope that the community will grow enough that 1000 sites would only be a minority (๐Ÿ™ ๐Ÿ˜„)

jenlampton commented 5 years ago

The PMC has voted to make config storage swappable.

It's unclear to me whether the ConfigDatabaseStorage class should be included in core or needs to be provided by contrib, but we will need it to test the swapability anyway, so work can proceed on this issue even without a clear decision on where it will live.

I will ask for clarification in the PMC issue.

klonos commented 1 year ago

I realize that there hasn't been any movement here recently, but I don't want to dismiss this by jumping to conclusions re demand for this feature. I think that at the very least core should make this easy/possible in contrib (without sacrificing performance for the rest of the "80%" that is).

With #285 now done, and https://backdropcms.org/project/backdrop/telemetry providing us a decent amount of data (I'm sure we'd love it if the sites reporting back metrics were more - that will come with time), wondering if it'd be worth it capturing usage stats for Silkscreen ๐Ÿค” ...it would certainly help us make better-informed decisions in matters like this one here I think.

klonos commented 1 year ago

Hey @jlfranklin are you still actively maintaining the project by the way? ...asking because I see that there's a broken image in the homepage of https://www.silkscreencms.org and https://github.com/silkscreencms/silkscreen is reporting v1.21.5 as the most recent. ...is there anything we can do to help? ...can we automate the sync between the two projects perhaps?

Also, do you have any rough idea of usage of the project? (have you changed anything related to where Silkscreen installations send their stats/telemetry, or is it still the main b.org site?)

avpaderno commented 1 year ago

Saving the configuration data in the database would help those sites with a low disk usage limit for files. In my case, the database disk usage is limited to 50 GB, which is the same limit I have for files. There could be planes with a lower disk usage for files, which give more space for database tables.

klonos commented 1 year ago

Although a valid argument @kiamlaluno, still, how much space does config use up? A typical Backdrop site on my local has something like ~500KB of config files. So the disk space savings are kinda negligible.