Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EZP-26630: Support per-query raw filters #200

Open
wants to merge 1 commit into
base: master
from

Conversation

6 participants
@peterkeung
Copy link
Collaborator

commented Nov 19, 2016

https://jira.ez.no/browse/EZP-26630

In eZ Find, all filters expect to follow the pattern field:value. If you don't have a colon, ezfeZPSolrQueryBuilder::getParamFilterQuery() prepends a colon, effectively breaking some filters. This prevents you from adding special filters for the "fq" parameter in Solr, such as frange queries:

http://solr.pl/en/2011/05/30/quick-look-frange/

An example use case is to enforce a minimum relevance score:

{!frange l=1}query({!edismax v=$q})

You can get around this by using RawFilterList in ezfind.ini, but that's applied to every query.

This proposed solution would add a new parameter "raw_filter" to the ezfind/search fetch function:

'raw_filter', array( '{!frange l=1\}query({!edismax v=$q\})' ),

@peterkeung

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 19, 2016

@moismailzai

This comment has been minimized.

Copy link

commented Nov 21, 2016

+1 this looks good to me.

I can confirm this patch is functional and works well in practice (that is, the filter range is limiting results as expected).

@benkmugo

This comment has been minimized.

Copy link

commented Nov 21, 2016

The flexibility this adds is a good improvement I think. It avoids having to use workarounds or going via the raw solr fetch in template land.

+1

@gggeek

This comment has been minimized.

Copy link
Collaborator

commented Nov 26, 2017

+1 (apart from remark)

@@ -80,7 +81,7 @@ public function getFilterParameters()
* @return array Search result
*/
public function search( $query, $offset = 0, $limit = 10, $facets = null,
$filters = null, $sortBy = null, $classID = null, $sectionID = null,
$filters = null, $rawFilters = null, $sortBy = null, $classID = null, $sectionID = null,

This comment has been minimized.

Copy link
@gggeek

gggeek Dec 6, 2017

Collaborator

why not add it as last param? better for BC

This comment has been minimized.

Copy link
@peterkeung

peterkeung Dec 6, 2017

Author Collaborator

Sure -- easy change to make. @andrerom who else would we need approval from before merging?

This comment has been minimized.

Copy link
@andrerom

andrerom Dec 7, 2017

Member

Well this BC point should probably be fixed and then maybe there is another approval coming :)

This comment has been minimized.

Copy link
@peterkeung

peterkeung Dec 7, 2017

Author Collaborator

OK, $rawFilters has been moved to the last param now!

@peterkeung peterkeung force-pushed the peterkeung:per-query-raw-filters branch from ea19ed3 to 639b532 Dec 7, 2017

@pkamps

This comment has been minimized.

Copy link
Collaborator

commented Dec 14, 2017

I don't like that the parameters filter and raw_filter take an array for the value. I think it will combine multiple array elements with a logical AND. That's something not very obvious for a developer.
But that's how the parameter filter was developed and raw_filter is now consistent to that approach - so I'm OK with it.
Maybe we add inline doc saying something about how multiple array entries are handled.

+1

@benkmugo

This comment has been minimized.

Copy link

commented Dec 14, 2017

I think adding a brief inline doc note about those two parameter values would be a good idea.

@peterkeung peterkeung force-pushed the peterkeung:per-query-raw-filters branch from 639b532 to 95ccbe8 Dec 22, 2017

@peterkeung

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 22, 2017

@pkamps @benkmugo I've added additional comments where the filters are built

@benkmugo

This comment has been minimized.

Copy link

commented Jan 1, 2018

+1

@peterkeung

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 18, 2018

Let's get this merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.