I developed additional lock backends, some of them work using an OOP API behind the scenes, and do not use the global $locks. Nothing in documentation says you can rely on this global, it only exists in the lock.inc file that can be replaced or dropped.
Your code will trigger the ctools_css_flush_caches() when running cron using such backend.
You have to find another way to check wether or not you are in cron context. You must attempt to get the cron lock instead, and if it does fail then you can skip your cache flush:
function ctools_flush_caches() {
// Do not actually flush caches if running on cron. Drupal uses this hook
// in an inconsistent fashion and it does not necessarily mean to *flush*
// caches when running from cron. Instead it's just getting a list of cache
// tables and may not do any flushing.
if (!lock_acquire('cron')) {
return;
}
lock_release('cron');
ctools_include('css');
ctools_css_flush_caches();
}
Or you can attempt a lock_may_be_available() function to check if lock is available or not, but I wouldn't advise you to use it for two reasons:
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | interdiff.txt | 477 bytes | pfrenssen |
| #13 | 1313368-13-ctools-css_cache.patch | 4.61 KB | pfrenssen |
Comments
Comment #1
merlinofchaos commentedI tried that. It doesn't work.
The lock is per page load; if the page already has the lock, it succeeds.
Lock API does not give me a way to determine lock status.
Without this code, things get flushed that MUST NOT be flushed.
If your lock backend is willing to just set the global variable so that I can look at it, that would be great. If you would prefer not to, I need an alternative solution. Simply trying toa cquire the lock does not work; we already have the lock.
Comment #2
pounardIt would require a deep design change, and other could potentially do the same. Further, your method remains non reliable if I fix my module.
Comment #3
merlinofchaos commentedA unreliable method is better than my alternative which is a non-functioning method.
Comment #4
pounardYou can also check some other stuff that can help you:
* drupal_save_session(), explictely set to FALSE by cron (but can be in some other use cases).
* $queues = module_invoke_all('cron_queue_info'); If you implement this hook, you know if cron if running.
* lock_may_be_available() does not check the page id.
* You could also skip your own cache clear when user is anonymous.
* module_invoke_all('cron'); By implementing this, you also can guess if the current page ran the cron or not (why didn't I see that in the first place?).
* You can also check for the emptyness of variable_del('drupal_css_cache_files'); which is explicitely cleared during drupal_flush_all_caches().
Just pick anyone.
Comment #5
pounardYou can also implement the ctools_css_cache as a modified version of system cache table, declare it as a bin using the hook_flush_caches(), then it would be emptied as any other cache table. You can finish by checking for outdated files in real hook_cron() and remove them at this point. This solution's only problem is that unused files would stall while hook_cron() is not being called.
Comment #6
pounard@merlin does this feature is unit tested? If you have an easy way to test it, I will attempt some patches.
Comment #7
merlinofchaos commentedctools_css_cache should not be cleared except to remove old entries and is not the problem.
The problem is the removal of files stored in the files directory. The problem is, IMO, a misuse of hook_flush_caches() which simply gets a list of caches rather than actually flushing caches at the hook would lead you to believe.
It's possible we could use hook_cron() to set a semaphore of our own.
While there are tests for the object cache, there aren't tests for this particular circumstance where I was simply shotgunning to prevent what is a seriously major problem: .css files being removed when cached HTML that relies on them was not. That's totally awesome. :)
It shouldn't be too hard to test this circumstance.
Remove the lock code entirely and you'll be presented with these known
results:
Test #1:
Test #2
A solution that works correctly will do this:
Test #1:
Test #2
Comment #8
pounardOk nice, I keep this in my mind for later don't have time right now, but I'll do it if not anyone do it before I get back here.
Comment #9
Ashlar commentedComment #10
pfrenssenWhy has this been closed with "Won't fix"? This is an actual bug in CTools. This can be solved by providing our own implementation of DrupalCacheInterface. Reopening.
Comment #11
pfrenssenHere's a patch. It solves the problem by implementing DrupalCacheInterface, which has been added to Drupal 7 to solve these kind of problems.
I also added a test that ensures that merlinofchaos' expectations from #7 work as intended: the cache is not cleared on cron, but it is when the caches are cleared manually.
I was hitting this problem after enabling Elysia Cron on a website. That module (mistakenly?) clears the cron lock before starting its cron jobs. ctools_flush_caches() would not recognize it was actually called during a cron run and happily clears the CSS cache, causing incompletely styled pages to appear. Indeed as mentioned above hook_flush_caches() should only return its cache bins, not do any actual clearing.
Comment #12
pfrenssenFound a bug in my patch. If you would flush the cache twice before performing the database updates, drupal_flush_all_caches() would try to truncate the non-existing table 'cache_ctools_css', throwing a fatal error. In my case this occurred in a test that was running a large number of update hooks, some of which containing calls to drupal_flush_all_caches(). This might also occur if a site administrator would forget to perform the database updates.
Updated the patch to only return the 'ctools_css' cache bin if the cache handler was successfully activated.
Comment #13
pfrenssenCoding standards fix. Forgot a line of whitespace.
Comment #14
kristofdg commentedPatch seems to work fine. CSS directory doesn't get removed on cron run anymore but does get removed on a drush cc all.
Comment #15
japerryIt even comes with a test and it passes! This is pretty big for those needing to run cron often. Let it simmer while I test it on some active sites and if others find it works I'll push it up.
Comment #16
pfrenssenThanks! We're also using it in production since last week. It's now live on a dozen or so websites. You're welcome for the test, our project requires full test coverage and this also extends to contributed code :)
Comment #17
isolate commentedBump. When is this going in ? :)
Comment #18
japerrySimmering has been good, and although ctools doesn't have really any test coverage to validate against I think we're okay here. Committed!