Add support for "between" operator

works ok.

The combination of "Is smaller than" and "Is greater than" doesn't always lead to the same results as a "between" operation.
E.g. when using solr an expression like field[1 TO *] AND field[* TO 10] doesn't necessary return the same results as field[1 TO 10]. (I'm not sure how this comes or what precondition may apply to provoke such a behaviour.)
However, it would be nice to have the possibility to explicitly choose a "between" operation.

We are introducing a discrepancy here, in which:

$query->condition($field, array($min, $max), 'between')

can have different results then:

$query->condition($field, $min, '>=')
$query->condition($field, $max, '<=')

This is *not* ok.

That all sounds like a bug in Solr that should be investigated and fixed.

Thanks for the suggestion, and for putting so much work into this!

This is *not* ok.

Why shouldn't it be? If you use different operators, you get different results. That hardly is a no-go.
By the way: the discrepancy should only occur for multi-valued fields, and has a pretty simple cause. field >= A AND field <= B means: find results where one of the field's multiple values is greater than A, and one is less than B; field BETWEEN A AND B means: find results where one of the field's multiple values is both greater than A and less than B.
Not giving users the ability to do the latter really was an oversight on my part when first creating this module.
I'm also pretty sure it should occur in the DB backend, too. Has one of you checked it really works like "between" there?

