Per chat discussion - this doesn't work right as a clas const, due to early static binding in PHP 5.2 which means self::FILTER_KEY is usually going to be the const defined in the abstract class.

Instead, we should convert this to an instance variable, and provide at least a getter method.

CommentFileSizeAuthor
#6 1214772-6.patch6.49 KBcpliakas
#5 1214772-5.patch6.49 KBcpliakas
#3 1214772-3.patch6.19 KBcpliakas
#2 1214772-2.patch2.59 KBpwolanin
#1 1214772-1.patch2.6 KBpwolanin

Comments

pwolanin’s picture

pwolanin commented
Status: Active » Needs review
StatusFileSize
new 1214772-1.patch2.6 KB
pwolanin’s picture

pwolanin commented
StatusFileSize
new 1214772-2.patch2.59 KB

opps, getter should not be a static function

cpliakas’s picture

cpliakas commented
StatusFileSize
new 1214772-3.patch6.19 KB

This looks good. Reworked the patch a little to add a setter method, and I am also taking the opportunity to do a little refactoring based on this change. The adapter now has a getQueryString() method where you can explicitly pass in a facet definition. This might help with the work you are doing for the slider, as you may be able to use this as an API method to build the query string.

cpliakas’s picture

cpliakas commented
Status: Needs review » Needs work

Thinking about this further, maybe a setter method isn't appropriate. Maybe it should be set in the setParams method since the two are so closely tied to one another. I am thinking something like public function setParams(array $params = array(), $filter_key = 'f') {. Setting the filter key after setting the params lead to unintended results and give developers too much rope to hang themselves with.

cpliakas’s picture

cpliakas commented
Status: Needs work » Needs review
StatusFileSize
new 1214772-5.patch6.49 KB

Here is another patch, with the idea in #4.

cpliakas’s picture

cpliakas commented
StatusFileSize
new 1214772-6.patch6.49 KB

Same patch, fixed error in a comment.

cpliakas’s picture

cpliakas commented
Status: Needs review » Reviewed & tested by the community

I think this is RTBC. Passed the tests, and I think it is a more flexible approach that doesn't seem to break anything.

cpliakas’s picture

cpliakas commented
Status: Reviewed & tested by the community » Fixed

Committed at a7cd37b.

Status: Fixed » Closed (fixed)

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