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.
Trivial fix.
Comment | File | Size | Author |
---|---|---|---|
#31 | tQ_followup.patch | 5.25 KB | chx |
#29 | dbtng-queryTemporary-1225356549.patch | 6.22 KB | hswong3i |
#29 | 325895.diff | 4.99 KB | hswong3i |
#27 | tQ_followup.patch | 5.16 KB | chx |
#26 | tQ_followup.patch | 5.14 KB | chx |
Comments
Comment #1
chx CreditAttribution: chx commentedwe pass on $options from queryTemporary, it's not defined, so we pass a NULL , so query dies at $options += ....
Comment #2
webchickYes, I followed this logic through and see the bug. query() requires $options and queryTemporary is incapable of passing it along.
Thanks, committed. :)
However! I just noticed that pgsql didn't get done. :P CNW. :P
Comment #3
chx CreditAttribution: chx commentedSo there.
Comment #4
webchickThanks. ;) pgsql loves you too. ;)
Comment #5
Crell CreditAttribution: Crell commentedWow that was fast... Thanks, chx!
Comment #6
chx CreditAttribution: chx commentedFollowup -- made the function headers consistent with the rest of core and added a test.
Comment #7
chx CreditAttribution: chx commentedEven more followup, detailed PHPdoc!
Comment #8
chx CreditAttribution: chx commentedComment #9
chx CreditAttribution: chx commentedComment #10
chx CreditAttribution: chx commentedComment #11
webchickThis looks good to me, but I'd still like to get Crell's 2 cents. Hopefully he can find time tomorrow between sessions @ Drupalcamp Chicago. :D
Comment #12
hswong3i CreditAttribution: hswong3i commentedShould we keep focus on #322457: [DBTNG + simpletest]: queryTemporary() is buggy and need simpletest test case for centralize tracing?
Comment #13
chx CreditAttribution: chx commentedThe problem over there is that your test does not test the temporary-ness of the queryTemporary.
Comment #14
Crell CreditAttribution: Crell commentedThe simpletest group title shouldn't have a period on it.
The menu callback should probably not use so generic a name. Let's use a path that is specific to this test (e.g. "database_temp_table_test") so that we can easily add more fake callbacks if needed.
The comment on the testQueryTemporary() method spans 2 lines. Druplicon says: This is a bug. Please file a patch! Also, I think the text of that line could be made easier to parse in English. How about:
"Confirm that temporary tables work and are limited to one request."
Otherwise this looks good. Should all be easy to fix up. Thanks, chx!
Comment #16
hswong3i CreditAttribution: hswong3i commentedComment sync with #322458: [DBTNG + simpletest] queryRange() need update and simpletest test case.
Comment #17
chx CreditAttribution: chx commentedSniff.
Comment #18
chx CreditAttribution: chx commentedOh. Have not seen hswong3i's patch. However, his adds db_query_range comments to this issue -- that's a separate issue. Moves the test under admin -- nothing to do with admin. Also, his removes wrongly the prefixtables call -- we still need to prefix the tables in the SELECT -- the only thing not prefixed is the created temporary table. Adding "AS" to postgresql is, however a good idea, and MySQL works with it too so I moved those into mine.
Comment #19
chx CreditAttribution: chx commentedAH! I now understand, query will prefix so we do not need to. Now. If both mysql and pgsql has the same queryTemporary code what's the point in providing in both?
Comment #22
chx CreditAttribution: chx commentedA careful review of the mysql documentation reveals that it's a bad idea to use the heap engine (which should be
TYPE=MEMORY
anyways) because the resulting table size is limited by a setting leading to interesting bugs. As hswong3i rightly mentioned, other DBs do not support it anyways.I cleaned up this issue from hswong3i comments and my frustrated remarks. Let's stay on topic.
Comment #23
chx CreditAttribution: chx commentedWhat hswong3i never was able to communicate through is that the comments on db_query_temporary are copy-pasted from db_query_range thus are wrong. It's not trivial to see this from just looking at the patch as the preceding function is db_query_range.
Comment #24
chx CreditAttribution: chx commentedComment #25
chx CreditAttribution: chx commentedEvery class name in database_test.test is DatabaseSomeThingTestCase so I am moving "Test" to the end, just before Case, so that it's DatabaseQueryTemporaryTestCase instead of DatabaseTestQueryTemporaryCase.
Comment #26
chx CreditAttribution: chx commentedAfter more discussion with hswong3i it came to me that deciding on whether to use the MEMORY engine or not, does not belong to this patch. With this, we are quite back to #10 with the following differences:
Comment #27
chx CreditAttribution: chx commentedBetter @param $tablename documentation for db_query_temporary -- temporaryQuery and Drupal 6 had this text. (It's strange to be db_query_temporary and temporaryQuery -- not consitent -- but that's a followup).
Comment #28
chx CreditAttribution: chx commentedPlease credit hswong3i as quite some of the late changes are his. It's not impossible, after all, to work with him -- it's just incredibly hard because of language issues but now he is quite willing to make a compromise and not force an Oracle-focus on every patch. And quite some of his findings are valid and useful.
Comment #29
hswong3i CreditAttribution: hswong3i commentedIt is also a good lesson for me, too. Thanks chx :D
Anyway, I give a parallel comparison between my #16 and chx's #27, and here are some propose changes (for detail please refer to the *.diff attached):
queryTemporary
can concentrate into single line;prefixTables
is now called within$this->query
so need not to duplicate.database_test_db_query_temporary
.Patch simpletest with MySQL 5.0.51a-16 (Debian), pass with no error. PostgreSQL's simpletest is now buggy so no way to test with it.
Comment #30
hswong3i CreditAttribution: hswong3i commentedComment #31
chx CreditAttribution: chx commentedOne minor change, in the module phpdoc you had a "tempory" other than that, we are good.
Comment #32
hswong3i CreditAttribution: hswong3i commentedThanks chx.
Comment #33
webchickI nitpicked this to death already in #drupal the other week. Gave it another visual once-over and see nothing amiss. Tests all pass. It's nice to see you two working together. :)
Committed to HEAD, thanks!