To replicate, have a exposed filter that returns the same results for 2+ values (I had a node type filter with only one node so all other node types returned empty results) and have the exposed form display with the view (ie. not a block). The views rows will be correct, but the exposed form stay with whatever the first one searched was.

Ie type filter with values image, file, video that all result in empty page. Click image and apply, aok shows 'image' selected. Click 'file' and apply, and it goes back to showing 'image' selected. Click 'video', same thing.

Patch adds build info the the key map in views_plugin_cache's get_output_key function; not sure if this or something else is desired, but looks to be where #636128: Don't have the cache rely on $_GET is heading to make a unique-ish key. Appears to be working so far.

Views3 looks to have the same issue and patch, other than line numbers, looks to be the same.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

voxpelli’s picture

Status: Needs review » Needs work

Why are you adding the entire build_info just to ensure that the data in an exposed form shouldn't be cached?

Seems to me like you should check whether there's actually an exposed form included in the rendered output of the display and if that's the case only add the variables in that form using eg. view::get_exposed_input()?

hefox’s picture

I figure that exposed filters are an example of what could make a view unique, but not necessarily the only way to make a view unique, so using build_info or something similar let other code hook into it. I'm not sure how big of a use case it is, but I've know I've used views hooks to change a view outside of exposed filters (for example adding pages to display changable pre-views3 via query string in the url/links, but $_GET isn't necessarily what would be, so all I'd need to say what is changed). So conclusion is that using something specific to exposed form is a bit unflexiable.

tomgf’s picture

Status: Needs work » Needs review
FileSize
1.01 KB

I was commenting a similar issue at #1137632: Views with Cache: Filters and Arguments and found your patch. I was pointed there to #997096: Disable caching on views that output forms, but it was not helpful for my case.

I will suggest to add an extra line with 'argument' => $this->view->argument.

I have created a new patch, taking what you've done and adding this extra value. On my tests, it works. But I'm not sure if this will serve in any given situation…

merlinofchaos’s picture

Status: Needs review » Needs work

There should be a $view->exposed_input

tomgf’s picture

Status: Needs work » Needs review
FileSize
1.24 KB

Alright, I have changed from $this->view->build_info to $this->view->exposed_input.

The $this->view->argument makes sense…?

tomgf’s picture

Sorry. I forgot to delete a dpm() call before creating the patch…

A clean version here.

tomgf’s picture

It seems to be that $view->exposed_input isn't always declared…

This new patch consider this situation setting a $exposed_input variable before add its value to the array.

rypit’s picture

Subscribing. Thanks for the patch, tomgf

johnv’s picture

Title: Queries with same result/view/display_id but different exposed input use same output key » Cached displays (with same result/view/display_id) do not respect changing exposed filters
Version: 6.x-2.x-dev » 7.x-3.x-dev

Patch #7 works fine for me, tested on Drupal 7.8 + Views dev 5-oct-2011, and patch still applies cleanly.

Changing the selections in the Exposed filter works fine (this issue is not relevant for not-exposed filters, i presume)
I have tested arguments, too, as it was doubted in [1137632#comment-4400482]
An argument www.example.com/myview/ is working fine: changing the tid gives good results
An 'argument' from the exposed filter ( such ad www.example.com/myview/expose_filter_tid= ) is also OK.

However, I have my doubts about the 'explosion' of caches here.. Doesn't it generate a cache for every combination of filters? Won't it blow up the cache tables and effectively only work for a second identical selection (which will hardly be the case in some websites)?
merlin's comment #4 is addressed in patch #7, but voxpelli's comment #1 is not;

Perhaps the solution must: first create and cache an unfiltered view, then fetch the cache and only in the last step apply the filters.
That sounds a lot like the 'Materialized Views' project: http://www.google.nl/search?q=drupal+materialized+views

I've been so bold to:
- set Views-version to latest version
- set the following issues to Duplicates of this one, since this one has a patch, and best resumé:
#997096: Disable caching on views that output forms
#1137632: Views with Cache: Filters and Arguments

johnv’s picture

Category: feature » bug

O ja, and I consider this a bug, since it makes caching with exposed filters unworkable.

hefox’s picture

Just to pop in:

I'm using a custom plugin that does use build info to create the key, and generally it the caching is fine (just found some calendar issues, but doubt that's the reason).

I don't believe there's anything in build info that would cause a cache explosion, unless the view is already in a configured state that it is unideal to cache anyway (storing current time?). I mean to say, the build_info should contain the information that was used to build, which should be the same if the view should use the same cache

The major advantage to that over exposed input is that it can easily be added to if something needs to indicate that this should be cached differently due to x y z reason (though I guess could also add to exposed filters, but that seams dirty).

dawehner’s picture

hefox’s picture

It looks the same; mark whichever patch you like less as duplicate?

RSpliet’s picture

In my humble opinion I think both patches aren't perfect to solve this problem. The patch at issue #1352362: Cache granulation is not working properly when using exposed forms proposes to add the exposed filters to get_results_key(), but fails to check if the variable exists. This patch proposes not only to add this key to get_results_key(), but also to get_output_key(). I feel the latter one is superfluous, as this is already covered by the 'results' element of the $key_data array. I would propose to use the top half of this patch (for get_results_key()), and disregard the second part. Or did I omit any details in this analysis?

