Fallback for CSS/JS aggregation for non-writable directories

Depending on the underlying file storage, this is_writable can be awfully expensive.

Requalifying as a critical bug.

justification and code look good to me.

Committed to CVS HEAD. Thanks.

subscribe; will be testing for 6.x sooner rather then later. We have the css dir directly mapped to S3 so when S3 went down today, bad things happened.
http://aws.amazon.com/s3/#protecting
http://twitter.com/search?q=amazon%20s3
Fun.

Related contrib issue http://drupal.org/node/652690#comment-3039006 since #228818: IE: Stylesheets ignored after 31 link/style tags is fixed in 7.x

Above patch fails to output css if you manually set drupal_build_css_cache() to return FALSE. This patch makes the css work in case this edge case happens.

      if ($preprocess_file) {
        $output .= '<link type="text/css" rel="stylesheet" media="'. $media .'" href="'. base_path() . $preprocess_file .'" />'."\n";
      }
      else {
        // Redo with preprocess_css turned off and return the new value.
        $org_preprocess_css = $conf['preprocess_css'];
        $conf['preprocess_css'] = FALSE;
        $data = drupal_get_css();
        $conf['preprocess_css'] = $org_preprocess_css;
        return $data;
      }

Added a check for css/js filesize of zero; throw a watchdog error when the fallback codepath is run.

subscribing

If the filesize ends up zero (which it can happen if using something like FUSE) then file_save_data will not throw an error since the file was saved with a filesize of zero. It happens (filesize zero) about once a day for me, so I find it very useful. There are some things that need to go upstream from my 6.x patch; the biggest one is the fallback code path #7. The filesize zero might be happen due to a race condition
#818818: Race Condition when using file_save_data FILE_EXISTS_REPLACE, still investigating.

I'm not sure of the 7.x function I need to modify, but I think its drupal_aggregate_css(). Patch for 7.x including the changes I did to the 6.x patch.

subscribing

Split patch: #828268: Prevent serving JS/CSS files if they have a filesize of zero
Reason filesize ended up in here was this is where the return FALSE code patch came from, which the filesize patch uses.

What do you think about adding in a lock to prevent multiple attempts at creating the same file? It would go in a separate issue.

I think the code is sound, I only rerolled to fix a typo in comments (sense should be since) and I retitled the issue because the is_writable fix has been committed since and so it was kinda confusing as the patch did not touch is_writable.

I'm setting the priority to 'normal' -- this seems like an edge case to me.

+++ includes/common.inc	2010-06-18 22:26:15 +0000
@@ -2893,16 +2893,28 @@ function drupal_group_css($css) {
+  $redo = FALSE;

$redo is an odd variable name. I don't understand why it is called $redo.

I don't think the code, with the handling of $conf, is particularly elegant.

Sorry about the critical; never changed the status once I found a hole in the logic and ported the 6.x code back up to 7.x.

I agree $conf is not that elegant; the other option would be to pass a variable so preprocess_css & preprocess_js can be disabled for the second call of the function. The more complex option is to redo the logic inside drupal_aggregate_css & drupal_get_js so it makes a copy of $processed or $css_groups. So if the writing of the aggregated file fails it can revert back to the unmodified version and not serve the page with a missing css/js file.

Should we stick with $conf or should I rewrite these functions. Rewrite so they are non destructive on the variable that holds the uri & thus this function doesn't need to be run twice; the logic will be slightly more complex most likely. What path should we take?

Reworked this. The way css is aggregated in comparison to the way js is aggregated is different. Taking these differences into account I was able to reduce the ugly in comparison to the previous patches. CSS is elegant while JS is a little ugly still. Fixing the ugly JS processing would probably be in a different issue.
#330082: Refactor drupal_get_js() to use render layer similar to drupal_get_css()
#595572: Clean up CSS and JS processing

Tested this code by having drupal_build_css_cache return FALSE and by having drupal_build_js_cache return FALSE. Without the patch, css and/or js is missing. With the patch they show up in a non aggregated form when aggregation fails.

EDIT: need to fix some comments in the patch; other then this, how does it look?

#21: drupal-755586-20.patch queued for re-testing.

+++ includes/common.inc	29 Jun 2010 23:21:36 -0000
@@ -3831,6 +3836,18 @@ function drupal_get_js($scope = 'header'
+      // If aggregation failed then add include each js file
+      else {
+        unset($processed[$key]);
+        foreach ($file_set as $item) {
+          if ($item['defer']) {
+            $js_element['#attributes']['defer'] = 'defer';
+          }
+          $query_string_separator = (strpos($item['data'], '?') !== FALSE) ? '&' : '?';
+          $js_element['#attributes']['src'] = file_create_url($item['data']) . $query_string_separator . ($item['cache'] ? $query_string : REQUEST_TIME);
+          $processed[$index++] = theme('html_tag', array('element' => $js_element));
+        }
+      }

It looks like this is duplicating lot of (much more complex) code. We need to intercept this special condition earlier in the process.

Powered by Dreditor.

Here's a different way of doing the JS part.

It looks like we could test this by attempting to write to a non-writeable directory?

Patch looks fine in principle but I agree it would be good to have tests here.

I also disagree this is even major, non-writeable directories are misconfiguration, and this does not have severe side-effects (i.e. it just doesn't work, then it starts working when the directories become writeable again).

Full drive, NFS issues, S3 issues. Reason why it was set to major. We are off of S3 so in my case it is no longer a major issue.

patch no longer applies.

#27: drupal-755586-27.patch queued for re-testing.

Tagging.

tags did not stick. trying again.

Rewritten patch for D8. Could need tests!

looks like that patch applies to 8.x, so removing needs reroll tag.

#755586-28: Fallback for CSS/JS aggregation for non-writable directories : It looks like we could test this by attempting to write to a non-writeable directory?

Yes, but I don't see how you can do that in a test. If you can change the directory permissions in the test then the call to file_prepare_directory() in drupal_build_js_cache and drupal_build_css_cache() will be able to change the permissions back to writable. You would have to change the owner of the directory to really test this out.

Anyone got an idea on how to write a test for this?

Novice issue cleanup.

Unsure if this is still an issue, but the patch above definitely no longer applies.

Rerolled the patch

Just a heads up that AdvAgg has this built in if someone is looking for a 7.x solution.

Unassigning for now. Bumping to 8.1 too.

Note that #1014086: Stampedes and cold cache performance issues with css/js aggregation removes all i/o calls from the main request.

Also #2506369: Cache CSS/JS asset resolving means that i/o is not every single request in 8.x.

Given those two issues, I'd be tempted to move this issue back to 7.x. Both agrcache and advagg cover this from contrib.

Even better, thanks @catch!