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.
Similar as case of #322457: [DBTNG + simpletest]: queryTemporary() is buggy and need simpletest test case, queryRange() is now missing simpletest test case (or maybe I miss that? please correct me if possible...).
It seems to be a good idea for an additional test case, because each database engine coming with different implementation, and simpletest can give a centralized testing for it :D
Comments
Comment #1
Crell CreditAttribution: Crell commentedHm, right you are. Can you take a shot at writing a few tests? It's probably best to give that its own class, since the database test setup method is really slow. :-(
Comment #2
hswong3i CreditAttribution: hswong3i commentedComment #3
hswong3i CreditAttribution: hswong3i commentedPatch update:
Comment #4
catchhswong3i, in case you haven't noticed, we still have a few hundred queries in core which require this backwards compatibility layer and need converting to the new API.
Comment #5
hswong3i CreditAttribution: hswong3i commented@catch: just take it easy that we are now able to remove these "duplicated" backward compatible code :-D
db_query_range() is now protected with backward compatible function _db_query_process_args():
Therefore we don't need to run those "tiny and incomplete" backward compatible code within queryRange() again...
On the other hand, db_query_range() is mainly used by pager_query(). By checking CVS HEAD, we can find that dblog.admin.inc line 111 is now using legacy syntax:
When checking admin/reports/pages with apply of this patch, it should still running correctly :-)
Comment #6
Crell CreditAttribution: Crell commentedhswong is correct that the BC layer is handled elsewhere now. That's older BC code that has since been centralized, so it's not needed anymore. That doesn't relate to unit testing, though, so should probably be a separate patch. Also there's no need to break the query() method call into two lines; it's fine as one.
For the unit tests themselves, on visual inspection:
- There's no need to iterate the result. Instead, do
$records = db_query(...)->fetchAll()
, to get back the full result set as an array. You can then just do count($records) to get the number of records returned.- Don't use assertTrue() here. It's cleaner/more semantic to use assertEqual(count($records), 2, t('message here'));
- As a convention I don't think we're going to support unnamed placeholders; in fact the documentation doesn't even mention that they exist. :-) Go ahead and drop that test, as it's unnecessary.
Thanks, Ed!
Comment #7
hswong3i CreditAttribution: hswong3i commentedFor point 3: we need to test unnamed placeholders, because it is supported by PDO and also documented for our db_query_range():
Others are simplify based on suggestion :D
Comment #8
hswong3i CreditAttribution: hswong3i commentedP.S. Just a little promotion: may we also give some love to its sister issue?
#322457: [DBTNG + simpletest]: queryTemporary() is buggy and need simpletest test case
Comment #9
hswong3i CreditAttribution: hswong3i commentedPatch reroll via CVS HEAD. No change. Pump as critical so we will not miss it :-)
Comment #10
Dries CreditAttribution: Dries commentedWaiting for Crell's review on this.
Comment #11
Crell CreditAttribution: Crell commentedThis is not critical.
- We don't need to test unnamed placeholders. Instead, we should fix the db_query_range() docs to not mention them. :-)
- As stated before, there's no need to break the query() method call into two lines; it's fine as one.
- We should include a test that doesn't start from 0 to make sure that works as well, say db_query_range(..., 1, 3), which should return 3 results.
Comment #12
hswong3i CreditAttribution: hswong3i commentedPatch revamp based on above suggestion.
Comment #13
hswong3i CreditAttribution: hswong3i commentedMerge with progress of #325895: [DBTNG + simpletest] queryTemporary is broken and need simpletest test case and sync their coding style. Other improvement:
DatabaseRangeQueryTestCase
is now extends fromDrupalWebTestCase
, so performance boosted.Patch via CVS HEAD and simpletest with MySQL, pass.
Comment #14
hswong3i CreditAttribution: hswong3i commentedComment #15
hswong3i CreditAttribution: hswong3i commentedMinor documentation update for getInfo()'s description.
Comment #16
hswong3i CreditAttribution: hswong3i commentedPatch reroll via CVS HEAD.
Comment #17
Crell CreditAttribution: Crell commentedThere's some offsets but still applies, looks good, and the relevant tests pass.
Comment #18
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.