johnv’s picture

Here is a new version of tomgf's #7.
- removed the 'argument' part, since the arguments are is already in $build_info->substitutions;
- kept 'exposed_input' both in Output cache and in Result cache;

#1352362: Cache granulation is not working properly when using exposed forms marked as a duplicate.
#1407044: Figure out the different between views->exposed_data and views->exposed_input is open as a relevant issue (as follow-up on #1352362).

girishmuraly’s picture

I had the misfortune of using a VBO view with additional exposed filters and then caching it. Sorry to say I cannot adequately describe the erroneous behaviour happening :)

The patch in #15 solves the problem with exposed filters, but the VBO actions just do not work. On performing an action on selected nodes, the page simply refreshes with no action taken and no feedback. I guess thats a separate issue?

vaartio’s picture

Attached is a patch that works with Views 7.x-3.3. Only line numbers has been changed.

tim.plunkett’s picture

Triggering the testbot.

Status: Needs review » Needs work

The last submitted patch, views_cache_keys-1055616-17.patch, failed testing.

johnv’s picture

Status: Needs work » Needs review
FileSize
1.08 KB

New patch against latest dev. Hope this passes testbot.

tim.plunkett’s picture

Status: Needs review » Needs work
+++ b/plugins/views_plugin_cache.incundefined
@@ -280,6 +280,7 @@ class views_plugin_cache extends views_plugin {
+        'exposed_input' => (isset($this->view->exposed_input)) ? $this->view->exposed_input : null,

The extra parens around isset() are redundant, and null should be NULL.
That's in both lines.

I'd love to see more test coverage on the different cache keys, but I don't know if this is the right issue to start that.

johnv’s picture

Status: Needs work » Needs review
FileSize
1.08 KB

new version of #17, #20, comments from tim.plunkett processed.

nicksanta’s picture

To throw another option in the ring - isn't the main issue that the query iteself is used as the cache key? My patch from #1469950: "Query results" caching does not respect changed values of exposed filters does this very elegantly with a 1 line change.

heatherwoz’s picture

I had more success with #22 than I did with the patch provided in #1469950: "Query results" caching does not respect changed values of exposed filters. The exposed filter values did not change and get out of sync with the view results (as they did when I tested the other patch). I don't understand the innards of what's going on here, but those are the results I had.

giorgio79’s picture

Status: Needs review » Needs work

Hello,

I am coming from #1469950: "Query results" caching does not respect changed values of exposed filters and neither this or that patch solved my issue. As soon as I turn on caching for a taxonomy view that uses contextual filters the views returns an empty set, unfortunately even with the patch at #22. (I show similar nodes tagged with the same term on node pages, hence the contextual taxonomy filter where I provide a default value)

If you agree I set it back to needs work.

heatherwoz’s picture

Status: Needs work » Needs review

giorgio79, that sounds like a separate issue and you should open a new one. This thread and the other you mention have to do with exposed filters, not contextual filters. I'm setting this back to needs review since it seems to solve the problem that was initially raised.

mototribe’s picture

#22 worked fine for me. I manually applied it to 7.3.3

giorgio79, you are probably referring to #1430650: Taxonomy filter with depth, Drupal 7.12, and views query caching failure

cheers

UWE

giorgio79’s picture

That looks like it, much appreciated mototribe :)

johnv’s picture

Marked #1469950: "Query results" caching does not respect changed values of exposed filters as a duplicate of this issue.
#1469950 addresses this problem in a different way:

class views_plugin_cache extends views_plugin {
  ...
  function get_results_key() {
    ...
    $query = clone $build_info[$index];
    $query->preExecute();
-   $build_info[$index] = (string)$query; // <<< This is the line I am referring to
+   $build_info[$index] = strtr($query, $query->getArguments());
    ...
  } 
}

The first version does not contain the exposed filters in the $query, the new version does.

johnv’s picture

This new patch adopts the method from #29 (credits to nicksanta).
This way, it avoids presentation/output data (like $build_info['breadcrumb'], $build_info['title']) to be included in the key, to avoid #1606292: Share result-cache between displays with same results and different output

tim.plunkett’s picture

FileSize
1.5 KB

That could theoretically cause notices, let's define $key_data first.

johnv’s picture

I guess this line can be removed, too?
'roles' => array_keys($user->roles),

Anonymous’s picture

#31 manually applied to 7.x-3.3: seems to work fine. Cached views with different exposed filter parameters result in different content now. I didn't check, if the caching really takes place, though.

@johnv: Roles need to be respected for cached views, and I'd like to ask you to explain your assumption a little bit. Would access checks apply for cached views somewhere else then?

johnv’s picture

@Shnapoo, IMO the access to the view should be checked BEFORE executing the query.
If a role hase no access, then we will never reach this point of code.
In case a role influences the results (WHERE role = X) , that is done in
$key_data['build_info'][$index] = strtr($query, $query->getArguments());

Anonymous’s picture

I think the role is taken into account so Node access is respected. Otherwise cached views could leak restricted content.

