processwire / processwire-requests

ProcessWire feature requests.
40 stars 0 forks source link

WireCache expiry selector does not update #342

Open karltdev opened 5 years ago

karltdev commented 5 years ago

Short description of the issue

When using WireCache $cache, the cache does not clear as the expiry is not updating when the expiry string is changed. This results in the unwanted cache after some pages are added or removed.

Expected behavior

When expiry string changed, the expiry should use the new string but not the old string.

Actual behavior

When expiry string changed, the expiry is still using the old string.

Optional: Suggestion for a possible fix

Check if the expiry string is updated when calling $cache->get().

Steps to reproduce the issue

  1. Open a template file and add the following code

$ids = $pages->find('*');

echo $cache->get('test', "id=$ids", function(){ return "test"; });

  1. view the page and have the cache saved in the database
  2. add / remove any page in the tree
  3. view the page again and check for the expiry string in database. The expiry string is unchanged.

Setup/Environment

apeisa commented 5 years ago

I have not had this issue, but many times invalidated cache by changing expiry string manually during development. Maybe id string is too long and gets snipped somewhere? Maybe try calculating md5 hash from it?

karltdev commented 5 years ago

I always change the expiry to zero during development too and it works. However, when I change expiry from string to string, the expiry is reading the old string stored in database.

karltdev commented 5 years ago

I have a suggested fix. Change line 290 to the code below

if($this->looksLikeJSON($value)){
    $selector = json_decode($value, true);
    if(is_array($selector) && isset($selector['selector']) && $selector['selector'] !== $_expire){
        $value = null;
    }else{
        $value = $this->decodeJSON($value);
    }
}

inside WireCache.php

This gives a chance to have a check for the selector string update. This could fix the issue that I faced.

ryancramerdesign commented 5 years ago

@karltdev Thanks, I think I understand what you are trying to do here. That selector string option is for you to set a string that matches pages that when saved, expire the cache. That operation happens after a page save that matches the cached value. It does not happen when getting a previously cached value. So that particular selector string won't come into play here unless it doesn't find a cached copy. Though I like what you are trying to do here, and it does seem like it could potentially be a good feature for us to add, though I'd probably call it a token string or something, rather than a selector. For now, if you want to force a save, your best bet is to specifically call a $cache->save(…); when you know a new value needs to be saved. It sounds like in your case, you'll want to get() your cache value then verify that it's up to date with your external factor (a list of page IDs), and skip the cache and save a fresh copy of it isn't. So this is a good case where you'd want to keep separate get() and save() calls. I will look into the possibility of supporting token strings for a future version here, as I can see from your examples it's a good use case.

netcarver commented 5 years ago

@ryancramerdesign Should this be moved to processwire-requsts now?