check_markup() hardcodes cache expiration

No activity, Close..

Did you ever get an answer? I can see it's still in HEAD.

- Arie

That sounds like a challenge ;-)

Patch attached. Let's see what people think.

- Arie

Still not much attention. Setting to bug report as this arguably is a bug.

- Arie

@NancyDru: Could you test this patch against HEAD so it can be set to RTBC ?

- Arie

its make total sense, i also had the issue when writing a filter, that setting cache_lifetime didn't changed anything

BUT
every code in core uses cache_lifetime with default_value: 0

./modules/system/system.admin.inc:1352:  $form['page_cache']['cache_lifetime'] = array(
./modules/system/system.admin.inc:1355:    '#default_value' => variable_get('cache_lifetime', 0),
./modules/system/system.install:1144:        'description' => "The time of this user's last post. This is used when the site has specified a minimum_cache_lifetime. See cache_get().",
./modules/simpletest/tests/cache.test:99:    variable_set('cache_lifetime', $time);
./includes/cache.inc:22:  if ($cache_flush && ($cache_flush + variable_get('cache_lifetime', 0) <= REQUEST_TIME)) {
./includes/cache.inc:36:    if ($cache->expire == CACHE_PERMANENT || !variable_get('cache_lifetime', 0)) {
./includes/cache.inc:159:    if (variable_get('cache_lifetime', 0)) {
./includes/cache.inc:171:      elseif (REQUEST_TIME > ($cache_flush + variable_get('cache_lifetime', 0))) {

Here is the result of running the tests:
8138 passes, 0 fails, and 0 exceptions

here is the patch

Things that are run through check_markup should always be cached because we can always cache them -- there is no harm.

This patch can result in severe performance losses.

so here is a current patch with a default of 86400s

OK back to the original patch (rerolled) to get this issue moving again.

- Arie

Added a comma to the comment (yes really) :-) per discussion with dereine on IRC.

- Arie

i used this method of cache lifetime and replacing 24*60*60 with the calculated value 86400

hey you also removed the s from days ^^

+    // Store in cache using the system's expiration time or an expiration time of 1 day if it is not set.

...should rather use "default" here. Also, no need to repeat "expiration". And, additionally, comments need to wrap at 80 chars.

-      cache_set($cache_id, $text, 'cache_filter', REQUEST_TIME + (60 * 60 * 24));
+      cache_set($cache_id, $text, 'cache_filter', REQUEST_TIME + variable_get('cache_lifetime', 86400));

Please revert to 60 * 60 * 24 instead of 86400. If we are going to change this, then we will change this everywhere in core (in a separate issue); changing it just here means inconcistency == bad.

Rerolled and modified per #17

- Arie

Could have sworn I set this...

- Arie

rerole

Changed files:

misc/form.js
modules/filter/filter.module
modules/statistics/statistics.admin.inc
modules/statistics/statistics.module
modules/statistics/statistics.test

I'm sure you didn't mean to eat a kitten. :P

uh

thats strange :)

The cache_lifetime variable is widely misunderstood. Possibly including by me. I think of it as the delay until temporary/expired entries will actually be removed from the cache, once a cache wipe of such entries has been requested. AFAIK it was not intented to be used as suggested here.

Quite possibly :P (http://drupal.org/node/227228#comment-1501876).

In that related issue #227228: cache_clear_all and cache_get fail to clear caches when Minimum cache lifetime is on I suggested renaming it to cache_flush_delay: http://drupal.org/node/227228#comment-1278550. Never got round to it at the time; maybe it is time to do that now!

And more ... #256416: Editing a node does not wipe its cache entry

This should never be set to the value of cache_lifetime.

However it's also a bit senseless to hard code this to one day. I would make it a variable (with no user interface for it), defaulting to around 2-3 weeks so that entries that are never going to be used can be expired from the database cache, but we're not recreating the same caches over and over again on sites with a lot of content.

Meanwhile, I think this is fundamentally flawed.

A minimum cache expiration time does not really make any sense at all. The filtered text either changes or it does not. If it doesn't then there's no reason for flushing the cache.

The filtered text can only change when

1. the input changes - which means that the cache hash automatically changes, too; the original cache "expires" automatically, it's just simply obsolete.
2. the filter configuration for the text format is changed

I was thinking we might want some garbage collection here, but that should be handled by full cache clears, or using a cache backend like memcache that does LRU. It's not much different to any other cache in Drupal on that front and you'd need a lot of content to run into issues - by that time you'll be running into the same issue elsewhere.

Filters can declare themselves uncacheable, so that is already handled (and 24 hours is arbitrary anyway).

RTBC.

Tagging issues not yet using summary template.

Good catch, and might actually yield a small performance improvement.

Committed to 7.x and 8.x.

The performance improvement here is not "small". Majority of search engine traffic is from low frequency keywords hitting rarely accessed pages. So for most of these visits the filter cache is missing and visitor experiences slow page generation time.
The patch from sun makes total sense and the pre-patch behavior was simply dumb.

This was never backported to D6. Was looking at performance issues on a site being very, very heavily crawled, and rebuilding the filter cache was taking a lot of time in a lot of pages.

We should take over and backport the inline comment from #31, too.

The @see can be replaced with filter_admin_format_form_submit()

With that, the backport totally is RTBC (I just double-checked a couple of cache and filter functions for their D6 behavior).