Closed (fixed)
Project:
Apache Solr Search
Version:
6.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
1 Jul 2009 at 23:17 UTC
Updated:
16 Jul 2009 at 06:00 UTC
Jump to comment: Most recent file
Comments
Comment #1
pwolanin commentedhere's a start
Comment #2
pwolanin commentedI'm not sure about the variable return type of method get_solrsort() versus adding another method to the interface.
Comment #3
pwolanin commentedI'm not sure about the variable return type of method get_solrsort() versus adding another method to the interface.
Comment #4
pwolanin commentedslight cleanup
Comment #5
JacobSingh commentedOverall 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.
Comment #6
JacobSingh commentedif (!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.
Comment #7
pwolanin commentedGuess I left my test field aliases in there...
The '|' is for the later regex so we validate that we only have an allowed sort.
Comment #8
JacobSingh commentedLooks good to me.
Comment #9
pwolanin commenteddoxygen fix
Comment #10
pwolanin commentedcommitted to 6.x