The problem

cache_clear_all() used to clear all of the cache in about 2004.

Since then it hasn't, but the API fundamentally hasn't changed since then, except for gradually overloading the arguments more and more to support different things (like clearing arrays of $cids which was added in D7).

This makes the logic extremely hard to follow - cache_clear_all(), cache_clear_all(NULL, $bin); and cache_clear_all('*', $bin, TRUE); all mean subtly different things.

Also, during Drupal 7 we added the _cache_get_object() factory, an interface for cache backends etc., but this is all hidden behind procedural wrappers.

Proposed solution

The patch does two things:

First, it adds these methods to the cache interface:

 /**
   * Delete an item from the cache.
   *
   * @param $cid
   *    The cache ID to delete.
   */
  function delete($cid);

  /**
   * Delete multiple items from the cache.
   *
   * @param $cids
   *   An array of $cids to delete.
   */
  function deleteMultiple(Array $cids);

  /**
   * Delete items from the cache using a wildcard prefix.
   *
   * @param $prefix
   *   A wildcard prefix.
   */
  function deletePrefix($prefix);

  /**
   * Flush all cache items in a bin.
   */
  function flush();

  /**
   * Clear temporary items from cache.
   *
   * @todo: this should not be hard-coded to the block and page caches.
   */
  function expire();

  /**
   * Perform garbage collection on a cache bin.
   */
  function garbageCollection();

These cover all the things that cache_clear_all() does now, believe it or not.

Second, it renames _cache_get_object() to simply cache(), and begins the process of using the factory directly, rather than enclosed in a procedural wrapper, for example:

  if ($cached = cache()->get($cid)) {
    return $cached->data;
  }
  else {
     $data = do_stuff();
     cache()->set($cid, $data);
  }

  // Or with a $bin other than 'cache'.
  $cache = cache('bootstrap');
  $cache->get($cid);
  $cache->set($cid, $data);
  $cache->getMultiple($cids);
  $cache->flush();
}

For now, the patch does not remove the procedural wrappers, since that allows the rest of core to work without converting everything in one go. Only cache.test and bootstrap.inc are converted in the patch.

Original report

Convert cache_clear_all() to dedicated functions and update the upgrade docs.

cache_clear_all doesn't clear all the cache anymore, it should be renamed to cache_flush or similar.

this is a follow-up from http://drupal.org/node/72290

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dsoundmn’s picture

how about

cache_reset()

or maybe

cache_renew()

agentrickard’s picture

I would vote for three functions, since that's what cache_clear_all() actually did.

To clear a unique cache item, use cache_remove($cid).

To clear a whole class of items (like menus), use cache_flush($wildcard).

To clear the entire cache, use cache_empty().

If, however, we're only looking for one function name, I like cache_remove().

kbahey’s picture

cache_expire() ?

matt westgate’s picture

First off, great cache splitting patch. Thanks for cranking that one out!

As far as the cache_clear_all(), how about cache_flush_group() or cache_expire_group() ?

kbahey’s picture

*_flush_*() may imply that it is flushed back to the database and written somewhere.

reset or expire are more descriptive of what actually is done ...

ChrisKennedy’s picture

Title: rename cache_clear_all() » Refactor caching system
Version: x.y.z » 6.x-dev
Category: bug » feature

