Problem/Motivation

If you add the following line to your settings.php

  $settings['page_cache_without_database'] = TRUE;

View your site as an anonymous user with page caching turned on you will get the following error:

Fatal error: Call to undefined function Drupal\Core\Config\db_query() in .../core/lib/Drupal/Core/Config/DatabaseStorage.php on line 22

Proposed resolution

  • 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?
  • Provide the ability for simpletest to test this kind of situation
Files: 
CommentFileSizeAuthor
#54 xhprof.png42.64 KBBerdir
#53 fix-page-cache-without-config-1576322-53.patch3.33 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,906 pass(es). View
#38 1576322-page-cache-without-database-38.patch844 bytesheyrocker
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View
#29 config.cache_.29.patch8.81 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config.cache_.29.patch. Unable to apply patch. See the log in the details link for more information. View
#29 interdiff.txt9.72 KBsun
#26 24-26-interdiff.txt540 bytesalexpott
#26 1576322_26_page_cache_without_database.patch7.31 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 37,010 pass(es). View
#24 22-24-interdiff.patch1.02 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 22-24-interdiff.patch. Unable to apply patch. See the log in the details link for more information. View
#24 1576322_24_page_cache_without_database.patch7.3 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 37,031 pass(es). View
#22 21-22-interdiff.patch1.25 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 21-22-interdiff.patch. Unable to apply patch. See the log in the details link for more information. View
#22 1576322_22_page_cache_without_database.patch7.55 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 37,029 pass(es). View
#21 17-21-interdiff.txt2.91 KBalexpott
#21 1576322_21_page_cache_without_database.patch7.56 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 37,016 pass(es). View
#17 1576322_17_page_cache_without_database.patch7.27 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 36,836 pass(es). View
#16 1576322_16_page_cache_without_database.patch7.35 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 36,836 pass(es). View
#15 1576322_14_early_config.patch6.68 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 36,695 pass(es). View
#12 1576322_12_early_config.patch7.6 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1576322_12_early_config.patch. Unable to apply patch. See the log in the details link for more information. View
#9 1576322_9_early_config.patch6.51 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 36,584 pass(es), 1 fail(s), and 0 exception(s). View
#5 1576322_5_early_config.patch6.14 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 36,574 pass(es), 21 fail(s), and 0 exception(s). View
#1 1576322_1_early_config.patch4.67 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 36,580 pass(es), 1 fail(s), and 0 exception(s). View

Comments

alexpott’s picture

FileSize
4.67 KB
FAILED: [[SimpleTest]]: [MySQL] 36,580 pass(es), 1 fail(s), and 0 exception(s). View

The patch attached provides the ability for SImpleTest to test this situation.

It does by creating a simpletest_settings.php file in the simpletest directory and then in drupal_settings_initialize() after including settings.php it does the following:

  // Enable simpletest to include additional settings so overrides and requests
  // that don't use the database can be tested
  if ($test_prefix = drupal_valid_test_ua()) {
    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';
    }
  }

The attached patch provides a test that currently fails... I think the actual resolution of this requires quite a bit of discussion.

alexpott’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1576322_1_early_config.patch, failed testing.

catch’s picture

Priority: Normal » Critical

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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.14 KB
FAILED: [[SimpleTest]]: [MySQL] 36,574 pass(es), 21 fail(s), and 0 exception(s). View

Reverting the system performance settings will not fix this... the issue then moves here:

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

However, in finding this out I've discovered that fixing config to be available even if the db is not available is quite simple... patch attached (will still fail but this is due to the error above...). With this fix overriding config in settings.php works as in D7... ie. it will return the value from settings.php :)

What do people think of the method proposed in the patch for testing $conf overrides?

Status: Needs review » Needs work

The last submitted patch, 1576322_5_early_config.patch, failed testing.

catch’s picture

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?

alexpott’s picture

I think I might have just opened a can of the wriggly stuff...

Test name Pass Fail Exception
ExpandBoot and exit hook invocation (HookBootExitTestCase) [Bootstrap] 15 4 0
ExpandConfiguration use if database unavailable (ConfEarlyConfigGetTestCase) [Configuration] 2 1 0
ExpandPage cache test (BootstrapPageCacheTestCase) [Bootstrap] 69 8 0
ExpandSession tests (SessionTestCase) [Session] 225 2 0
ExpandStatistics logging tests (StatisticsLoggingTestCase) [Statistics] 33 2 0
ExpandUser poll vote capability. (PollVoteCheckHostname) [Poll] 136 4 0
alexpott’s picture

FileSize
6.51 KB
FAILED: [[SimpleTest]]: [MySQL] 36,584 pass(es), 1 fail(s), and 0 exception(s). View

Turns out it was not so difficult after all... just needed to refresh the system.performance config object once the db is available... tests should turn green now.

After chatting with Catch on irc test is now checking for the php error caused by undefined function Drupal\Core\Cache\db_query() in .../core/lib/Drupal/Core/Cache/DatabaseBackend.php. It will be difficult for testing to provide an alternative cache backend that works and the test now proves that config set in settings.php can be accessed before the database is available... which is the point.

alexpott’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1576322_9_early_config.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.6 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1576322_12_early_config.patch. Unable to apply patch. See the log in the details link for more information. View

Hopefully have devised a test using cache's NullBackend - which I've had to fix as it seems to have fallen behind in implementing CacheBackendInterface... no longer testing for a php error which kind of sucks and didn't work on testbot anyway (even though it worked locally).

Berdir’s picture

Status: Needs review » Needs work
+++ 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.

sun’s picture

