• the page cache setting is a radio button, whereas the block cache setting is a checkbox
  • the "Minimum cache lifetime" setting is also used as a "Maximum page cache age", which is a weird semantic mix-up
Files: 
CommentFileSizeAuthor
#13 performance page.png45.35 KBDamien Tournoud
#12 730046-performance-settings-12.patch11.26 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 20,294 pass(es).
[ View ]
#11 730046-performance-settings-11.patch11.26 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 20,311 pass(es).
[ View ]
#9 730046-performance-settings-9.patch9.35 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 20,318 pass(es), 0 fail(s), and 31 exception(es).
[ View ]
#8 730046-performance-settings-8.patch2.5 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 20,288 pass(es).
[ View ]
#1 cache-before.png28.5 KBDamien Tournoud
#1 cache-after.png32.2 KBDamien Tournoud
#1 730046-performance-settings-inconsistent.patch2.58 KBDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 18,192 pass(es).
[ View ]

Comments

StatusFileSize
new2.58 KB
PASSED: [[SimpleTest]]: [MySQL] 18,192 pass(es).
[ View ]
new32.2 KB
new28.5 KB

Here is a patch:

Before:

After:

Status:Active» Needs review

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.

StatusFileSize
new2.5 KB
PASSED: [[SimpleTest]]: [MySQL] 20,288 pass(es).
[ View ]

ok, guess that was more than a day.

StatusFileSize
new9.35 KB
FAILED: [[SimpleTest]]: [MySQL] 20,318 pass(es), 0 fail(s), and 31 exception(es).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 730046-performance-settings-9.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new11.26 KB
PASSED: [[SimpleTest]]: [MySQL] 20,311 pass(es).
[ View ]

oops - didn't fix all the constants.

StatusFileSize
new11.26 KB
PASSED: [[SimpleTest]]: [MySQL] 20,294 pass(es).
[ View ]

improved description text via discussion w/ Damien.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new45.35 KB

Here is the result. That looks good to me.

Status:Reviewed & tested by the community» Fixed

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.

That's for cache_get()/cache_set().

Status:Fixed» Needs work

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

Status:Needs work» Fixed

It looks like this issue can be closed.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.