cache serialization

What about if you don't want to store serialized data using the cache, just plain text? Maybe it should be an optional param for cache_get/set. Also, you have some .project cruft in patch and features/API changes go into HEAD.

I don't see any harm in adding a SERIALIZE flag to control whether the cached data needs serialization. This would allow strings or primary types to be stored without the serialization overhead. It would also not interfere with the ability to memcache.

This patch is important housecleaning that needs to happen. Having the serialization happen outside of cache_get and cache_set significantly reduces our claim to having a pluggable caching system.

+1 for the patch with or without a SERIALIZE flag.

@slantview, yeah I guess a flag isn't a huge deal. Most of the time we're going to be serializing, so taking the extra hit of a serialize isn't that bad. Just trying to be forward looking and see if there is any case where we won't want to serialize, but I'm fine without it.

Rather than adding a flag, can't we make the cache a tad smarter?

<?php
function cache_set(... $data ...) {
if (is_object($data) || is_array($data)) {
$data = serialize($data);
}
}

We'd need an extra field in the cache tables so that cache_get() knows how to retrieve that data ...

Rather than adding a second data field, could we add a small status field? I'd propose to call it "serialized" and it could be an "unsigned int(1)". I think that will provide a better memory/table layout.

Otherwise, this looks very nice.

And yes, we'll need to document this in the module upgrade instructions once this gets committed (see module developers handbook).

Keep up the great work slantview.

Also, we should add one or two sentences to the PHPdoc code of cache_set/cache_get. Extend the explanation of the $data parameter and state that $data can be a mixed type. Explain what will happen when you pass in an object or array. Also mention that this won't happen when you pass in a string.

Good documentation is key, so let's work on that some more as well! :)

These else clauses are redundant:

+      else {
+        $cache->data = $cache->data;
+      }

Otherwise this patch looks good. Feel free to mark it RTBC after it has been tested/checked by someone else. Thanks slantview!

This is really a big deal, and makes the cache system just that much easier and more straightforward for people to interact with. The fact that it makes the cache system easier to replace is an even bigger bonus. :-)

Major +1.

Steve, great patch -- looked it over and it looks good to me.

The upgrade docs will have to be updated since this patch affects modules like Views & CCK and more.

+1 for consistency and consolidation and memcache goodness :-D

I've committed this patch to CVS HEAD. Good job.

I'm marking this 'code needs work' because the module upgrade instructions in the handbook need to be updated. Please update those, and mark as 'fixed' afterwards. Thanks!