Per discussion on #459980: Rework admin/settings/performance (especially 35-37), the "Clear Cache" button and function doesn't quite belong on the Performance page (admin/settings/performance). The current performance user interface also has the problem that hitting causes the cache to be cleared instead of the settings to be applied; moving cache to its own page is one way to deal with this.

Questions:
* What should the page be called?
* Does it require a confirmation dialog (rather than just warning text)?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano’s picture

Title: Create new "Cache Maintenance" page in admin settings (move from Performance page) » Detailed cache clear interface
Assigned: Unassigned » Xano
Category: task » feature
Status: Active » Needs review
FileSize
18.03 KB

The attached patch adds Administer > Site configuration > Performance > Clear caches where users can select modules for which to clear the caches, rather than hitting a single button which empties ALL caches. It deprecates hook_flush_caches() in favour of hook_cache_info() and hook_cache_clear(). caches_clear($modules = array()) can now be used to clear caches for a selection of modules.

field_cache_clear() is an existing function that causes trouble, since it cannot fully be converted to hook_cache_info(). This is because the $rebuild_schema parameter. The only consequence of this is that clearing cache_field needs to be done in field_cache_clear and not in caches_clear().

Whaddayathink?

catch’s picture

Why isn't the page cache on the screenshot. Haven't reviewed patch itself yet.

rfay’s picture

@xano: Thanks for doing this!

The patch applied cleanly, although I wasn't able to find how to get to the interface, and hitting admin/settings/performance/clear-caches didn't get me there (even after I manually cleared caches).

A couple of opinions:

1. All items should be checked by default (making it the equivalent of the old clear cache button)

2. All the tables should be rolled into one checkbox: perhaps "database cache tables". Very few users are going to understand the difference or care. And mostly this is used in a development or changing environment, when people really do want to clear everything. It's normally superstitious behavior.

Xano’s picture

@catch: page cache is part of System module. We should make this clear in the description or make the options even more granular.
@rfay: Go to admin/build/modules. The menu needs to be rebuilt before the page is accessible after applying the patch.

sun’s picture

"System" needs to be split into its atomic caches. See admin_menu for a working implementation.

Also, this does not necessarily have to be a form. admin_menu implements simple callbacks to quickly flush all or a particular cache (accessible for privileged users only).

catch’s picture

If we're going to have this level of detail, then yeah let's split the system module provided caches into its components. Having a button for the field cache but not the page cache just looks wrong. I know chx will object that the menu router isn't a cache, but do we want to add 'rebuild menus' here? If we do, could we remove the automatic one from admin/build/modules?

Dries’s picture

With this extra level of detail, this might no longer belongs under 'Site configuration' -- maybe it should be moved to 'Development' instead? For example, most people won't know what 'Field cache' _really_ is, and when it needs to be flushed.

For 99% of the people, this extra level of detail is not necessary. In 99% of the cases, it doesn't really hurt to flush all caches at once, except maybe when you have a _really_ big site. If we decide to have this level of detail, I think, we should check all checkboxes and give people the option to disable what they don't want to flush (instead of asking them to enable what they want to flush).

I flush the caches 24 times a day as part of my patch review work -- having to click 4-6 checkboxes would become very tedious. I can imagine that developers often have to do the same.

Xano’s picture

This is just a mockup. An actual patch is in progress, but I want some reviews on the basics before finishing it.

/**
 * Implement hook_cache_info().
 */
function system_cache_info() {
  $caches['system_general'] = array(
    'title' => t('General cache'),
    'bin' => 'cache',
  );
  $caches['system_page'] = array(
    'title' => t('Page cache'),
    'bin' => 'cache_page',
  );
  $caches['system_css_js'] = array(
    'title' => t('CSS and JavaScript files'),
    'description' => t('Clear cached aggregated and compressed CSS and JavaScript files.'),
    'callback' => 'cache_clear_css_js',
  );
  $caches['system_registry'] = array(
    'title' => t('Code registry'),
    'bin' => 'cache_registry',
  );
}

