#348907 follow-up: privatemsg_sql_list_sent() ORDER BY string manipulation smells bad [#429674]

This just smells bad. :)

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

Comment	File	Size	Author
#5	privatemsg_order_better_doc.patch	1.07 KB	Berdir
#5

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Liam McDermott CreditAttribution: Liam McDermott commented 10 April 2009 at 05:23

Ahh, should have used the preview. Drupal stripped my <quote> tags out of there. This:

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.

Should be:

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.

Comment #2

Liam McDermott CreditAttribution: Liam McDermott commented 10 April 2009 at 05:31

There's another instance of smelliness in privatemsg_sql_list(). :)

Comment #3

Berdir

German

Switzerland

CreditAttribution: Berdir commented 10 April 2009 at 16:24

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:

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

Comment #4

NaheemSays CreditAttribution: NaheemSays commented 10 April 2009 at 16:27

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.

Comment #5

Berdir

German

Switzerland

CreditAttribution: Berdir commented 30 October 2009 at 15:24

Component:	Code	» Documentation
Status:	Active	» Needs review

File	Size
privatemsg_order_better_doc.patch	1.07 KB

Let's try with a better comment.

BTW:

  return $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.