Temporary file cleanup needs some love (UnitTest included!)
drewish - November 5, 2008 - 22:00
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | system.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | drewish |
| Status: | closed |
Description
I was looking at system_cron() and noticed that the temporary file clean up code could use some loving.
I've updated it to use the DBTNG queries and removed DELETE that was basically a no-op because file_delete() removes the record. I changed the SELECT query to match the status as a bitmapped field I need to double check that this is the correct query. Tests are included but I haven't been able to run them due to some problems with SimpleTest on my machine.
| Attachment | Size |
|---|---|
| system_cron.patch | 4.26 KB |
| Testbed results | ||
|---|---|---|
| system_cron.patch | failed | Failed: Failed to install HEAD. a href=http://testing.drupal.org/pifr/file/1/system_cron.patchDetailed results/a |
#1
Got the tests working and was able to get the status matching working correctly.
#2
maybe that'll lure in some reviews ;)
#3
This looks pretty solid. All the relevant tests pass. I did remove this line:
+file_put_contents('/Users/amorton/Sites/dh/sites/default/files/drupal.log', print_r($row,1), FILE_APPEND);#4
good catch on that... also noticed that i'd accidentally rolled #329226: Store file_test.module's values in variables rather than globals in with it. so here's your patch minus #329226.
#5
Ah, much clearer now. The unit tests still look good.
#6
The last submitted patch failed testing.
#7
Weird, the bot tested the wrong patch. #4 is the latest.
#8
The last submitted patch failed testing.
#9
here's a re-roll.
#10
In the code comments,
that'sshould bethat is, not?Do these binary operations work on PostgreSQL and SQLite?
#11
It's a standard ANSI SQL operator so it should work. installed sqlite and started to run all the system test but sweet jesus is it slow so just ran the cron tests and everything looked good.
I thought "that's" was a valid contraction of "that is" but i'm not looking to hold the patch up over an English quiz ;)
#12
The last submitted patch failed testing.
#13
Patch is failing because it was accidentally applied as part of #314870: UNSTABLE-4 blocker: Add api.php for every core module.
#14
Breaks badly on PostgreSQL, with message:
PDOException: SELECT fid FROM {files} WHERE status & :permanent != :permanent AND timestamp < :timestamp - Array ( [:permanent] => 1 [:timestamp] => 1227604875 ) SQLSTATE[42P18]: Indeterminate datatype: 7 ERROR: could not determine data type of parameter $2 in system_cron() (line 1511 of /var/www/drupal/revamp/modules/system/system.module).Back to the drawing board :)
#15
This PDOException is breaking the postgresql installer. After commenting out the query in system_cron, the install works fine.
I found a similar issue, that was resolved with some PDO config magic:
http://bugs.php.net/bug.php?id=36652
#16
I traced this down to the fact that the timestamp column is of type int_unsigned, and along the way filed this issue: #339588: Column type of int_unsigned causing pdoexception.
#17
Should the bitwise operator be removed? I didn't think that was standard, acceptable thing to do. Plus the following just look weird...
WHERE status & :permanent != :permanent AND timestamp < :timestamp', array(':permanent' => FILE_STATUS_PERMANENT, ...#18
Dave Reid, i think the problem was the way we did data types on pgsql. as far as i can tell pgsql supports them when you've got the right data types: http://www.postgresql.org/docs/7.4/static/functions-math.html
#19
#341910: file_space_used() not checking properly checking bitmapped status value (adds unit tests) was running into the same situation. the patch on #339588: Column type of int_unsigned causing pdoexception gets this passing and i'd guess addresses #341910 as well.
#20
postponing until #339588: Column type of int_unsigned causing pdoexception get committed.
#21
#22
The last submitted patch failed testing.
#23
okay that's freaking annoying... at least re-test it before slamming the status back...
#24
Committed to CVS HEAD. Thanks, drewish.
#25
The issue was that it is invalid to use both times the same placeholder:
<?php+ $result = db_query('SELECT fid FROM {files} WHERE status & :permanent != :permanent AND timestamp < :timestamp', array(':permanent' => FILE_STATUS_PERMANENT, ':timestamp' => REQUEST_TIME - DRUPAL_MAXIMUM_TEMP_FILE_AGE));
?>
Here you are trying to use ":permanent" two times. This will fail when using the C prepare statement on PostgreSQL, and even of some PHP/MySQL configurations as reported in #352956: PDOException on install.
Please review the fix there.
#26
Automatically closed -- issue fixed for two weeks with no activity.