Posted by pwolanin on July 1, 2009 at 11:17pm
| Project: | Apache Solr Search Integration |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Currently the sort parameters don't respect any field aliases that are set into the query object - since this affects Drupal.org, we want to fix it. We also have sort validation happening in the module instead of inside the query object where it belongs.
Comments
#1
here's a start
#2
I'm not sure about the variable return type of method get_solrsort() versus adding another method to the interface.
#3
I'm not sure about the variable return type of method get_solrsort() versus adding another method to the interface.
#4
slight cleanup
#5
Overall good, here's one small point:
- function get_solrsort();
+ function get_solrsort($as_array = FALSE);
Generally speaking, a function should have only one return type. I suggest doing something like:
function get_sort_column()
function get_sort_direction()
or always return a structure.
#6
if (!empty($fv['apachesolr_search']['retain-filters']) && $fv['apachesolr_search']['querystring']) {
- $querystring = $fv['apachesolr_search']['querystring'] . '&retain-filters';
+ $querystring = $fv['apachesolr_search']['querystring'] . '&retain-filters=1';
}
This isn't really related, but whatever.
ummm.....
'type' => 'bigass'
?!?
+ $this->parse_sortstring($sortstring);
Too bad we don't have overloading in PHP. Really would be nicer as an overloaded set_solrsort.
public function set_available_sort($field, $sort) {
- $this->available_sorts[$field] = $sort;
+ public function set_available_sort($name, $sort) {
+ // We expect non-aliased sorts to be added.
+ $this->available_sorts[$name] = $sort;
+ }
+
A little unclear about this...
+ protected function parse_sortstring($sortstring) {
+ // Validate and set sort parameter
+ $fields = implode('|', array_keys($this->available_sorts));
Where is the | being used here? The function to set available sorts doesn't look like it would be expecting a pipe in the array key.
#7
Guess I left my test field aliases in there...
The '|' is for the later regex so we validate that we only have an allowed sort.
#8
Looks good to me.
#9
doxygen fix
#10
committed to 6.x
#11
Automatically closed -- issue fixed for 2 weeks with no activity.