TableSort extender should not force tablesort orderby to the end of the statement

Moving the bulk of the code from the execute to the setHeader will have the desired affect:

  public function execute() {
    return $this->query->execute();
  }
  public function setHeader(Array $header) {
    $this->header = $header;
    $ts = $this->init();
    if ($ts['sql']) {
      // Based on code from db_escape_table(), but this can also contain a dot.
      $field = preg_replace('/[^A-Za-z0-9_.]+/', '', $ts['sql']);

      // Sort order can only be ASC or DESC.
      $sort = drupal_strtoupper($ts['sort']);
      $sort = in_array($sort, array('ASC', 'DESC')) ? $sort : '';
      $this->orderBy($field, $sort);
    }
    return $this;
  }

Until the header is set, you can't actually add the sorting information to the query, so you can't apply any of the sorting until at _least_ this point in the chain. This seems to be a reasonable place, then, to insert the orderBy execution, but I'm open to hear what other people think, too.

I would rather override the behavior of orderBy() on TableSort, so that order by set after the extension are "queued" locally, and added to the end of the query at execute() time. Does putting this logic in setHeader() really makes sense?

$query->orderBy('sticky')->setHeader()->orderBy('created') - and the setHeader works just like the other orderBy() functions. It doesn't seem to make sense to me to make orderBy() work in one way if you're using setHeader and a different way if you're not.

In that case, you will have to rename setHeader() to something like orderByHeader()?

I feel like orderByHeader is more indicative of the true nature of the function with this change, so I'd vote for a change to orderByHeader despite the subsequent cleanup required on existing instances of setHeader.

There should only be 3-4 uses so far, I think. It's only been around about 3 weeks. :-)

I'm open to changing the name, although Damien's concept of a "smarter orderBy queue" is interesting as well. I guess I don't have a strong preference there until I see code for what Damien's would look like.

I think the name change makes sense, in two ways:
- it indicates that the method is chainable (it was already before, but all other set* and add* functions are not).
- with the changed name, it also makes sense to have the logic there.. imho.

Re-rolled ch'x patch with the name change.

The tests failed because tracker.module is currently broken, moving the logic to orderByHeader just revealed that. Applied the same fix as DamZ has done in #394582-4: DBTNG: Tracker module

I also support the name change. It always found setHeader() a strange name, and orderByHeader() makes it a bit more clear. The word "header" is still a bit odd for those not in the know.

I'm undecided about the two approaches though -- I'm asking Crell to make that call. The order by queue seems a bit more exotic. Would be good to see an example of how DamZ's syntax would be used (without actual implementation) to solve the forum problem.

I can see how the original proposal would work, but I'm not sure I follow DamZ's. DamZ, can you provide a code snippet or pseudocode or something so we can see what you mean?

Re-roll with the new setHeader() calls renamed. Still needs DamZ's input...

Re-rolled with more setHeader -> orderByHeader conversions..

Re-rolled with more setHeader -> orderByHeader conversions..

I was sure I posted something here already. The basic idea in #5 was to allow the query write to use:

$query = db_select('node', 'n');
$query->orderBy('n.sticky', 'DESC');
$query->extend('TableSort');
$query->orderByHeader($forum_topic_list_header);
$query->orderBy('n.created', 'DESC');

The orderBy() before the extension are set in the main query object, but the orderBy after the extension are queued locally in the TableSort object, and only pushed to the main query object on execute().

Thinking about it, there is not much to gain from that. With the rename, the new function behavior is perfectly ok.

Let's get this in, and move to more DB:TNG conversion awesomeness.

Great, thanks. Committed to CVS -- nice clean-up and fix!