Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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)?
Comment | File | Size | Author |
---|---|---|---|
#21 | cache_api_07.patch | 73.4 KB | Xano |
#20 | clear_cache_06.patch | 73.87 KB | Xano |
#19 | clear_cache_05.patch | 65.72 KB | Xano |
#18 | clear_cache_04.patch | 65.29 KB | Xano |
#17 | clear_cache_03.patch | 38.66 KB | Xano |
Comments
Comment #1
XanoThe 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 ofhook_cache_info()
andhook_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 tohook_cache_info()
. This is because the$rebuild_schema
parameter. The only consequence of this is that clearingcache_field
needs to be done infield_cache_clear
and not incaches_clear()
.Comment #2
catchWhy isn't the page cache on the screenshot. Haven't reviewed patch itself yet.
Comment #3
rfay@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.
Comment #4
Xano@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.
Comment #5
sun"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).
Comment #6
catchIf 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?
Comment #7
Dries CreditAttribution: Dries commentedWith 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.
Comment #8
XanoThis is just a mockup. An actual patch is in progress, but I want some reviews on the basics before finishing it.
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 - becausehook_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()
invokeshook_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 currentcache_clear_all()
has been renamed tocache_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.Comment #9
catchNot got time to review now but re-titling since this isn't an interface clean-up patch any more.
Comment #10
Dries CreditAttribution: Dries commentedBefore 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.
Comment #11
catchI 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.
Comment #12
Xano1) 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.
Comment #13
sunWhat 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.
Comment #14
XanoIt 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 usecache_clear('menu')
, where the menu cache insystem_cache_info()
hascache_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 setmenu_rebuild()
as the callback for the menu cache, and setcache_menu
as the bin, but then we'd need to rebuild the menu throughcache_clear('menu')
. I don't have trouble with that, but I understand if others don't find this very semantical.Comment #15
XanoPreliminary 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.
Comment #17
XanoUpdated the patch with a few conversions and fixes. The page is now accessible at Development > Clear caches.
Comment #18
XanoAll calls to the old
cache_clear_all()
have been converted tocache_clear()
orcache_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()
andcache_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 throughhook_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.
Comment #19
XanoI forgot to update the API docs.
Comment #20
Xano- Change
cache_clear_item()
tocache_del()
for consistency withcache_set()
andcache_get()
and thevariable_*()
functions.- Split up
DrupalDatabaseCache->clear()
intoclear()
(to wipe every single item from the cache, expired or not),del()
(to delete a single item from a cache) anddel_expired()
(to delete only expired items i.e. during cron runs)I am wondering if
DrupalDatabaseCache->get()
andDrupalDatabaseCache->del_expired()
can be made any simpler. As fas as I can see we can just execute the query fromDrupalDatabaseCache->del_expired()
to wipe expired data and do a simpleif ($cache->expire < REQUEST_TIME) {return FALSE}
inDrupalDatabaseCache->get()
Comment #21
Xano- Removed
cache_clear_all()
to force developers to manually pass on caches tocache_clear()
. This is an attempt to prevent accidental clearing of dangerous caches likecache_form()
.Comment #22
Dries CreditAttribution: Dries commentedI 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?
Comment #24
XanoUnderstandable. 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.
Comment #25
XanoJust a personal reminder to check something out:
Comment #26
catchShame this didn't make it. Definitely needs sorting out for Drupal 8.
Comment #27
amateescu CreditAttribution: amateescu commentedCache API has been refactored in #81461: Clean up the cache API (stop overloading function arguments, remove procedural wrappers).
Comment #28
Xano