Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#54 | xhprof.png | 42.64 KB | Berdir |
#53 | fix-page-cache-without-config-1576322-53.patch | 3.33 KB | Berdir |
#38 | 1576322-page-cache-without-database-38.patch | 844 bytes | gdd |
#29 | config.cache_.29.patch | 8.81 KB | sun |
#29 | interdiff.txt | 9.72 KB | sun |
Comments
Comment #1
alexpottThe 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:
The attached patch provides a test that currently fails... I think the actual resolution of this requires quite a bit of discussion.
Comment #2
alexpottComment #4
catchNo 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.
Comment #5
alexpottReverting 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?
Comment #7
catchIs it the page cache that
is failing on? Or a different cache item?
Comment #8
alexpottI think I might have just opened a can of the wriggly stuff...
Comment #9
alexpottTurns 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.
Comment #10
alexpottComment #12
alexpottHopefully 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).
Comment #13
BerdirWe 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.
Comment #14
sunThe addition of a test/UA specific settings.php bugs me. Need to think about that.
Comment #15
effulgentsia CreditAttribution: effulgentsia commentedJust a reroll to chase HEAD, plus the PSR-0 change from #13. I have not reviewed this otherwise. CNR for bot.
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedThis 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.
Comment #17
effulgentsia CreditAttribution: effulgentsia commentedFirst line is silly (gets overridden by last), so this patch removes it.
Comment #18
effulgentsia CreditAttribution: effulgentsia commentedEven 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.
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedread 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.
Comment #20
Dries CreditAttribution: Dries commentedCurious to learn if @sun has more to say about the addition of the UA stuff.
Comment #21
alexpottTalked 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.
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.
Comment #22
alexpottFixed a couple of code comment issues.
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedwe 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:
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?
Comment #24
alexpottBeejeebus is right the delete method is wholly unnecessary.
Comment #25
effulgentsia CreditAttribution: effulgentsia commentedWill 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.
Comment #26
alexpott#25 Excellent point!
Comment #27
sunI 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
It would make sense to prepare this filepath as a variable instead of repeating the long string concatenation.
I do not understand why this variable is not part of system.performance.
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.
#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.
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
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.
Should use DRUPAL_ROOT.
We totally don't need coding standard conforming code here. ;) The opening PHP tag and a \n is definitely sufficient.
Don't think we need to repeat "simpletest" in the filename; just settings.php should be sufficient.
Comment #28
ojardila CreditAttribution: ojardila commented#12: 1576322_12_early_config.patch queued for re-testing.
Comment #29
sunAttached patch implements everything of #27.
It even goes beyond that. This should even enable us to test the Drupal installer. YARRRR! ;)
Comment #31
sunok - 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.
Comment #32
sunExcellent review by @chx:
Comment #33
andypost#29: config.cache_.29.patch queued for re-testing.
Comment #35
webchickI 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.
Comment #36
catchWhether 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.
Comment #37
gddFor #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.
Comment #38
gddI'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.
Comment #39
gddComment #41
gddPfeh 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.
Comment #42
effulgentsia CreditAttribution: effulgentsia commented#29 has some test coverage we might still want to add, unless equivalent coverage has already been added in other issues.
Comment #43
effulgentsia CreditAttribution: effulgentsia commentedNot sure if missing test coverage counts as a bug, so moving this to a task.
Comment #44
catchJust 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.
Comment #45
effulgentsia CreditAttribution: effulgentsia commentedIf 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?
Comment #46
catchThe 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.
Comment #47
catchComment #48
Berdirpage_cache_without_database is now a setting, how does this affect this issue exactly?
Comment #49
sunComment #50
dburiak CreditAttribution: dburiak commentedComment #51
mtiftClosed as duplicate of #1833516: Add a new top-level global for settings.php - move things out of $conf
Comment #52
alexpottI'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.Comment #52.0
alexpottUpdated issue summary.
Comment #52.1
mtiftUpdated issue summary.
Comment #53
BerdirRe-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.
Comment #54
BerdirForgot 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)
Comment #55
znerol CreditAttribution: znerol commentedMaking this a sub-issue of #2257695: [meta] Modernize the page cache.
Comment #56
sun@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?
Comment #57
znerol CreditAttribution: znerol commented@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.Comment #58
Berdir#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...
Comment #59
BerdirVerified 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.