The DX Files: Static caching in Drupal

This is part four of my series, The DX Files: Improving Drupal Developer Experience. I started this series with fairly simplistic suggestions. They proved not very popular and some of them I agree were of questionable benefit due to PHP’s nature. I was pleased to discover, however, that they nevertheless had quite an impact on raising the visibility of “Developer Experience” within the Drupal community. I am therefore ready to move on to some of the more complex DX issues in Drupal.

The most important DX change Drupal needs to make is switching from a form-driven model to an API-driven model. There are many parts to such a change. Today’s topic: static caching.

A simple example

Much static caching in Drupal core and contrib is based on the assumption of a “normal” page request in which only a fairly controlled set of operations can occur during a single execution. Consider this fairly typical pattern (from D7 HEAD as of today):

<?php
function taxonomy_get_term_data($tid, $reset = FALSE) {
  static $terms = array();
  if (!isset($terms[$tid]) || $reset) {
    $terms[$tid] = db_query('SELECT * FROM {term_data} WHERE tid = :tid', array(':tid' => $tid))->fetchObject();
  }
  return $terms[$tid];
}
?>

This works great so long as no term ever changes during the course of execution. As soon as some code calls other taxonomy “API” functions, the interface can fail. Suppose we have a term whose tid is SOME_TID and whose name is “my name”. Consider:

This program outputs “my name” twice, not “my name” and then “some new name.” Bottom line: The taxonomy_save_term() has no effect during a single page request!

It is horrible DX to have “API” functions that behave this way. Sadly, not just the taxonomy module has this problem; the pattern is widespread within Drupal.

The $reset non-solution

The typical solution within Drupal is an optional “clear the cache” argument like $reset for taxonomy_get_term_data(). When $reset is TRUE, the static cache is erased. This would be fine if it were always used by all the taxonomy_*() functions to maintain the API’s specification (in that case, the “taxonomy class” would simply be doing internal housekeeping on “private member variables”), but it isn’t. Callers using the taxonomy “API” need to know to pass the $reset argument themselves.

The problem gets even worse when you consider all the myriad contrib modules that use the same technique. It seems everybody has their own static cache and you need to call the right magic “clear the cache” function to get anything done. The API docs rarely spell this out and it is a PITA.

A real-world example of where this can bite badly: Try writing hook_update_N() functions that create or change CCK types, Views, taxonomy terms, etc. Disaster! Why should I have to figure out to call views_invalidate_cache(), content_clear_type_cache(), etc. etc.?

Static caching-induced bugs

The technique can also lead to bugs/security vulnerabilities when the static cache, conceived to operate in a normal page request, is used in a longer-running program.

Case in point: drupal_validate_form(). Here’s an excerpt:

drupal_validate_form() caches the fact that a form successfully validated. This means that you cannot call the same form twice via drupal_execute() in one execution safely. The second and all successive calls will not call the form’s validate function. Instead, they will always invoke the submit handler.

Practice Statics

Sometimes the operation being cached is not even that expensive or called that often. It’s just being cached “for practice.” Here’s an excerpt from node.module:

<?php
function _node_revision_access($node, $op = 'view') {
  static $access = array();
  if (!isset($access[$node->vid])) {
    // compute and cache the access for $node and $op here
  }
  return $access[$node->vid];
}
?>

This is another example of static caching introducing a potential security hole. The $access static cache is based on $node->vid but not on $op. Suppose an attacker has permission to view a node but not delete it. If the attacker can cause _node_revision_access($node, 'view') to be invoked before _node_revision_access($node, 'delete), he can perform an unauthorized action.

This would be an important security hole in Drupal except it turns out that _node_revision_access() is never called more than once in a single page request. But… wait a minute! If the function is only ever called once per page request, why does it use a static cache at all? It shouldn’t.

This is just ridiculous. The code is unnecessarily complex in order to optimize a code path whose performance was obviously never measured and as a result introduces what would be a security hole but only if the static cache was ever actually used!

Solving the problem

So, what should we do? My suggestions:

1. Eliminate unnecessary static caching completely. Unless and until you have empirical data proving that a particular static cache makes an important improvement in real-world usage scenarios, just don’t use one. If you already have a static cache without such data, remove it. The drupal_validate_form() and _node_revision_access() examples fall into this category.

2. Maintain API integrity. If you must implement a static cache inside an API function, fine, but make sure that the API maintains its documented behavior within a page request. This means you have to make sure to update every “read” static cache in your code every time a “write” function is performed. Do NOT leave this to your callers. In the taxnomony example above, taxonomy_save_term() should invalidate or update the taxonomy_get_term_data() cache entry for the affected term.

http://drupal.org/node/254491 proposes a centralized static cache API for Drupal. I haven’t reviewed it recently, but it will probably help. However, even if it works really well, remember that not doing static caching at all will lead to simpler, less error-prone code. If you do not have a solid reason for using a static cache, just skip it until you do.