1. disable drupal's page caching mechanism, if it's enabled
2. view any page on the site

expected behavior: no queries should be run on the cache_page table

actual behavior: page_get_cache() gets run, which calls cache_get() on the cache_page table.

CommentFileSizeAuthor
#7 162945-D5.patch1.11 KBWesley Tanaka
#3 162945.patch1 KBWesley Tanaka
#1 162945.patch1 KBWesley Tanaka

Comments

Wesley Tanaka’s picture

Wesley Tanaka commented
Status: Active » Needs review
StatusFileSize
new 162945.patch1 KB

in the case that caching is disabled for a site (the default behavior of drupal, IIRC), the upcoming patch will, by fixing the above-described bug, reduce by one the number of database queries on every single page view without affecting functionality in any way.

Wesley Tanaka’s picture

Wesley Tanaka commented

s/upcoming/attached/

Wesley Tanaka’s picture

Wesley Tanaka commented
Version: 5.2 » 5.6
StatusFileSize
new 162945.patch1 KB

Same patch, rerolled for 5.6

sinasalek’s picture

sinasalek commented

Hi,
reasonable patch. two question
1. you added below code which is not in the unpatched version. why did you add it?

    else {
      ob_start();
    }

2.why don't you add your code to page_get_cache() function? so it won't get called anywhere when cache is disabled

Wesley Tanaka’s picture

Wesley Tanaka commented
Version: 5.6 » 5.7

1. The ob_start() is needed to keep the ob_start() and ob_get_contents() calls balanced in that particular case.

2. Because the switch(variable_get('cache')) is already in _drupal_cache_init().

A slightly cleaner change would pull ob_start() out of page_get_cache() and into _drupal_cache_init(), but this would change the semantics of page_get_cache() which is technically a public function, even if it's called from only one place in core.

Patch still cleanly applies to 5.7, bumping version.

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Needs review » Needs work

Looks okay, and does still apply. I think the only thing missing is a code comment explaining why ob_start() is called.

Wesley Tanaka’s picture

Wesley Tanaka commented
Version: 5.7 » 5.14
Status: Needs work » Needs review
StatusFileSize
new 162945-D5.patch1.11 KB

Same patch, rerolled for 5.14, and with an explanatory comment above the ob_start() call

tr’s picture

tr commented
Status: Needs review » Closed (won't fix)

This is not needed anymore because in Drupal 6.x page_get_cache() is never called when CACHE_DISABLED is TRUE. This was done in a slightly different way than the above patch. From the Drupal 6.x source for bootstrap.inc, this is the only line where page_get_cache() is invoked:

      $cache = $cache_mode == CACHE_DISABLED ? '' : page_get_cache();

As this has already been addressed in Drupal 6.x, and as Drupal 5.x is no longer supported, we can safely close this issue.