The views_plugin_cache::get_output_key() method used to generate the cache key based on a hash that included, among other things, the actual results of the database query. This was very beneficial in terms of setting up a useful relationship between the output cache and the results cache —you could set the results cache to expire after 5 minutes, while leaving the output cache set to something much longer, and still guarantee that the output cache would regenerate if the database results changed.il

This feature was removed based on comments made in https://www.drupal.org/node/1055616#comment-6451790, indicating that it was redundant —since get_cache_key() already uses $this->view->build_info in the cache key, it wasn't necessary to pass the full result set. I disagree, given that build_info does not appear to contain enough information to be able to detect when the result set has changed.

Can we please add $this->view->results back into the $key_data array for the output cache? Again, this will make the following configuration possible:

Desired behavior:

  • Given:
    • Results cache times out after 5 minutes.
    • Output cache times out after 6 days.
    • $key_data for output cache key includes $this->view->result, as it did before
  • When:
    • One of the nodes in the result set changes, or a new node is added that would be part of the view's result set
  • Then:
    • The view will update to reflect those changes within, at most, 5 minutes, since the output cache key will be different.

Current behavior (not desired):

  • Otherwise, Given:
    • Results cache times out after 5 minutes.
    • Output cache times out after 6 days.
    • $key_data for output cache key does not include $this->view->result, as is now the case
  • When:
    • One of the nodes in the result set changes, or a new node is added that would be part of the view's result set
  • Then:
    • The view will not update to reflect those changes until the output cache times out, which could be up to 6 days, since the output cache key will not be different.
CommentFileSizeAuthor
#5 time_based_output_cache-2628960-5.patch1.15 KBfabianx

Comments

jazzslider created an issue. See original summary.

jazzslider’s picture

jazzslider commented
Project: Drupal core » Views (for Drupal 7)
Version: 7.x-dev » 7.x-3.x-dev
Component: cache system » Code

Accidentally filed this under Drupal core, changing to Views.

dandaman’s picture

dandaman commented

I agree this is an issue. To solve this, I added this line:

'result' => $this->view->result,

to views_plugin_cache->get_output_key() in plugins/views_plugin_cache.inc:

  function get_output_key() {
    if (!isset($this->_output_key)) {
      $key_data = array(
        'result' => $this->view->result,
        'theme' => $GLOBALS['theme'],
      );
      $this->_output_key = $this->view->name . ':' . $this->display->id . ':output:' . $this->get_cache_key($key_data);
    }

    return $this->_output_key;
  }
fabianx’s picture

fabianx commented
Priority: Normal » Critical
Status: Active » Reviewed & tested by the community

Yup, this is a regression from #1055616-152: Query arguments should be replaced before generating cache ID.

RTBC - I had the same (and more issues).

fabianx’s picture

fabianx commented
Title: Time-based output cache key no longer changes when database results change » Time-based output cache key no longer changes when database results change, results cache no longer depends on _GET parametrs
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new time_based_output_cache-2628960-5.patch1.15 KB

Here is a proper patch that solves both regressions.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work 
+++ b/plugins/views_plugin_cache.inc
@@ -289,7 +289,14 @@ class views_plugin_cache extends views_plugin {
+      foreach (array('exposed_info', 'page', 'sort', 'order', 'items_per_page', 'offset') as $key) {

May I ask why we need this bugfix? Please add a inline comment why a) this should not be part of the get_cache_key() method and b) the variation on the build query is not enough, as that is what get_cache_key() does.

fabianx’s picture

fabianx commented
Status: Needs work » Needs review

#6:

- a) First of all this is a regression and was in the original code base and was removed without reasoning.
- b) The build query does not contain the LIMIT clause at the point the cache key is created and therefore every AJAX request returns the same page over and over again.

e.g. the cache key is the same for all pages, but only when using a pager_element = 1.

This is very hard to reproduce so I opt-ed to restore the original behavior first.

dawehner’s picture

dawehner
Primary language German
commented

Do you understand why the limit is not on there?

fabianx’s picture

fabianx commented

#8 Unfortunately I have no idea and more unfortunately no time to debug right now.

I can only presume that the pager data is just added to the query afterwards as the paging works correctly - just the cache_key() is wrong.

  • dawehner committed c1d6042 on 7.x-3.x authored by Fabianx 
    Issue #2628960 by Fabianx: Time-based output cache key no longer changes...
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Fixed

Let's fix the critical regression.

fabianx’s picture

fabianx commented

Thank you!

For anyone finding this issue, my issue was related to views_php, which was used on a project.

views_php does remove the paging mechanism of the database and instead always returns all results and filters those results, which means the cache key is the same for every page. Re-adding the $_GET parameters back however should never make the caching worse as that is what was in the original caching plugin for some years. And the referenced issue removing those took great care to not break views_php already.

And the output cache issue was obviously critical in itself.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.