Problem/Motivation
While working on a patch for Search API I noticed, Drupal wouldn't let me create database conditions using the NOT BETWEEN
operator.
I couldn't find any reference, why this is missing in Condition::mapConditionOperator()
(even in Drupal 7 it's not there) so I guess (hope) it had been simply forgotten.
Proposed resolution
Add NOT BETWEEN
along with its handling directives to mapConditionOperator().
Remaining tasks
Create change record about the changed behavior of opBetween()
?
User interface changes
none
API changes
opBetween()
behaves differently when using "not between". Before, the boundaries where part of the result set; with the patch, the boundaries are excluded (which is the default behavior).
Comments
Comment #2
dawehnerWhile looking at the patch I'm wondering whether there is any place where the available operators are documented.
This fix here could also simplify
\Drupal\views\Plugin\views\filter\NumericFilter::opBetween
a bit.Comment #3
stBorchertThe only place, where I saw a "listing" of the available operators was the function
mapConditionOperator()
, I've updated here.Comment #4
timmillwoodComment #5
dawehner@stBorchert
Well this code:
could leverage this new operator.
Comment #6
daffie CreditAttribution: daffie commentedTests are needed to make sure that the operator works for all supported databases.
Comment #7
stBorchertUpdated the code and tests.
But ... (and its a very big "but"): the current implementation of
opBetween()
(shown in comment 5) uses<=
and>=
to mimic "not between". This is wrong."between" is defined as being an inclusive range, meaning the results may contain the boundaries itself. As "not between" is obviously the opposite of "between", it cannot include the boundaries.
So, unfortunately, it seems, we did this wrong all the time :/
As you can see in the tests,
FilterNumericTest::testFilterNumericBetween()
expected the same result for both operators ("26").The main problem here is that some users might get different results now, since the behavior of
opBetween()
changes (at least for "not between"). A change record is necessary for this.Comment #9
stBorchertTestbot, I don't believe you ;) I guess, the server outage did something strange here.
Comment #10
daffie CreditAttribution: daffie commentedI think you are right stBorchert, the implementation of "NOT BETWEEN" is correct. The borders are not part of the "NOT BETWEEN" operator. I have checked all three supported databases and all three implement "NOT BETWEEN" without the borders. For me this is a bug and should be corrected. The problem is that you can also look at it with the view that this is a API change and that is not allowed. I have added the issue tag "Needs subsystem maintainers review". It would be nice if both subsystem maintainers from the database and views have a look at this.
If we are going to fix this then we should also fix the views part. The "NOT BETWEEN" operator works for numbers, strings and dates. We must be sure that for all three there is enough testing. Especially for the border stuff.
Comment #11
daffie CreditAttribution: daffie commentedChanging the category to bug. See previous comment #10.
Comment #12
stBorchertUpdated the patch to test the range borders for dates. I don't see, where
StringFilter
is using BETWEEN or NOT BETWEEN (its not listed inoperators()
); do you have an example?Comment #13
dawehnerI talked with @stborchert before about that, and given that the database API is working different, is a clear sign that this is a bug of views IMHO.
Comment #14
daffie CreditAttribution: daffie commented@stBorchert: According to the database documentation you can use "NOT BETWEEN" with numbers, dates and strings. I did not know that drupal did not use it with strings.
@dawehner: Thank you for your input.
The patch looks good to me.
All the changes for the database drivers are there.
All the changes for views are also there.
Changed so test for border cases. Views had a bug where borders where included in the result for "NOT BETWEEN" selections.
For me it is RTBC.
Good work stBorchert.
Comment #15
chx CreditAttribution: chx commentedComment #16
chx CreditAttribution: chx commentedRevering before someone takes insult of me linking to the SQL standard.
Comment #18
daffie CreditAttribution: daffie commentedBack to RTBC.
Comment #19
alexpottCommitted and pushed f1d5d6595154453cebadfc39e7988499fb71a121 to 8.2.x and 8219239 to 8.1.x. Thanks!One potential problem is that apparently not all databases do BETWEEN and NOT BETWEEN the same way - vis a vis - how the borders are included or not. But, as @chx added and then removed from the IS the ANSI standard SQL-92 is quite clear...
So we cna't be responsible for other DBs bugs.
I thought about only committing this to 8.2.x because of the slight change in behaviour to views but I think this is a bug and ti does not matter when it is fixed.Comment #20
alexpottI decided not to push because I realised that adding "not between" is a feature addition and therefore not eligible for 8.1.x and additionally as a feature addition should have a CR to tell people about it.
Comment #21
daffie CreditAttribution: daffie commentedAdded a change record.
Comment #22
alexpottCommitted 5601e98 and pushed to 8.2.x. Thanks!
Comment #25
daffie CreditAttribution: daffie commentedBack to fixed.
Comment #26
drunken monkeyTypo in the change record:
Pretty sure it should be "XOR X>Z".
Comment #27
daffie CreditAttribution: daffie commentedType fixed.
Thanks drunken monkey!