johnv’s picture

@Shnapoo, so the current patch is OK or not OK regarding roles?

Anonymous’s picture

Yes, #31 seems okay.

'roles' => array_keys($user->roles) mustn't be removed. Access checks may result in different query results for every role.

slashrsm’s picture

Status: Needs review » Reviewed & tested by the community

I agree with #37. Roles must be included due to access checks.

Can we call this RTBC?

Dmitriy.trt’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
6.34 KB

Actually, the patch #31 doesn't solve the problem. We still get the same output for different exposed input values with equal query results. Attaching updated patch with the test to be sure this problem will not appear again. Improvements:

  • Include compiled DB queries to both results and output keys. This fixes the problem. Probably we should add static cache to this part.
  • Move common key building functionality to separate function. Child classes can use this function and override default segmentation by additional key data.
  • Removed additional segmentation by the list of $_GET keys, because all these keys are included in compiled queries.

Status: Needs review » Needs work

The last submitted patch, views-1055616-39.patch, failed testing.

johnv’s picture

Move common key building functionality to separate function.

This is a nice feature, It would benefit modules like View Argument Cache.

Dmitriy.trt’s picture

FileSize
8.28 KB

Fix for notices and warnings is here: #1636024: Warning on enabled cache and no JS added

And the problem with cache segmentation is much deeper than it looks like. The limit and offset are added to the query on the execution step, which is wrong, because there is a building step for this purpose. So, new patch moves the code which set range from the execute() to the build() method of views_plugin_query_default.

There are still possible caching problems with modules that rewrite views queries, but they were already there before this patch.

Dmitriy.trt’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, views-1055616-42.patch, failed testing.

Dmitriy.trt’s picture

Status: Needs work » Needs review

Fix to #1636024: Warning on enabled cache and no JS added was committed, now this test should pass.

Dmitriy.trt’s picture

#42: views-1055616-42.patch queued for re-testing.

Dmitriy.trt’s picture

FileSize
8.39 KB

Re-roll on top of the latest changes.

johnv’s picture

#47 works fine.
Attached new version is against latest -dev.
It also leaves the construction of the cache-id in the respective get--key functions.
This way the granularity of the key can be determined by other plugins as well.

johnv’s picture

a better version of #48.

Some examples of other trying to influence the cache_key:
module Views Argument Cache
#1606292: Share result-cache between displays with same results and different output

alesr’s picture

I can confirm #49 is working for me with Views 7.x-3.x (June 26, 2012) + patch

Dmitriy.trt’s picture

FileSize
9.47 KB

There is a problem with the patch #49: there is no theme and no results in output cache key. Patch with a fix and interdiff from #49 attached.

Dmitriy.trt’s picture

FileSize
1.58 KB
dawehner’s picture

