Performance settings page is inconsistent

the page cache setting is a radio button, whereas the block cache setting is a checkbox
I think of the page cache setting as a list of available cache backend page cache implementations, so I think this should be be radios. To be honest, though, I don't know of any project that currently alters this form element.

+++ modules/system/system.admin.inc
@@ -1644,6 +1644,13 @@ function system_performance_settings() {
     '#description' => t('The minimum amount of time that will elapse before the caches are recreated.')

Can we change this into:

"The amount of time that needs to elapse before caches are recreated."

i.e.
s/minimum//
s/will/needs/
s/the//

+ if you can think of a phrasing to kill the "amount of", that would even more cool.

+++ modules/system/system.admin.inc
@@ -1644,6 +1644,13 @@ function system_performance_settings() {
+    '#description' => t('The maximum amount of time a page can be served from cache.')

Same here,

"The amount of time a page can be served from cache."

still, "amount of time" reads strange + too lengthy

143 critical left. Go review some!

The time that must elapse before caches are recreated.

Caches are not re-created within the chosen time-frame.

Caches are re-created after the selected interval only.

Created caches are kept for this minimum retention time.

...

Note: The wording should make clear that caches will be kept + used + not rebuilt during the minimum lifetime.

...to make it even more complex, the minimum cache lifetime setting will prevent the entries in cache_page from being cleared, *but* the expiration headers that Drupal will sent out are controlled by the maximum page cache setting...

So the label for maximum should actually be like: "Expiration of cached pages"

+ we could perhaps drop the description

I'll try to re-roll with new text tomorrow.

ok, guess that was more than a day.

We have defined constants like CACHE_NORMAL that we are using. The code just happens to work since CACHE_NORMAL == 1.

So, if we want to use a simple checkbox (on/off) we should just stop using those constants.

oops - didn't fix all the constants.

improved description text via discussion w/ Damien.

Looks good. Committed to CVS HEAD. Thanks.

+++ includes/bootstrap.inc
@@ -49,16 +49,6 @@ define('CACHE_PERMANENT', 0);
 define('CACHE_TEMPORARY', -1);

What's that, btw?

Powered by Dreditor.

I just filed a new related issue: #804864: External page caching does only work when "page_cache_invoke_hooks" is set to FALSE. (Old "Aggressive Mode" is on)

Sorry for arriving late in this ticket, but I have some comments:

+    variable_set('cache', 1);

Why is this variable an integer and not a boolean? It seems that it is only 0 or 1 (except in statistics.test where it actually is TRUE).

     '#default_value' => variable_get('cache_lifetime', 0),
     '#options' => $period,
-    '#description' => t('The minimum amount of time that will elapse before the caches are recreated.')
+    '#description' => t('Cached pages will not be re-created until at least this much time has elapsed.')

cache_lifetime is not specific to the page cache, is it?

+    '#title' => t('Expiration of cached pages'),
+    '#default_value' => variable_get('page_cache_maximum_age', 0),
+    '#options' => $period,
+    '#description' => t('The maximum time an external cache can use an old version of a page.')

I don't think the wording makes it very clear that this has to do with HTTP caching. I suggest e.g. #title => 'HTTP caching', and #description => 'The maximum time a proxy server or browser may cache a page. This only applies to anonymous users.'.

@c960657 - checkboxes or radios return values generally of 0 or 1, not TRUE/FALSE, so setting to 1 is consistent with how it will be set via the UI.

0|1 TRUE|FALSE values for redios and checkboxes fixed in #654796: Identify fix bugs with checkbox element

It looks like this issue can be closed.