By arhak on

Short reasons for why I'm posting this in the forums instead of issues:
- I'm not a Drupal developer (means I don't have Drupal in CVS, thus I make diff manually)
- This was quickly written to make a proof and it worked, but the code standard is currently inadmissible (needs design)
- I made this for Drupal 6.3 and won't check it against Drupal 6.4 before posting it since it would need work anyway. Although I'm certain it should work the same.
- The discussion on D7 still goes (#181003) BUT they are far from getting things done (don't even talk about other issues like JavaScript translation)
Though, as there are some related issues here goes this. (see also #181003: private download method and dynamically generated CSS and JS)
Note: the whole post will be partitioned to make it more readable/followable.

Enabling css/js aggregation, color module and/or JavaScript translations for the private download method

I'm going to post a group of three related patches to solve three different (but related) issues with the same approach.
The common denominator is the private download method and the restrictions it imposes.

1-) The most important issue (of these three) for anyone concerned about performance will be the css/js aggregation (admin/settings/performance) which is disabled once private download method is chosen.

2-) The most perceptible (although prescindible for non-newbies) is the color module neutralization for those themes which supports it (admin/build/themes/settings/garland). If private download method is chosen the easy fanciness is one of the prices to pay.

3-) Last but not least, is the JavaScripts translations issues. If the private directory is out of the web server's reach (as it has to be for being actually private without relaying on .htaccess) then the JavaScript's translations will be done but will get 404 Not Found when pages are loaded. Resulting in the neutralization of the JavaScript's translations.

Of course, there is a workaround (without patch) for issues #2 & #3. Which consist on having a private directory (chosing private download method) under the web server's reach, protected with "Order allow,deny" and a <FilesMatch "\.(css|js)$"> with an Allow from all (as this is beside the point I'm just naming it, contact me for more info on this workaround)

These three patches have a lot in common, but as they can be separately issued/applied/tested I'll will submit it like three independent patches and also as whole patch.

The three of them relay on 6 minor modifications in common. Beyond that, the biggest one only adds 4 minor modifications. Every modification is related with an extended optional parameter to take into account.

The three cases are independent and as such can be applied one at the time, the three at the same time or whatever combination of two of them.

Also note that using js aggregation with patch for case #2 will incidentally do the trick for case #3. Even though disabling js aggregation will always require the third patch.

The core of these three patches is the same: enabling a public folder for those issues unrelated with the private download method.

The way to go is applying a common patch to the file.inc file (it will NOT brake)