/**
 * Load all cache information.
 *
 * @return
 *   An array containing information about all caches defined through
 *   hook_cache_info().
 */
function cache_info() {
  $cache_info = &drupal_static(__FUNCTION__, NULL);

  if (!$cache_info) {
    $cache_info = module_invoke_all('cache_info');
  }

  return $cache_info;
}

/**
 * Clear a cache.
 *
 * @param $cache
 *   The name of the cache to clear.
 */
function cache_clear($cache) {
  $cache_info = cache_info();

  if (isset($cache_info[$cache]['callback'])) {
    call_user_func($cache_info[$cache]['callback']);

  if (isset($cache_info[$cache]['bin'])) {
    _cache_get_object($cache_info[$cache]['bin'])->clear('*', TRUE);
  }
}

/**
 * Clear all caches.
 */
function cache_clear_all() {
  $cache_info = cache_info();
  foreach ($cache_info as $cache) {
    cache_clear($cache);
  }
}

/**
 * Clear a specific item from a cache.
 *
 * @param $cid
 *   The cache item ID to delete.
 * @param $cache
 *   The cache to delete the item from.
 * @param $wildcard
 *   When is TRUE, cache IDs starting with $cid are deleted in addition to the
 *   exact cache ID specified by $cid. If you want to clear an entire cache, use
 *   cache_clear() instead of $wildcard = TRUE and $cid = '*'.
 */
function cache_clear_item($cid, $cache, $wildcard = FALSE) {
  $cache_info = cache_info();
  _cache_get_object($cache_info[$cache]['bin'])->clear($cid, $wildcard);
}

Caches are exposed through hook_cache_info(). They have a title, an optional description, an optional callback and an optional bin. The title and description are used by the UI like the first patch in this issue demonstrates. The bin is the name of the 'thing' the data is actually being cached. The current caching system uses a mix of bin and table for this. I decided to keep the API consistent with the cache interface in cache.inc. The callback is a function that cleans up cached data that cannot be cleaned up by simply flushing a bin. They are optional - however one is required for the cache information to make sense and actually do something - because hook_cache_info() is only used by modules that want to tell the rest of the code that and how their caches may be cleared by external code.

This mockup also introduces four new functions used for clearing caches. cache_info() invokes hook_cache_info() and caches the results statically. cache_clear() clears one single cache, which means a bin may be emptied and a callback may be called or both. cache_clear_all() is nothing like what is is in HEAD now. It allows developers to clear all exposed caches with one function call. The current cache_clear_all() has been renamed to cache_clear_item(). It works the same as it did before when clearing single items. All other functionality has been stripped in favour of the new functions mentioned earlier.

catch’s picture

Title: Detailed cache clear interface » Refactor cache clearing
Category: feature » task

Not got time to review now but re-titling since this isn't an interface clean-up patch any more.

Dries’s picture

Before we review the code, it is probably worth discussing if it is worth the effort. Personally, I never had a need for this level of granularity. It might not be worth the extra UI and code complexity (i.e. a new cache registry API).

We should discuss the UI seperate from the code -- the code could be useful without the UI. I need to think about it some more, but it feels like the code could be useful.

catch’s picture

I agree the code and UI are separate issues - if the code can clear up the mess which is http://api.drupal.org/api/function/drupal_flush_all_caches/7 (are those really all the core cache tables we have, no they're not, so why is it called drupal_flush_ALL_caches() then?), then it'd be some really nice cleanup.

Xano’s picture

1) We may not require the UI to be granular like this, but a simple and yet granular API is what we want, since unnecessarily clearing page and block caches is not something we should envourage.
2) If we go for a granular API, why not update the UI to work with it? As you can see in the original patch creating the UI is hardly any work.
3) As catch points out, the current API is a load of cruft.

//Edit: I should note that the code mockup above doesn't really add anything new. It merely changes some existing functions to clean up the API and to add new features. The mockup shows the code is not so complex at all.

sun’s picture

