There's a critical bug in the if-conditions of boost_exit(), which in some cases causes it to skip checks whether a page is uncacheable by status code or drupal_page_is_cacheable():

if (!isset($_boost['cache_this'])) {
    if (!isset($_boost['is_cacheable'])) {
      return;
    }
    elseif (!$_boost['is_cacheable']) {
      return;
    }
    //if both of these inner conditions were false, we do not exit here
    //but the following conditions are not evaluated any more
  }
  elseif ($_boost['cache_this'] == FALSE) {
    return;
  }
  elseif (!$_boost['is_cacheable']) { //this elseif has to become an if!
    return;
  }
  elseif ($_boost['menu_item']['status'] != 200) {
    return;
  }
  elseif (!drupal_page_is_cacheable()) {
    $_boost['is_cacheable'] = FALSE;
    return;
  }
CommentFileSizeAuthor
#1 boost-1677512.patch344 bytesbgm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bgm’s picture

bgm CreditAttribution: bgm commented
Status: Active » Fixed
FileSize
boost-1677512.patch344 bytes

Good catch. Although I'd be curious to see in which situations this can happen, it's safer to split that 'if'.

ralf.strobel’s picture

ralf.strobel CreditAttribution: ralf.strobel commented

Maybe you actually want to check the logic of the upper ifs again and see if you can't simplify them in the first place. Some of them seam to repeat further below. You can probably use empty(x) instead of !isset(x) + !x.

And since all of these statements fire a return if true, I would not use else at all.
I don't know the exact usage of the $_boost values, so this is only a suggestion:

  if (empty($_boost['is_cacheable'])) {
      return;
  }
  if (empty($_boost['cache_this'])) {
      return;
  }
  if ($_boost['menu_item']['status'] != 200) {
      return;
  }
  if (!drupal_page_is_cacheable()) {
    //$_boost['is_cacheable'] = FALSE; //is this ever evaluated after we return?
    return;
  }

As for the situation in which it can happen: Boost was caching 404 pages for me, although it shouldn't.

bgm’s picture

bgm CreditAttribution: bgm commented
Status: Fixed » Needs review

Makes sense, but I don't have much time to test.
Can you test and let us know how it works?

ralf.strobel’s picture

ralf.strobel CreditAttribution: ralf.strobel commented

I've been using my own heavily modified version of boost for a while, so I don't assume my tests would guarantee functionality for everyone.

ralf.strobel’s picture

ralf.strobel CreditAttribution: ralf.strobel commented

Well, here is a version that I tested (on my boost branch). There was in fact something that didn't work with the "cache_this" value. Now it seems pretty straightforward and safe, but still, like I said, no guarantees.

  if (isset($_boost['cache_this']) && !$_boost['cache_this']) {
      return;
  }
  if (empty($_boost['is_cacheable'])) {
      return;
  }
  if ($_boost['menu_item']['status'] != 200) {
      return;
  }
  if (!drupal_page_is_cacheable()) {
      return;
  }