When Advagg encounters empty bundle, advagg_process_css() function completely disables aggregation, which is wrong, imo. My site theme has non-existant print.css just to disable parent theme css with the same name (a common practice). Its also the only one file in the print group, which leads to advagg_css_js_file_builder() return array of the form array(0 => FALSE) and advagg_process_css() completely disables aggregation when it processes it.
Proper behavior would be just to skip empty bundles completely. Whether to report empty bundle as error is debatable, but my opinion we are doing nothing criminal here, so we don't need this case in watchdog at all. As I said, referencing non-existant files has been an acceptable practice in theming.
Maybe it's just a problem in advagg_css_js_file_builder() function that it returns array(0 => FALSE) - i'm not sure of the best way to solve the problem so I leave it to you :)

CommentFileSizeAuthor
#2 advagg-1192824-2.patch1001 bytesmikeytown2

Comments

crea’s picture

Title: Bad handling of non-existant files files: empty css bundle completely disables aggregation » Bad handling of non-existant files: empty css bundle completely disables aggregation
mikeytown2’s picture

Status: Active » Needs review
StatusFileSize
new1001 bytes

Issue arises from the possibility that the files directory is in a read only state and writes are not working. Yes this is coding for a real life example we had when dealing with the files dir being an S3 mounted file system. The assumption is if the aggregate can not be created, or if a aggregate with the filesize of zero is created then S3 failed in this instance. We've since switched the files dir to NFS and we still have write issues every now and then so having this fallback does come in handy.

Your use case if perfectly valid; remove core's css files that get automatically added; the "print" media being a sticking point as there in only one css file a lot of times.

In your case this is how this is triggered in the code.
advagg_process_css() hits the $media = "print" loop and there is only one file in it; a file that doesn't exist after theme override code runs.

  foreach ($css as $media => $types) {
...

advagg_css_js_file_builder() is called to build an aggregate for a file that doesn't exist. In your case, I'm guessing that you have async mode turned off; if it's on please let me know as this could change the potential solution. We then run into this bit of code

      if (empty($data) && !$force) {
        if (function_exists('lock_release')) {
          lock_release($lock_name);
        }
        continue;
      }

$data will be empty because the file doesn't exist; we then continue the loop, but there is only one file so the loop exits. It then runs into this code

  if (empty($output)) {
    $output[] = FALSE;
    return $output;
  }

This is making an assumption that if no bundles where created to disk then the files dir is currently having issues. What needs to happen is special handling if the output key is empty ($output[]). The files dir being in read only mode should be taken care of in this code further up

      // If file save was not good then downgrade to non aggregated mode.
      if (!$good) {
        $output[$filepath] = FALSE;
        $cacheable = FALSE;
        continue;
      }

Here is a patch that I hope takes care of this issue and still allows the fallback to work correctly.

crea’s picture

In your case, I'm guessing that you have async mode turned off; if it's on please let me know as this could change the potential solution.

Yes, it was off. But I was going to try async mode, actually.

crea’s picture

The patch helped with async mode disabled. With async mode turned on I have broken jQuery tabs, though, but it could be different problem.

mikeytown2’s picture

Status: Needs review » Fixed

That is a different issue most likely. Please open a new issue for the broken jQuery tabs. Committed #2 and marking this issue as fixed.

In your new issue be sure to attach the debug output.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.