The update.php cache clearing issue (http://drupal.org/node/104107) brought this to my attention.

It's too late to change the cache API for 5.x, but for 6.x we should rework the functions and add appropriate hooks to allow modules to clear/expire their custom caches.

My first thought is to do something like this:

  • cache_clear($table, $cid = NULL, $wildcard = NULL) - clear all cache entries on $table; use $cid or $wildcard if specified.
  • cache_expire($table, $cid = NULL, $wildcard = NULL) - clear all expirable cache entries on $table; use $cid or $wildcard if specified.
  • cache_clear_module($module, $cid = NULL, $wildcard = NULL) - clear all cache entries on $module by invoking hook_cacheapi with $op = clear; use $cid or $wildcard if specified.
  • cache_expire_module($module, $cid = NULL) - clear all expirable cache entries on $module by invoking hook_cacheapi with $op = expire; use $cid or $wildcard if specified.
  • cache_clear_all() - clear all cache entries on all tables and invoke hook_cacheapi on all modules with $op = clear.
  • cache_expire_all() - expire all cache entries on all tables and invoke hook_cacheapi on all modules with $op = expire.
ChrisKennedy’s picture

Title: Refactor caching system » Restructure caching api
Pasqualle’s picture

Version: 6.x-dev » 7.x-dev
catch’s picture

Title: Restructure caching api » Clean up cache API functions
Version: 7.x-dev » 8.x-dev
Component: base system » cache system
Category: feature » task
Issue tags: +Performance, +caching

Reviving this, I really don't like cache_clear_all(), and it's even more overloaded in D7 than D6 (which is partly my own fault).

I'd like to keep this issue completely separate from #636454: Cache tag support which is about adding a layer on top of the direct clears to cache bins, this one should be just about cleanup of cache.inc itself.

Current list of functions is here: http://api.drupal.org/api/drupal/includes--cache.inc/7

Here's what I'd propose instead of cache_clear_all(), not far off from the previous selections:

D8 D7
cache_flush($bin) cache_clear_all('*', $bin, TRUE)
- cache_clear_all()
cache_remove($cid, $bin) cache_clear_all($cid, $bin)
cache_remove_multiple(Array $cids, $bin) cache_clear_all($cids, $bin)
cache_remove_wildcard($cid, $bin); cache_clear_all($cid, $bin, TRUE);
cache_garbage_collection($bin) cache_clear_all(NULL, $bin);

cache_clear_all() with no arguments should be deprecated and replaced with the cache invalidation API.

I'd also like to add cache_set_multiple($values, $bin); - this would be supported by at least the default SQL backend, memcached, MongoDB and Redis.

Jeremy’s picture

Subscribing. I'd very much like to see this cleanup as it would greatly improve code readability and our ability to ensure the correctness of alternative caching backend implementations.

catch’s picture

Assigned: Unassigned » catch

This is quite a simple one although the patch will be big. We could keep cache_clear_all() (just the no arguments version) temporarily until the tag clearing is sorted. Also this change would be mostly backwards compatible with the D7 API.

catch’s picture

Title: Clean up cache API functions » Replace cache_clear_all() with dedicated functions
Priority: Normal » Major
Status: Active » Needs review
FileSize
9.87 KB

Here's a patch. The installer works, cache tests almost pass.

Don't have time to do a write up, so I will copy and paste the relevant section of DrupalCacheInterface:


  /**
   * Delete an item from the cache.
   *
   * @param $cid
   *    The cache ID to delete.
   */
  function delete($cid);

  /**
   * Delete multiple items from the cache.
   *
   * @param $cids
   *   An array of $cids to delete.
   */
  function deleteMultiple(Array $cids);

  /**
   * Delete items from the cache using a wildcard prefix.
   *
   * @param $prefix
   *   A wildcard prefix.
   */
  function deletePrefix($prefix);

  /**
   * Flush all cache items in a bin.
   */
  function flush();

  /**
   * Clear temporary items from cache.
   *
   * @todo: this should not be hard-coded to the block and page caches.
   */
  function flushTemporary();

  /**
   * Perform garbage collection on a cache bin.
   */
  function garbageCollection();


For now I have left cache_clear_all()/clear() in - this acts as a bc layer so core doesn't explode in my face. Once function names have been bikeshedded it'd be possible to go through core to rationalize the function calls. This could either be done before commit so that everything is in sync, or in a separate issue to avoid a massive change landing all at once. I'm fine either way but it's easier to review in the early stages.

Another reason for leaving this in is also so this patch can be backported to a contrib project for Drupal 7. Also if the tests pass with the bc layer in, it proves there's no functional changes at all.

Note the intention of this patch is only to rationalize the function names/interface and remove the use of overloaded arguments and the magic wildcard parameter. I am not trying to add any new features or fix any bugs here since they have detailed issues of their own.

I have been thinking about how to tackle cache system improvements in Drupal 8, and while I'm very keen to get started on the other issues, I really want to get this in first so that there's a cleaner base to start from.

Cache tests pass, didn't run any others.

catch’s picture

FileSize
9.87 KB

Minor cleanup, moved some functions around and renamed clearTemporary to flushTemporary.

andypost’s picture

Looks great but whats about cache_is_empty() which seems useless

catch’s picture

I don't know why it's in there, but this patch doesn't add it ;)

catch’s picture

Looked in git, was added by #575360: API function to check if a cache bin is empty, rolling that back is not for this issue either way.

podarok’s picture

subscribe

catch’s picture

Issue tags: +API change, +API clean-up

The patch here doesn't actually introduce an API change yet, but adding tags.

xjm’s picture

Tagging issues not yet using summary template.

pillarsdotnet’s picture

Testing this.

pillarsdotnet’s picture

Made a couple of changes in response to test errors.

bfroehle’s picture

--- b/includes/cache.inc
+++ b/includes/cache.inc
@@ -476,7 +476,7 @@
-    if ($cache->expire != CACHE_PERMANENT && variable_get('cache_lifetime', 0) && $user->cache > $cache->created) {
+    if ($cache->expire != CACHE_PERMANENT && variable_get('cache_lifetime', 0) && isset($user->cache) && $user->cache > $cache->created) {

A similar change has been kicked around since January in #1015946-10: Eliminate $user->cache and {session}.cache in favor of $_SESSION['cache_expiration'][$bin], but with different logic when $user->cache isn't defined.

(!isset($user->cache) || $user->cache > $cache->created))

vs.

isset($user->cache) && $user->cache > $cache->created

That patch was met with generally negative comments from code reviewers (but positive comments from actual users). For example:
Damien Tournoud:

This is nearly won't fix. Some modules in your installations are messing up with the global user object in a way that breaks the minimum lifetime feature of Drupal. You will have to hunt which module is responsible and ask the maintainer to fix it.

Hiding the error under the carpet is the worse solution.

catch:

Moving metadata and subscribing. There's another patch in the queue dealing with $user->cache bugs which iirc just uses the session value directly. That might be a more straightforward fix here. Can't locate it now but subscribing for later.

pillarsdotnet’s picture

So where exactly is the memcache module failing to define $user->cache?

bfroehle’s picture

Made a couple of changes in response to test errors.

Can you be more specific about what triggered these errors? It would be useful for debugging the root cause of $user->cache not getting defined. Either that or we'll need to decide on what $user->cache being undefined means (i.e. does it really mean +infinity? or 0?).

pillarsdotnet’s picture

FileSize
10.41 KB

Sorry; I triggered the errors by starting with a fresh git checkout of Drupal 7 and Memcache API and Integration, and running the Memcache tests. See #1246796-9: Memcache fails its own tests.

Posted what I believe to be the correct solution to the $user->cache issue at #1015946-64: Eliminate $user->cache and {session}.cache in favor of $_SESSION['cache_expiration'][$bin].

Corrected patch for this issue attached.

catch’s picture

FileSize
10.36 KB

@pillarsdotnet, please leave the $user->cache fixes in the issue that deals with that bug. Any change made here will either need that patch applying to it once it's in, or that patch will need an 8.x version if this gets in first, but it's not directly related at all.

I thought about this some more and I think we should move away from procedural wrappers around factories around classes in Drupal 8, so new patch which starts on that.

All the new methods in the interface are the same.

However I removed all the wrappers except for the existing ones that are already in cache.inc (which are still left there for bc, I'm not planning to roll a 500kb patch until the API is agreed on or possibly until after this is actually committed).

So once things are converted it'll look more like this:

$cache = cache($bin);
if ($cached = $cache->get($cid)) {
   return $cached->data;
 }
 else {
   // blah blah blah....
   $cache->set($cid, $data);
}

That way if we need to extend the cache API, it is just a case of adding a new method to the interface and implementations - no more overloading of function arguments, no more adding extra procedural wrappers. We'll want to do that at least for setMultiple() in D8 and I hope for cache tag support.

catch’s picture

Title: Replace cache_clear_all() with dedicated functions » Clean up the cache API (stop overloading function arguments, remove procedural wrappers)

Re-titling.

Status: Needs review » Needs work

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

Anonymous’s picture

Title: Clean up the cache API (stop overloading function arguments, remove procedural wrappers) » Replace cache_clear_all() with dedicated functions
Status: Needs work » Needs review

i really like this.

we get to keep the factory, so we're not opening ourselves up to 'ZOMG, we need to change all those "new Cache..." instances'.

we have an interface, lets use it. "Here's our cache interface, oh, and here are all the wrappers, which are similar, but not the same" should die, IMO.

not sure about keeping a BC version as a stepping stone, but i guess we should get committer feedback before doing the work of ripping it all out.

msonnabaum’s picture

I agree with Justin. Would like to see the wrappers gone, but #26 is still a step in the right direction.

pillarsdotnet’s picture

Title: Replace cache_clear_all() with dedicated functions » Clean up the cache API (stop overloading function arguments, remove procedural wrappers)

Restoring title change by catch in #27

catch’s picture

FileSize
22.22 KB

Here's a new version of the patch. Only change is I fixed the test failures, and also converted modules/simpletest/cache.test to use the new API.

The wrappers that are still in there are just a temporary BC layer. I'd like to get this reviewed/RTBCed and past webchick/Dries before attempting to convert all uses of the cache API in core to the new one.

We could even consider committing it with the BC layer then doing the conversion and removal of bc layer in follow-ups, but not too worried about that - mainly about deciding on an API we're happy with before doing the full conversion (which will break a lot more patches if committed, and make this patch a lot more fragile for re-rolls).

At the moment this patch does not add anything new or change functionality at all, it just re-organises the existing code/usage - there's other issues open to improve the actual functionality.

pillarsdotnet’s picture

Small correction to patch from #26.
EDIT: Sorry for the corss-post. Obviously the patch in #32 is the one that should be considered.

catch’s picture

No worries about cross-post.

One note - there is a hunk of the test that's just removed, this confirms that cache_clear_all('*', $bin); does not wipe the cache - that test is irrelevant when we're not magically changing the meaning of the first argument based on the value of the third.

catch’s picture

Priority: Major » Normal

Downgrading to normal since this is just API refactoring.

I bumped #891600: Page and block caches are cleared the same way on cron as on content change to major split out the 7.x-blocking bug though

Anonymous’s picture

Status: Needs review » Needs work

bump. i still like this patch.

i gave #32 another read over, only two things bother me:

1. the @todo in the DrupalCacheInterface::flushTemporary() docblock looks out of place - i think we can safely just keep that next to the implementation, rather than the interface.

2. do we need a @todo or @deprecated in the DrupalCacheInterface::clear() docblock? IMO, that should go, hopefully sooner rather than later, as all its functionality is replaced by better methods in the interface.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
23.74 KB

ran the comments in #36 past catch, and he's ok with them, so here's reroll with them incorporated.

catch’s picture

FileSize
29.19 KB

Here's a new patch based on #37, did not make any changes except for updating bootstrap.inc to use the new syntax.

I don't want to do any more than that until this has been signed off (or potentially it could be done after commit in follow-up patches).

Status: Needs review » Needs work

The last submitted patch, cache.81461.38.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
29.2 KB

One conversion was messed up, I think this one will pass.

chx’s picture

Status: Needs review » Needs work

while cache()->foo() is nice if we are touching every cache_* call then i would nuke cache_ from the bin names in one go as well. trivial to add to the BC layer if (substr($bin, 0, 6) != 'cache_') $bin = 'cache_' . $bin; and go from there. Then, at the end of the conversion add an upgrade to rename cache bins wholesale.

catch’s picture

Status: Needs work » Needs review
FileSize
30.38 KB

chx reviewed in irc and suggested we stop using the database cache assumption of prefixing bins with 'cache'. That makes sense to me so I've added another temporary bc layer to add that prefix if it doesn't exist - we'll need to remove that all over core before this can be removed.

Also renamed flushTemporary() to expire().

catch’s picture

So removing that bc layer will look like this:

 - cache_get('cache_foo', $cid);
 + cache('foo')->get($cid);

Once that's done everywhere, remove the hunk in cache(). Then in the database cache, add the prefix back in there.

Status: Needs review » Needs work

The last submitted patch, cache.81461.41.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
30.47 KB

The BC layer for prefixed bin names is now one line, and there's a new hunk in DrupalDatabaseCache that will stay. Also fixed tests.

catch’s picture

FileSize
30.47 KB

Typo in a code comment...

catch’s picture

Issue summary: View changes

Updated issue summary.

catch’s picture

FileSize
30.48 KB

Justin pointed out a missing global $user in expire().

Fortunately there's another issue somewhere that changes that to use $_SESSION, but not for here.

Anonymous’s picture

if the bot comes back green, i think this is RTBC.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

ok, all green, RTBC.

catch’s picture

Once this is in I'll open a critical or major task to convert the rest of core to use the API and remove the bc layer. That patch is going to touch a lot of code (I count 663 lines in core with 'cache_' in them via a quick grep) so we may want to consider scheduling the patch similar to what's been done with /core.

Lars Toomre’s picture

This patch will in time touch quite a bit of core Drupal. Should not this issue queue include comments about the performance implications that result from this key patch? I suspect that there will be little, if any, effects. However, it would be great for confirmation as well.

catch’s picture

This actually reduces the amount of code executed, however it's literally skipping a single function call so shouldn't be measurable at all - cache_get() is rarely called more than 50-60 times per page at most.

In D7:

cache_get()
_cache_get_object()
CacheClass::get()

In D8:

cache()
CacheClass::get()

pounard’s picture

deletePrefix() does not makes sense to me, it delete() function should accept wildcard in the $cid and proceed with it normally. Maybe even a deleteWildcard() would be a good thing.

Following a chat with catch on IRC, it seems that actual Database implementation does not fully support wildcard (it can delete "cid*" but not "*cid" or "*cid*"). The fact is that the actual D7 API doc does not document this, and just says that it's a wildcard. IMHO, Database backend is bugguy and full wildcard should be supported.

Catch says that %cid% like queries are not indexed, that may be true. I would not provide a half-feature because MySQL is slower than all other DBMS. It's yes, something that could need a bit more debate, but a lot of cache backends would support this nicely.

I say, this would be a good feature to have. An, even if %like% queries can make a site going down, but just for fun:

pounard@guinevere /tmp $ git clone --branch 7.x http://git.drupal.org/project/drupal.git
Cloning into drupal...
remote: Counting objects: 123192, done.
remote: Compressing objects: 100% (30372/30372), done.
remote: Total 123192 (delta 91672), reused 117565 (delta 86443)
Receiving objects: 100% (123192/123192), 36.92 MiB | 147 KiB/s, done.
Resolving deltas: 100% (91672/91672), done.
pounard@guinevere /tmp $ cd drupal/
pounard@guinevere /tmp/drupal $ grep -Rn "db_like" * | wc -l
26
pounard@guinevere /tmp/drupal $ grep -Rn "LIKE" * | wc -l
86
chx’s picture

If you want more than deletePrefix then file a separate feature request but expect a lot of downvotes. This issue is a very straight change and cleanup and not new features. You are off topic and overlong as usual.

pounard’s picture

I will potentially open a new issue for this one, catch as a good point and it should not swell on this issue right now.

catch’s picture

Just a note we discussed this further in irc, the documentation does specify it's a 'foo%' match rather than a '%foo%' match

cache_clear_all():

"If $wildcard is TRUE, cache IDs starting with $cid are deleted"

DrupalCacheInterface::clear()

 "$wildcard If set to TRUE, the $cid is treated as a substring to match rather than a complete ID. The match is a right hand match."

However the fact this documentation is in two different places worded slightly differently, and buried in amongst all the other possibilities for what $cid could possible mean, is a big motivator of this patch. So hopefully this kind of confusion won't arise once it's in.

pounard’s picture

Yes I couldn't agree more: your patch proposal clarify things drastically and this is a really good thing.

Crell’s picture

Subscribing, since I didn't see this issue before. I fully +1 it. catch: I wouldn't actually have one massive fix-it patch for BC. Rather, we can do a file by file conversion like we did for DBTNG. That should reduce the breakage for other patches. It also should result in a whole bunch of Novice-level tasks we can farm out to new contributors. :-)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

I reviewed this patch and loved it. Much cleaner. I committed it to 8.x.

If necessary, we can continue to tweak the API in follow-up issues. It is better to separate out those discussions.

We should create an issue to convert the rest of core. That should be a critical issues such that it hits our gates.

Thanks! Committed.

webchick’s picture

Priority: Normal » Critical
Status: Fixed » Needs work
Issue tags: +Needs change record

This needs a change notice http://drupal.org/node/add/changenotice

Great work, folks! :D

Anonymous’s picture

Status: Needs work » Needs review
FileSize
1.12 KB

i agree with crell on the file-by-file approach, so here's a patch to kick it off with theme.inc.

Anonymous’s picture

FileSize
3.4 KB

and here's one for common.inc.

ok, i'll stop now.

Anonymous’s picture

Status: Needs review » Active

i've created #1272686: convert existing cache_* calls to the new cache()-> API to track the conversion patches.

catch’s picture

Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record

Opened #1272706: Remove backwards compatibility layer for cache API for the actual removal of the bc layer. If there end up being followup docs issues etc. that should probably happen in there.

Justin posted a change notification at http://drupal.org/node/1272696

Marking this fixed!

Status: Fixed » Closed (fixed)
Issue tags: -Performance, -Needs issue summary update, -caching, -API change, -API clean-up

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

jenlampton’s picture

Priority: Normal » Critical
Status: Closed (fixed) » Active

I'm not sure if this is the correct issue to re-open since it looks like there are a lot of associated issues, but the change record for the cache system needs to be updated. I don't think the code examples that are in there are still correct (even the Drupal 7 code examples.) If they are accurate, they aren't explained in a way that updating contrib from D7 to D8 is helped much by this notice.

It might he helpful to look at how a module actually uses the cache, and use those real-world examples in the record - along with any necessary context.

catch’s picture

Title: Clean up the cache API (stop overloading function arguments, remove procedural wrappers) » Change notice update: Clean up the cache API (stop overloading function arguments, remove procedural wrappers)
Assigned: catch » Unassigned
Priority: Critical » Major
Issue tags: +Missing change record

There's 2-3 different change records about the cache system, but yeah not a single accurate 7-8 any more. Let's keep this open to track the updating.

jessebeach’s picture

I've updated the existing New Cache API change record.

https://drupal.org/node/1272696

Mostly it points to our Cache API documentation, which is where developers should go to learn how to use this system. The change record is there to simply point out differences between D7 and D8 and then usher them to documentation quickly.

jessebeach’s picture

Title: Change notice update: Clean up the cache API (stop overloading function arguments, remove procedural wrappers) » Clean up the cache API (stop overloading function arguments, remove procedural wrappers)
Priority: Major » Normal
Status: Active » Fixed
Issue tags: -Missing change record

Shifting the issue details back to what they were before the change notice marking was put in place.

Status: Fixed » Closed (fixed)

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