Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
- 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
Comment | File | Size | Author |
---|---|---|---|
#13 | performance page.png | 45.35 KB | Damien Tournoud |
#12 | 730046-performance-settings-12.patch | 11.26 KB | pwolanin |
#11 | 730046-performance-settings-11.patch | 11.26 KB | pwolanin |
#9 | 730046-performance-settings-9.patch | 9.35 KB | pwolanin |
#8 | 730046-performance-settings-8.patch | 2.5 KB | pwolanin |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere is a patch:
Before:
After:
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #3
Eric_A CreditAttribution: Eric_A commentedthe 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.
Comment #4
sunCan 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.
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!
Comment #5
sunThe 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.
Comment #6
sun...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
Comment #7
pwolanin CreditAttribution: pwolanin commentedI'll try to re-roll with new text tomorrow.
Comment #8
pwolanin CreditAttribution: pwolanin commentedok, guess that was more than a day.
Comment #9
pwolanin CreditAttribution: pwolanin commentedWe 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.
Comment #11
pwolanin CreditAttribution: pwolanin commentedoops - didn't fix all the constants.
Comment #12
pwolanin CreditAttribution: pwolanin commentedimproved description text via discussion w/ Damien.
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere is the result. That looks good to me.
Comment #14
Dries CreditAttribution: Dries commentedLooks good. Committed to CVS HEAD. Thanks.
Comment #15
sunWhat's that, btw?
Powered by Dreditor.
Comment #16
Damien Tournoud CreditAttribution: Damien Tournoud commentedThat's for cache_get()/cache_set().
Comment #17
c960657 CreditAttribution: c960657 commentedI 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:
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).
cache_lifetime is not specific to the page cache, is it?
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.'
.Comment #18
pwolanin CreditAttribution: pwolanin commented@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.
Comment #19
andypost0|1 TRUE|FALSE values for redios and checkboxes fixed in #654796: Identify fix bugs with checkbox element
Comment #20
TravisCarden CreditAttribution: TravisCarden commentedIt looks like this issue can be closed.