file_space_used() not checking properly checking bitmapped status value (adds unit tests)

Clean Drupal 7 install on MySQL

File -> File Validator tests

Pre-patch: 27 passes, 0 fails, and 0 exceptions
Post-patch: 27 passes, 0 fails, and 0 exceptions

Testing with CVS HEAD and PostgreSQL 8.1

Without Patch:

SELECT SUM(filesize) FROM {files} WHERE uid = :uid AND status & :status - Array ( [:uid] => 3 [:status] => 1 ) SQLSTATE[42804]: Datatype mismatch: 7 ERROR: argument of AND must be type boolean, not type integer

After patch I ran the File SimpleTest:

215 passes, 0 fails, and 0 exceptions

Happy to mark this as reviewed.

Please use diff -up to create patches for core. See http://drupal.org/patch/create for more details.

Plus, are bitwise operators a SQL standard? I can't find if they are and if they're not, they should be replaced.

From system_schema, the {files}.status is simply an integer, so what reason do we have to do status & status > 0 instead of simply status > 0?

we couldn't do status > 0 because you should be able to find the filesize sum of a group of file with a particular status. FILE_STATUS_TEMPORARY = 0 which means with bitwise comparison, you could never find the total filesize for temporary files with this function. so I think a simple WHERE status = :status would be better and from testing that I've done, it faster than bitwise comparison not to mention easier to understand for most people.

That's a much better, more easily understood solution. The bitwise was just too confusing. Passed tests, looks good and RTBC.

Committed to CVS HEAD.

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

Actually this is totally incorrect. The status field is documented as a bitmapped field not a simple integer I've been trying to get #330633: Temporary file cleanup needs some love (UnitTest included!) updated to check it correctly.

Dave Reid, grrrrr... just realized you'd been commenting on the other issue. i wish you'd have linked the two up but at least now i've got a pgsql fix for the other issue.

Sorry drewish.

no, i apologize. it's really my fault for keeping an eye on the queue and for not installing pgsql and addressing #330633: Temporary file cleanup needs some love (UnitTest included!) a month ago. seems like #339588: Column type of int_unsigned causing pdoexception might be at the root of this.

marking this as postponed pending #339588: Column type of int_unsigned causing pdoexception. the attached patch works with pgsql once it's applied.

Would be great if someone using PostgreSQL could run the tests with and without the latest patch. If this is PostgreSQL specific, I recommend adding PostgreSQL to the title.

Dries, I ended up figuring out how to setup pgsql just to test this patch so I can say clearly that it works with both MySQL and PostgreSQL. At this point it's not a pgsql specific fix since the pgsql "fix" reverted the desired behavior.

Well, but on MySQL there is no test failing with or without this patch. Does it mean that we don't have test coverage for this problem?

Right the tests don't create files with multiple statues. I'll expand the function's test.

oh, forgot to add that the current code has zero tests for file_space_used(). I'm not sure how it was over looked. The current patch adds tests.

In the test code, can we stick to using the defines instead of integers? It would make the tests a lot easier to read/review, and it makes for better example code.

Dries, 1 is the only core defined status and its constant is used. The other status values were arbitrarily selected for testing purposes. Are you suggesting that I should define constants in the test case? Or in core?

bumping back to CNR since i think the current tests are correct.

Nope, I'm not suggesting that you define constants for testing purpose. Maybe just add some more code comments to the test code. Explain why you are testing things this way, and mention that these values are arbitrarily selected. That should do it for me. Thanks!

just a note that this issue needs to keep any eye on #352956: PDOException on install and use a similar fix.

Went ahead and converted this into a full DBTNG query. Added tests to the comments.

When I run the tests, I get 2 failures:

Found the size of the first user's files with status 8.1	Other	file.test	188	FileSpaceUsedTest->testUserAndStatus()	
Found the size of the first user's files with status 2.	1	file.test	189	FileSpaceUsedTest->testUserAndStatus()

sorry about that. it looks like i'd just re-submitted the last patch. here's the correct version.

Committed to CVS HEAD. Thanks all!

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