Nevertheless these are only working patches which certainly will need to be rethought to accommodate the same functionality in a cleaner/standard way (see #181003: private download method and dynamically generated CSS and JS).

@TODO: deciding whether the required directory must be part of Drupal distribution or programmatically created to kept it configurable via a variable (as of now it must be created manually when applying the patch)

P.S: Sorry, but I don't work with Drupal in CVS, I tried to do the best I could to provide these patches with the diff tool (diff -U 0 -p newfile oldfile > filename.patch), to apply them I use patch -p 0 -u -i filename.patch being in the Drupal's targeted directory.

⋅ Categories: Drupal 6.x

Comments

arhak’s picture

arhak commented

The common patch for file.inc

--- /drupal6.3/includes/file.inc	Wed Apr 23 14:18:10 2008
+++ includes/file.inc	Fri Aug 15 18:46:52 2008
@@ -64,2 +64,2 @@ function file_create_url($path) {
-function file_create_path($dest = 0) {
-  $file_path = file_directory_path();
+function file_create_path($dest = 0, $ensure_public = FALSE) {
+  $file_path = file_directory_path($ensure_public);
@@ -216,2 +216,2 @@ function file_check_location($source, $d
-function file_copy(&$source, $dest = 0, $replace = FILE_EXISTS_RENAME) {
-  $dest = file_create_path($dest);
+function file_copy(&$source, $dest = 0, $replace = FILE_EXISTS_RENAME, $ensure_public = FALSE) {
+  $dest = file_create_path($dest, $ensure_public);
@@ -330 +330 @@ function file_destination($destination,
-function file_move(&$source, $dest = 0, $replace = FILE_EXISTS_RENAME) {
+function file_move(&$source, $dest = 0, $replace = FILE_EXISTS_RENAME, $ensure_public = FALSE) {
@@ -333 +333 @@ function file_move(&$source, $dest = 0,
-  if (file_copy($source, $dest, $replace)) {
+  if (file_copy($source, $dest, $replace, $ensure_public)) {
@@ -753 +753 @@ function file_validate_image_resolution(
-function file_save_data($data, $dest, $replace = FILE_EXISTS_RENAME) {
+function file_save_data($data, $dest, $replace = FILE_EXISTS_RENAME, $ensure_public = FALSE) {
@@ -764 +764 @@ function file_save_data($data, $dest, $r
-  if (!file_move($file, $dest, $replace)) {
+  if (!file_move($file, $dest, $replace, $ensure_public)) {
@@ -961 +961,4 @@ function file_directory_temp() {
-function file_directory_path() {
+function file_directory_path($ensure_public = FALSE) {
+  if ($ensure_public && variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC) != FILE_DOWNLOADS_PUBLIC) {
+    return variable_get('file_public_directory_path', 'misc/dynamic');
+  }

Note that the diff says changes are in some functions they weren't whenever the signatures were changed

What happened until here:
1- The manually creation of a directory for handling those public files only in case private download method is chosen (otherwise everything will remain the same).
2- The alternative is taken into account when returning the file_directory_path @@ -961 +961,5
3- The same goes for file_create_path @@ -64,2 +64,2
4- Ditto for file_copy @@ -216,2 +216,2
5- Ditto for file_move @@ -330 +330, @@ -333 +333
6- Ditto for file_save_data @@ -753 +753, @@ -764 +764

Until this point everything works the same.
All the added argument are optional and by default they behave the same it used to be.

arhak’s picture

arhak commented

The patch for case #1

NOTE: For any of these patches to work a directory misc/dynamic must be manually created AND the above file.inc patch is required.

--- /drupal6.3/includes/common.inc	Wed Jul 09 17:48:28 2008
+++ includes/common.inc	Fri Aug 15 12:38:44 2008
@@ -1691,2 +1691,2 @@ function drupal_get_css($css = NULL) {
-  $directory = file_directory_path();
-  $is_writable = is_dir($directory) && is_writable($directory) && (variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC) == FILE_DOWNLOADS_PUBLIC);
+  $directory = file_directory_path(TRUE);
+  $is_writable = is_dir($directory) && is_writable($directory);
@@ -1765 +1765 @@ function drupal_build_css_cache($types,
-  $csspath = file_create_path('css');
+  $csspath = file_create_path('css', TRUE);
@@ -1791 +1791 @@ function drupal_build_css_cache($types,
-    file_save_data($data, $csspath .'/'. $filename, FILE_EXISTS_REPLACE);
+    file_save_data($data, $csspath .'/'. $filename, FILE_EXISTS_REPLACE, TRUE);
@@ -2043,2 +2043,2 @@ function drupal_get_js($scope = 'header'
-  $directory = file_directory_path();
-  $is_writable = is_dir($directory) && is_writable($directory) && (variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC) == FILE_DOWNLOADS_PUBLIC);
+  $directory = file_directory_path(TRUE);
+  $is_writable = is_dir($directory) && is_writable($directory);
@@ -2237 +2237 @@ function drupal_build_js_cache($files, $
-  $jspath = file_create_path('js');
+  $jspath = file_create_path('js', TRUE);
@@ -2250 +2250 @@ function drupal_build_js_cache($files, $
-    file_save_data($contents, $jspath .'/'. $filename, FILE_EXISTS_REPLACE);
+    file_save_data($contents, $jspath .'/'. $filename, FILE_EXISTS_REPLACE, TRUE);

--- /drupal6.3/modules/system/system.admin.inc	Mon May 19 03:27:36 2008
+++ modules/system/system.admin.inc	Fri Aug 15 08:15:36 2008
@@ -1318,2 +1318,2 @@ function system_performance_settings() {
-  $directory = file_directory_path();
-  $is_writable = is_dir($directory) && is_writable($directory) && (variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC) == FILE_DOWNLOADS_PUBLIC);
+  $directory = file_directory_path(0, TRUE);
+  $is_writable = is_dir($directory) && is_writable($directory);

What happened until here:
1 trough 6- The above file.inc's patch
7- The alternative is taken into account when invoking the drupal_build_css_cache @@ -1765 +1765, @@ -1791 +1791
8- The same goes for drupal_build_js_cache @@ -2237 +2237, @@ -2250 +2250
9- Ditto for drupal_get_css @@ -1691,2 +1691,2, @@ -2043,2 +2043,2
10- Enabling css/js aggregation regardless of the current download method system_performance_settings @@ -1318,2 +1318,2
11- Optionally change the description (see below) to point out the current possible causes for these options to be disabled (private download method will be no longer a possible cause)

--- /drupal6.3/modules/system/system.admin.inc	Fri Aug 15 08:15:36 2008
+++ modules/system/system.admin.inc	Fri Aug 15 08:17:36 2008
@@ -1315 +1315 @@ function system_performance_settings() {
-    '#description' => t('<p>Drupal can automatically optimize external resources like CSS and JavaScript, which can reduce both the size and number of requests made to your website. CSS files can be aggregated and compressed into a single file, while JavaScript files are aggregated (but not compressed). These optional optimizations may reduce server load, bandwidth requirements, and page loading times.</p><p>These options are disabled if you have not set up your files directory, or if your download method is set to private.</p>')
+    '#description' => t('<p>Drupal can automatically optimize external resources like CSS and JavaScript, which can reduce both the size and number of requests made to your website. CSS files can be aggregated and compressed into a single file, while JavaScript files are aggregated (but not compressed). These optional optimizations may reduce server load, bandwidth requirements, and page loading times.</p><p>These options are disabled if you have not set up your files directory, or if your download method is set to private and directory \'misc/dynamic\' does not exists or isn\'t writable.</p>')

Note this single one might look big, but it's only changing the explanation about controls being disabled (the last sentence). On the other hand, the new explanation refers to 'misc/dynamic' directory and it is possible to change this by means of variable file_public_directory_path as considered in the above file.inc @@ -961 +961,5

arhak’s picture

arhak commented

The patch for case #2

NOTE: For any of these patches to work a directory misc/dynamic must be manually created AND the above file.inc patch is required.

--- /drupal6.3/modules/color/color.module	Wed Jan 23 05:43:26 2008
+++ modules/color/color.module	Fri Aug 15 17:10:06 2008
@@ -35,6 +34,0 @@ function color_form_alter(&$form, $form_
-    if (variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC) != FILE_DOWNLOADS_PUBLIC) {
-      // Disables the color changer when the private download method is used.
-      // TODO: This should be solved in a different way. See issue #181003.
-      drupal_set_message(t('The color picker only works if the <a href="@url">download method</a> is set to public.', array('@url' => url('admin/settings/file-system'))), 'warning');
-    }
-    else {
@@ -50 +43,0 @@ function color_form_alter(&$form, $form_
-    }
@@ -297 +290 @@ function color_scheme_form_submit($form,
-  $paths['color'] = file_directory_path() .'/color';
+  $paths['color'] = file_directory_path(TRUE).'/color';
@@ -315 +308 @@ function color_scheme_form_submit($form,
-    file_copy($source, $paths['target'] . $base);
+    file_copy($source, $paths['target'] . $base, FILE_EXISTS_REPLACE, TRUE);
@@ -445 +438 @@ function _color_save_stylesheet($file, $
-  file_save_data($style, $file, FILE_EXISTS_REPLACE);
+  file_save_data($style, $file, FILE_EXISTS_REPLACE, TRUE);

What happened until here:
1 trough 6- The above file.inc's patch
7- Enabling color support regardless of the current download method, by means of removing the whole if block with watchdog and comment to issue #181003: private download method and dynamically generated CSS and JS along function color_form_alter @@ -35,6 +34,0, @@ -50 +43,0
8- The alternative is taken into account when invoking the function color_scheme_form_submit @@ -297 +290, @@ -315 +308
9- The same goes for _color_save_stylesheet @@ -445 +438

Note: the statements that were in the else block (resulting lines 35 through 43) weren't properly re-indented to keep the diff as readable as possible

arhak’s picture

arhak commented

The patch for case #3

NOTE: For any of these patches to work a directory misc/dynamic must be manually created AND the above file.inc patch is required.

--- /drupal6.3/modules/locale/locale.module	Wed Jul 09 17:48:28 2008
+++ modules/locale/locale.module	Fri Aug 15 09:56:34 2008
@@ -501 +501 @@ function locale_update_js_files() {
-  $dir = file_create_path(variable_get('locale_js_directory', 'languages'));
+  $dir = file_create_path(variable_get('locale_js_directory', 'languages'), TRUE);

--- /drupal6.3/includes/locale.inc	Wed Jul 09 17:48:28 2008
+++ includes/locale.inc	Fri Aug 15 11:58:02 2008
@@ -2137 +2137 @@ function _locale_rebuild_js($langcode =
-  $dir = file_create_path(variable_get('locale_js_directory', 'languages'));
+  $dir = file_create_path(variable_get('locale_js_directory', 'languages'), TRUE);
@@ -2153 +2153 @@ function _locale_rebuild_js($langcode =
-    if (file_save_data($data, $dest)) {
+    if (file_save_data($data, $dest, FILE_EXISTS_REPLACE, TRUE)) {

What happened until here:
1 trough 6- The above file.inc's patch
7- The alternative is taken into account when invoking the locale_update_js_files @@ -501 +501
8- The same goes for _locale_rebuild_js @@ -2137 +2137, @@ -2153 +2153

arhak’s picture

arhak commented

Note: consider that invocations to file_save_data with the third argument omitted is equivalent to passing FILE_EXISTS_RENAME which I found without sense in the cases in question and that's why I use FILE_EXISTS_RENAME instead whenever I need to use the forth argument (wasn't because the fun of it). Nevertheless, fell free to change it back to FILE_EXISTS_RENAME at your will

END OF THE THREE SEPARATED CASES - BELOW IS THE SAME THREE CASES AS WHOLE PATH

arhak’s picture

arhak commented
The three patches as a whole (for cases #1, #2 & #3)

NOTE: For this whole patch to work a directory misc/dynamic must be manually created.

--- /drupal6.3/includes/file.inc	Wed Apr 23 14:18:10 2008
+++ includes/file.inc	Fri Aug 15 18:46:52 2008
@@ -64,2 +64,2 @@ function file_create_url($path) {
-function file_create_path($dest = 0) {
-  $file_path = file_directory_path();
+function file_create_path($dest = 0, $ensure_public = FALSE) {
+  $file_path = file_directory_path($ensure_public);
@@ -216,2 +216,2 @@ function file_check_location($source, $d
-function file_copy(&$source, $dest = 0, $replace = FILE_EXISTS_RENAME) {
-  $dest = file_create_path($dest);
+function file_copy(&$source, $dest = 0, $replace = FILE_EXISTS_RENAME, $ensure_public = FALSE) {
+  $dest = file_create_path($dest, $ensure_public);
@@ -330 +330 @@ function file_destination($destination,
-function file_move(&$source, $dest = 0, $replace = FILE_EXISTS_RENAME) {
+function file_move(&$source, $dest = 0, $replace = FILE_EXISTS_RENAME, $ensure_public = FALSE) {
@@ -333 +333 @@ function file_move(&$source, $dest = 0,
-  if (file_copy($source, $dest, $replace)) {
+  if (file_copy($source, $dest, $replace, $ensure_public)) {
@@ -753 +753 @@ function file_validate_image_resolution(
-function file_save_data($data, $dest, $replace = FILE_EXISTS_RENAME) {
+function file_save_data($data, $dest, $replace = FILE_EXISTS_RENAME, $ensure_public = FALSE) {
@@ -764 +764 @@ function file_save_data($data, $dest, $r
-  if (!file_move($file, $dest, $replace)) {
+  if (!file_move($file, $dest, $replace, $ensure_public)) {
@@ -961 +961,4 @@ function file_directory_temp() {
-function file_directory_path() {
+function file_directory_path($ensure_public = FALSE) {
+  if ($ensure_public && variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC) != FILE_DOWNLOADS_PUBLIC) {
+    return variable_get('file_public_directory_path', 'misc/dynamic');
+  }
--- /drupal6.3/includes/common.inc	Wed Jul 09 17:48:28 2008
+++ includes/common.inc	Fri Aug 15 12:38:44 2008
@@ -1691,2 +1691,2 @@ function drupal_get_css($css = NULL) {
-  $directory = file_directory_path();
-  $is_writable = is_dir($directory) && is_writable($directory) && (variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC) == FILE_DOWNLOADS_PUBLIC);
+  $directory = file_directory_path(TRUE);
+  $is_writable = is_dir($directory) && is_writable($directory);
@@ -1765 +1765 @@ function drupal_build_css_cache($types,
-  $csspath = file_create_path('css');
+  $csspath = file_create_path('css', TRUE);
@@ -1791 +1791 @@ function drupal_build_css_cache($types,
-    file_save_data($data, $csspath .'/'. $filename, FILE_EXISTS_REPLACE);
+    file_save_data($data, $csspath .'/'. $filename, FILE_EXISTS_REPLACE, TRUE);
@@ -2043,2 +2043,2 @@ function drupal_get_js($scope = 'header'
-  $directory = file_directory_path();
-  $is_writable = is_dir($directory) && is_writable($directory) && (variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC) == FILE_DOWNLOADS_PUBLIC);
+  $directory = file_directory_path(TRUE);
+  $is_writable = is_dir($directory) && is_writable($directory);
@@ -2237 +2237 @@ function drupal_build_js_cache($files, $
-  $jspath = file_create_path('js');
+  $jspath = file_create_path('js', TRUE);
@@ -2250 +2250 @@ function drupal_build_js_cache($files, $
-    file_save_data($contents, $jspath .'/'. $filename, FILE_EXISTS_REPLACE);
+    file_save_data($contents, $jspath .'/'. $filename, FILE_EXISTS_REPLACE, TRUE);
--- /drupal6.3/includes/locale.inc	Wed Jul 09 17:48:28 2008
+++ includes/locale.inc	Fri Aug 15 11:58:02 2008
@@ -2137 +2137 @@ function _locale_rebuild_js($langcode =
-  $dir = file_create_path(variable_get('locale_js_directory', 'languages'));
+  $dir = file_create_path(variable_get('locale_js_directory', 'languages'), TRUE);
@@ -2153 +2153 @@ function _locale_rebuild_js($langcode =
-    if (file_save_data($data, $dest)) {
+    if (file_save_data($data, $dest, FILE_EXISTS_REPLACE, TRUE)) {
--- /drupal6.3/modules/system/system.admin.inc	Mon May 19 03:27:36 2008
+++ modules/system/system.admin.inc	Fri Aug 15 08:17:36 2008
@@ -1315 +1315 @@ function system_performance_settings() {
-    '#description' => t('<p>Drupal can automatically optimize external resources like CSS and JavaScript, which can reduce both the size and number of requests made to your website. CSS files can be aggregated and compressed into a single file, while JavaScript files are aggregated (but not compressed). These optional optimizations may reduce server load, bandwidth requirements, and page loading times.</p><p>These options are disabled if you have not set up your files directory, or if your download method is set to private.</p>')
+    '#description' => t('<p>Drupal can automatically optimize external resources like CSS and JavaScript, which can reduce both the size and number of requests made to your website. CSS files can be aggregated and compressed into a single file, while JavaScript files are aggregated (but not compressed). These optional optimizations may reduce server load, bandwidth requirements, and page loading times.</p><p>These options are disabled if you have not set up your files directory, or if your download method is set to private and directory \'misc/dynamic\' does not exists or isn\'t writable.</p>')
@@ -1318,2 +1318,2 @@ function system_performance_settings() {
-  $directory = file_directory_path();
-  $is_writable = is_dir($directory) && is_writable($directory) && (variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC) == FILE_DOWNLOADS_PUBLIC);
+  $directory = file_directory_path(0, TRUE);
+  $is_writable = is_dir($directory) && is_writable($directory);
--- /drupal6.3/modules/color/color.module	Wed Jan 23 05:43:26 2008
+++ modules/color/color.module	Fri Aug 15 17:10:06 2008
@@ -35,6 +34,0 @@ function color_form_alter(&$form, $form_
-    if (variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC) != FILE_DOWNLOADS_PUBLIC) {
-      // Disables the color changer when the private download method is used.
-      // TODO: This should be solved in a different way. See issue #181003.
-      drupal_set_message(t('The color picker only works if the <a href="@url">download method</a> is set to public.', array('@url' => url('admin/settings/file-system'))), 'warning');
-    }
-    else {
@@ -50 +43,0 @@ function color_form_alter(&$form, $form_
-    }
@@ -297 +290 @@ function color_scheme_form_submit($form,
-  $paths['color'] = file_directory_path() .'/color';
+  $paths['color'] = file_directory_path(TRUE).'/color';
@@ -315 +308 @@ function color_scheme_form_submit($form,
-    file_copy($source, $paths['target'] . $base);
+    file_copy($source, $paths['target'] . $base, FILE_EXISTS_REPLACE, TRUE);
@@ -445 +438 @@ function _color_save_stylesheet($file, $
-  file_save_data($style, $file, FILE_EXISTS_REPLACE);
+  file_save_data($style, $file, FILE_EXISTS_REPLACE, TRUE);
--- /drupal6.3/modules/locale/locale.module	Wed Jul 09 17:48:28 2008
+++ modules/locale/locale.module	Fri Aug 15 09:56:34 2008
@@ -501 +501 @@ function locale_update_js_files() {
-  $dir = file_create_path(variable_get('locale_js_directory', 'languages'));
+  $dir = file_create_path(variable_get('locale_js_directory', 'languages'), TRUE);
arhak’s picture

arhak commented

new patch can be found at 181003#comment-1972764

arhak’s picture

arhak commented

#572516: make private download method support css/js aggregation, color module and js translations