I use several vocabularies as facets and it appears apache solr module does not always filter down the categories.
What I mean is that (on occasion) it may choose to replace the existing filter instead of adding it.
Example :
Say I filtered down for tid:45 from vocab 1; when I hover on the individual(already filtered) facets of vocab 2 I sometimes get a link that causes the original filter to be replaced eg http://drupalydo/search/apachesolr_search/product?filters=tid:73 where tid is the new filter instead of what I would expect to have http://drupalydo/search/apachesolr_search/product?filters=tid:45 tid:73
Has anyone noticed that or am I doing something wrong?

CommentFileSizeAuthor
#1 Solr_Base_Query.php_.clone_.patch905 bytesmkalkbrenner

Comments

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented
StatusFileSize
new Solr_Base_Query.php_.clone_.patch905 bytes

I ran into this bug too while I was working on this new feature: #401234: Reflect hierarchical taxonomy vocabulary in facet

The problem is located in Solr_Base_query.php. There is a static counter in method add_field which cause the issue if the query gets cloned.
Cloning a query object happens in facet blocks. So in this case you don't add but overwrite a field if you click on a facet link.

I attached a patch that fixes this issue.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented
Assigned: Unassigned » mkalkbrenner
Priority: Normal » Critical
Status: Active » Needs review
Anonymous’s picture

Anonymous (not verified) commented

I had occasionally seen this behavior, but hadn't had the time to investigate.

I'll help check this patch out, hopefully today.

pwolanin’s picture

pwolanin commented

I don't really see why this is happening, but the patch looks reasonable.

Anonymous’s picture

Anonymous (not verified) commented

Tested the patch and it seems to have corrected the problem without introducing any new issues.

JacobSingh’s picture

JacobSingh commented

heh, okay we know why Robert had the microtime() now.

I think his reason was something to do with the breadcrumb order? I really don't remember, but this is getting a bit ugly.

Otherwise, the patch looks reasonable to me, although I imagine there is some hidden problem here, I think we can commit it, it shouldn't be any worse.

Let's think also about ways to do this without cloning... Cloning is typically a source of memory leaks and ugly scoping issues like this.

pwolanin’s picture

pwolanin commented

well, cloning is really the right solution - we need to start fresh each loop

pwolanin’s picture

pwolanin commented
Version: 6.x-1.0-beta5 » 6.x-1.x-dev
Status: Needs review » Fixed

committed to 6.x - thanks.

Status: Fixed » Closed (fixed)

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