In the case the facet field alias contains a colon (which do most entity api implemented fields), the facet filters aren't removed due to a missmatch on the rawurlencoded and rawurldecoded version of the field alias.

I will post a patch for that in the next comment.

CommentFileSizeAuthor
#17 facetapi-processactiveitems-phpnotice.patch992 bytesAdam Clarey
#6 1884152-processactiveitems-addactivefilters-6.patch563 bytesduozersk
#5 1884152-processactiveitems-addactivefilters-5.patch477 bytesduozersk
#1 facetapi-rawurlencoded_filter_alias-1884152-1.patch768 bytesderhasi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

derhasi’s picture

derhasi CreditAttribution: derhasi commented
Status: Active » Needs review
FileSize
facetapi-rawurlencoded_filter_alias-1884152-1.patch768 bytes

And there is the patch.

derhasi’s picture

derhasi CreditAttribution: derhasi commented
Version: 7.x-1.2 » 7.x-1.x-dev

The patch is applied relative to current 7.x-1.x-dev.

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
Priority: Normal » Major

Thanks for the patch! This makes sense to me, will test it out soon.

duozersk’s picture

duozersk
he/him
Primary language Russian
Location Moscow
CreditAttribution: duozersk commented
Issue summary: View changes

I have recently created another issue with a patch for this bug - #2206649: processActiveItems() / addActiveFilters() compatibility
I have opted to fix it another way - will close it now as duplicate and post another patch here.

Thanks
AndyB

duozersk’s picture

duozersk
he/him
Primary language Russian
Location Moscow
CreditAttribution: duozersk commented
Related issues: +#2206649: processActiveItems() / addActiveFilters() compatibility
FileSize
1884152-processactiveitems-addactivefilters-5.patch477 bytes

Here is a patch from #2206649: processActiveItems() / addActiveFilters() compatibility

duozersk’s picture

duozersk
he/him
Primary language Russian
Location Moscow
CreditAttribution: duozersk commented
FileSize
1884152-processactiveitems-addactivefilters-6.patch563 bytes

Here as another version of the patch in #5 - without using any additional calls to rawurldecode/rawurlencode functions.

Thanks
AndyB

duozersk’s picture

duozersk
he/him
Primary language Russian
Location Moscow
CreditAttribution: duozersk commented
Status: Needs review » Needs work
duozersk’s picture

duozersk
he/him
Primary language Russian
Location Moscow
CreditAttribution: duozersk commented
Status: Needs work » Needs review
cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
Status: Needs review » Postponed (maintainer needs more info)

Hi duozersk.

I am not seeing the issue mentioned in the OP. Could you provide steps to reproduce on a clean install of Drupal w/ implementing search modules in order to highlight the bug?

Thanks,
Chris

duozersk’s picture

duozersk
he/him
Primary language Russian
Location Moscow
CreditAttribution: duozersk commented
Status: Postponed (maintainer needs more info) » Needs review

Chris,

I have made a demo on http://andrewberezovsky.ru/demo/demotest/
It uses the Facet API, Search API, Search API DB and Facet API Bonus modules. There is a dependency set on the facets (Brand and Model are 2 separate taxonomy vocabularies) - show Model facet only after selecting any value in the Brand facet (and remove the Model facet value after removing the Brand facet value).

The bug is that if after selecting the Brand and Model you remove the Brand facet value - then the Model is not removed from the Current search block and the breadcrumbs.

The reason for this bug is explained in the referenced issue - #2206649: processActiveItems() / addActiveFilters() compatibility
"The processActiveItems() uses the raw, not urldecoded key to set the $this->activeItems['filter'][__KEY__] while addActiveFilters() tries to remove the facet that didn't meet the dependencies using the 'field alias' key which is urldecoded."

Hope it helps.
AndyB

cpliakas’s picture

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

Hi duozersk .

Thanks for taking the time to set up the demo. The bug is crystal clear to me now, and your description has helped me to replicate it locally as well. Marking your patch in #6 as RTBC as I can confirm that it fixes the issue.

Thanks for the contribution and efforts to help me understand the bug,
Chris

  • cpliakas committed c5b1a8c on 7.x-1.x authored by duozersk 
    Issue #1884152 by duozersk, derhasi: Some active facets are not removed...

  • cpliakas committed 8984602 on authored by duozersk 
    Issue #1884152 by duozersk, derhasi: Some active facets are not removed...
cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
Status: Reviewed & tested by the community » Fixed
duozersk’s picture

duozersk
he/him
Primary language Russian
Location Moscow
CreditAttribution: duozersk commented

Great!
Thank you for coming back to this issue, really appreciate your efforts on Facet API.

Status: Fixed » Closed (fixed)

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

Adam Clarey’s picture

Adam Clarey CreditAttribution: Adam Clarey as a volunteer commented
Status: Closed (fixed) » Active
FileSize
facetapi-processactiveitems-phpnotice.patch992 bytes

I've attached a patch to prevent php notices occurring when the $parts array only contains a single item by declaring $filter within the if statement.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm as a volunteer commented
Status: Active » Closed (fixed)

See #2378693: Notice: Undefined offset: 1 in FacetapiAdapter->processActiveItems() line 312 for fixing the notice.