TableSorts and PagerDefault queries broken

It's somewhat more than a week... :-) Are you still working on this, or should someone else jump in?

This is open for anyone who wants it.

Working on this now!

Also working on this atm. Can somebody give me a quick sanity check to see if I'm doing this right? tx!

$max = db_query('SELECT MAX(wid) FROM {watchdog}')->fetchObject();
should use ->fetchResult();

Noticed that just after I submitted it... otherwise ok? (rerolled)

foreach ($results AS $object) {
should just be lowercase 'as' :)

Other than that, it looks ok. I don't have anything to test that it actually works, but I guess the testbot can help us there.

Oh really! I've been doing it wrong all this time!

Thanks for the help.

Looks like it failed.. not sure why. Try this once more.

[Edit] Sorry, hadn't refreshed the page in a while. I think my patch applies and all tests pass. Big frown on duplicate work but I guess we'll all get a little more experience with DBTNG!

[Edit Edit] Durp! Also I missed the admin.inc, oh well! Hopefully your patch works!

Fixed up a few other things in admin.inc, I do think everything is done and working now. All pretty straightforward stuff, switched all of the while loops to foreach.

[Edit - I found a problem with my setup that caused that fail, I also realized that none of us have updated the test. Working on that now]

Ok, I think I have this working. Really this time.

Went through the test file and updated that as well as fixing my previous issue (and typos that accompanied it).

+    $result = db_query('SELECT wid FROM {watchdog} WHERE uid = :uid', array(':uid' => $user->uid));
+    foreach($result as $row) {
+      $ids[] = $row->wid;

The foreach block can be replaced with $ides = db_query()->fetchCol(); I believe that appears twice.

   $result = db_query('SELECT DISTINCT(type) FROM {watchdog} ORDER BY type');
-  while ($object = db_fetch_object($result)) {
+  foreach ($result as $object) {
     $types[] = $object->type;
   }

This can also be condensed into just fetchCol().

And of course there's still a pager query.

Here is the dblog.admin.inc Still having issues with setHeader and setCountQuery, but simpletest seems to pass.

I'll roll this patch into the one I'm repairing as per Crell's comments above.

Sorry for the delay! Patch re-rolled with bubbasan's patch and Crell's comments above implemented. Hopefully this works!

Rolled a cleaner patch, not sure if the last one would have worked or not but I think this one will.

Changing title to reflect that this patch is for the whole dblog module and not just dblog.admin.inc

Committed to CVS HEAD. Thanks!

The recent log events tablesort is now completely broken and sorting by filter does not work as well. I'm looking into fixing it.

This at least gets the table sort working. Filtering is still broken completely.

Note to anyone doing DBTNG conversion. To convert table sorts, you must do:

...
$query = $query->extend('PagerDefault')->limit(your_limit);
$query = $query->extend('TableSort')->setHeader($header);
...

These have to be separate lines and cannot be chained. This happened in dblog, poll, and comment.

Left in an unwanted debugging statement.

That seems wrong then. I know I ran into a case where ordered mattered, but you should be able to extend in a chain as long as you do assign back to $query at the end.

That seems like it would work. Comparing:

   $query = db_select('watchdog', 'w');
   $query = $query
     ->addExpression('COUNT(wid)', 'count');
     ->fields('w', array('message', 'variables'))
     ->condition('w.type', $type)
     ->groupBy('message')
     ->groupBy('variables')
     ->extend('PagerDefault')
     ->extend('TableSort')
     ->limit(30)
     ->setHeader($header)
     ->setCountQuery($count_query);
   $result = $query->execute();

to:

   $query = db_select('watchdog', 'w');
   $query->addExpression('COUNT(wid)', 'count');
   $query->fields('w', array('message', 'variables'));
   $query->condition('w.type', $type);
   $query->groupBy('message');
   $query->groupBy('variables');
   $query = $query->extend('PagerDefault')->limit(30);
   $query = $query->extend('TableSort')->setHeader($header);
   $query->setCountQuery($count_query);
   $result = $query->execute();

The latter is easier for me to see what is going on. I also remember something about some parts of db_select not able to be chained (fields and conditions?), but I guess it depends on what we're deciding is our offical DBTNG syntax.

I believe the standard is "chain whenever possible", because it's easier to read overall. addField() and addExpression() are not chainable, neither are the join*() methods. extend() ischainable, however, the returned object is the extender, not the original query, so if you don't call ->execute() in that chain then you have to assign back to $query, otherwise the extending object gets lost.

Same here,

Regarding coding style..

After talking with Crell/webchick, I try to follow the following rules when working with DBTNG:

- The docs at http://drupal.org/node/310069 should be used as examples for coding style
- Chain methods if possible
- If chaining is used, all methods should be on their own line
- Extend early (see #425872: DX problem with DB extenders)

I know that some disagree with point 2 for example, but we should really agree on something, whatever it is..

Re-roll.

Re-rolled.

Also cleaned up a few queries that didn't early extend, even if they were working correctly.

Re-roll.

Re-roll, forgot to change a setHeader() call...

Should we write tests for this?

We have generic paging tests and we have tests for the functionality of those pages. Moshe was against writing paging tests for every page where we used it, see #394582-8: DBTNG: Tracker module.

I'd say that with the new early extend CS, it is unlikely that it is used wrong and usually, we don't protect against wrong usage of our API.

Fair enough. Committed to CVS HEAD.