Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

we pass on $options from queryTemporary, it's not defined, so we pass a NULL , so query dies at $options += ....

webchick’s picture

Status: Reviewed & tested by the community » Needs work

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

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
697 bytes

So there.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Thanks. ;) pgsql loves you too. ;)

Crell’s picture

Wow that was fast... Thanks, chx!

chx’s picture

Status: Fixed » Needs review
FileSize
3.67 KB

Followup -- made the function headers consistent with the rest of core and added a test.

chx’s picture

FileSize
3.9 KB

Even more followup, detailed PHPdoc!

chx’s picture

chx’s picture

chx’s picture

webchick’s picture

This 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

hswong3i’s picture

chx’s picture

The problem over there is that your test does not test the temporary-ness of the queryTemporary.

Crell’s picture

Status: Needs review » Needs work

The 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!

hswong3i’s picture

chx’s picture

chx’s picture

FileSize
4.53 KB

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

chx’s picture

FileSize
4.33 KB

AH! 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?

chx’s picture

FileSize
4.32 KB

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

chx’s picture

Title: queryTemporary is broken. » #
FileSize
4.9 KB

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

chx’s picture

Title: # » queryTemporary is broken.
chx’s picture

FileSize
4.9 KB

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

chx’s picture

FileSize
5.14 KB

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

  1. the copypasted comments are removed.
  2. Engine=HEAP replaced with TYPE=MEMORY for MySQL
  3. Engine=HEAP replaced with AS for pgsql
  4. the test path is database_test_query_temporary
  5. Fixed the multiline comment
chx’s picture

FileSize
5.16 KB

Better @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).

chx’s picture

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

hswong3i’s picture

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

  1. Implementation of queryTemporary can concentrate into single line; prefixTables is now called within $this->query so need not to duplicate.
  2. Minor update of name of menu callback and its callback function, as database_test_db_query_temporary.
  3. Other minor documentation clean up (optional).

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.

hswong3i’s picture

Title: queryTemporary is broken. » [DBTNG + simpletest] queryTemporary is broken and need simpletest test case
chx’s picture

FileSize
5.25 KB

One minor change, in the module phpdoc you had a "tempory" other than that, we are good.

hswong3i’s picture

Status: Needs review » Reviewed & tested by the community

Thanks chx.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I 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!

Status: Fixed » Closed (fixed)

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