src/applications/maniphest/query/ManiphestTaskQuery.php
582–586	I wanted to make this more general, but different queries have different table aliases, and the underlying class only knows about the aliases of JOINs it does for application search, not the primary object's table alias.

This is awesome! I think I caught one real issue (incorrect construction of ORDER BY clause in the presence of a "Group by" selection in the UI). Some general rambling elsewhere, but all of that seems really nitpicky and easy to change later if we hit real issues. I think it largely does a good job of balancing complexity with making the feature work, since we don't need any of the full-power generalizations yet.

src/applications/maniphest/query/ManiphestTaskQuery.php
582–586	I think this is reasonable as-is, especially for the first implementation. I think it needs to get built later down, though. Otherwise, we're dropping the order constraints added by `$this->groupBy`. Specifically, "group by X, order by custom field Y" in the UI should produce an "ORDER BY x, y" clause. By returning early, we'll just get an "ORDER BY y" clause. I would expect the UI to show the wrong results if a "Group by:" is selected.
src/applications/search/engine/PhabricatorApplicationSearchEngine.php
772	I think this logic is OK for now, but it does mean non-configured custom fields can't provide orderings. For example, "Title" in Differential is a custom field, but can't generate a meaningful result for `buildOrderIndex()`. If some of this logic was on the Field and the call looked more like: $field->applyApplicationSearchOrderToQuery($this, $query) ...(similar to `applyApplicationSearchConstraintToQuery()`), that would let fields like "Title" implement the method as `$query->setOrderBy(SOME_TITLE_CONSTANT)` while custom fields could call `$query->withApplicationSearchOrder(...)`. This is an easy generalization to make later, though.
817	I wonder if we should make this separately configurable (e.g., add a new `sort` or `order` key to field configuration, which activates it in this UI). For now, I think it's fine as-is -- it's all in a dropdown anyway.
src/infrastructure/customfield/field/PhabricatorCustomField.php
641	This feels a little bit awkward, but I think it's OK until we have a clearer picture of things. (Some inlines elsewhere flesh this out more.)
src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php
366–367	Docs disagree with actual parameters.
625	At some point, although not necessarily yet, I think this should probably be something like `$field->buildOrderIndex()->getIndexValue()`. In particular, you could imagine a field which stores a different value than it indexes. This would require `buildOrderIndex()` to sometimes return a populated index and sometimes return a placeholder object, though. I don't really see a way to structure this elegantly offhand. I think this is fine for now, but we'll probably tweak it in the future.

This revision now requires changes to proceed.Aug 1 2014, 5:02 PM

epriestley awarded a token.Aug 1 2014, 5:02 PM

btrahan awarded a token.Aug 1 2014, 5:22 PM

Fix up ordering when "Group By" is present

Harbormaster completed remote builds in B1994: Diff 24326.Aug 2 2014, 1:35 AM

I'm going to hold off doing any generalisations or major refactorings. I think the Order By behaviour probably needs to be generalised (including the "built-in" order bys). While we're at it, generalising Group By would probably be good as well, by moving a lot of that query construction into PhabricatorCursorPagedPolicyAwareQuery.

Moving this kind of logic into the base class would vastly simplify the ManiphestTaskQuery implementation, would allow other queries to take advantage of it and would probably make a bunch of this "if custom field ordering, ignore switch statement" kind of logic go away.