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.
the shorthand 'status & :status' doesn't work on postgres. Postgres needs the comparison operator ' > 0' appended on the end
currently, File validator tests throws an Exception due to this.
patch simply appends the comparison on the end
Comment | File | Size | Author |
---|---|---|---|
#29 | file_341910.patch | 4.76 KB | drewish |
#27 | file_341910.patch | 4.91 KB | drewish |
#20 | file_341910.patch | 4.22 KB | drewish |
#14 | file_341910.patch | 1.03 KB | drewish |
#6 | file.inc_.patch | 1.01 KB | Josh Waihi |
Comments
Comment #1
kaotien CreditAttribution: kaotien commentedClean 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
Comment #2
serenecloud CreditAttribution: serenecloud commentedTesting with CVS HEAD and PostgreSQL 8.1
Without Patch:
After patch I ran the File SimpleTest:
Happy to mark this as reviewed.
Comment #3
Dave ReidPlease use
diff -up
to create patches for core. See http://drupal.org/patch/create for more details.Comment #4
Dave ReidPlus, are bitwise operators a SQL standard? I can't find if they are and if they're not, they should be replaced.
Comment #5
Dave ReidFrom system_schema, the {files}.status is simply an integer, so what reason do we have to do
status & status > 0
instead of simplystatus > 0
?Comment #6
Josh Waihi CreditAttribution: Josh Waihi commentedwe 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.
Comment #7
Dave ReidThat's a much better, more easily understood solution. The bitwise was just too confusing. Passed tests, looks good and RTBC.
Comment #8
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD.
Comment #10
drewish CreditAttribution: drewish commentedActually 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.
Comment #11
drewish CreditAttribution: drewish commentedDave 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.
Comment #12
Dave ReidSorry drewish.
Comment #13
drewish CreditAttribution: drewish commentedno, 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.
Comment #14
drewish CreditAttribution: drewish commentedmarking this as postponed pending #339588: Column type of int_unsigned causing pdoexception. the attached patch works with pgsql once it's applied.
Comment #15
drewish CreditAttribution: drewish commentedComment #16
Dries CreditAttribution: Dries commentedWould 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.
Comment #17
drewish CreditAttribution: drewish commentedDries, 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.
Comment #18
Dries CreditAttribution: Dries commentedWell, 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?
Comment #19
drewish CreditAttribution: drewish commentedRight the tests don't create files with multiple statues. I'll expand the function's test.
Comment #20
drewish CreditAttribution: drewish commentedComment #21
drewish CreditAttribution: drewish commentedoh, 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.
Comment #22
Dries CreditAttribution: Dries commentedIn 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.
Comment #23
drewish CreditAttribution: drewish commentedDries, 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?
Comment #24
drewish CreditAttribution: drewish commentedbumping back to CNR since i think the current tests are correct.
Comment #25
Dries CreditAttribution: Dries commentedNope, 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!
Comment #26
drewish CreditAttribution: drewish commentedjust a note that this issue needs to keep any eye on #352956: PDOException on install and use a similar fix.
Comment #27
drewish CreditAttribution: drewish commentedWent ahead and converted this into a full DBTNG query. Added tests to the comments.
Comment #28
Dries CreditAttribution: Dries commentedWhen I run the tests, I get 2 failures:
Comment #29
drewish CreditAttribution: drewish commentedsorry about that. it looks like i'd just re-submitted the last patch. here's the correct version.
Comment #30
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks all!