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
Comment | File | Size | Author |
---|---|---|---|
#62 | common.cache_.patch | 3.4 KB | Anonymous (not verified) |
#61 | theme.cache_.patch | 1.12 KB | Anonymous (not verified) |
#47 | cache.patch | 30.48 KB | catch |
#46 | cache.patch | 30.47 KB | catch |
#45 | cache.patch | 30.47 KB | catch |
Comments
Comment #1
dsoundmn CreditAttribution: dsoundmn commentedhow about
cache_reset()
or maybe
cache_renew()
Comment #2
agentrickardI 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().
Comment #3
kbahey CreditAttribution: kbahey commentedcache_expire() ?
Comment #4
matt westgate CreditAttribution: matt westgate commentedFirst 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() ?
Comment #5
kbahey CreditAttribution: kbahey commented*_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 ...
Comment #6
ChrisKennedy CreditAttribution: ChrisKennedy commentedThe 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:
Comment #7
ChrisKennedy CreditAttribution: ChrisKennedy commentedComment #8
PasqualleComment #9
catchReviving 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:
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.
Comment #10
Jeremy CreditAttribution: Jeremy commentedSubscribing. 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.
Comment #11
catchThis 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.
Comment #12
catchHere'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:
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.
Comment #13
catchMinor cleanup, moved some functions around and renamed clearTemporary to flushTemporary.
Comment #14
andypostLooks great but whats about cache_is_empty() which seems useless
Comment #15
catchI don't know why it's in there, but this patch doesn't add it ;)
Comment #16
catchLooked 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.
Comment #17
podaroksubscribe
Comment #18
catchThe patch here doesn't actually introduce an API change yet, but adding tags.
Comment #19
xjmTagging issues not yet using summary template.
Comment #20
pillarsdotnet CreditAttribution: pillarsdotnet commentedTesting this.
Comment #21
pillarsdotnet CreditAttribution: pillarsdotnet commentedMade a couple of changes in response to test errors.
Comment #22
bfroehle CreditAttribution: bfroehle commentedA 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.vs.
That patch was met with generally negative comments from code reviewers (but positive comments from actual users). For example:
Damien Tournoud:
catch:
Comment #23
pillarsdotnet CreditAttribution: pillarsdotnet commentedSo where exactly is the memcache module failing to define $user->cache?
Comment #24
bfroehle CreditAttribution: bfroehle commentedCan 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?).
Comment #25
pillarsdotnet CreditAttribution: pillarsdotnet commentedSorry; 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.
Comment #26
catch@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:
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.
Comment #27
catchRe-titling.
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commentedi 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.
Comment #30
msonnabaum CreditAttribution: msonnabaum commentedI agree with Justin. Would like to see the wrappers gone, but #26 is still a step in the right direction.
Comment #31
pillarsdotnet CreditAttribution: pillarsdotnet commentedRestoring title change by catch in #27
Comment #32
catchHere'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.
Comment #33
pillarsdotnet CreditAttribution: pillarsdotnet commentedSmall correction to patch from #26.
EDIT: Sorry for the corss-post. Obviously the patch in #32 is the one that should be considered.
Comment #34
catchNo 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.
Comment #35
catchDowngrading 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
Comment #36
Anonymous (not verified) CreditAttribution: Anonymous commentedbump. 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.
Comment #37
Anonymous (not verified) CreditAttribution: Anonymous commentedran the comments in #36 past catch, and he's ok with them, so here's reroll with them incorporated.
Comment #38
catchHere'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).
Comment #40
catchOne conversion was messed up, I think this one will pass.
Comment #41
chx CreditAttribution: chx commentedwhile 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.
Comment #42
catchchx 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().
Comment #43
catchSo removing that bc layer will look like this:
Once that's done everywhere, remove the hunk in cache(). Then in the database cache, add the prefix back in there.
Comment #45
catchThe BC layer for prefixed bin names is now one line, and there's a new hunk in DrupalDatabaseCache that will stay. Also fixed tests.
Comment #46
catchTypo in a code comment...
Comment #46.0
catchUpdated issue summary.
Comment #47
catchJustin pointed out a missing global $user in expire().
Fortunately there's another issue somewhere that changes that to use $_SESSION, but not for here.
Comment #48
Anonymous (not verified) CreditAttribution: Anonymous commentedif the bot comes back green, i think this is RTBC.
Comment #49
Anonymous (not verified) CreditAttribution: Anonymous commentedok, all green, RTBC.
Comment #50
catchOnce 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.
Comment #51
Lars Toomre CreditAttribution: Lars Toomre commentedThis 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.
Comment #52
catchThis 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()
Comment #53
pounarddeletePrefix() 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:
Comment #54
chx CreditAttribution: chx commentedIf 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.
Comment #55
pounardI will potentially open a new issue for this one, catch as a good point and it should not swell on this issue right now.
Comment #56
catchJust 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():
DrupalCacheInterface::clear()
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.
Comment #57
pounardYes I couldn't agree more: your patch proposal clarify things drastically and this is a really good thing.
Comment #58
Crell CreditAttribution: Crell commentedSubscribing, 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. :-)
Comment #59
Dries CreditAttribution: Dries commentedI 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.
Comment #60
webchickThis needs a change notice http://drupal.org/node/add/changenotice
Great work, folks! :D
Comment #61
Anonymous (not verified) CreditAttribution: Anonymous commentedi agree with crell on the file-by-file approach, so here's a patch to kick it off with theme.inc.
Comment #62
Anonymous (not verified) CreditAttribution: Anonymous commentedand here's one for common.inc.
ok, i'll stop now.
Comment #63
Anonymous (not verified) CreditAttribution: Anonymous commentedi've created #1272686: convert existing cache_* calls to the new cache()-> API to track the conversion patches.
Comment #64
catchOpened #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!
Comment #65.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #67
jenlamptonI'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.
Comment #68
catchThere'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.
Comment #69
jessebeach CreditAttribution: jessebeach commentedI'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.
Comment #70
jessebeach CreditAttribution: jessebeach commentedShifting the issue details back to what they were before the change notice marking was put in place.