Add a development mode to allow changes to be picked up even when CSS/JS aggregation is enabled

forehead = sore from slapping it.

This is a great idea ... brilliant, simple AND brilliant.

+1

Great, now all we need is aggregation on by default on this 31 stylesheet issue will be fixed!

I like this idea, but I have to go -1. Here's the reasons why.

1. Performance. Just testing this on a vanilla D7 install, I compared the times from the current method to this method. As I suspected, doing a filemtime/stat on every css file for every request has a pretty decent performance hit. With 14 css files I saw the loop processing times jump from 0.06ms to 5.5ms. That will be felt on higher performance sites and sites with a lot of CSS files.

2. Page cache. The problem is if a new CSS file is generated automatically, with a new file name, the pages stored in page cache will still have the old CSS hash/file name linked in them. This will create more problems. If you are changing the CSS file, chances are you are logged in, so you won't see a cached version of the page. You quickly forget that your anonymous users will get the cached page, hence giving them the old CSS file.

My suggestion would be to change expand the "clear cache" options. Add checkboxes to it for exactly what should be cleared; each cache bucket, rebuild registries and menu router and then JS and CSS aggregated files (including a toggle all/none option - having it default to all). That way if someone is changing their CSS they could just clear the CSS cache out and the aggregated files will still have the same filename. Yes people will still have to manually clear the cache, but they won't be taking the performance hit of clearing the entire cache.

@intoxication, can you expound on how you were testing? Also, I think some testing would need to be done to verify your second point in comment #3.

I agree with #3, on both counts.

