#348907 follow-up: privatemsg_sql_list_sent() ORDER BY string manipulation smells bad
Liam McDermott - April 10, 2009 - 05:12
| Project: | Privatemsg |
| Version: | 6.x-1.x-dev |
| Component: | Documentation |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
| Issue tags: | 348907 |
Description
This just smells bad. :)
<?php
// We can use tablesort_sql now, but we don't need the ORDER BY part of the query.
// Because of that, we need to cut off the first 9 characters of the generated string.
$order_by = drupal_substr(tablesort_sql(_privatemsg_list_headers( FALSE, array('subject', 'recipient', 'last_updated'))), 9);
$fragments['order_by'][] = $order_by;
?>The documentation is ambiguous. I couldn't work out why we were generating an ORDER BY statement, then removing it. So we should clarify that we're literally removing the string 'ORDER BY' from the front of the returned value from tablesort_sql. Then there's We can use tablesort_sql now, that's understandable from the point-of-view of someone reading a patch but means little to someone who's perusing privatemsg.module, trying to understand the code.
Some documentation of what the function is for, and returns, would help put this in context too.
#1
Ahh, should have used the preview. Drupal stripped my
<quote>tags out of there. This:Should be:
#2
There's another instance of smelliness in
privatemsg_sql_list(). :)#3
It is *BAD*. But... always the same question, how much effort do we want to put into our own query builder? Not more than improving the documentation imho.
In D7, it will work like that:
<?php$query = $query->extend('TableSort');
$query->setHeader(_privatemsg_list_headers( FALSE, array('subject', 'recipient', 'last_updated'))));
?>
Which is *a lot* nicer than everything we can come up with D6.
I'm open for suggestions for the wording of the inline doc :)
#4
I am trying to get rid of list_sent in #303087: Move "Sent Messages" to privatemsg_filter, Add inbox, improve user filtering. so we may not want to pay too much attention to this problem here?
Ofcourse that still leaves the one in the list query.
#5
Let's try with a better comment.
BTW:
<?phpreturn $query
->condition('pmi.uid', $account->uid)
->condition('pmi.deleted', 0)
->groupBy('pmi.thread_id')
->orderBy('MAX(pmi.is_new)', 'DESC')
->orderByHeader(_privatemsg_list_headers( FALSE, array('subject', 'last_updated') + $fields))
->limit(variable_get('privatemsg_per_page', 25));
?>
This is how the code looks for Drupal 7, which is obviously *a lot* cleaner.
#6
Fixed in 6.x-1.x-dev.
#7
Automatically closed -- issue fixed for 2 weeks with no activity.