Issue tags: +Testing system

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

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
6.68 KB
PASSED: [[SimpleTest]]: [MySQL] 36,695 pass(es). View

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

effulgentsia’s picture

FileSize
7.35 KB
PASSED: [[SimpleTest]]: [MySQL] 36,836 pass(es). View

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.

effulgentsia’s picture

FileSize
7.27 KB
PASSED: [[SimpleTest]]: [MySQL] 36,836 pass(es). View
+++ 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.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

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.

beejeebus’s picture

read through the patch, and it looks great. i think it is RTBC too.

got me thinking though - wouldn't it be lovely if the child site read ONLY the created settings file? but that is totally for another issue.

Dries’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.56 KB
PASSED: [[SimpleTest]]: [MySQL] 37,016 pass(es). View
2.91 KB

Talked with sun about this patch @devdays. He pointed out that we can use drupal_var_export to make it easier to use the createSimpletestSettingsFile method.

Now you just create a $conf array and save it.

+    $conf = array(
+      'page_cache_without_database' => TRUE,
+      'cache_backends' => array(
+        'core/modules/system/tests/modules/bootstrap_test/lib/Drupal/bootstrap_test/Cache/TestBackend.php',
+      ),
+      'cache_classes' => array(
+        'cache' => 'Drupal\Core\Cache\DatabaseBackend',
+        'page' => 'Drupal\bootstrap_test\Cache\TestBackend',
+      ),
+    );
+
+    $this->createSimpletestSettingsFile($conf);

I also realised that the test needs to remove the simpletest.settings.php file it creates as this could / will affect other tests so I've added a deleteSimpletestSettingsFile method.

alexpott’s picture

FileSize
7.55 KB
PASSED: [[SimpleTest]]: [MySQL] 37,029 pass(es). View
1.25 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 21-22-interdiff.patch. Unable to apply patch. See the log in the details link for more information. View

Fixed a couple of code comment issues.

beejeebus’s picture

Status: Needs review » Needs work

we don't need to delete the settings file created by a test method. every test method is wrapped in a ->setUp() and ->tearDown(), and tearDown() runs this:

    file_unmanaged_delete_recursive($this->originalFileDirectory . '/simpletest/' . substr($this->databasePrefix, 10));

also, deleting at the end wouldn't help if the settings file would effect another test, because tests must be able to run with a concurrency greater than 1, right?

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.3 KB
PASSED: [[SimpleTest]]: [MySQL] 37,031 pass(es). View
1.02 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 22-24-interdiff.patch. Unable to apply patch. See the log in the details link for more information. View

Beejeebus is right the delete method is wholly unnecessary.

effulgentsia’s picture

Status: Needs review » Needs work
+++ 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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.31 KB
PASSED: [[SimpleTest]]: [MySQL] 37,010 pass(es). View
540 bytes

#25 Excellent point!

sun’s picture

Status: Needs review » Needs work
+++ 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: Setup 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.

oscarardila’s picture

Status: Needs work » Needs review

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

sun’s picture

Assigned: Unassigned » sun
FileSize
9.72 KB
8.81 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config.cache_.29.patch. Unable to apply patch. See the log in the details link for more information. View

Attached patch implements everything of #27.

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

Status: Needs review » Needs work

The last submitted patch, config.cache_.29.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

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.

sun’s picture

Status: Needs review » Needs work

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
andypost’s picture

Status: Needs work » Needs review
Issue tags: -Configuration system, -Testing system

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

Status: Needs review » Needs work
Issue tags: +Configuration system, +Testing system

The last submitted patch, config.cache_.29.patch, failed testing.

webchick’s picture

Status: Needs work » Postponed (maintainer needs more info)

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.

catch’s picture

Status: Postponed (maintainer needs more info) » Active

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.

heyrocker’s picture

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.

heyrocker’s picture

FileSize
844 bytes
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View

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.

heyrocker’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1576322-page-cache-without-database-38.patch, failed testing.

heyrocker’s picture

Status: Needs work » Closed (works as designed)

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: Follow-up: 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.

effulgentsia’s picture

Title: If page_cache_without_database = TRUE site returns a php error » Missing test coverage for page_cache_without_database
Priority: Critical » Normal
Status: Closed (works as designed) » Needs work

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

effulgentsia’s picture

Title: Missing test coverage for page_cache_without_database » Add test coverage for page_cache_without_database
Category: bug » task

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

catch’s picture

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.

effulgentsia’s picture

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?

catch’s picture

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.

catch’s picture

Title: Add test coverage for page_cache_without_database » page_cache_without_database doesn't return cached pages without the database
Berdir’s picture

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

sun’s picture

Assigned: sun » Unassigned
dburiak’s picture

mtift’s picture

Status: Needs work » Closed (duplicate)
alexpott’s picture

Status: Closed (duplicate) » Needs work

I've just been testing what happens if we have this set - atm the moment we connect to the database in DrupalKernel::initializeContainer() way before we get to _drupal_bootstrap_page_cache(). The question I'm wondering is should we even be trying to do this. If you want to do this surely we should be telling people to use a reverse proxy caching solution such as Varnish. Tempted to change this issue to remove this.

alexpott’s picture

Issue summary: View changes

Updated issue summary.

mtift’s picture

Issue summary: View changes

Updated issue summary.

Berdir’s picture

Priority: Normal » Major
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +Performance
FileSize
3.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,906 pass(es). View

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.

Berdir’s picture

FileSize
42.64 KB

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)

znerol’s picture

sun’s picture

@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?

znerol’s picture

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

Berdir’s picture

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

Berdir’s picture

Status: Needs review » Closed (duplicate)

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.