This is a beginning. The actual mechanics of what to load when should be a separate issue as cache needs to be there very, very early so I have not touched this issue. For a change, this patch should be 100% backwards compatible, I just shuffled things around and added a layer of abstraction.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
13.41 KB
Dries’s picture

I'd support this patch -- also as an interim step. However, I recommend that we rename clear_all() to clear().

chx’s picture

FileSize
13.4 KB
Dries’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me -- but clearly an intermediate state. Marking as RTBC so we can get some feedback.

webchick’s picture

Let's get these phpDoced please:

+function _get_cache_object($table) {
+interface DrupalCacheInterface { #this one in particular is important.
+class DrupalCache implements DrupalCacheInterface { #so is this though, to describe an overview of what the default cache mechanism does.

Would it make sense to name this something like StandardDatabaseCache rather than DrupalCache, so it's more descriptive of what it does, or is it considered standard to name a class after the interface it implements?

+    // Garbage collection necessary when enforcing a minimum cache lifetime

is necessary. Also, should end this with a period.

+  /**
+   *
+   * Expire data from the cache. If called without arguments, expirable

Extra line above "Expire"

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Needs a little bit of work, it seems.

Frando’s picture

FileSize
16.92 KB

Great patch. Handlers would be better, but until we get these, this is a huge improvement over what we have in HEAD currently!

I added the missing docblocks, improved the documentation a bit and fixed the issues webchick raised.

Other changes:

  • Renamed DrupalCache to DrupalDatabaseCache, to be more descriptive
  • Renamed _get_cache_object to _cache_get_object (proper namespacing++)
  • _cache_get_object now just uses the value of the variable as the class name, without appending 'Cache'. We don't want to introduce more unneeded magic, do we.
  • added a variable cache_default_class with a default value of DrupalDatabaseCache. This is used for all cache bins for which no variable was set (or set to FALSE). This makes it easier to replace all caching instead of only specific bins (which is of course still possible). Needed only 2 LOCs more.
  • Renamed "table" to "bin", both in docs and in variable names. Table makes sense only for database cache implementations. Bin, though, fits all kinds of implementations and is a common term in the caching context (e.g. memcache uses it). In the docs it is explained that in the default implementation, each bin corresponds to a database table.
Frando’s picture

Status: Needs work » Needs review
chx’s picture

Status: Needs review » Reviewed & tested by the community

Seems to be good to me. All the concerns above are adressed.

moshe weitzman’s picture

Is there no way to make this IDE friendly? When I call $object = _cache_get_object() in Komodo, it has no idea how to autocomplete methods for $cache even though it $cache is a nice DrupalDatabaseCache instance ... And once we solve that, it is pretty lame to have no Doxygen on DrupalDatabaseCache methods since those will be shown by IDE. Our docs are in DrupalCacheInterface. Because of the IDE issue, we should consider duplicating the Doxygen.

chx’s picture

This is a problem for DBTNG as well and we discussed adding something like @return DrupalDatabaseCache to the _cache_get_object because otherwise any static tool will not be able to find any methods. However a) this is an interim step as indicated several times in the issue b) it needs to be solved together with DBTNG -- so def next issue.

Edit #363802: Document class methods with class, factory and defining interfaces

moshe weitzman’s picture

chx - that works quite well. as an alternative, komodo sort of understands @return DrupalCacheInterface. It autocompletes methods but fails to understand the params or doxygen for those methods.

ok, i think this ide problem is solveable and can wait for its own issue.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Some more minorish stuff then I have no problems committing this, since Dries approved of it above.

There are lines that are longer than 80 chars. Please wrap them.

+ *   The bin to store the data in. Valid core values are
  *   'cache_filter', 'cache_menu', 'cache_page', or 'cache'.
...
+ * The persistent cache is split up into four cache bins.

There are at least 9 such bins by my count:

* cache_block
* cache_field
* cache_filter
* cache_form
* cache_menu
* cache_page
* cache_path
* cache_registry
* cache_update

Why are only 4 of these elaborated on? Basically because no one's touched these docs since 4.7? :P I find it unlikely that we touch them in another 4 releases either, so could we take the opportunity to de-specify them and talk in a more general sense? Ideally, I'd love to see these grouped in some way that helps those who are trying to wrap their heads around the cache system make intelligent decisions. For example, I remember cache_form causing all sorts of problems when it was stuck into a memcache bin along with other stuff on Drupal.org. So I picture something like:

"Cache bins normally fall into 3 types:
- Blargy floos, such as the cache_smoosh bin, which are only smargled once in a smurgel and can therefore be treated as gudunkadunk.
- Smargy dargs, such as the cache_smeesh bin, which are for plonking the plinkies.
- Smishy Smashes, such as the cache_blinvinsible, which yuka yuka bork bork. Be careful about razzerfazzing their yizzlefizz."

(replace nonsense words with actual words. ;))

Also, if we're changing the terminology from $table to $bin, we ought to do so everywhere. That means the cache.test file needs some updates, too.

chx’s picture

Status: Needs work » Needs review
FileSize
18.48 KB

grep -h 'cache_' */*install|grep schema| cut -d\' -f 2|sort|uniqalso counted nine bins :) so there is quite unlikely that there is more. I have upped the comments in cache.inc but not in cache.test

Dries’s picture

I'll let webchick drive this home because of #14.

chx’s picture

FileSize
18.48 KB
chx’s picture

FileSize
25.61 KB
webchick’s picture

Status: Needs review » Needs work
Issue tags: +Needs documentation

Awesome!

The comments are more detailed than I was after, but very descriptive. Testing bot is happy, and Dries has signed off from an architectural perspective. I made a couple more minor adjustments to the comments and... committed to HEAD!

Please document in the 6.x => 7.x upgrade page.

chx’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)
Issue tags: -Needs documentation

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