Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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;
}
Comment | File | Size | Author |
---|---|---|---|
#1 | boost-1677512.patch | 344 bytes | bgm |
Comments
Comment #1
bgm CreditAttribution: bgm commentedGood catch. Although I'd be curious to see in which situations this can happen, it's safer to split that 'if'.
Comment #2
ralf.strobel CreditAttribution: ralf.strobel commentedMaybe 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:
As for the situation in which it can happen: Boost was caching 404 pages for me, although it shouldn't.
Comment #4
bgm CreditAttribution: bgm commentedMakes sense, but I don't have much time to test.
Can you test and let us know how it works?
Comment #5
ralf.strobel CreditAttribution: ralf.strobel commentedI'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.
Comment #6
ralf.strobel CreditAttribution: ralf.strobel commentedWell, 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.