What I like in here is hook_cache_info(). We badly need. The cache system has to be more intelligent. Contrib modules can add additional caches and I've already proposed several times (elsewhere) that Drupal needs more knowledge about caches. Potentially enhance this cache registry with info about what is cached later on and we can drastically improve cache requests and flushes.

Xano’s picture

Status: Needs work » Needs review

It just occured to me that the example I posted 1) has a bug or 2) redefines how we rebuild data.

Let's say we convert menu_rebuild() to use cache_clear('menu'), where the menu cache in system_cache_info() has cache_menu as the bin. Now we can clear the menu cache through the new cache APi, but we cannot rebuild it, so it's kind of useless. We can set menu_rebuild() as the callback for the menu cache, and set cache_menu as the bin, but then we'd need to rebuild the menu through cache_clear('menu'). I don't have trouble with that, but I understand if others don't find this very semantical.

Xano’s picture

Status: Needs work » Needs review
FileSize
38.76 KB

Preliminary patch that implements the example in #8. Untested and unfinished. Use at your own risk, because your computer will most likely blow up or something.

Status: Needs review » Needs work

The last submitted patch failed testing.

Xano’s picture

Status: Needs review » Needs work
FileSize
38.66 KB

Updated the patch with a few conversions and fixes. The page is now accessible at Development > Clear caches.

Xano’s picture

Title: Refactor cache clearing » Refactor cache API
Status: Needs review » Needs work
FileSize
65.29 KB

All calls to the old cache_clear_all() have been converted to cache_clear() or cache_clear_item() with the exeption of cache.test (I need some more time and energy to write new tests).

Also, you will note the parameters for cache_get() and cache_set() have changed. They no longer accept a bin name, but a cache name. This is for consistency and to force developers to expose their caches through hook_cache_info().

The title property of exposed cached is no longer required. If provided, the cache can be cleared through the UI. If omitted, the cache can only be cleared through the API. So far this has only been done for cache_form(). We don't want site administrators to clear cached forms when people are still using them.

Xano’s picture

FileSize
65.72 KB

I forgot to update the API docs.

Xano’s picture

FileSize
73.87 KB

- Change cache_clear_item() to cache_del() for consistency with cache_set() and cache_get() and the variable_*() functions.
- Split up DrupalDatabaseCache->clear() into clear() (to wipe every single item from the cache, expired or not), del() (to delete a single item from a cache) and del_expired() (to delete only expired items i.e. during cron runs)

I am wondering if DrupalDatabaseCache->get() and DrupalDatabaseCache->del_expired() can be made any simpler. As fas as I can see we can just execute the query from DrupalDatabaseCache->del_expired() to wipe expired data and do a simple if ($cache->expire < REQUEST_TIME) {return FALSE} in DrupalDatabaseCache->get()

Xano’s picture

Status: Needs work » Needs review
FileSize
73.4 KB

- Removed cache_clear_all() to force developers to manually pass on caches to cache_clear(). This is an attempt to prevent accidental clearing of dangerous caches like cache_form().

Dries’s picture

I still think there is some messiness around del() and clear(). I suggest we use more explicit names like delete_item($cid), delete_range($wildcard), delete_expired(), and delete_all() or something. Thoughts on that?

Status: Needs review » Needs work

The last submitted patch failed testing.

Xano’s picture

Understandable. This was just done for consistency with the cache_set(), cache_get() and the variable_*() functions. These functions use 'del' instead of 'delete' and don't use 'item' in their names. Also, when deleting a range we shouldn't pass on wildcards. Ranges work with from and to values.

Basically: we should be consistent. If we decide to go with 'delete' instead of 'del', we should create other issues that update other occurences as well.

Xano’s picture

Just a personal reminder to check something out:

Xano__: it caches menu trees per page or something, look in menu.inc for cache_get and you'll see it.
catch

catch’s picture

Version: 7.x-dev » 8.x-dev

Shame this didn't make it. Definitely needs sorting out for Drupal 8.

amateescu’s picture

Status: Needs work » Closed (duplicate)
Xano’s picture

Assigned: Xano » Unassigned