page_cache_without_database doesn't return cached pages without the database

Fix config system to work in this instance - should we revert in instances where the db is not available to using the canonical storage (at the moment the xml file in sites/all/files?

No the file store is not canonical storage, so we can't fall back to it.

This is a regression. If anything we should revert the system performance settings in config then look at this over again, page caching settings are designed to be readable from settings.php and we've lost that feature.

I know Crell has plans to convert page caching over to HttpCache but I've not seen a plan for how to do that yet.

Is it the page cache that

Fatal error: Call to undefined function Drupal\Core\Cache\db_query() in .../core/lib/Drupal/Core/Cache/DatabaseBackend.php on line 66

is failing on? Or a different cache item?

+++ b/core/lib/Drupal/Core/Config/DatabaseStorage.phpundefined
--- a/core/modules/config/config.test
+++ b/core/modules/config/config.testundefined

+++ b/core/modules/config/config.testundefined
+++ b/core/modules/config/config.testundefined
@@ -293,3 +293,44 @@ class ConfUpdate7to8TestCase extends WebTestBase {

@@ -293,3 +293,44 @@ class ConfUpdate7to8TestCase extends WebTestBase {
     $this->assertEqual($config->get('config_test_bar'), $this->testContent);
   }
 }
+
+/**
+ * Tests using Configuration if the database is not available
+ */
+class ConfEarlyConfigGetTestCase extends WebTestBase {
+
+  public static function getInfo() {
+    return array(
+      'name' => 'Configuration use if database unavailable',

We are starting the conversion of test classees to PSR-0. I suggest you already move this one to the new structure, that's one less to port tomorrow. http://drupal.org/node/1543796 should contain all information you need.

The addition of a test/UA specific settings.php bugs me. Need to think about that.

Just a reroll to chase HEAD, plus the PSR-0 change from #13. I have not reviewed this otherwise. CNR for bot.

This merges #15 with #1323120-110: Convert cache system to PSR-0 and removes redundancies. I think it's good to go, but needs someone to RTBC.

+++ b/core/modules/system/lib/Drupal/system/Tests/Bootstrap/PageCacheTest.php
@@ -112,6 +112,22 @@ class PageCacheTest extends WebTestBase {
+      "\$conf['cache_classes']['cache'] = 'Drupal\\Core\\Cache\\NullBackend';",
+      "\$conf['cache_backends'][] = 'core/modules/system/tests/modules/bootstrap_test/lib/Drupal/bootstrap_test/Cache/TestBackend.php';",
+      "\$conf['cache_classes']['cache'] = 'Drupal\\Core\\Cache\\DatabaseBackend';",

First line is silly (gets overridden by last), so this patch removes it.

Even though some of the code in #17 is mine, there's not much non-test code, and of what there is, the majority is alexpott's, so I feel ok with RTBC'ing this.

Re #14, it's been a month, and no alternative has been proposed. Even if this lands, if we come up with something better later, we can follow-up with that, but for now, we should be able to test what this patch tests, and I can't think of a better way to do it.

Curious to learn if @sun has more to say about the addition of the UA stuff.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -852,4 +864,32 @@ abstract class TestBase {
+    $contents .= "\$conf += " . drupal_var_export($conf) . ";\n";

Will be irrelevant if we end up doing #19, but for the interim between this issue and that one, in case of key conflict, I think the simpletest settings should take priority, so $contents .= drupal_var_export($conf) . "+ \$conf;\n";

Otherwise, this looks RTBC to me, but given #20, leaving for sun to mark it as such.

+++ b/core/includes/bootstrap.inc
@@ -675,6 +675,14 @@ function drupal_settings_initialize() {
   if (file_exists(DRUPAL_ROOT . '/' . $conf_path . '/settings.php')) {
     include_once DRUPAL_ROOT . '/' . $conf_path . '/settings.php';
   }
+
+  // If running as a SimpleTest child site, load additional per-test settings.
+  if ($test_prefix = drupal_valid_test_ua()) {

I thought we would load the test environment's settings.php instead of the regular settings.php. This would potentially allow us to test the full Drupal installer. However, since that potentially involves security concerns, I'm fine with deferring the approach of an entirely different settings.php (instead of overriding $conf only) to a separate issue; possibly #630446: Allow SimpleTest to test the non-interactive installer

+++ b/core/includes/bootstrap.inc
@@ -675,6 +675,14 @@ function drupal_settings_initialize() {
+    if (file_exists(DRUPAL_ROOT . '/' . conf_path() . '/files/simpletest/' . substr($test_prefix, 10) . '/settings/simpletest_settings.php')) {
+      include_once DRUPAL_ROOT . '/' . conf_path() . '/files/simpletest/' . substr($test_prefix, 10) . '/settings/simpletest_settings.php';

It would make sense to prepare this filepath as a variable instead of repeating the long string concatenation.

+++ b/core/includes/bootstrap.inc
@@ -2305,14 +2313,14 @@ function _drupal_bootstrap_page_cache() {
-      if (variable_get('page_cache_invoke_hooks', TRUE)) {

I do not understand why this variable is not part of system.performance.

+++ b/core/includes/bootstrap.inc
@@ -2305,14 +2313,14 @@ function _drupal_bootstrap_page_cache() {
+      if (!variable_get('page_cache_without_database') && variable_get('page_cache_invoke_hooks', TRUE)) {
...
+      if (!variable_get('page_cache_without_database') && variable_get('page_cache_invoke_hooks', TRUE)) {

1) The two configuration values should be prepared once in two variables instead of re-requesting them twice.

2) If I understand the entire changes in this patch correctly, then I don't really understand how this advanced page cache without database feature was able to work in the past. How were you able to serve a page from cache without a DB, if there are various calls to retrieve variables/configuration involved? Was the intention for this feature to only work if you manually hard-code all of the required variables into $conf in settings.php, so that variable_get() would always have a value and would never try to consult the database?

If that was the intention and still is, and if we want or have to retain that feature, then we need to do much more drastic changes to the config system architecture, because this kind of $conf-only operation via settings.php is not really foreseen at all in the current architecture (and neither in the refactored) at this point.

+++ b/core/lib/Drupal/Core/Config/DatabaseStorage.php
@@ -19,13 +19,15 @@ class DatabaseStorage extends StorageBase {
+    if (function_exists('db_query')) {

#1605324: Configuration system cleanup and rearchitecture will make this change obsolete, since it allows the config system to fully operate as soon as the class autoloader has been registered.

I'd therefore prefer to postpone this issue on that change.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -653,6 +658,13 @@ abstract class TestBase {
+    $this->additionalSettingsDirectory = $this->originalFileDirectory . '/simpletest/' . substr($this->databasePrefix, 10) . '/settings';

Instead of moving the settings into a subdirectory, we should move the public files directory into a ./files subdirectory. In turn, we'll essentially resemble the directory structure of a regular site in /sites/[name]; i.e.:

./config/
./files/
./private/
./temp/
./settings.php

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -852,4 +864,32 @@ abstract class TestBase {
+  protected function createSimpletestSettingsFile($conf = array()) {

1) Should be drupalCreateSettingsFile().

2) I'd also like to see the corresponding removal function drupalDeleteSettingsFile() to be added back, since we'll likely need that when #1411074: Add a flag to set up test environment only once per test class; introduce setUpBeforeClass() and tearDownAfterClass() [PHPUnit] is done. Concurrent test runners only operate on a single test class at once, and each test class gets its own simpletest environment files directory. In light of that, it would be great if we could add a call to drupalDeleteSettingsFile() at the end of the new testPageCacheWithoutDatabase() test method.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -852,4 +864,32 @@ abstract class TestBase {
+    include_once './core/includes/utility.inc';

Should use DRUPAL_ROOT.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -852,4 +864,32 @@ abstract class TestBase {
+    $contents = <<<EOF
+<?php
+/**
+ * @file
+ * Simpletest specific configuration file.
+ *
+ */
+
+EOF;

We totally don't need coding standard conforming code here. ;) The opening PHP tag and a \n is definitely sufficient.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -852,4 +864,32 @@ abstract class TestBase {
+    file_put_contents($this->additionalSettingsDirectory . '/simpletest_settings.php', $contents);

Don't think we need to repeat "simpletest" in the filename; just settings.php should be sufficient.

#12: 1576322_12_early_config.patch queued for re-testing.

Attached patch implements everything of #27.

It even goes beyond that. This should even enable us to test the Drupal installer. YARRRR! ;)

ok - this is seriously veeeery odd ;)

Tests are passing for me locally, both with Simpletest as well as run-tests.sh. I don't see why they should not.

Excellent review by @chx:

02:48	<chx>	sun: you are putting database credentials inside the files directory?
02:50	<sun>	oink. that's a problem. indeed. ;)
02:52	<sun>	hm. Sometimes I wish we'd just simply say: "You want to run tests on a public/production site? Are you nuts?" :P

#29: config.cache_.29.patch queued for re-testing.

I cannot seem to duplicate the issue in the OP. I thought it was maybe that the variable had been moved to CMI subsequently, but bootstrap.inc still shows "if (variable_get('page_cache_without_database')) {" Cleared caches and the main page still shows up fine for anon.

Whether the specific database error shows up or not, this is definitely broken at the moment - i.e. page_cache_without_database is either going to trigger errors, or it's not going to prevent hitting the database. We either need to fix it, or remove it and just accept that internal page caching is going to be slow as mollasses in Drupal 8.

Was reminded of this issue by #1824778: Convert css_gzip_compression and js_gzip_compression variables to CMI system.

For #35, you actually have to have page cache turned on for this error to occur (I spent some time trying to chase down why I wasn't getting this error too, and that was it.)

The error is now

Fatal error: Call to undefined function module_list() in /Users/gdd/Sites/drupal/core/includes/bootstrap.inc on line 1130

but this is definitely still an issue.

I'm not entirely sure what has changed in the config system since this issue was originally posted, but the attached patch will make things not crash when $page_cache_without_database is set to TRUE. Basically, when set to TRUE, drupal does not bootstrap to DRUPAL_BOOTSTRAP_VARIABLES, which is where module.inc is loaded, which causes the call to module_list() in bootstrap_invoke_all() to fail. I moved this include to bootstrap_invoke_all() instead. I'm not sure that is the best place, but it does seem to be the first place that module.inc is needed. It still doesn't address the testing problems.

Now, whether everything is still working as intended or $page_cache_without_database is doing what its original intention was, I'm unsure. That will require more investigation. One change that happened since the original issue was posted is that we moved away from using db_query() to instead use the DB classes directly. So a lot of config calls are probably still happening, they just aren't bombing anymore.

Pfeh I'm dumb, that just moved the problem. However, talking to catch in IRC, he pointed out that $page_cache_without_database = TRUE; is not really supposed to work except in conjunction with $page_cache_invoke_hooks = FALSE;. If you set both of these variables properly, then HEAD works as planned.

Now, it does still hit the database on a default install because config() still gets called (16 times), however that probably deserves to be addressed more in #1187726: Add caching for configuration / rework config object loading (Was: Memory usage and i/o from config objects) or a related CMI performance issue, and it's not critical either.

Therefore, I'm closing this.

#29 has some test coverage we might still want to add, unless equivalent coverage has already been added in other issues.

Not sure if missing test coverage counts as a bug, so moving this to a task.

Just talked to heyrocker about this in irc.

There are still at least three config() calls (and possibly more to come) that get fired when this setting is on. These hit the db on cache hits, but with pluggable CMI caching they could be APC hits instead, although that was also true for the variable cache in that it could be taken out of the db.

I'm not sure all this is going to matter greatly with the container initialization that's not going to be very skippable, but either way the original intention of this feature is still broken at the moment - i.e. that you could serve cached page with the only i/o being fetching the page from the cache.

I'm not sure all this is going to matter greatly with the container initialization that's not going to be very skippable, but either way the original intention of this feature is still broken at the moment - i.e. that you could serve cached page with the only i/o being fetching the page from the cache.

If I'm reading #1597696-44: Consider whether HttpCache offers any significant benefit over the existing page cache correctly, the HttpCache will operate pre-kernel, and therefore, pre-containter-initialization. I don't know if that issue makes this one moot entirely: is it valuable to add test coverage for what we have now before switching to HttpCache?

The main thing I think is whether the configuration system should be able to skip database lookups if a configuration object is overridden. Right now individual keys can be overridden in $conf, but since config() gets an full configuration object, it'll still hits the database since there's no way to know if all the necessary keys are there or not.

If the entire configuration object could be overridden, we could skip that step and then serve pages without the i/o. This would be another reason to do things like $conf overrides via storage controllers instead of events (although then that wouldn't allow for partial overrides so maybe it's not a reason).

Assuming we still have a configuration option for internal page caching or not (and things like max_age etc.), then that still feels relevant.

page_cache_without_database is now a setting, how does this affect this issue exactly?

Closed as duplicate of #1833516: Add a new top-level global for settings.php - move things out of $conf

Updated issue summary.

Re-starting this. We have three additional config loads on a page hit even with this setting enabled. One is the module list, which is removed in #2228215: Remove module check in DrupalKernel.

And then we have to load the timezone settings (default timezone) and the performance settings anyway (max_age). I'm fixing that by stuffing that on the cache object when saving the cache.

This patch in combination with #2228215: Remove module check in DrupalKernel results in a 33% performance improvement when still using the database for the cache because we no longer need to to initialize all the config crap.

The variable is however confusing now, it has nothing to do with the database anymore but the config system. See also #1215464: Variable 'page_cache_without_database' is poorly named. Maybe page_cache_without_config?

We also discussed other possible improvements, like checking the session cookie as the very first thing, we don't have to check the performance setting if there is one.

Forgot the benchmarks.

Before:
Requests per second: 188.94 [#/sec] (mean)
Time per request: 5.293 [ms] (mean)

After:
Requests per second: 266.85 [#/sec] (mean)
Time per request: 3.747 [ms] (mean)

Making this a sub-issue of #2257695: [meta] Modernize the page cache.

@Berdir's patch in #53 makes ton of sense to me. Doesn't really address the original/actual issue though, so perhaps we want to move those improvements into a separate issue?

@sun: I've incorporated #53 into #2257709: Remove the interdependence between the internal page cache and management of the Cache-Control header for external caches. The most important thing is that we do not load the configuration from within drupal_serve_page_from_cache() and thus effectively store anything into the cache which is necessary to generate a proper response. Luckily we can reach that by just storing the whole response into the cache after making sure that any relevant headers are on it.

#53 does address problem described in this issue *title* IMHO, by getting rid of all config calls that happen while checking for and displaying a cached response.

The issue summary is no longer accurate, because that error doesn't happen anymore, it simply doesn't work as you'd expect it to because a few lines after the skipped change it loads the exact same configuration anyway. The error you can see in the issue summary was caused by that call but now we just lazy-load the database stuff once we need it.

Is there anything that will be left here once the referenced issue happened? If not, then we can close this as a duplicate I guess?

The setting does have a weird name now, it's not about the database, it's about skipping to bootstrap the configuration system. Unlike 7.x, where using a non-database cache backend in combination with this setting was a requirement, in 8.x, it simply loads the database stuff when requested by the cache backend. Not sure if we should rename it (and to what?) in here, or open a new issue for that, though...

Verified that this is now fixed, awesome!

We have #1215464: Variable 'page_cache_without_database' is poorly named to fix the name, so we can close this one as a duplicate.