The performance impact is hard to measure, because it will vary significantly from system to system (how good is the OS's file system cache, is NFS being used, is the disk under heavy load). I think it's reasonable to assume that at least for some systems, stats will incur some performance hit that isn't desirable. And on a production site where CSS files aren't being changed, there's no benefit that makes the performance hit worthwhile. So at the very least, I'd want to see this have a setting so it can be turned off on production sites. This patch is most valuable during development, when CSS files are being changed, and while it nicely solves one of the main drawbacks of using aggregation during development, it doesn't solve all of them. You would still lose ability to know source file name and line number when debugging CSS with Firebug.

I think the page cache point is obvious. Just because you changed a source CSS file doesn't mean you've emptied pages from the page cache. So, when drupal_get_css() runs, it will generate a new md5 hash and therefore a new CSS file name for the aggregate file, because the timestamp of one of the source files have changed, but pages in the page cache will still be referring to the old aggregate file name. But this is true with all file changes. When you change a tpl.php file, the change doesn't auto-apply to pages in the page cache. So developers should already be used to the idea that changing any file needs to be followed with emptying the page cache. This is yet another reason why this patch is not so useful for sites in production (that have page cache turned on), but is useful for sites in development (where page cache is either not turned on, or developers know to empty it when updating any file).

So I think this patch does have a role: for sites in development where the developer doesn't need access to CSS source line number when debugging their CSS. Given that limited scope, I think it makes more sense as a contrib module (what an awesome showcase of why the new hook_css_alter() hook is so cool), though I'd be open to it being in core if it came with a setting to disable it.

Sub. This could be a KILLER feature if we can get the gotchas worked out.

+

I see this is a solution for 7, but does anyone know if there is a working solution for version 6.15? There seems to be a lot of failed patches floating around for 6.

Thanks.

There is no accepted solution yet ... once there is a solution it will be for D7 first. Only then might it be back-ported to D6.

Because of #3/#6 and because #228818: IE: Stylesheets ignored after 31 link/style tags is now RTBC, I don't see this as a bug, but a feature request. I'm wondering what folks would think of it being part of http://drupal.org/project/devel instead. My reasoning is based on #6: that the audience for this is developers, and that sites in production shouldn't normally use this (of course, nothing prevents a production site from having the devel module enabled and using this if that's where we put it), because if the site has page caching enabled, you need to empty the cache anyway when making a css change. And because of this, I think a setting on the admin/config/development/performance page would just confuse Drupal novices, but to me, a setting on the admin/config/development/devel page would make sense: just below the "Rebuild the theme registry on every page load" setting, we could have a "Rebuild CSS / JS aggregate files if any source files have changed" setting. Because this settings page is targeted to developers, it could even be more complex and feature rich (like also have an option to "Rebuild CSS and JS aggregate files on every page load").

If there's agreement to move this to the Devel project, I'll do what I can to help get it in there.

Since no one objected to this being moved to Devel, let's give that a shot.

Sorry, didn't see your suggestion to move to Devel until now.

Putting it into core gives the added benefit that we can enable aggregation by default, so I think it'd be good to see if we can get it into 7.x after all. If you still disagree, feel free to move it back.

I agree with mcrittenden

Putting it into core gives the added benefit that we can enable aggregation by default

Yeah, that's a good argument.

We are striving to make Drupal 7 more usable "out of the box" than prior versions of Drupal have been, and part of what that means is matching default settings with best-practice knowledge, or at least, not worst-practice. And a site going live without aggregation just because a novice administrator didn't know about a setting is pretty bad: we should not have default settings be set to what we all know to be worst-practice.

But, we're also trying to not scare away new themers (or novice administrators who want to theme a little), and making aggregation that isn't sensitive to file changes the default was rejected by webchick as too likely to do that: #228818-260: IE: Stylesheets ignored after 31 link/style tags.

So, here's a patch with my recommendation: change the checkboxes to radios (disabled, enabled with file change sensitivity, enabled without file change sensitivity) and make the middle one the default. It's the one that's the most logical choice as a default. Developers wanting source file and line number information from Firebug can explicitly set to "disabled". People running high traffic sites where the extra file stats are a problem (#3.1) can set to "aggressive".

Several tests assume aggregation is disabled. This patch makes those tests ensure that.

Those file stat times look like a commonly known issue on Windows. Did you test on a Windows box by coincidence?

In Drupal you generally cannot expect to be able to change module or theme (or core) files without clearing caches, rebuilding registries etc. When developing, this may be annoying, but it shouldn't be a problem on a production site. The problem is not specific to style sheets but to many different parts.

Making CSS/JS aggregation sensitive to files changes may be relevant during development (though it may be easier simply to turn off aggregation), but if we implement this, I don't think the UI for the setting belongs on the page where JS/CSS is enabled, but on a separate development page where all development-related caches may be disabled, including e.g. the "rebuild theme registry in every request" setting that I have seen implemented in some contrib themes.

just linking to related discussion on turning CSS/JS aggregation on by default: #678160: Turn CSS aggregation on by default

@#19: The numbers in #3 are probably not representative of a standard Linux production server, and with Linux file system caching, mtime is probably cheap enough to not worry about. But, we shouldn't assume a standard Linux file system. There's NFS, GlusterFS, and countless other server configurations. On some of them, file stats are noticeable. That's why APC provides a setting to turn them off, and so should we.

@#20: If you haven't already, please read #12-#16. The setting makes more sense to me as part of devel.module, but #14 makes a good point for it being in core, and core doesn't yet have a separate "development" settings page. Seems like overkill to create a new core settings page for this issue alone, but if there are other uses for it, then I like the idea.

Issue Summary:

• We want aggregation enabled by default, because it makes a huge difference to front-end performance, and administrators new to Drupal might not know about the Performance setting right away.
• We don't want aggregation as it currently is enabled by default, because someone new to Drupal will start trying to tweak a CSS file and be thoroughly confused why their changes aren't taking effect.
• This issue started with a brilliant idea to make aggregation sensitive to file modification time of source files, thereby addressing both of the above.
• The problem with making aggregation sensitive to file modification time is that it requires a file stat on every source file being aggregated on every page request. Usually, that's not a problem, but on some deployments, it's a performance hit with no benefit.
• #18 tries to solve above by turning the aggregation checkbox into a 3-option radio choice. There's some debate (#20) as to whether that's a good thing to do. Even if there were consensus on it being a good thing to do, the text of the radio choices needs refining.

Tagging.

Thanks for that summary!

Please note that #769226: Optimize JS/CSS aggregation for front-end performance and DX will have a major impact on this discussion here. At least as far as JS is concerned, "enabled by default" will then mean that only always-loaded default JS files are aggregated, which normally shouldn't be altered in the first place.

Env: D6.16 ,IE7, CSS and JS Optimize Enabled
otterbay.dksy.net

The home page does not display correctly in IE - OK in Firefox.

Is there an accepted solution yet ...
and is it back-ported to D6
or, since I would like to use Panels, is it time to migrate to D7?

Thanks

@dekalaked: I'm not sure what the bug you're seeing is due to, but it's not related to this issue. Try to get some help in IRC or the support forums for pin-pointing the exact cause of the problem you're having, to find out whether it's a Drupal 6 core bug, and if so, whether it's been fixed in Drupal 7.

or, since I would like to use Panels, is it time to migrate to D7?

Please read the Drupal 7 Alpha 4 announcement. In short, it's still too soon to be using Drupal 7 for a "real" website. Also, there isn't even a semi-working alpha release of Panels for Drupal 7 yet.

Best of luck to you in troubleshooting and getting your website working!

The issue was related to the combination of gallery and image modules that I had been experimenting with. When I started over with a minimal set of panel, view and image modules, the 2 column panel worked: http://otterbaycottage.dksy.net/

+++ includes/common.inc	28 Jul 2010 10:23:37 -0000
@@ -152,6 +152,34 @@
 /**
+ * Constants defining aggregation of JavaScript and CSS files.
+ */

It would be great to append these constants below the already existing JS/CSS constants, instead of just at the end.

+++ includes/common.inc	28 Jul 2010 10:23:37 -0000
@@ -2937,8 +2965,7 @@
-  $preprocess_css = (variable_get('preprocess_css', FALSE) && (!defined('MAINTENANCE_MODE') || MAINTENANCE_MODE != 'update'));
-
+  $preprocess_css = (!defined('MAINTENANCE_MODE') || MAINTENANCE_MODE != 'update') ? variable_get('preprocess_css', DRUPAL_AGGREGATION_ENABLED) : 0;

@@ -3770,7 +3810,7 @@
-  $preprocess_js = (variable_get('preprocess_js', FALSE) && (!defined('MAINTENANCE_MODE') || MAINTENANCE_MODE != 'update'));
+  $preprocess_js = (!defined('MAINTENANCE_MODE') || MAINTENANCE_MODE != 'update') ? variable_get('preprocess_js', DRUPAL_AGGREGATION_ENABLED) : 0;

I'm pretty sure that you need to drop the MAINTENANCE_MODE exception, as this kind of exception actually states that it should try to look up a variable, and on top of that, default to enabled -- during installation.

The previous code worked, because it defaulted to FALSE.

+++ modules/profile/profile.test	28 Jul 2010 10:23:37 -0000
@@ -339,7 +339,12 @@
+    // disable JavaScript aggregation for this request. ¶

Trailing white-space.

+++ modules/system/system.admin.inc	28 Jul 2010 10:23:38 -0000
@@ -1680,15 +1680,25 @@
+      DRUPAL_AGGREGATION_DISABLED => t('Disabled. This can be useful during development, but slows down page download and display.'),
+      DRUPAL_AGGREGATION_ENABLED => t('Normal (recommended)'),
+      DRUPAL_AGGREGATION_ENABLED_WITHOUT_STAT => t('Aggressive. This setting skips the normal verification of changes to source JavaScript files, potentially reducing disk load on high traffic website.'),

When introducing new constants for this, we absolutely have to make sure that the #options can be extended by contrib. For example, with a DRUPAL_AGGREGATION_ENABLED_PLUS_MINIFICATION or similar.

For that sake, I wonder whether it would make sense to use bit flags (like the menu system constants).

Powered by Dreditor.

contains everything but the bit flag suggestion

#42: aggregation-mtime-678292-42.patch queued for re-testing.

FWIW, this specific problem raised by the OP (developing / testing on IE) can be solved by this module:
http://drupal.org/project/ie_css_optimizer

+++ b/modules/system/system.admin.inc
@@ -1696,15 +1696,25 @@ function system_performance_settings() {
+      DRUPAL_AGGREGATION_ENABLED => t('Normal (recommended)'),
+      DRUPAL_AGGREGATION_ENABLED_WITHOUT_STAT => t('Aggressive. This setting skips the normal verification of changes to source CSS files, potentially reducing disk load on high traffic website.'),

Typo in the last sentence ("high traffic websiteS").

I am generally against adding something like this to the UI, but if that has been decided, can we please not name it "Aggressive"?
1. Aggressive generally has the connotation of having malicious side-effects, which this doesn't have at all.
2. Lots of people will think this is the re-introduction of Drupal 6's aggressive page cache, which actually did have some malicious side effects, depending on the modules you were running.

In general, I think the distinction between what is now "Normal" and "Aggressive" could be clarified IMO.

on a related matter..on Drupal 7.12 site. how do I get the aggregated css file to match my latest settings?..It holds onto the old setting,,even though I clear drupal cache/ firefox cache/ run cron.. I changed a margin from 30px to 3px in my layout.css ,but the aggregated file is always at old value 30px...IF i shut off Aggregate and compress CSS files. i get 3px in .. if i turn it on again i get 30px...but of course i want 3px with Aggregate and compress CSS files. ON.....any way I can do this? I'm not sure how the aggregate and compress phenomenon happens, obviously. I am not sure if the aggregated compressed file can be rebuilt, or how to do it.
I don't have command line access at the moment...I'm on a shared linux hosting.

Please don't hijack the issue.

Patch is really old.

@solodky: clearing drupal cache should get you a new aggregate. if it's not, please open a new issue for that.

Adding the mobile tag, because this might allow us to add Respond.js to core, getting around #1322794-18: Make Stark use a responsive layout, and thereby, solving #1471382: Add IE-conditional classes to html.tpl.php nicely.

#56 does not yet include changing the default in system.performance.xml.

JavaScript for making a layout responsive? This can be done with pure CSS.

Re #58: The issues linked in #57 have more info on this and are the better place for any follow-up discussion on this, but the quick summary is, yes, CSS media queries are great, but IE8 doesn't support them. We can have ie8-specific stylesheets, but that's annoying stuff to have to maintain, and "https://github.com/scottjehl/Respond">Respond.js is a cool library used by many projects (and therefore, not requiring Drupal developers to maintain) for basically adding CSS media query support to IE8, but it doesn't work with CSS added via @import. Anyway, not trying to hijack this issue: just pointing out that a side benefit of it might be to help the Mobile initiative.

NR for the bot.

Tests pass because #56 doesn't contain any new tests or test changes, and as per #57, doesn't change the default configuration.

Untagging "mobile", remarks in #57 are now irrelevant.

I think I am in favor of this approach. The performance win of enabling CSS/JS aggregation out of the box is HUGE.

Care to reroll? #352951: Make JS & CSS Preprocessing Pluggable causes this to need a reroll.

Also in favor of this. Shipping with 3 options makes this work.

removing tag...

.

I would welcome some additional settings for aggregated files too, and this solves an annoying downside of aggregated js during development.

Even if this is only used during development, with the sourcemap issue we get close to a 'good' DX working with js and aggregated files.

I think this would be a good idea as a developer mode, we could possibly add it in a similar way to #3278493: Make it easier for theme builders to enable Twig debugging and disable render cache.

Re-titling since CSS and js aggregation have been enabled by default for some time now, so this is more about allowing them to be enabled during development too without requiring cache clears.

I think there's a possible way to do this now that #3370828: Ensure that edge caches are busted on deployments for css/js aggregates is in 10.1

I think what we'd want, is a setting to disable the caching in AssetResolver::getCssAssets() and AssetResolver::getJsAssets() - this would force the URLs to be generated on every page request. Then unversioned libraries would result in a different URL every time the file changes.

A more aggressive approach would be to do the file_get_contents() check on every file regardless of version when that setting is enabled.