I haven't replicated the issue reported at #1182614: Integrate with Facet API, however I would assume it is because Facet API expects facets in a key:value pairing separated by a colon. If the "key" is double encoded, we might be able to get around this issue.

CommentFileSizeAuthor
#5 facetapi-1209490-5.patch3.23 KBkatbailey
#4 facetapi-1209490-4.patch1.87 KBkatbailey
#3 facetapi-1209490-3.patch1.55 KBcpliakas
#2 facetapi-1209490-2.patch954 bytescpliakas

Comments

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented

Subscribing.

cpliakas’s picture

cpliakas commented
Status: Active » Needs review
StatusFileSize
new facetapi-1209490-2.patch954 bytes

Haven't tested this at all, however it at least illustrates the problem. I suspect it will work.

cpliakas’s picture

cpliakas commented
StatusFileSize
new facetapi-1209490-3.patch1.55 KB

This is probably better since colon's in the value are currently handled correctly.

katbailey’s picture

katbailey commented
StatusFileSize
new facetapi-1209490-4.patch1.87 KB

Thanks for this Chris - however, I needed to make one change to your patch to get the filters to work. At line 168 of adapter.inc, you're doing 

$field_alias = drupal_encode_path($parts[0]);

but this actually double-encodes the colon. So I've changed it to 

$field_alias = urldecode($parts[0]);

because the $field_alias needs to be the name in its original form.

There's also a problem in facetapi.block.inc because of the fact that the deltas are being constructed with colons separating searcher, realm name and facet name. I've addressed this in the patch (just using urlencode and urldecode which may not be the best solution).

katbailey’s picture

katbailey commented
StatusFileSize
new facetapi-1209490-5.patch3.23 KB

Oops, that last patch didn't have the block fix in it... here we go.

cpliakas’s picture

cpliakas commented

This looks good. I smell Kat-commit number 2. I am going to try to write some unit tests for this, then we can push it through. Feel free to write the test if you are inclined to do so, but I feel like you are probably coding some more important things :-).

cpliakas’s picture

cpliakas commented

Added supporting tests in commit 34bbe23.

cpliakas’s picture

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

Tests fail without patch and succeed with patch.

cpliakas’s picture

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

Committed at 5cff397.

pwolanin’s picture

pwolanin commented
Status: Fixed » Needs work

we should be using rawurlencode() I think - that's what Drupal uses everywhere.

urlencode() is only for the outdated POST data spec.

cpliakas’s picture

cpliakas commented

Agreed :( Missed that one.

cpliakas’s picture

cpliakas commented
Status: Needs work » Fixed

Marking this as issue fixed, posted a new issue at #1215066: Replace deprecated urlencode() with rawurlencode().

Status: Fixed » Closed (fixed)

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