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.

CommentFileSizeAuthor
#11 file_330633.patch4.58 KBdrewish
#9 file_330633.patch4.61 KBdrewish
#4 file_330633.patch4.58 KBdrewish
#3 file_330633_1.patch.txt11.81 KBgrendzy
#1 file_330633.patch12.2 KBdrewish
system_cron.patch4.26 KBdrewish

Comments

drewish’s picture

drewish commented
Status: Needs work » Needs review
StatusFileSize
new file_330633.patch12.2 KB

Got the tests working and was able to get the status matching working correctly.

drewish’s picture

drewish commented
Title: Temporary file cleanup needs some love » Temporary file cleanup needs some love (UnitTest included!)

maybe that'll lure in some reviews ;)

grendzy’s picture

grendzy commented
StatusFileSize
new file_330633_1.patch.txt11.81 KB

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);

drewish’s picture

drewish commented
StatusFileSize
new file_330633.patch4.58 KB

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.

grendzy’s picture

grendzy commented
Status: Needs review » Reviewed & tested by the community

Ah, much clearer now. The unit tests still look good.

Anonymous’s picture

Anonymous (not verified) commented
Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

grendzy’s picture

grendzy commented
Status: Needs work » Reviewed & tested by the community

Weird, the bot tested the wrong patch. #4 is the latest.

Anonymous’s picture

Anonymous (not verified) commented
Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

drewish’s picture

drewish commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new file_330633.patch4.61 KB

here's a re-roll.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work

In the code comments, that's should be that is, not?

Do these binary operations work on PostgreSQL and SQLite?

drewish’s picture

drewish commented
Status: Needs work » Needs review
StatusFileSize
new file_330633.patch4.58 KB

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 ;)

Status: Needs review » Needs work

The last submitted patch failed testing.

drewish’s picture

drewish commented

Patch is failing because it was accidentally applied as part of #314870: UNSTABLE-4 blocker: Add api.php for every core module.

damien tournoud’s picture

damien tournoud commented

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 :)

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Priority: Normal » Critical

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

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

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.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

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, ...

drewish’s picture

drewish commented

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

drewish’s picture

drewish commented

#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.

drewish’s picture

drewish commented
Status: Needs work » Postponed

postponing until #339588: Column type of int_unsigned causing pdoexception get committed.

drewish’s picture

drewish commented
Status: Postponed » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

drewish’s picture

drewish commented
Status: Needs work » Needs review

okay that's freaking annoying... at least re-test it before slamming the status back...

dries’s picture

dries commented
Status: Needs review » Fixed

Committed to CVS HEAD. Thanks, drewish.

damien tournoud’s picture

damien tournoud commented

The issue was that it is invalid to use both times the same placeholder:

+  $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.

Status: Fixed » Closed (fixed)

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