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

pwolanin’s picture

Status: Active » Needs work
StatusFileSize
new5.77 KB

here's a start

pwolanin’s picture

Status: Needs work » Needs review
StatusFileSize
new10.82 KB

I'm not sure about the variable return type of method get_solrsort() versus adding another method to the interface.

pwolanin’s picture

StatusFileSize
new10.82 KB

I'm not sure about the variable return type of method get_solrsort() versus adding another method to the interface.

pwolanin’s picture

StatusFileSize
new10.88 KB

slight cleanup

JacobSingh’s picture

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.

JacobSingh’s picture

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.

pwolanin’s picture

StatusFileSize
new10.23 KB

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.

JacobSingh’s picture

Looks good to me.

pwolanin’s picture

StatusFileSize
new10.27 KB

doxygen fix

pwolanin’s picture

Status: Needs review » Fixed

committed to 6.x

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.