The attached patch cleans up Administer > Site configuration > Performance:
- Block and page caching settings are merged, since they share the same cache lifetime setting and creating a new fieldset for that select list didn't make sense.
- A lot of text has been rewritten. It is now more to the point and smaller, making the page less cluttered.
- Enabled/Disabled radios have been converted to checkboxes. This isn't (yet) possible for block caching, since it uses a key-value pair for its options.
- Page compression is being hidden using JS if page caching is disabled.
- Module names are now human readable names instead of system names.
- Error messages telling administrators why CSS or JS optimisation isn't available instead of telling them at all times that they may or may not be available.
Comment | File | Size | Author |
---|---|---|---|
#71 | 459980-follow-up-3.patch | 2.27 KB | c960657 |
#70 | 459980-follow-up-2.patch | 4.16 KB | c960657 |
#69 | 459980-follow-up-1.patch | 1.68 KB | c960657 |
#61 | performance_settings_13.patch | 15.92 KB | Xano |
#60 | performance_settings_13.patch | 15.92 KB | Xano |
Comments
Comment #1
Dries CreditAttribution: Dries commentedLooking at the screenshots, it feels like 'page compression' now belongs under bandwidth optimizations instead of 'caching'.
Maybe we should rename 'Optimize CSS' to 'Compress CSS'? Ditto for JS. That would make the 3 options nicely aligned, and it provides a bit more information to the reader.
One of the things that has always annoyed me, is how the 'clear cache' button is all the way at the bottom. As a developer, reviewer and committer I do a lot of navigation to that page, and then scrolling down to that button. This patch will help because it makes the page shorter.
Comment #2
XanoI'd say we use the current descriptions of CSS and JS optimisation as their titles. Page compression might move there, but then it loses its ties to page caching on which it depends.
Comment #3
catchWe don't actually compress CSS and JS - they're neither packed nor gzipped in core, just aggregated together into one file, so I'd be wary of using compression there.
Comment #4
XanoCSS is partially compressed, JS isn't compressed at all.
Comment #5
XanoBandwidth optimisations have been improved and the (error) messages for aggressive caching are hidden using JS if aggressive caching is disabled.
Comment #6
Dries CreditAttribution: Dries commentedI think the last patch is an improvement. When testing the patch, I wondered if we could hide the cache lifetime field when neither the block or the page cache are disabled.
Either way, I'd move the lifetime setting to the bottom of the caching-fieldset. The primary concern is not the lifetime but enabling or disabling the caching.
Comment #7
XanoTrue, but does caching work if the lifetime isn't set?
Also, the JS needs some good review. It looks kinda ugly, but I haven't been able to make it smaller than this.
Comment #8
XanoHiding cache lifetime is tricky, since the block cache setting is added using hook_form_alter() and the page cache setting is defined in the form builder itself. Hiding it would mean extra and duplicate code in system.admin.inc and block.module.
I left in a redundant #weight. The attached patch fixes this.
Comment #10
andypost#472698: admin/settings/performance: Keyboard <enter> key clears cache instead of applying settings marked as duplicate
Comment #11
JirkaRybka CreditAttribution: JirkaRybka commentedOK, to pull some information from the duplicate issue: The initial problem over on that issue is, that using ENTER [keyboard] to submit the form performs the cache-clear operation instead, which is unexpected and changes to settings are lost (not saved). That's because the form have in fact two submit buttons, cache-clear being first.
rfay posted a patch to change the cache-clear button into a simple link. There seems to be agreement that such approach would need a confirmation form, but IMO that's not the way to go (making the simple cache-flush operation even more complicated on UI).
I mentioned, that we might split the page into two separate forms, each with it's own submit. The current flushing button is not really part of the settings already, so having it outside of the form makes sense. rfay said that he had problems with this approach due to system_settings_form() encapsulating some features, but I guess just doing
return system_settings_form($form) . drupal_get_form($another_form);
might work.If the feature stays inside the form, it should change to something else than a submit button - we still have other options than plain link: Perhaps a checkbox to clear caches on the whole form's submit? That might be checked by default even (serving more as just a visual reminder/description of the flushing feature), as the other settings involve a lot of flushing already. I'm not sure whether we may have a button inside form, that's not submit [i.e. not treated like one in browser, when keyboard ENTER is used to submit] - I didn't investigate on this, there might be another chance in there too.
Personally, I would prefer two separate forms.
Comment #12
XanoChanging the button to a link is a no-go, since buttons indicate actions and links indicate references to other pages or documents of another kind. Moving the "Clear cache" form to another local task might be an idea. This would allow modules to add their cache flushing settings to a central page.
Comment #13
JirkaRybka CreditAttribution: JirkaRybka commentedAnother tab means another click needed :-( And I don't think there's really a need to add to the page - we already have a hook to add to this central cache flush, whose basic idea is to flush just *everything* so that the user gets simply and reliably out of *any* trouble with stale data, without further options to understand.
Can we just concantenate two forms on the same page, as I mentioned above, or is there some other problem with that? It won't change the page that much. Modules may even hook_form_alter() the flushing form, should they need that in some edge case.
Comment #14
andypostAnother opinion, now we have ertical tabs in core so it's possible to split cache settings/actions with different tabs.
As for me it's different functionality and which one is more recently used this should be opened by default.
Suppose settings is visited rarely then settings.
Comment #15
rfayHow do we move this issue forward? I'll be happy to help if I can. It seems the patch doesn't even apply any more.
Comment #16
XanoI'll take another run tomorrow. @rafy: If you want to help, apply the patch, inspect the reject files and redo those tidbits that the patch can't do anymore, because HEAD has changed already.
- I think we can do this one without Vertical Tabs. The page is not too long anymore and VT just hides a bunch of stuff that could otherwise be reached using a bit of scrolling on smaller screens.
- JS needs review.
- CSS and JS optimization descriptions need to be merged with their titles.
- "This option should be disabled when using a webserver that performs compression." -> "Unnecessary if your webserver compresses pages."?
- A default minimum cache lifetime is a good thing. Now users need to set two settings at least, but with a default lifetime they could save a step in some situations.
- Find a way to toggle block caching using a checkbox. Currently it uses constants as values for the radios, but this is bad for the UI.
Comment #17
JirkaRybka CreditAttribution: JirkaRybka commentedThis might be not just "unnecessary" - I seem to recall there used to be issues with double-compressed pages. But I may be wrong, better check.
Comment #18
XanoNow I think of it, can't we do a check to see if the webserver does page compression?
Comment #19
Xano- CSS and JS optimisation descriptions were already merged.
- The checkbox for page compression is not being added if the server already compresses pages.
- Since we toggle caching anyway, there is no need for a blank default cache lifetime. It now defaults to 1 minute.
- Block caching is now toggled using a checkbox. The constants it uses had the values 0 and 1, which works perfectly with the return values of a checkbox.
- Minimum cache lifetime has moved to the end of the fieldset as per Dries in #6.
Comment #20
XanoAnd again I left in pieces of a previous patch... Here's the right one.
And a *pretty* screen shot:
Comment #21
Dries CreditAttribution: Dries commentedThe "cache blocks" settings is visually odd. I think it would look more balanced with the page cache if we'd used radio-buttons in this case.
I
Comment #22
JirkaRybka CreditAttribution: JirkaRybka commentedSince #472698: admin/settings/performance: Keyboard <enter> key clears cache instead of applying settings was closed in favor of this, I was looking for some mention of the cache-flush button getting fixed, but found none in this thread. The patch seems to change type from "submit" to "button" though. Does that help with the [keyboard] Enter submitting issue? (Let's state that here)
Doesn't block caching checkbox need a bit of description, to indicate what it is, and to be in-line with rest of the page?
Comment #23
Xano@JirkaRybka: the change to a regular button was a leftover from an attempt to get it working, but it didn't work. It was accidentally left in and the submit handler got removed. I'll fix this in a follow-up, but after my lunch :-P
Comment #24
andypostDoes we really need "Cache blocks" ? Every block already with own cache settings, suppose this should be deprecated.
Comment #25
XanoThe same can be said of pages afaik. You may have a good point, but frankly said is not within the scope of this patch.
Comment #26
alpritt CreditAttribution: alpritt commented--Page caching--
I'm not sure why we provide the option to choose between normal and aggressive caching. If any enabled modules will break under aggressive caching we should just disable it. And if none will break, we should just enable aggressive caching to give us the best performance.
As far as I can tell we are just requiring humans to make a technical decision that should be made in code.
This is particularly problematic because no one can really make an informed choice here unless they understand how caching works as a developer.
This would be another issue, of course. But this may save us worrying about this problem in this issue if others agree.
--Page caching 2--
We've lost the page caching description starting 'Enabling the page cache will offer a significant performance boost' and not replaced it with anything. I don't think this text was redundant.
--Block caching--
No explanation at all? I think we are assuming a lot of knowledge here. (Though the old version had a lot of redundant text, so this is actually probably better).
--Explanations for developers--
If you are a developer you should know that caching can break stuff. I think we should get rid of these warnings.
Comment #27
XanoThe whole idea behind a cache is that it increases performance. IMO an explanation about this doesn't belong in the description of any of the cache-related settings. We could add an explanation to the fieldset's description, though. Block caching is more of the same.
Comment #28
XanoFancy new patch. I made some improvements and undid the bug that was introduced by the previous patch.
Comment #29
Dries CreditAttribution: Dries commentedThe new patch doesn't seem to address some of the concerns raised?
Comment #30
XanoYou're right. People here think we should add an explanation about caching, Bojhan says otherwise. There is no proper solution for the clear cache button yet. As far as I know I have given explanations as to why I haven't addressed some of the other concerns with this patch.
Comment #31
JirkaRybka CreditAttribution: JirkaRybka commentedAs for the clearing button, my suggestion is the same as before: Render the "Clear cache" fieldset as separate form, and append it to the same page after the settings form (or before, like Views do - visually at least, otherwise it suffers the same problem).
Also, the description of that button seems to be gone (on screenshot above), but I believe it should be kept. This is a troubleshooting feature, not only for developers. Users need to see this direction in troubleshooting, somewhere, which is why the button was first added. There's no obvious connection between "help, this my new translation doesn't take effect" and "Clear all caches", unless you know a lot.
Comment #32
Xano1) Adding the Clear cache fieldset as a separate form causes it to be displayed either under the submit buttons of the settings form or at a separate page. I'd like some more opinions on these options before going through with either of them.
2) The major problem with the page as it is in HEAD is that there are explanations about what a cache is and does all over the place and none of them are short and simple. If we want to explain users what a cache is, we should decide how and where do this. Also, the last place a user is going to look when his theme doesn't work as expected is the bottom of a page called Performance.
Comment #33
JirkaRybka CreditAttribution: JirkaRybka commentedI believe this makes even more sense than what we have now, as this is not a part of the settings submitted by regular buttons.
Agreed. But we don't have a central "Troubleshooting" page, and it's better to have it somewhere, than nowhere. We don't need to explain caching all around, but we need that bit about "troubleshooting modules, themes, and translations" to make the feature recognizable for everyone.
Comment #34
XanoIf we want an explanation about what caching is and does, we need suggestions as to how and where we explain this. As far as I can see the best place is in the Caching fieldset, but it seems far from ideal. I hope someone else can come up with a better solution.
In response to the issue about the clear cache button: we can use hook_flush_caches() to gather data about all modules that use the caching system. Instead of a single button we can create a set of checkboxes where users can select for which modules to flush the cache. We would no longer need a specific button for this, but if we want to we can put this form at a separate page. If we decide to do this, this issue should leave the Clear cache button as it is now and a new issue should focus on changing it to the selection form.
Comment #35
tstoeckler+1 for #34.
Exactly, but they will click on a page called "Troubleshooting". I think taking the "Clear caches" button away from performance makes sense, as it has nothing to do with performance (except draining it...) on an application level. It of course has to do with performance in that the caches that are flushed are only ever introduced to improve performance, but when you click that button, it has nothing to do with performance optimizations or performance settings.
Definitely a different patch, though.
Comment #36
rfayI second the motion on #34: The clear cache possibly doesn't even belong here.
But having checkboxes for
Might work just fine here.
Comment #37
Dries CreditAttribution: Dries commentedMoving it to a separate page could work for me (but might require some additional thought).
I still think we need to do something extra with 'block caching'.
Comment #38
rfayI'd be happy to roll a patch for it on a different page.
What would we call it? Cache Maintenance?
It could have checkboxes for the various items, pre-checked, with a submit button and some explanatory text.
Comment #39
rfayI created #496248: Refactor cache API
Comment #40
XanoJust because it looks odd or because you think the current solution is bad?
//Edit: the menu item for admin/settings/performance needs an update as well.
Comment #41
rfayJust trying out the patch from #28 -
On firefox or seamonkey (Ubuntu or Windows) when you go to admin/settings/performance it shows the screen but immediately redirects to admin/settings/clean-urls. You see the performance screen go by, but there's an immediate redirect.
On IE8/WinXP, the patch works fine.
Comment #42
Xano@rfay: See #479604: Drupal.behaviors.cleanURLsSettingsCheck() in system.js redirects to clean urls settings page incorrectly..
Comment #43
rfayThis patch seems to work fine if #479604: Drupal.behaviors.cleanURLsSettingsCheck() in system.js redirects to clean urls settings page incorrectly. is installed.
I note that the default settings are quite different than they used to be. (normal page caching, for example, used to be the default; now it seems to be no caching).
Comment #44
Dries CreditAttribution: Dries commentedXano: I think it lacks some context and help. If we used radio buttons, we could have two options: "(i) Disabled" and "(ii) Normal (recommended)". At least it would make a recommendation for people that don't understand block caching. I don't know, the checkbox feels 'very naked' to me without anything else.
Logically, I also think 'block caching' is secondary to 'page caching'. It's just a "feeling" but it makes me want to put it below page caching.
Comment #45
XanoPutting it below page caching gives me the feeling it belongs with page caching due to lack of a bold label. Also, what help do you want to be added? I agree it looks naked; I have the same feeling. However, most developers have a tendency to add descriptions to form elements without there being a real need for them.
I had some discussions with people and the general consensus is that we shouldn't tell users how to manage their sites. We should tell them how Drupal works and let them make the decisions. The only reason I have kept "Normal (recommended)" in for page caching is that aggressive caching has side effects.
@rfay: this patch doesn't change the default value for page caching.
Comment #46
Dries CreditAttribution: Dries commentedIf it were radio buttons, you could put it below page caching because it would look as if it is on the same level a bit more -- I think.
Comment #47
SteveBayerIN CreditAttribution: SteveBayerIN commentedI'd place the "cache blocks" option just above the cache life time option. I wouldn't use radio buttons for only 2 options though (a check box looks much cleaner than 2 radio buttons.)
Overall, the patch really cleans up the performance page by replacing radio widgets (only useful when there are 3 or more options) with check boxes.
Comment #48
XanoDrupalDatabaseCache->clear()
in cache.inc seems to expect the system variable cache_lifetime can have a value of 0, so we cannot remove that option from the select list.Comment #50
rfay+1 Still hoping we can get this done for D7. Especially getting that clear cache button to a happier place :-)
Comment #51
XanoI added the option for 0 cache lifetime back in. Here's a screenshot of how it looks in Seven.
Comment #52
XanoChanged "Bandwidth optimizations" to "Bandwidth optimization". It is a general term so afaics it doesn't matter, but in the weird case one of the optimizations gets removed from the form it will still be correct, and it is shorter in most languages.
Comment #53
rfayI did a casual once-over and it looks fine to me and works fine for me, except of course that I want the "Clear cache" to move out of here.
"Cache blocks" might be improved with a few more words, since both of those words can be either nouns or verbs, and it can be a little confusing. It might be a bit clearer as "Cache Drupal blocks for unauthenticated users" or something of that type.
Comment #54
XanoIs page caching for anonymous users as well?
Comment #55
rfayPage caching is exclusively for anonymous users. You can't have it for logged in users, unless there was a big change in D7.
Comment #56
XanoThe last patch was an empty file for reasons unknown to me.
Comment #57
XanoComment #58
XanoBojhan advised to move the clear cache button to the top of the form. I rewrote the description of aggressive caching with the help of merlinofchaos. This version avoids technobabble, but is still enough for developers to see what it does exactly.
From & to:
Comment #59
catchBlocks are cached for all/any users, pages are cached for anonymous users only.
Comment #60
XanoFixed. I also moved block_form_system_performance_settings_alter() from block.module to block.admin.inc.
Comment #61
XanoFixed. I also moved block_form_system_performance_settings_alter() from block.module to block.admin.inc.
Comment #62
Dries CreditAttribution: Dries commentedI think this is a big step forward, so I committed it to CVS HEAD. We can follow-up with more refinements! Thanks Xano.
Comment #63
mikeytown2 CreditAttribution: mikeytown2 commentedvariables should use a bool instead of an int since it is a checkbox.
$_SERVER['HTTP_ACCEPT_ENCODING']
is the wrong check. Thats what the browser tells the server, if it will accept gzip encoding; I getgzip,deflate
. Speaking of deflate, if checking, that needs to be checked for as well. One way to check if the server is already sending compressed output would be similar to how we check for clean urls; but look at the headers via http://api.drupal.org/api/function/drupal_http_request/7. Be sure to send it out withAccept-Encoding: gzip,deflate
. Just make sure it hits a page that won't be cached when checking forContent-Encoding: gzip
(do a post request).Comment #64
mikeytown2 CreditAttribution: mikeytown2 commentedbtw, while everyone is here please look at this: #543892: Win XAMPP 1.7.2 Regex PHP Bug - Optimize css & long css comments fails
Comment #65
mikeytown2 CreditAttribution: mikeytown2 commentedone more thing, saying compression on
Aggregate and compress CSS files into one file.
is not clear, especially whenCompress cached pages
is right above it. Changing these to "gzip compress" & "whitespace compress" would be better.Comment #66
Owen Barton CreditAttribution: Owen Barton commentedI agree that $_SERVER['HTTP_ACCEPT_ENCODING'] is plain incorrect and should be removed or replaced by a http request and header check (gzipping could also be being done by a reverse proxy, so there is really no way to check this without a network request).
Comment #67
mikeytown2 CreditAttribution: mikeytown2 commentedThis is, in it's current form a bug IMHO, and a bad one at that.
Comment #68
peterjmag CreditAttribution: peterjmag commentedIs normal vs. aggressive caching now selected automatically, as suggested in #26, or was it removed entirely? If the former is true, perhaps there should be some indication of what caching mode has been selected for you (and why). As it stands in HEAD, the behavior seems ambiguous. Of course, this might only be an issue for long-time Drupal users; overall, I'm sure the Performance page makes a lot more sense to new users now.
Comment #69
c960657 CreditAttribution: c960657 commentedThis replaces the HTTP_ACCEPT_ENCODING check with a check for zlib.output_compression. At least this catches one common configuration with transparent compression.
As mentioned, a more thorough check would require a HTTP callback. I'm not sure it's worth it, though. Users who installed mod_gzip or a compressing proxy are more likely to know what they are doing.
Comment #70
c960657 CreditAttribution: c960657 commentedHiding the checkbox does not turn off the setting but just prevents the user from changing it, even when it has been set to off. We need to do the same check when the variable is accessed.
Comment #71
c960657 CreditAttribution: c960657 commentedActually the page cache works fine with zlib.output_compression enabled, and no double encoding takes place. zlib.output_compression only compresses the pages that are not served from the cache.
Comment #72
andypostGood hit, no need to check this ($_SERVER['HTTP_ACCEPT_ENCODING'], 'gzip') in admin settings because it depends on visitor settings from browser so this is a job for the page serving system. This was fixed in #43462: cache_set and cache_get base_url brokenosity but still not backported to 6
Comment #73
webchickNice clean-up. Committed to HEAD, thanks!
Comment #75
mrj0909 CreditAttribution: mrj0909 commentedHi!
I am new to Drupal. And I have to create single file for ALL CSS and JS file for caching purpose. Can Anyone help me.
And I want o know how to use this patch.
Waiting for quick response.
Thank You.
Comment #76
rfay@mrj0909: Welcome to Drupal.
This isn't really the place for your support request, though. You can create a new support request for Drupal 6, and find out about other support options at http://drupal.org/support.
The Drupal 6 admin/settings/performance does aggregate your CSS files into a single file, likewise with your javascript files, with some exceptions. Just go to admin/settings/performance.
Comment #77
Xano