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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lilou’s picture

+1

m3avrck’s picture

Version: 5.1 » 6.x-dev

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.

Gribnif’s picture

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.

keith.smith’s picture

Status: Needs review » Needs work

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

alvarezrilla’s picture

FileSize
3.96 KB

I've corrected it.

alvarezrilla’s picture

FileSize
4.09 KB

Ooops. That didn't work either. This one does.

pwolanin’s picture

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.

Hetta’s picture

The patch doesn't apply anymore: failed hunks in the system.module.

Pancho’s picture

Title: Patch to allow CSS preprocessing in private mode » Allow CSS aggregation in private mode
Version: 6.x-dev » 7.x-dev

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.

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.

lilou’s picture

lilou’s picture

Status: Needs work » Needs review
FileSize
3.53 KB

Reroll.

pwolanin’s picture

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.

lilou’s picture

Status: Needs review » Closed (duplicate)
lilou’s picture

Status: Closed (duplicate) » Needs review
lilou’s picture

Title: Allow CSS aggregation in private mode » Allow JS/CSS aggregation in private mode
Status: Needs review » Needs work
Steve Dondley’s picture

I'm a little confused by the thread here. Is there a patch I can apply against d6 to get this capability?

c960657’s picture

Status: Needs work » Needs review
FileSize
11.57 KB

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() ?

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.

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.

hass’s picture

Please don't forget that the patch in #166759: Public/Private File Handling would "automatically" solve this issue. I would WON'T FIX this issue here.

Owen Barton’s picture

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

dageshi’s picture

Gents apologies if this isn't the right place but I have a workaround for this problem in d6, since I found this thread searching on this problem I figure others might.

When I setup my current d6 install with private files, I left the files directory in the webroot as it came e.g. sites/default/files and used .htaccess "deny from all" in order to prevent access.

The obvious workaround for this problem is to generate the aggregate css/js files in their directories site/default/files/js and css respectively then place a .htaccess in the js and css directories with the "allow from all" directive.

Anyone attempting to access any files directly gets 403 forbidden except for the css and js files. Private files work as before.

The steps to do this are as follows

1) Replace $is_writable = is_dir($directory) && is_writable($directory) && (variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC) == FILE_DOWNLOADS_PUBLIC);
with
$is_writable = is_dir($directory) && is_writable($directory); on line 1319 of system.admin.inc in the system module.

2) Replace $is_writable = is_dir($directory) && is_writable($directory) && (variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC) == FILE_DOWNLOADS_PUBLIC);
with
$is_writable = is_dir($directory) && is_writable($directory); on line 1800 of the includes/common.inc file.

3) Replace $is_writable = is_dir($directory) && is_writable($directory) && (variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC) == FILE_DOWNLOADS_PUBLIC);
with
$is_writable = is_dir($directory) && is_writable($directory); on line 2153 of includes/common.inc

4) Go to admin/performance and enable css and js aggregation.
5) Go to the JS and CSS directories just created and place a .htaccess inside each with the directive "allow from all"
6) Update the .htaccess in the root of your files directory with the diretive "deny from all"

Obviously all the code hacks do is tell performance and the two aggregation functions to ignore the filesystem type.

WARNING! I have no clue what will happen when you come to upgrade drupal core with this hack active, strongly suggest you turn off aggregation of js and css before upgrade and then attempt to reapply.

Hope this helps.

grendzy’s picture

I would also favor #166759: Public/Private File Handling, that would be a really nice resolution. Maybe a decision to » wontfix should be made by one of this patch's authors, though.

wretched sinner - saved by grace’s picture

Status: Needs work » Closed (won't fix)

Since noone has posted to this issue in 4 months, I think it is safe to move this to "Wont Fix" and get efforts focused on #166759: Public/Private File Handling

arhak’s picture

for those looking for workarounds for private download support:

according to issue 146611#comment-1182361 at #146611: Allow JS/CSS aggregation in private mode D7 is expected to ship with public/private by file functionality, so creating a new folder for D6 was denied

nevertheless, making a public folder available to support features unavailable for private download method is a very affordable cost

so I'll be maintaining such as a path for D6 at #181003: private download method and dynamically generated CSS and JS