As for the patch, it looks good, but I'd have to test and review in more detail. The main problem I see is not the technical side, but introducing such an API change now that the module is (or should be) stable. There are several backends not maintained by me, too, after all.
At the very least, we'll need more testers (thanks @ Spaiz, btw), especially feedback from other service class maintainers would be great. I'll post this issue in the important project announcements.
Even better would be if we could introduce this in a backwards-compatible way. A nice way for this that springs to mind is adding a search_api_between feature, and only using the between operator for servers that support that feature. I think that should fix this issue, and only need minimal code (in Views, just check for that feature before adding the value, and use filters for '>=' and '<=' otherwise.

Please tell me what you think about that!

+++ b/service.inc
@@ -949,6 +956,8 @@ class SearchApiSolrService extends SearchApiAbstractService {
+        return "$field:{{$value[0]} TO {$value[1]}}";

This is an exclusive range, I think the between operator should be inclusive.

For reference, here is an updated Solr patch rebased on #1846254: Remove the SolrPhpClient dependency, which will hopefully be committed very soon.

OK, I now got the chance to do a more detailed review and testing. See my attached patch for corrections/suggestions.

I only now realized that the changes in the base Views filter handler will also break most third-party filters that build on that. This might be an even bigger issue for other developers than the API change.
In my patch, I tried to mitigate this at least a bit, falling back to the old behavior for filters that explicitly set other operator options. But of course there'll be some which just inherit the options and will then probably break because they assume $this->value is scalar. Do you, or does anyone else, have any ideas for how we could improve this?
Otherwise we'll just have to hope enough maintainers of such filters will find this issue in time.

One other change I made is to add some error handling when only "min" or "max" is present. Previously, this lead to an error (at least for Solr), now it should just fall back to using "<=" or ">=".

Additionally, the date filter doesn't seem to work, at least for me. All three form fields are displayed when the form is exposed, and errors are thrown. Please try to fix that.

+++ b/contrib/search_api_views/includes/handler_filter.inc
@@ -39,47 +60,188 @@ class SearchApiViewsHandlerFilter extends views_handler_filter {
+    unset($form['value']['value']['#dependency']);

What's the reasoning behind this? When could this be set?

I've applied the patch to a site I'm building now. You're right that it doesn't work so well.

One issue is that combining strtotime() with REQUEST_TIME will result in a timestamp using the time of day in REQUEST_TIME appended to date selected. Totally incorrect!

The min date must use a time of 00:00:00.000 and the max date must use a time of 23:59:59.999. Sure, indexes might have the granularity set to date and they normalise the input timestamp to 00:00:00.000 but not all backends will do it so we might as well get it right. Please note that there are colons (:) and one period (.) in there.

Also the patch had a problem with the options filter not finding the value when rendering the value_form() and also when altering the query.

I've redone the patch to fix the two problems above and corrected the date range filtering along with additional future support for "not between", which is quite trivial to add but I've run out of time. The query() method has the correct operator if there are two values and it's not "in between".

There's a chance the patch will need to be reworked a little for 7.x-1.x. I'm building on 7.x-1.7 and don't have the time right now to apply and test on the dev branch.

NOTE: patch will need a different -p than usual

Here is an updated patch with correct file paths and some improvements that enable the exposed form.

Fixed patch to pass automated testing.

*sigh* rerolled so it doesn't need fuzzy.

switching to needs review lol

EDIT: Posted to wrong issue

Switching back to needs review.

.

Thanks for reviewing the patch. Can you give me some information about the view's filter configuration? I don't need the full view export. Just information about the filter configuration.

In the attached patch I have fixed exposed filters having FAPI #states not working and rerolled the patch.

.

.

Thanks again for testing the patch. I have been working from 7.x-1.7 code to build the patch because it was what my client has currently. We are now updating it to the newer 1.8 code and that should be closer to 7.x-1.x.

I'll post a patch once it's ready.

I want to be clear about something. I'm not implementing integer ranges in my patch. I'm only dealing with dates since that is what is needed by my client.

Thanks, looking forward to the patch here also.

+1 waiting for the patch for 7.x-1.8 or the last dev.
Thank you for all the effort.

.

.

.

Also #28 doesnt work...

Hi.
#31 says the patch works only for 7.x-1.7

.

@ pinkonomy: Yes, that's the best you can currently do instead. (Just adding the conditions on the query itself works the same, though.)
However, the problem is that with multi-valued fields this is not equivalent.

I have finally had some time to refactor this woeful patch. This patch is by no means complete. It's hopefully a reasonable foundation. So, understand that it will have problems and that you should not put this on a production site.

However, please do test it on copies of your production sites.

This patch ONLY adds support for date filters for now. I want to limit the scope to the most complicated of types so we can hone that and handle the simpler numeric filters later.

For maintainers: I have pulled fresh code from views and mirrored their implementation as much as I could. It minimises maintenance and allows you to pull fresh functionality from there more easily. That said, there remains a couple issues I haven't been able to get around. It's when the filter is exposed on a panels pane display and the filter configuration is edited in IPE. These are my findings:

1. Each refresh of the IPE results in a new id attribute generated for exposed operator select lists (perhaps others). The original id has --N appended where N is the number of times you've changed something. I have refactored the copied views code to use the name attribute of the select list instead to get around it. Would be nice if this was kept the same as views though.
2. More important: Input values are nested and don't have the saved value when the form is displayed. I get array('min' => NULL, 'max' => NULL, 'value' => array('min' => NULL, 'max' => NULL, 'value' => NULL)). I spent 7 hours trying to figure it out and I reckon it's better if someone else looks at it with fresh eyes. I have put in a hack to pull values in but I'm not proud of it.

I did refactor handler_filter as well only so it reflects views' built in handlers.

Without much further ado... here are the patches! One is for 7.x-1.x and the other, with do-not-test suffix, is for 7.x-1.9. Please remember! It's not meant to be perfect!

Removed extra parts from the patch that are already in 7.x-1.x.

Thanks a lot for your continued work on this! And sorry that I wasn't able to look at this earlier.
When testing, I spotted the following faults:

• #states doesn't seem to work. At least for me, all three fields are always shown (except in an exposed form with fixed operator).
• The following appears when showing a view with exposed date filter: Notice: Undefined index: type in accept_exposed_input() (line 103 of …/search_api/contrib/search_api_views/includes/handler_filter_date.inc).

Other than that, the filter seems to work fine. (I used the patch from #41, though, the one in #43 seems to be only for 1.9 again.)

On the code side, this still needs quite some work, though:

• As said, please use a feature to determine whether a backend supports the "between" operator. See the use of search_api_mlt in this module if you are unsure of how to go about doing that.
• Adding a whole new (incredibly complicated) "numeric" filter class just as a base class, without actually using it for numeric fields seems like a waste. Wouldn't it just work for them as well, if we make the necessary changes to _search_api_views_add_handlers()? We shouldn't just introduce this massive change just for the benefit of date fields.
• The new class (and the new code in the date filter) has several copy-paste errors from being ripped out of the standard Views handlers. E.g., we don't use allow empty in the definitions and there are no add_where() and add_where_expression() methods on the query class. (Though this doesn't seem to lead to any PHP erros, curiously enough – making me wonder whether that code is even needed?)
  Also, because it's copy-pasted from Views, coding standards compliance is horrible.

In general, if it is done cleanly enough (with a feature) I would commit a patch adding this feature to D7 – even though I don't really understand the code and will probably regret it. However, my focus (especially regarding new features) is now on D8 (where we will surely add a between operator), so I won't really have time to work on this patch myself. If you (not sammys in particular, but people in general) want this in D7, please work on the patch so it becomes clean enough for me to commit! (Also, the less complicated the code is, the better.)

Hi Sammy,

I am indeed very interested by your work, to bad drunken monkey can't validate it, but seems to be a lot of stress with the D8 release so it's understandable.
Have you got any update with this patch, correcting the 2 issues drunken monkey mentioned?

43: 1783746-42-search_api-7.x-1.x-between_operator.patch queued for re-testing.

This is just a clean up version of @sammys patch #43.

If it's just a clean-up, it's still "Needs work". See #45.

@ lahode: Thanks for understanding! It's really hard to look at the D7 issues at all and still make enough progress with D8.

I just applied this patch and it works for our project. It applies perfectly on the 1.14 version of search_api.

Just an update for patch #15 for the last 7.x-1.x commit, and I've added the 'not between' option.

This patch applies to search_api_solr, I think it should be there, so I created an issue on it https://www.drupal.org/node/2459457

Well, I created a new patch, it applies on contrib/search_api_facetapi/plugins/facetapi/query_type_term.inc.

The patches #15 or https://www.drupal.org/node/2459457 doesnt do anythings on it's own, it needs a facet or something which uses the between operator. This patch allows facets to use it, when they do range search like:

some_field:[somehting TO something]

date_facets is one example.

Thanks for creating a separate issue for the Solr code, makes sense of course! (The operator could also easily be supported by the DB backend, too, I think. Maybe we should have an issue there, too? However, as long as no-one wants to try and implement it, I guess there's not much of a point.)
Adding support for this to date facets of course also makes sense. But please merge the patch with the existing one so we have a single one to work on for this module, otherwise it gets really confusing.

Apart from that, my remarks from #45 seem to be largely un-addressed still, so I haven't really reviewed this more.

This is my attempt at fixing it, I'm working with Solr, so I haven't update the db driver.

Used the ctools dependency managment to show/hide the form. tested it locally.

First time patching here. sorry for the duplicates.

Thank you for your help here!
However, I cannot apply your patch. First off, please create patches with the module directory as the root, so that they apply with git apply in the module repository.
Secondly, your patch seems to be based off an older version. Please always base patches off the current dev version!
(Otherwise, the test bot in the issue queue will also not work.)

Slightly reworked "59: search_api-support-for-between-8557331-59-7.39.patch". Works for me.

This patch adds whitespace errors.

Also, as said in #45, this should use a (search backend) feature to determine whether the current search server supports the "between" operator, and otherwise hide it. (See contrib/search_api_views/README.txt for examples of feature definitions – "More like this" and "Random sorting" in this case.)

This is #66 with the whitespace fixes.

Does not address drunken monkey's concerns in the above comment.

Here is #68 against 1.16.

Here is patch to latest dev. Didn't address drunken monkey's concerns either. I am guessing we need to see if the server supports between or ranges or something to that nature just not sure how to go about it. I also adjusted the #dependencies form variable as it was complete wiping out the form all together.

I am guessing we need to see if the server supports between or ranges or something to that nature just not sure how to go about it.

You'd need to add a feature for that and then check with $server->supportsFeature(). (E.g., see the search_api_views/README.txt file for two features we currently define.)

E.g. when using solr an expression like field[1 TO *] AND field[* TO 10] doesn't necessary return the same results as field[1 TO 10].

I found perfect example.
I'm using commerce with product variotion and I have price range UI facet and settings as commerce_search_api module.

So I have node product with two variation.
Prices: 10, 100.
This prices is in my SOLR index as multivalued field.

After I use facet api UI range to pick products for price 40 - 50 field[40 TO *] AND field[* TO 50] Solr return me this node.
Because 40 < 100, 50 > 10.

I just ran into a little issue with the patch in #71 where I could not set a value to another, normal (not between) filter. So I cleaned up the patch and removed a bit of code.

I'll leave this issue on 'needs work' because of #73.

It looks like I removed too much code! Oops. Attached is a new patch with the cleaned up code and also a few fixes:

• admin summary works again
• the #dependency of the value form field works again, also for empty/not empty.

Ignore that last patch, I forgot to add the tree = TRUE back in...

• admin summary works again
• the #dependency of the value form field works again, also for empty/not empty.

And it looks like I reintroduced a bug I initially wanted to solve: (not exposed) non-between filters don't keep the selected/entered values.

Hopefully the last patch file for a while! :)

Added a bit to SearchApiViewsHandlerFilterOptions so the options uses a normal value instead. That solved not being able to save the other filters.

I still have an issue when I add a date filter: views gives an error when going to step 2 (after selecting the field):

ResponseText: Error: [] operator not supported for strings in views_ui_regenerate_tab() (line 2399 of /../drupal/sites/all/modules/contrib/views/includes/admin.inc).

I don't know why views_ui_regenerate_tab would be passed a string. During a bit of tracing/debugging I saw that this function was called 3 times; twice with an array and once with a string (which fails).

Pressing "Apply and continue" again closes the dialog. And after reopening it, everything works fine. No idea why.

Thanks for that! Does anyone want to test these latest changes?
In any case, it would be great if someone could find the time to properly define this as a feature, so this has a chance of being committed. It's really not that hard, surely easier than all this debugging with Views.

Hello,

Thanks for this great patch, i tested it and everything work fine. But there is a warning (Warning :Illegal string offset 'value' in handler_filter.inc line 121 and 124).
I added a patch which check if value['value'] exist.

Thanks

@ Anas_maw: Thanks, good change! However, this appears to be a problem for several other places in the code, too.

I tried the patch in 81 and didn't get any warnings while creating or using the filter.

What this was doing though was breaking normal equality. I moved the 'value' key above 'min' and 'max', and that let reset($this->value) work correctly.

Unfortunately this change bleeds through to SearchApiViewsHandlerFilterEntity where it causes several problems.

The between operator has no place in SearchApiViewsHandlerFilter. It needs to be added in a filter handler specialized on numeric values from which other numeric handlers like SearchApiViewsHandlerFilterDate can extend. This would be similar to how it's done in Views itself.

Since SearchApiViewsHandlerFilterEntity relies heavily on methods defined in SearchApiViewsHandlerFilter, reversing the modifications there does not seem a viable approach either.

getting errors on patch #88 and #81

Warning: Illegal string offset 'value' in SearchApiViewsHandlerFilter->admin_summary() (line 183 of /xxxx/docroot/sites/all/modules/search_api/contrib/search_api_views/includes/handler_filter.inc).

Notice: Uninitialized string offset: 0 in SearchApiViewsHandlerFilter->admin_summary() (line 183 of /xxxx/docroot/sites/all/modules/search_api/contrib/search_api_views/includes/handler_filter.inc).

I combined this patch with the one from #1595230 which adds date-filter support for between.
I added a small fix for the min/max fields which did not use the correct date_format and year-range.

I think I also added a fix for the admin_summary() notices (from #90): a similar fix was in the other issue in the SearchApiViewsHandlerFilterDate::query() method.

Thanks for your continued work!
The attached patch finally adds a proper feature definition, moves the functionality to a new filter class (as stated in #89, we don't want this for all our filters) and generally tries to clean up the code.

A few specific remarks:

• What is $this->value[0]['max'] > 0 supposed to do? I think you just want to check for the empty string there? I don't see how a negative upper bound (or 0, or (for non-numeric fields) a non-numeric string) would be invalid.
• Also, regarding this weird conflict between $this->value and $this->value[0] – do we have any idea, where that's coming from, and when we can expect one or the other?
  If we can't, shouldn't the code for min/max also have this check? There, we always use $this->value[0] for some reason.
• As in D8, the same filter plugin, although named "numeric", will also be used for strings, since we don't really make any difference here. (We could force the input to numeric, or integers, depending on the type, but that would be out-of-scope here anyways.)

Please test/review, everyone, and we can hopefully finally get this committed soon!

C'mon, 45 followers and not a single one prepared to test and finally get this committed?
(Should of course be tested in conjunction with #2459457-7: Support between operator.)

Along with #2982443: Support between operator for search_api_db this worked for me.

Hi,

1783746-92--between_operator.patch + #2459457-7: Support between operator works for me too.

Thanks drunken monkey ;)

OK, thanks a lot for your feedback!
Then let's finally do this, I guess: committed!
Thanks a lot again for everyone's work on this!

Was a bit tricky to curate the contribution credits. If you feel I have mistakenly omitted you, even though you've made a significant contribution (more detailed review or patch with non-trivial changes) please speak up!
In any case, I didn't go to the trouble of re-ordering the credits for the commit message, as I usually do. Peter, as the initiator, deserves main credit, I'd say, but beyond that it would've been a bit too tricky to order.

Follow-up: #2989578: "Not between" Min/Max form issues

Follow-up:

@das-peter, @drunken-monkey

I am not sure if I have missed anything in addition to this patch, but after I upgraded Search API to dev version (7.x-1.25+4-dev from Oct 4, 2018), I see an issue with Search API based views Date filter when operator is a simple "Is equal to".

I have Solr index and the field is indexed as Date type. When debugging the code, I see that my filter value is being cut off to a single character inside /search_api/contrib/search_api_views/includes/handler_filter_date.inc query() function on line 154.

This happens due to the value being assigned with $this->value[0]['value']. My data doesn't have this structure for some reason:

I have checked Git history and the value here was assigned differently before this commit:

So, committed change breaks filters in my case.

If I replace this

$value = isset($this->value[0]) ? $this->value[0]['value'] : $this->value['value'];

with this:
$value = isset($this->value[0]) ? (is_array($this->value[0]) ? $this->value[0]['value'] : $this->value[0]) : $this->value['value'];

it all works again.

Any insight on whether this is actually a bug or maybe I am missing any other config or module updates? Do I need to open another issue?

Any insight on whether this is actually a bug or maybe I am missing any other config or module updates? Do I need to open another issue?

@veronicaSeveryn: I've experienced the same problem last week with some views suddenly being broken after the update, so I think you should open a new issue for this.

Not sure if this was intentional, but setting a default of "" will trigger a validation of the element. I think the value should instead default to null.

+++ b/contrib/search_api_views/includes/handler_filter_numeric.inc
@@ -0,0 +1,209 @@
+      if (!empty($form_state['exposed']) && !isset($form_state['input'][$identifier])) {
+        $form_state['input'][$identifier] = isset($this->value['value']) ? $this->value['value'] : '';
+      }

Edit: The relevant section in _form_builder_handle_input_element():

      if (isset($element['#value']) || (!empty($element['#required']))) {
        $element['#needs_validation'] = TRUE;
      }