Allow JS/CSS aggregation in private mode
Gribnif - May 24, 2007 - 19:56
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | system.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | patch (code needs review) |
Description
5.x adds a nifty feature to aggregate multiple CSS files into one, so that page loads happen more quickly. However, the current implementation is disabled if the file download method is set to "private".
The attached patch against 5.1 allows the feature to work in private mode, as well. It installs a new menu callback function that accepts the name of the CSS cache file to load. Care is taken to ensure that only CSS cache files can be loaded this way.
| Attachment | Size |
|---|---|
| css-preprocess.patch | 3.81 KB |
#1
+1
#2
The problem with this approach is this requires a full Drupal bootstrap to load a CSS file -- e.g., you can perform twice the number of queries / double the load on your server for every page hit.
This is indeed a problem but there needs to be a better solution for private files which are in a state of flux in HEAD. I'd like to hear Darrel's response to this since he has some ideas that might make this work with no performance drains.
#3
Taking the bootstrap into consideration, I do agree that it's more wasteful than it probably could be. But I'll bet it's still better than fetching (or trying to fetch with If-Modified-Since) 10 separate CSS files.
Perhaps a dedicated script file, which only serves up preprocessed CSS pages, would help? If there is any interest, and nobody has tried yet, I can look into it.
#4
Patch no longer applies
# patch -p0 < css-preprocess.patch
patching file includes/common.inc
Hunk #1 succeeded at 1544 (offset 91 lines).
Hunk #2 succeeded at 1539 (offset 2 lines).
patching file modules/system/system.module
Hunk #1 FAILED at 304.
Hunk #2 FAILED at 706.
Hunk #3 succeeded at 2753 with fuzz 2 (offset 371 lines).
2 out of 3 hunks FAILED -- saving rejects to file modules/system/system.module.rej
#5
I've corrected it.
#6
Ooops. That didn't work either. This one does.
#7
Wouldn't be easier to *always* require a public files directory for this to work? Those sites using private files for thye upload module, etc, could then add a second, different directory, for that.
#8
The patch doesn't apply anymore: failed hunks in the system.module.
#9
We had that discussion again, some weeks ago. Personally I would agree with you, but D7 is expected to ship with public/private by file functionality, so creating a new folder for D6 was denied.
So let's hope for D7.
#10
See also Lynn issues :
D5 : #298180: Allow CSS Aggregation to Work with Private File Mode
D7 : #298190: Allow CSS Aggregation to Work with Private File Mode
#11
Reroll.
#12
could be related: http://drupal.org/node/306750
Also, I don't really like the direction of this patch. Why not simply require the admin to set up 2 separate files directories in order to use private downloads. Thus, they will always set up a public downloads dir (e.g. for aggregated CSS, JS, etc) and must set up a separate one that is checked to be not accessible by http in order to enable private downloads.
#13
#14
#15
#16
I'm a little confused by the thread here. Is there a patch I can apply against d6 to get this capability?
#17
Updated for HEAD and expanded to support JS too.
Now uses hook_file_download() rather than a menu hook, so the preprocessed files are served as any other files.
The files are sent with headers that allow caching in browsers and proxies. This is particularly relevant in a reverse proxy setup.
I added a warning to the admin/settings/performance explaining the performance penalty this may cause when used with private files and without a reverse proxy.
BTW does anyone know why there is an file_exists() check in drupal_get_css() but not in drupal_get_js() ?
The fact that you have to select either public or private files is far from perfect. Often you would want some files to be static (like images on the front page of the intranet) and some files to be private (like the board meeting notes), so a hybrid would be very useful. However, the scope of this patch is to provide a simple solution within the existing framework.
Another perspective that probably is not relevant to that many people: In our setup we have a cluster of web servers that have access to a shared disk. We use private files with a stream wrapper to emulate a shared filesystem. If the generated files are saved in the local filesystem, they have to be created on or copied to each individual web server (because the server generating the aggregated file and outputs the <link rel="stylesheet" href=".."> reference is not necessarily the same as the one serving the aggregated file) - something that is currently not necessary for other files.
#18
Please don't forget that the patch in #166759: SoC 2007: Public/Private File Handling would "automatically" solve this issue. I would WON'T FIX this issue here.
#19
I agree with hass here - I think that fixing the underlying public/private mechanism is a much more elegant approach and fixes a bunch of other problems too.