TableSorts and PagerDefault queries broken
R.Muilwijk - August 31, 2008 - 13:42
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | database system |
| Category: | task |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed |
| Issue tags: | DBTNG Conversion, dc2009 code sprint |
Description
I've started working on this and are at like 80% now. I will finish this somewhere this week.
#1
It's somewhat more than a week... :-) Are you still working on this, or should someone else jump in?
#2
Sorry, I'll put in on top of my list for next thursday. We have open source thursday at our company so should have plenty of time then.
#3
This is open for anyone who wants it.
#4
#5
Working on this now!
#6
Also working on this atm. Can somebody give me a quick sanity check to see if I'm doing this right? tx!
#7
$max = db_query('SELECT MAX(wid) FROM {watchdog}')->fetchObject();should use ->fetchResult();
#8
Noticed that just after I submitted it... otherwise ok? (rerolled)
#9
#10
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.
#11
The last submitted patch failed testing.
#12
Oh really! I've been doing it wrong all this time!
Thanks for the help.
#13
Looks like it failed.. not sure why. Try this once more.
#14
[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!
#15
The last submitted patch failed testing.
#16
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]
#17
The last submitted patch failed testing.
#18
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).
#19
<?php+ $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.
<?php$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.
#20
Here is the dblog.admin.inc Still having issues with setHeader and setCountQuery, but simpletest seems to pass.
#21
I'll roll this patch into the one I'm repairing as per Crell's comments above.
#22
Sorry for the delay! Patch re-rolled with bubbasan's patch and Crell's comments above implemented. Hopefully this works!
#23
Rolled a cleaner patch, not sure if the last one would have worked or not but I think this one will.
#24
Changing title to reflect that this patch is for the whole dblog module and not just dblog.admin.inc
#25
Committed to CVS HEAD. Thanks!
#26
The recent log events tablesort is now completely broken and sorting by filter does not work as well. I'm looking into fixing it.
#27
This at least gets the table sort working. Filtering is still broken completely.
#28
Note to anyone doing DBTNG conversion. To convert table sorts, you must do:
<?php...
$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.
#29
Left in an unwanted debugging statement.
#30
#31
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.
#32
That seems like it would work. Comparing:
<?php$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:
<?php$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.
#33
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.
#34
#35
Same here,
#36
Re-roll.
#37
The last submitted patch failed testing.
#38
Re-rolled.
Also cleaned up a few queries that didn't early extend, even if they were working correctly.
#39
The last submitted patch failed testing.
#40
Re-roll.
#41
The last submitted patch failed testing.
#42
Re-roll, forgot to change a setHeader() call...
#43
Should we write tests for this?
#44
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.
#45
Fair enough. Committed to CVS HEAD.
#46
#47
Automatically closed -- issue fixed for 2 weeks with no activity.