Status: Needs review » Needs work
+++ b/plugins/views_plugin_cache.incundefined
@@ -252,56 +252,57 @@ class views_plugin_cache extends views_plugin {
-  function get_results_key() {

People extend this methods and actually expect them to be there so what you need to do is to call get_cache_key out of get_results_key. I'm sorry for that, but i know at least 1 contrib and several custom implementations.

Anonymous’s picture

Status: Needs work » Needs review

Is views expected to expose the cache key as an API and maintain backward compatibility?

Drupal 7 API docs state get_results_key() is used by views_plugin_cache.inc only. See http://api.drupal.org/api/views/plugins%21views_plugin_cache.inc/functio...

This patch is important and should be blocked for good reason only.

johnnydarkko’s picture

I hope I'm posting in the correct queue, a related and previously closed issue referred to this issue as a duplicate.

Having this issue in 6.x-3.0 & 6.x-3.0+73-dev (2012-Jul-27)
When I have a view with AJAX enabled that has a full pager set to 50 items per page has exposed filters - default filters has 3 results.
When I change the filter, the view now has 50+ results, but the pager doesn't appear - all results are displayed. If this is not related, I apologize, let me know and I'll open a new issue ticket.

kenrbnsn’s picture

Can anyone confirm whether this patch is in the latest dev version, 7.x-3.3+228-dev?

Thanks in advance.

tim.plunkett’s picture

It's not. Otherwise this issue would be marked fixed.

ygerasimov’s picture

I believe there is some leftovers in patch #51. I have removed changes of file plugins/views_plugin_query_default.inc. Also I changed a test little bit to use DrupalWebTestCase::xpath() method.

@dawehner sorry, I don't understand your comment #53. Could you please be more detailed in it?

tim.plunkett’s picture

Status: Needs review » Needs work
+++ b/plugins/views_plugin_cache.incundefined
@@ -252,56 +252,57 @@ class views_plugin_cache extends views_plugin {
-  function get_results_key() {

I think he meant "don't remove this method, people need it"

+++ b/plugins/views_plugin_cache.incundefined
@@ -252,56 +252,57 @@ class views_plugin_cache extends views_plugin {
+  function get_results_key() {

But it's re-added later in the patch :)

ygerasimov’s picture

Status: Needs work » Needs review
FileSize
8.53 KB

My bad about removing changes from plugins/views_plugin_query_default.inc. That is really needed (see #42). Thanks to the tests!

Here is patch #51 with my corrections for the tests.

Dmitriy.trt’s picture

FileSize
7.83 KB

Enable tests back (accidentally disabled by #60).

Jaza’s picture

I can confirm that the patch in #61 (applied to latest stable Views, 7.x-3.5) fixes the bug for me. Let's get this in. (Although I didn't review the code of this patch very thoroughly, so will leave for someone else to RTBC this).

Anonymous’s picture

I've replaced Views in a production environment with the latest dev + this patch. It works as expected. At least nobody complains about it.

But there is a PHP notice in my logs:

Notice: Array to string conversion in views_plugin_cache->get_cache_key() (Line 283 of /sites/all/modules/views/plugins/views_plugin_cache.inc).

The responsible code:

$key_data['build_info'][$index] = strtr($query, $query->getArguments());

Comes up very often, perhaps once per returned row of a view or so. Absolutely spamming watchdog.

Dmitriy.trt’s picture

FileSize
7.91 KB

Looks like PHP notice was generated because of array arguments, e.g. for IN operator. Let's keep placeholders in SQL query and just add arguments to the key data.

Anonymous’s picture

Neat! Notices are gone now (verified). The change reads fine, but I didn't test it functional.

johnshortess’s picture

The patch in #65 works for me. A quick review of the code makes sense to me, but I don't feel familiar enough with the inner workings of Views to recommend RTBC.

Anonymous’s picture

Status: Needs review » Needs work

After reading all the issue and the code again, there is one thing that comes to my mind:

+++ b/plugins/views_plugin_cache.incundefined
@@ -252,56 +252,60 @@ class views_plugin_cache extends views_plugin {
   function get_output_key() {
-    global $user;
     if (!isset($this->_output_key)) {
       $key_data = array(
         'result' => $this->view->result,
-        'roles' => array_keys($user->roles),
-        'super-user' => $user->uid == 1, // special caching for super user.
         'theme' => $GLOBALS['theme'],
-        'language' => $GLOBALS['language']->language,
-        'base_url' => $GLOBALS['base_url'],

get_output_key() passes the view->result to get_cache_key(). This looks redundant to me, because get_cache_key() can assess, if the result of the view and its arguments will be the same. It doesn't need to know the result of the query to fulfill this job.

incaic’s picture

Hello,

I'm trying to cache views that are displayed on a search results page through the display suite dynamic fields. I'm testing this out, so no other cache other than the views caching is turned on to time-based 6 days/6 days. I've tried http://drupal.org/node/1055616#comment-6410030 but still seems to miss the cache. I'm looking at xhrof results and still see a good amount of time (2s) accessing the database and executing views code. My stripped down debug page only has title and 1 view with 2 fields displayed per result and there are only 43 results. The entire page on my local mac takes upto 4 seconds.

Any suggestions would be greatly appreciated.

Thanks.

de-fa’s picture

Status: Needs work » Needs review

#65: views-1055616-65.patch queued for re-testing.

johnv’s picture

FileSize
7.62 KB

Attached patch has 2 changes to #65:
- Shnapoo #68 is right, you don't need to pass the result, since the build_info is already used.
- Changed the order of the functions to easier read the patch.

it works for Dimitri's 'different filter, same output' use case, too.

johnv’s picture

There is still an issue with missing caches, as incaic says.
@incaic , I get max 2 output-records. How many do you get?

Here is my analysis:
- in table cache_views_data, I have 1 results-record (which is OK), and 1 or 2 output-records (which is wrong)
- after inserting a dpm() like below, I find that the ['build_info']['...query']['arguments'] names are not consistent (but only for the results-cache). Mostly they are ':db_condition_placeholder_8 ' to ':db_condition_placeholder_11', but sometimes from 12-15 ??

  function get_cache_key($key_data = array()) {
   . . .
dpm($key_data);
    $key = md5(serialize($key_data));
    return $key;
  }
}
mErilainen’s picture

Neither of the last two patches (#65 nor #71) change anything for me. I'm just trying to enable caching on view with exposed filters. Selecting the filters won't change the results and they don't stay selected. Disabling caching makes the view work just like it should.

Working with latest dev version of Views in vanilla Drupal 7.15.

de-fa’s picture

I had the same problem but finally realized that the filter was only broken with one view containing linked content with the module "References". After I completely setup the view again, everthing was working fine again. Maybe this helps someone.

GustavoL’s picture

In my case, no references in usage, I'm having the problem in the standard taxonomy view, with the addition of a filter of one parm. Changes in the filter after the first change, does not take effect and the view remains with the first filter forever. For example, filtering by "a" value works fine. Then changing to "b" or "c", the view will remain with the "a" filter, ignoring any other setup.
thanks

johnv’s picture

Status: Reviewed & tested by the community » Needs review

Cross-posting from #1469188: Views failing to cache effectively when offsets are used in timestamp filters (D6 -> D7 regression).
That issue states that caches are missed when using a Time offset, resulting in '***CURRENT_TIME***' in the SQL.
We might test if this patch resolves that issue.
[EDIT] I tested it, it doesn't.

Greg Boggs’s picture

Has there been any progress or is this plugin still broken on exposed filter views?

Anonymous’s picture

@gregboggs: you could test #65 for your use-case. No final fix has been committed as yet. The issue has "needs review" status and participation is appreciated.

mrded’s picture

Status: Needs review » Reviewed & tested by the community

#65 patch made my day! It works fine for me, thanks!

bgm’s picture

#65 also works for me. I had a simple view with an exposed filter for the year, cached for 1h. It was displaying the same content for all values of the exposed filter.

Should we also test #71?

Anonymous’s picture

Please use and test #71. It's a feature equivalent version of #65 containing some cleanup. Was my fault to recommend #65 above :-/

johnv’s picture

I have created a follow-up issue for the findings in #72: #1886554: :db_condition_placeholder leads to cache misses and superfluous cache_set

rogierbom’s picture

Tested and confirmed #71, solved the issue for me.

johnv’s picture

A follow-up on the missed caches in #69, #72, #82:
#1430650-39: Taxonomy filter with depth, Drupal 7.12, and views query caching failure is another long issue about caches. It adds the followingchange below.
I added that to our patch, tested that, and it resolves the problem.
I searched in the issue queue for old issues regarding the 'clone' statement and found none.
However, someone else must confirm that the cloning can be skipped, and this change added to the patch:

         // If the default query back-end is used generate SQL query strings from
         // the query objects.
         if ($build_info[$index] instanceof SelectQueryInterface) {
-          $query = clone $build_info[$index];
+          $query = $build_info[$index];
           $query->preExecute();
           $build_info[$index] = (string) $query;
         }
GustavoL’s picture

Hi,
Confused... Applying #84 would be enough to solve the views cache problem? And, where to apply this?
thanks for your support
Gustavo

johnv’s picture

@GustavoL, the line in #84 (in file /plugins/views_plugin_cache.inc) would only solve #1430650.
If you want to solve 'exposed filters and caching', you need to apply #71, (and add #84 on top manually).

freezypop’s picture

#65: views-1055616-65.patch queued for re-testing.

johnv’s picture

New patch, including the proposed change in #84. That change was accepted by several people in #1430650: Taxonomy filter with depth, Drupal 7.12, and views query caching failure

b2f’s picture

Status: Needs review » Reviewed & tested by the community

I made a quick test with #88 and it seems to fix caching issues with Taxonomy exposed filters and Views Selective Exposed Filters as well.

jcisio’s picture

BrockBoland’s picture

Status: Reviewed & tested by the community » Needs review

Updating status: this was set to RTBC for #65, but there are a couple new versions.

b2f’s picture

Update: #88 is OK with "Time Based" but it does not work with "Time-based Per Domain" caching type.

johnv’s picture

@Daz, that sounds like a contrib caching mechanism. Which module does it belong to? It needs to be changed over there. You'll need to post an issue in that issue queue, with reference to this issue. The patch makes it possible to override/re-use the logic.

greggles’s picture

@Daz - that sounds like it's provided by the Domain module, so I suggest using that module's issue queue.

agentrickard’s picture

We already have an issue for that, which is waiting for this one to be committed so we can follow the pattern. See #1882094: Views cache will cache exposed filter settings.

@Daz, please don't confuse this issue here. This patch is for Views proper.

hailu’s picture

Status: Needs review » Reviewed & tested by the community

#88 - Just tested and deployed to our production site. I couldn't break any of our views and it functions as expected. I'd love if this patch was in views for future releases.

GustavoL’s picture

#88 - Also works for me. Thanks for all your support.

essenceofginger’s picture

I was having a problem where a combination of exposed filters with view caching turned on caused the view to return all items, not respecting the 'items per page' filter.

The patch in #88 sorted it for me too. Legendary!

johnv’s picture

Marked #1802056: Improve cache key for views with exposed filters. as a duplicate. It is the D6-version of this issue.

rafaqz’s picture

any chance someone could re-roll the patch against the latest dev?

johnv’s picture

@rafaqz , I just did that. No problems applying #88.

BarwonHack’s picture

#88: views_1055616_81_cache.patch queued for re-testing.

BarwonHack’s picture

Applied Patch #88 to a Domain Access site and can confirm that it works with Views 3.7.

Caching per domain option does not work. That said, results per domain were not the same across domains so caching per domain is in fact working, just the settings are generic across domains. My point? Caching per domain is somewhat superfluous.

agentrickard’s picture

As I said before, caching per domain is not relevant here. It can be fixed in #1882094: Views cache will cache exposed filter settings but only *after* this patch is committed.

pjcdawkins’s picture

Applying the patch in #88 worked well for me - actually it's pretty much changed my life. Commit please!

harrrrrrr’s picture

I tested #88 and can confirm that my views with exposed filters are now cached correctly.

agentrickard’s picture

Priority: Normal » Major

Bumping priority for the committers. This has been RTBC for 2 months.

Lux Delux’s picture

Another bump, the fix works magic.

Cool example usage with finder module, it now works with a cached view. Bliss!

Renee S’s picture

This works for me, helps immensely. One more vote on RTBC...

geek-merlin’s picture

Crosslinking #2009464: views_exposed_form_cache() flaw, eg. leads to exposed forms redirecting to frontpage: exposed form caching can have similar problems which is a different issue.

geek-merlin’s picture

I suppose this will get committed very quickly as soon as someone forward-ports it to 8.x views-in-core.

pinkonomy’s picture

My exposed filter keeps the same value,even though I do not cache the view.This patch doesn't work if the view is not cached.How I can solve this?thanks

rbennion’s picture

Hi there, any timeline on when this patch will make it into DEV or Production Views release?

kevinquillen’s picture

kevinquillen’s picture

DuaelFr’s picture

#88 is really working well (and applies on Views 3.7)

In my case I had a View with exposed filters and Ajax enabled. The first time that exposed filters are submitted there was no problem but on the second time, all values were replaced by first ones.

milton_segretti’s picture

Patch #88 is great and worked like a charm until I noticed an issue.

After applying the patch in #88 I noticed my pagers disappeared from the bottom of my view.

I created a basic view with just the title and the pagers remained. Adding fields to the view one at a time, it wasn't until I added a Global: PHP field to the view that the pager disappeared. I have replicated the issue on two sites. Just the addition of the field is enough to make the pagers disappear, it doesn't have to have any code in it. Reverting the patch brings the pagers back as does removing the PHP field.

At first I thought it was related to the litepager module but upon uninstall the issue still occurs.

I realise the use of the PHP field is controversial in the Drupal community and I am working to replace that functionality somehow but I thought it best to bring attention to it.

I am using:

  • Drupal 7.22
  • Views 7.x-3.7
  • Views PHP 7.x-1.x-dev

ps. This is my first post so please be gentle if I have gone about things in the wrong way.

Edit: Scratch that. It seems to be a bit more complicated than I thought and involves Better Exposed Filters too. I will investigate some more. Sorry just a caching thing, my initial report still stands.

Exploratus’s picture

subscribe.

Exploratus’s picture

Global PHP field also breaks my pager when I apply this patch (#88) ... Something in the code doesn't play well.

DuaelFr’s picture

@milton_segretti : Thank you for your post. You are right, all that is related to PHP code put in a textarea (eval is evil) is highly controversed in the PHP community in general. Working to replace it is a really good idea. However, the issue is real and must be fixed. Did you try to reproduce this issue on a clean drupal install with only Views & Views PHP but no other modules ?

@Exploratus : https://drupal.org/node/1306444

milton_segretti’s picture

@DuaelFr: I have now tested the issue on a clean install and can confirm it can be reproduced.

You will also be glad to know I have nearly removed all the Global PHP fields from my project. Just one more to figure out.

Exploratus’s picture

The issue for me is the Flag field, which requires the use of PHP Field to put into a view.

Update: I was able to add the field without php, so everything works now. Nevertheless, this should be addressed.

erald’s picture

Version: 7.x-3.x-dev » 7.x-3.7

Having the same issue.Trying to get rid of the php.

tim.plunkett’s picture

Version: 7.x-3.7 » 7.x-3.x-dev
tim.plunkett’s picture

#88: views_1055616_81_cache.patch queued for re-testing.

erald’s picture

I also used php code in the filter area and that is not executed at all.

catch’s picture

Title: Cached displays (with same result/view/display_id) do not respect changing exposed filters » Query arguments should be replaced before generating cache ID

Just ran into this, started working a patch (for the argument replacement, didn't run into the other issue yet), then found this when I went to open an issue. Patch here is fine.

Also needs to be fixed in 8.x no?

pjcdawkins’s picture

Status: Reviewed & tested by the community » Needs work

This issue has been RTBC (patch #88) for 4½ months.

Unfortunately #117 (from 1 month ago) suggests this should be 'needs work'.

drumm’s picture

Issue tags: +Drupal.org 7.1

I think we are running into this at https://git7site.devdrupal.org/project/project_module/index which has exposed filters and uses views_content_cache. I expect this will work for us since we don't even have a pager or PHP field.

hhschoone’s picture

Priority: Major » Normal

Patch #88 seems to be breaking the exposed form style 'input required'. When no input is given, it shows all search results, for both the autosubmit as normal form. And, with or without cache enabled. Drupal 7.22, Views 7.x-3.7 + patch #88

nvm: it appears to be something else I did..

pjcdawkins’s picture

Priority: Normal » Major

Resetting priority back to major, because this bug means that Drupal 7 Views caching is really broken.

The patch in #88 is good, and has been good for more than 6 months now, if you don't use Views PHP.

But unfortunately this probably can't be RTBC'd until the issue with Views PHP is fixed. Or can it?

citlacom’s picture

Status: Needs work » Needs review
FileSize
8.13 KB

Included the view filters array in the generation of the view cache id due the arguments that differ in the query are added dynamically as add_where_expression is called and not already available at the first call of views_plugin_cache::cache_get(). i.e. this happens for the geofield_handler_filter.inc from geofield module that adds where expression for the proximity filter. The previous patch use same cache id disregard that the origin points from proximity are different. This issue is caused by using OpenLayers views pane that generate the view through a $view->preview() call at views_content_views_panes_content_type_render() not sure if the query() filter handler methods should be called in case this view is generated through the $view->execute() instead. As this is the approach as all the views panes are build probably the same problem could be caused in other filter handlers that add a where formula expression.

Status: Needs review » Needs work

The last submitted patch, views_1055616_include_filters_input_cache.patch, failed testing.

dawehner’s picture

Given that this is required for Drupal.org, does someone know whether this does fix the problem?

In general it would be great if the guy from #1055616-117: Query arguments should be replaced before generating cache ID could share a little bit more information.
A disappearing pager in general is a HUGE problem, so I don't think we should risk something here. Did someone else experienced the same problem
and can some more information how the view was configured?

catch’s picture

I've had #88 working for some time in production, with a similar view to the project issue one (which is what I assume needs the caching). I'm not actively on that project though and don't have a decent test case for the more recent patches, nor did I notice any bugs.

el1_1el’s picture

I can confirm the disappearing pager with a Views PHP field as both post 117 and 119 mention with the patch in 88.

As far as I can tell, all that needs to be done to reproduce the phenomena is to add the patch, set up a view with a pager of 1. The pager will be there. Then add a views php field, it doesnt need any code, and the pager will disappear. The view doesn't need caching for it to happen. To get the pager back, just roll back the patch, or remove the views php field.

Hopefully thats useful.

drumm’s picture

Issue tags: +affects drupal.org

tag

zipymonkey’s picture

Status: Needs work » Needs review
Issue tags: -affects drupal.org
FileSize
6.12 KB

It looks like views_php is using the pre_execute hook to modify the limit and offset of the query. So moving the query->range call into the query was causing issues with the pager (see #136)

I moved the set range back into the execute function and added current page parameter to the key_data. Not sure if using the current page to generate the key is a good or bad idea but it seems to work for my needs.

Status: Needs review » Needs work

The last submitted patch, views_1055616_138_cache.patch, failed testing.

greggles’s picture

Issue tags: +affects drupal.org

I assume this was accidental.

azinck’s picture

@greggles: dawehner claims it affects Drupal.org in #134. I can't vouch for whether this is true or not.

johnv’s picture

As a user of views_php, I am not sure if the views_php problem should be tackled in this issue. views_php seems unmaintained, and the issue list contains lots of proposed patches, some of which propose to change other hooks then the current version uses.

#132 and #138 cannot be both served in this one issue.(Assuming that geofield_handler_filter.inc does the query_alter in the correct way)

el1_1el’s picture

thanks zipymonkey, that patch seems to fix the views_php problem....of course now I need to see why this obscure exposed filter doesnt play nice with cache - https://drupal.org/project/views_fieldfilter ...but thats a separate issue

jthorson’s picture

jthorson’s picture

Quyen bui’s picture

Status: Needs work » Needs review

138: views_1055616_138_cache.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 138: views_1055616_138_cache.patch, failed testing.

The last submitted patch, 138: views_1055616_138_cache.patch, failed testing.

dokumori’s picture

FYI, the patch attached to the comment #88 works for d.o, as described in #2138855: Module project index page - exposed fields don't work

dawehner’s picture

+++ b/plugins/views_plugin_cache.inc
@@ -253,59 +253,61 @@ class views_plugin_cache extends views_plugin {
+          $query = $build_info[$index];
+          $query->preExecute();

I really think that we should NOT skip the cloning, as we should not risk something, see the following statement.

I searched in the issue queue for old issues regarding the 'clone' statement and found none.
However, someone else must confirm that the cloning can be skipped, and this change added to the patch:

And the problem with cache segmentation is much deeper than it looks like. The limit and offset are added to the query on the execution step, which is wrong, because there is a building step for this purpose. So, new patch moves the code which set range from the execute() to the build() 
method of views_plugin_query_default.

I absolutetly don't have a fundamental problem with breaking the views_php module. If you want to change the amount of items use hook_views_pre_build, or deal with the sql object in pre-execute/post-build.

dawehner’s picture

So the step we could do is to readd the cloning, someone tests it on drupal.org and I will commit it.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.6 KB
642 bytes

Patch with the change.

Désiré’s picture

Status: Needs review » Reviewed & tested by the community

I have reviewed and tested the last patch.
The fix worked, and I didn't see any problem in the code.

dokumori’s picture

I also tested the latest patch (#152) on a d.o dev site and can confirm it works as expected.

dawehner’s picture

Project: Views (for Drupal 7) » Drupal core
Version: 7.x-3.x-dev » 8.x-dev
Component: exposed filters » views.module
Status: Reviewed & tested by the community » Patch (to be ported)

Thank you very much for all the manual testing. Committed and pushed to 7.x-3.x

This needs a forwardport to D8.

tvn’s picture

Issue tags: -Drupal.org 7.1

Untagging, we have a separate issue in D.o 7.1 for this. Thanks everyone for fixing!

david_garcia’s picture

¿Has anyone noticed that any views that has a field from the View_PHP module looses it's pager after the Update?

catch’s picture

Issue tags: +VDC
blackandcode’s picture

@david_garcia_garcia and all others with same issues
We have the similar problem here. Just updated to the latest views dev verstion. Our Views_PHP sorter doesn't work any more.

@dawehner
Seems that there are more problems regarding this patch. Maybe you have fixed Pager but as you see sorter is now not working. Util this is solved I will use dev version from 2014-03-04(tim.plunkett)

blackandcode’s picture

I put issue on Views PHP module page so anyone is aware of this issue. https://drupal.org/node/2267281

@dawehner I think it is something wrong with this last view patch. Please can you look after this?

catch’s picture

Please open a new issue against the contrib Views module for the regression related to views_php.

I have no plans to hold up the forward port of this to core on that.

grasmash’s picture

Applying this patch appears to correctly caches views results, but it introduces a new, fairly elusive bug-- it arbitrarily caches the Views exposed form default values. This results in form options being pre-selected when the form is rendered, based on previous submissions.

This issue only because apparently during load testing when many simultaneous submissions were being made. It's therefore somewhat difficult to replicate. After digging through views a bit, it seems like the $form_state must be populated with the wrong #default_value when many requests are made simultaneously.

killua99’s picture

With the last stable views 7.x-3.8 is unable to patch it.

johnv’s picture

@killua99, this is already present in 3.8. It is to be ported from D7 to D8.

killua99’s picture

alright, I see it now, thanks @johnv

grasmash’s picture

Updating my previous comment #162.. it appears that the bug that I encountered manifests when Panels interacts with cached views. It seems to be a panels rather than a Views bug.

jphil’s picture

@Grasmash I wonder if you are right ... I had a view and an exposed filter displaying via a panels page. This view provided the functionality for a simple (default) drupal search and results. I experienced the broken caching and #152 fixed the issue. Job done, or so I thought. I have now rebuild the view using Search API instead of default Drupal (for unrelated reasons) and the bug has returned...

Some details:
- Panels caching is disabled (obviously)
- Views caching is turned on (EITHER time based or search specific - it does NOT affect the following)
- Views caching of query results works fine, no bug
- Views caching of rendered output, when turned on the bug is seen, when off, the bug is not seen

So, in my context it's caching of rendered output that's causing the issue... if I end up doing a massive xdebug session on the codebase I'll report findings here. Anyone else experienced this with Search API having previously fixed with #152?

drumm’s picture

#2250385: Issues feed for user is broken could be the same symptom on Drupal.org. That is a Search API based View. I don't see any current issues in the Search API issue queue, I think a followup there is what needs to happen. This issue should focus on making sure Views in 8.x is good.

jphil’s picture

Created https://drupal.org/node/2281535 on Search API

lmeurs’s picture

In different situations the query arguments are the exact same, but their placeholder ID's differ after cloning the query object. This results in different cache keys for the exact same data which causes duplicate cache entries. The problem does not occur with the patch from #88, but at #150 Views maintainer dawehner tells about the importance of cloning the query object, hence the patch from #152 being committed.

Right now both the unprocessed query string and the arguments array are stored in $key_data.

To remove possible different placeholder ID's, why not actually insert the arguments into the query string before creating the hash (as the title of this issue Query arguments should be replaced before generating cache ID suggests)? The $key_data array is only used by this function to create a hash key, so there seems no harm done changing the old structure.

$key_data['build_info'][$index] = array(
  'sql' => (string) $query,
  'arguments' => $query->getArguments(),
);

could become:

// Turn query object into a string with substituted placeholders.
$connection = Database::getConnection();
$sql = (string) $query;
$quoted = (array) $query->getArguments();

foreach ($quoted as $key => $val) {
  if (is_array($val)) {
    $quoted[$key] = implode(', ', array_map(array($connection, 'quote'), $val));
  }
  else {
    $quoted[$key] = $connection->quote($val);
  }
}

$key_data['build_info'][$index] = strtr($sql, $quoted);

This might solve problems for #1886554: :db_condition_placeholder leads to cache misses and superfluous cache_set and #1430650: Taxonomy filter with depth, Drupal 7.12, and views query caching failure as well, I attached a patch based upon the changes from above to the latter issue.

EDIT: It appeared that placeholder arrays can be multidimensional, so I had to alter the method to replace placeholders using code from views_ui_preview(). The replacing of the placeholders only costs 0.00007605s extra on my local W7 workstation for a query with three placeholders.

dawehner’s picture

Status: Patch (to be ported) » Fixed

We use cache contexts in Drupal 8, so they are replaced now.

Status: Fixed » Closed (fixed)

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

sp_key’s picture

Hi,

I'm curious to understand whether this patch is still relevant for the latest version of Drupal 7.

My website relies heavily on Views and Better Exposed Filters and ideally, I would like to cache Views.
A while ago, I was advised against doing this however, I just found out about this patch.

I'm now trying to validate whether the patch is indeed necessary or whether Drupal has resolved this at the core.
The way I'm trying to test this is by having multiple users selecting different filters from the same view.
Admittedly, I can't replicate any issue.

Can you please advise?
Cheers

Chris Charlton’s picture

+1 to #173 above.

DigitalFrontiersMedia’s picture

Just noting if anyone is still wondering per #173 & #174 above, this issue was fixed and commited per #155 above.